From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.8283.1591252247078509001 for ; Wed, 03 Jun 2020 23:30:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jA2hQ4o0; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1591252246; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=q1/pf4/ZYSOVoGER+cjh8NNqdxZORSNmCp4ck+a42VI=; b=jA2hQ4o00X6+uC3Tvdsc0UmXkemsN+Z3PaNjoZjVSm0U/Ym6MHolT+4wQjTmVX3hIsF019 XGpcz1wIJMF1ciWwZ3E30bViD8NbR20mW0phFaOn9U2t6jCrUnh1nbk/H28UZpT8hZos/X PnMsziFSSvwxlfaUSFo3AeTk18YMeZs= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-508-gqWMG11mPe2KNXGLu83T9g-1; Thu, 04 Jun 2020 02:30:42 -0400 X-MC-Unique: gqWMG11mPe2KNXGLu83T9g-1 Received: by mail-wm1-f71.google.com with SMTP id u11so1608153wmc.7 for ; Wed, 03 Jun 2020 23:30:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:cc:references:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=q1/pf4/ZYSOVoGER+cjh8NNqdxZORSNmCp4ck+a42VI=; b=hIboCDepOQygn2RcmG+3781WtQPo5UetpoizLZKfdZoi9iwQllKw5rQjXvfj6XuH4b PqXszq+loU0K3ID7dfeLldWPTd9SuDcMpKosUsYIFaZEuocxU+BbZtfdlEQ3S+jNfBAJ ZCe8R9DI1vTU+w7XDDgSXn213LBSGD9AZ6O7K9Oe79ag4CrbXOosjOhBzKYUgkJDswyR B/Had9wdJdL+ItoXVJzMW5DlaVe0xrvh4TLECB2MUMuoc7TL3ZKpIFO4YPXkIdTI4UXN ys+fzZZ3FlOlN4bTcubfLdMxcC0Oj/HE0JOXSZmjU7+opQ6egeiNhtsO7mnY3+/UWIpz I0bg== X-Gm-Message-State: AOAM533EMm+aA7dP0a/8iChfseTbsD6gUdG41emTdtKhOuGODzvkYZ0Y HX5FFfomApcM6Brv0Y9PXSB/h+ZN5wAJNS1UReXxv+bsYlUDT7Ufts1XmYewGpuVlE/hPS0pgSy L4U/PCi8nW9+ZyA== X-Received: by 2002:a1c:5541:: with SMTP id j62mr2349801wmb.64.1591252240927; Wed, 03 Jun 2020 23:30:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzv0kFXTovl17C4Wy1Gm9xMH18JJL0nRIIVJnK7tM/Uf+zKoO5afjSF6O04p/S03PCjgCiXFQ== X-Received: by 2002:a1c:5541:: with SMTP id j62mr2349779wmb.64.1591252240581; Wed, 03 Jun 2020 23:30:40 -0700 (PDT) Return-Path: Received: from [192.168.1.43] (181.red-88-10-103.dynamicip.rima-tde.net. [88.10.103.181]) by smtp.gmail.com with ESMTPSA id d18sm6359874wrn.34.2020.06.03.23.30.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 03 Jun 2020 23:30:39 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 08/11] ArmVirtPkg: Add Kvmtool NOR flash lib From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= To: devel@edk2.groups.io, sami.mujawar@arm.com Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, lersek@redhat.com, Alexandru.Elisei@arm.com, Andre.Przywara@arm.com, Matteo.Carlini@arm.com, Laura.Moretta@arm.com, nd@arm.com References: <20200514084019.71368-1-sami.mujawar@arm.com> <20200514084019.71368-9-sami.mujawar@arm.com> <6fca2acc-f9f2-59cd-92a4-5276b30c0f91@redhat.com> Autocrypt: addr=philmd@redhat.com; keydata= mQINBDXML8YBEADXCtUkDBKQvNsQA7sDpw6YLE/1tKHwm24A1au9Hfy/OFmkpzo+MD+dYc+7 bvnqWAeGweq2SDq8zbzFZ1gJBd6+e5v1a/UrTxvwBk51yEkadrpRbi+r2bDpTJwXc/uEtYAB GvsTZMtiQVA4kRID1KCdgLa3zztPLCj5H1VZhqZsiGvXa/nMIlhvacRXdbgllPPJ72cLUkXf z1Zu4AkEKpccZaJspmLWGSzGu6UTZ7UfVeR2Hcc2KI9oZB1qthmZ1+PZyGZ/Dy+z+zklC0xl XIpQPmnfy9+/1hj1LzJ+pe3HzEodtlVA+rdttSvA6nmHKIt8Ul6b/h1DFTmUT1lN1WbAGxmg CH1O26cz5nTrzdjoqC/b8PpZiT0kO5MKKgiu5S4PRIxW2+RA4H9nq7nztNZ1Y39bDpzwE5Sp bDHzd5owmLxMLZAINtCtQuRbSOcMjZlg4zohA9TQP9krGIk+qTR+H4CV22sWldSkVtsoTaA2 qNeSJhfHQY0TyQvFbqRsSNIe2gTDzzEQ8itsmdHHE/yzhcCVvlUzXhAT6pIN0OT+cdsTTfif MIcDboys92auTuJ7U+4jWF1+WUaJ8gDL69ThAsu7mGDBbm80P3vvUZ4fQM14NkxOnuGRrJxO qjWNJ2ZUxgyHAh5TCxMLKWZoL5hpnvx3dF3Ti9HW2dsUUWICSQARAQABtDJQaGlsaXBwZSBN YXRoaWV1LURhdWTDqSAoUGhpbCkgPHBoaWxtZEByZWRoYXQuY29tPokCVQQTAQgAPwIbDwYL CQgHAwIGFQgCCQoLBBYCAwECHgECF4AWIQSJweePYB7obIZ0lcuio/1u3q3A3gUCXsfWwAUJ KtymWgAKCRCio/1u3q3A3ircD/9Vjh3aFNJ3uF3hddeoFg1H038wZr/xi8/rX27M1Vj2j9VH 0B8Olp4KUQw/hyO6kUxqkoojmzRpmzvlpZ0cUiZJo2bQIWnvScyHxFCv33kHe+YEIqoJlaQc JfKYlbCoubz+02E2A6bFD9+BvCY0LBbEj5POwyKGiDMjHKCGuzSuDRbCn0Mz4kCa7nFMF5Jv piC+JemRdiBd6102ThqgIsyGEBXuf1sy0QIVyXgaqr9O2b/0VoXpQId7yY7OJuYYxs7kQoXI 6WzSMpmuXGkmfxOgbc/L6YbzB0JOriX0iRClxu4dEUg8Bs2pNnr6huY2Ft+qb41RzCJvvMyu gS32LfN0bTZ6Qm2A8ayMtUQgnwZDSO23OKgQWZVglGliY3ezHZ6lVwC24Vjkmq/2yBSLakZE 6DZUjZzCW1nvtRK05ebyK6tofRsx8xB8pL/kcBb9nCuh70aLR+5cmE41X4O+MVJbwfP5s/RW 9BFSL3qgXuXso/3XuWTQjJJGgKhB6xXjMmb1J4q/h5IuVV4juv1Fem9sfmyrh+Wi5V1IzKI7 RPJ3KVb937eBgSENk53P0gUorwzUcO+ASEo3Z1cBKkJSPigDbeEjVfXQMzNt0oDRzpQqH2vp apo2jHnidWt8BsckuWZpxcZ9+/9obQ55DyVQHGiTN39hkETy3Emdnz1JVHTU0Q== Message-ID: <190ec56b-99b0-f9cb-1ef0-366fca285787@redhat.com> Date: Thu, 4 Jun 2020 08:30:38 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <6fca2acc-f9f2-59cd-92a4-5276b30c0f91@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 5/27/20 1:59 PM, Philippe Mathieu-Daudé wrote: > Hi Sami, > > On 5/14/20 10:40 AM, Sami Mujawar wrote: >> Kvmtool places the base address of the CFI flash in >> the device tree it passes to UEFI. This library >> parses the kvmtool device tree to read the CFI base >> address and initialise the PCDs use by the NOR flash >> driver and the variable storage. >> >> Signed-off-by: Sami Mujawar >> --- >> ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c | 265 ++++++++++++++++++++ >> ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf | 50 ++++ >> 2 files changed, 315 insertions(+) >> >> diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c >> new file mode 100644 >> index 0000000000000000000000000000000000000000..2e43c2e21bc9ef7dd1dd198eebbd70c3b0b96d1c >> --- /dev/null >> +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c >> @@ -0,0 +1,265 @@ >> +/** @file >> + An instance of the NorFlashPlatformLib for Kvmtool platform. >> + >> + Copyright (c) 2020, ARM Ltd. All rights reserved.
>> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> + >> + **/ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/** Macro defining the maximum number of Flash Banks. >> + */ >> +#define MAX_FLASH_BANKS 4 >> + >> +STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[MAX_FLASH_BANKS]; > > This is confuse, the macro define 4 banks to a flash device, but then > you declare an array of 4 flash devices. > > I'm even more confused because I'm only aware of 2 devices on the Virt > machine. What am I missing? Pinging again in case this question has been missed. > >> +STATIC UINTN mNorFlashDeviceCount = 0; >> + >> +/** This function performs platform specific actions to initialise >> + the NOR flash, if required. >> + >> + @retval EFI_SUCCESS Success. >> +**/ >> +EFI_STATUS >> +NorFlashPlatformInitialization ( >> + VOID >> + ) >> +{ >> + DEBUG ((DEBUG_INFO, "NorFlashPlatformInitialization\n")); >> + // Nothing to do here >> + return EFI_SUCCESS; >> +} >> + >> +/** Initialise Non volatile Flash storage variables. >> + >> + @param [in] FlashDevice Pointer to the NOR Flash device. >> + >> + @retval EFI_SUCCESS Success. >> + @retval EFI_INVALID_PARAMETER A parameter is invalid. >> + @retval EFI_OUT_OF_RESOURCES Insufficient flash storage space. >> +**/ >> +EFI_STATUS >> +SetupVariableStore ( >> + IN NOR_FLASH_DESCRIPTION * FlashDevice >> + ) >> +{ >> + UINTN FlashRegion; >> + UINTN FlashNvStorageVariableBase; >> + UINTN FlashNvStorageFtwWorkingBase; >> + UINTN FlashNvStorageFtwSpareBase; >> + UINTN FlashNvStorageVariableSize; >> + UINTN FlashNvStorageFtwWorkingSize; >> + UINTN FlashNvStorageFtwSpareSize; >> + >> + FlashNvStorageVariableSize = PcdGet32 (PcdFlashNvStorageVariableSize); >> + FlashNvStorageFtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize); >> + FlashNvStorageFtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize); >> + >> + if ((FlashNvStorageVariableSize == 0) || >> + (FlashNvStorageFtwWorkingSize == 0) || >> + (FlashNvStorageFtwSpareSize == 0)) { >> + DEBUG ((DEBUG_ERROR, "FlashNvStorage size not defined\n")); >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + // Setup the variable store >> + FlashRegion = FlashDevice->DeviceBaseAddress; >> + >> + FlashNvStorageVariableBase = FlashRegion; >> + FlashRegion += PcdGet32 (PcdFlashNvStorageVariableSize); >> + >> + FlashNvStorageFtwWorkingBase = FlashRegion; >> + FlashRegion += PcdGet32 (PcdFlashNvStorageFtwWorkingSize); >> + >> + FlashNvStorageFtwSpareBase = FlashRegion; >> + FlashRegion += PcdGet32 (PcdFlashNvStorageFtwSpareSize); >> + >> + if (FlashRegion > (FlashDevice->DeviceBaseAddress + FlashDevice->Size)) { >> + DEBUG ((DEBUG_ERROR, "Insufficient flash storage size\n")); >> + return EFI_OUT_OF_RESOURCES; >> + } >> + >> + PcdSet32S ( >> + PcdFlashNvStorageVariableBase, >> + FlashNvStorageVariableBase >> + ); >> + >> + PcdSet32S ( >> + PcdFlashNvStorageFtwWorkingBase, >> + FlashNvStorageFtwWorkingBase >> + ); >> + >> + PcdSet32S ( >> + PcdFlashNvStorageFtwSpareBase, >> + FlashNvStorageFtwSpareBase >> + ); >> + >> + DEBUG (( >> + DEBUG_INFO, >> + "PcdFlashNvStorageVariableBase = 0x%x\n", >> + FlashNvStorageVariableBase >> + )); >> + DEBUG (( >> + DEBUG_INFO, >> + "PcdFlashNvStorageVariableSize = 0x%x\n", >> + FlashNvStorageVariableSize >> + )); >> + DEBUG (( >> + DEBUG_INFO, >> + "PcdFlashNvStorageFtwWorkingBase = 0x%x\n", >> + FlashNvStorageFtwWorkingBase >> + )); >> + DEBUG (( >> + DEBUG_INFO, >> + "PcdFlashNvStorageFtwWorkingSize = 0x%x\n", >> + FlashNvStorageFtwWorkingSize >> + )); >> + DEBUG (( >> + DEBUG_INFO, >> + "PcdFlashNvStorageFtwSpareBase = 0x%x\n", >> + FlashNvStorageFtwSpareBase >> + )); >> + DEBUG (( >> + DEBUG_INFO, >> + "PcdFlashNvStorageFtwSpareSize = 0x%x\n", >> + FlashNvStorageFtwSpareSize >> + )); >> + >> + return EFI_SUCCESS; >> +} >> + >> +/** Return the Flash devices on the platform. >> + >> + @param [out] NorFlashDescriptions Pointer to the Flash device description. >> + @param [out] Count Number of Flash devices. >> + >> + @retval EFI_SUCCESS Success. >> + @retval EFI_NOT_FOUND Flash device not found. >> +**/ >> +EFI_STATUS >> +NorFlashPlatformGetDevices ( >> + OUT NOR_FLASH_DESCRIPTION **NorFlashDescriptions, >> + OUT UINT32 *Count >> + ) >> +{ >> + if (mNorFlashDeviceCount > 0) { >> + *NorFlashDescriptions = mNorFlashDevices; >> + *Count = mNorFlashDeviceCount; >> + return EFI_SUCCESS; >> + } >> + return EFI_NOT_FOUND; >> +} >> + >> +/** Entrypoint for NorFlashPlatformLib. >> + >> + @param [in] ImageHandle The handle to the image. >> + @param [in] SystemTable Pointer to the System Table. >> + >> + @retval EFI_SUCCESS Success. >> + @retval EFI_INVALID_PARAMETER A parameter is invalid. >> + @retval EFI_NOT_FOUND Flash device not found. >> +**/ >> +EFI_STATUS >> +EFIAPI >> +NorFlashPlatformLibConstructor ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE * SystemTable >> + ) >> +{ >> + FDT_CLIENT_PROTOCOL *FdtClient; >> + INT32 Node; >> + EFI_STATUS Status; >> + EFI_STATUS FindNodeStatus; >> + CONST UINT32 *Reg; >> + UINT32 PropSize; >> + UINT64 Base; >> + UINT64 Size; >> + >> + if (mNorFlashDeviceCount != 0) { >> + return EFI_SUCCESS; >> + } >> + >> + Status = gBS->LocateProtocol ( >> + &gFdtClientProtocolGuid, >> + NULL, >> + (VOID **)&FdtClient >> + ); >> + if (EFI_ERROR (Status)) { >> + ASSERT_EFI_ERROR (Status); >> + return Status; >> + } >> + >> + for (FindNodeStatus = FdtClient->FindCompatibleNode ( >> + FdtClient, >> + "cfi-flash", >> + &Node >> + ); >> + !EFI_ERROR (FindNodeStatus) && (mNorFlashDeviceCount < MAX_FLASH_BANKS); >> + FindNodeStatus = FdtClient->FindNextCompatibleNode ( >> + FdtClient, >> + "cfi-flash", >> + Node, >> + &Node >> + )) { >> + Status = FdtClient->GetNodeProperty ( >> + FdtClient, >> + Node, >> + "reg", >> + (CONST VOID **)&Reg, >> + &PropSize >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: GetNodeProperty () failed (Status == %r)\n", >> + __FUNCTION__, Status)); >> + continue; >> + } >> + >> + ASSERT ((PropSize % (4 * sizeof (UINT32))) == 0); >> + >> + while ((PropSize >= (4 * sizeof (UINT32))) && >> + (mNorFlashDeviceCount < MAX_FLASH_BANKS)) { >> + Base = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[0])); >> + Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2])); >> + Reg += 4; >> + >> + PropSize -= 4 * sizeof (UINT32); >> + >> + // >> + // Disregard any flash devices that overlap with the primary FV. >> + // The firmware is not updatable from inside the guest anyway. >> + // >> + if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) > Base) && >> + (Base + Size) > PcdGet64 (PcdFvBaseAddress)) { >> + continue; >> + } >> + >> + DEBUG (( >> + DEBUG_INFO, >> + "NOR%d : Base = 0x%lx, Size = 0x%lx\n", >> + mNorFlashDeviceCount, >> + Base, >> + Size >> + )); >> + >> + mNorFlashDevices[mNorFlashDeviceCount].DeviceBaseAddress = (UINTN)Base; >> + mNorFlashDevices[mNorFlashDeviceCount].RegionBaseAddress = (UINTN)Base; >> + mNorFlashDevices[mNorFlashDeviceCount].Size = (UINTN)Size; >> + mNorFlashDevices[mNorFlashDeviceCount].BlockSize = SIZE_256KB; > > Hmm I'm worried that there is no contract enforcing the Virt machine to > use 256KiB sectors. Can we add a definition elsewhere and use it here, > instead of burying the fixed sector size here? > >> + mNorFlashDeviceCount++; >> + } >> + } >> + >> + // Setup the variable store in the last bank >> + if ((mNorFlashDeviceCount > 0) && >> + (mNorFlashDevices[mNorFlashDeviceCount - 1].DeviceBaseAddress != 0)) { >> + return SetupVariableStore (&mNorFlashDevices[mNorFlashDeviceCount - 1]); >> + } >> + >> + return EFI_NOT_FOUND; >> +} >> + >> diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf >> new file mode 100644 >> index 0000000000000000000000000000000000000000..8bd6f730dcb52e597b418e59766c1566a9519789 >> --- /dev/null >> +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf >> @@ -0,0 +1,50 @@ >> +#/** @file >> +# >> +# Copyright (c) 2020, ARM Ltd. All rights reserved.
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent >> +# >> +#**/ >> + >> +[Defines] >> + INF_VERSION = 0x0001001B >> + BASE_NAME = NorFlashKvmtoolLib >> + FILE_GUID = E75F07A1-B160-4893-BDD4-09E32FF847DC >> + MODULE_TYPE = DXE_DRIVER >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = NorFlashPlatformLib >> + CONSTRUCTOR = NorFlashPlatformLibConstructor >> + >> +[Sources.common] >> + NorFlashKvmtool.c >> + >> +[Packages] >> + ArmPkg/ArmPkg.dec >> + ArmPlatformPkg/ArmPlatformPkg.dec >> + ArmVirtPkg/ArmVirtPkg.dec >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + >> +[LibraryClasses] >> + BaseLib >> + DebugLib >> + PcdLib >> + UefiBootServicesTableLib >> + >> +[Protocols] >> + gFdtClientProtocolGuid ## CONSUMES >> + >> +[Pcd] >> + gArmTokenSpaceGuid.PcdFvBaseAddress >> + gArmTokenSpaceGuid.PcdFvSize >> + >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize >> + >> + >> +[Depex] >> + gFdtClientProtocolGuid >> + >> >