Re: [edk2-devel] [PATCH v1 08/11] ArmVirtPkg: Add Kvmtool NOR flash lib

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Philippe,

Thank you for reviewing this patch.
Please find my answers inline marked [SAMI]

Regards,

Sami Mujawar

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@redhat.com> 
Sent: 04 June 2020 07:31 AM
To: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com; lersek@redhat.com; Alexandru Elisei <Alexandru.Elisei@arm.com>; Andre Przywara <Andre.Przywara@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 08/11] ArmVirtPkg: Add Kvmtool NOR flash lib

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 <sami.mujawar@arm.com>
>> ---
>>  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..2e43c2e21bc9ef7dd1dd198eebb
>> d70c3b0b96d1c
>> --- /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.<BR>
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> + **/
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/NorFlashPlatformLib.h> #include 
>> +<Library/UefiBootServicesTableLib.h>
>> +#include <Protocol/FdtClient.h>
>> +
>> +/** 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.
[SAMI] The macro MAX_FLASH_BANKS should be changed to MAX_FLASH_DEVICES. I will send an updated patch with this fixed.
On kvmtool, there is only one flash device currently as this is all that is needed. However, in the future if more flash devices are provided then this code can handle multiple flash devices.
[/SAMI]
> 
>> +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..8bd6f730dcb52e597b418e59766
>> c1566a9519789
>> --- /dev/null
>> +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
>> @@ -0,0 +1,50 @@
>> +#/** @file
>> +#
>> +#  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR> #  
>> +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
>> +
>>
>