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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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..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.<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.

> 
>> +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.<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
>> +
>>
>