Re: [PATCH v3 06/15] ArmVirtPkg: Add Kvmtool NOR flash lib

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

Hi Sami,

On 6/24/20 3:34 PM, 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.
> 
> UEFI takes ownership of the CFI flash hardware, and
> exposes its functionality through the UEFI Runtime
> Variable Service. Therefore, disable the device tree
> node for the CFI flash used for storing the UEFI
> variables, to prevent the OS from attaching its device
> driver as well.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v3:
>       - ASSERT is sufficient to test Locating                     [Ard]
>         gFdtClientProtocolGuid as DEPEX ensures that this is
>         guaranteed to succeed.
>       - Removed additional error handling based on review         [Sami]
>         feedback.
>       - Fix confusion caused by use of macro MAX_FLASH_BANKS.     [Philippe]
>       - Renamed MAX_FLASH_BANKS to MAX_FLASH_DEVICES.             [Sami]
>       - Use macro to define block size for flash.                 [Philippe]
>       - Defined macro KVMTOOL_NOR_BLOCK_SIZE and also configured  [Sami]
>         to reflect the correct block size 64KB.
>       - Disable the DT flash node used for UEFI variable storage  [Sami]
>         as UEFI takes ownership of the flash device.
>         Ref: https://edk2.groups.io/g/devel/topic/74200914#60341
>     
>     v2:
>       - Library to read CFI flash base address from DT and initialise [Sami]
>         PCDs used for NOR flash variables.
> 
>  ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c      | 330 ++++++++++++++++++++
>  ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf |  49 +++
>  2 files changed, 379 insertions(+)
> 
> diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..8e9dcf31691b4b12b9c7bac1ad4ba8d3a534a1d8
> --- /dev/null
> +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
> @@ -0,0 +1,330 @@
> +/** @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 NOR block size configured in Kvmtool.
> +*/
> +#define KVMTOOL_NOR_BLOCK_SIZE  SIZE_64KB
> +
> +/** Macro defining the maximum number of Flash devices.
> +*/
> +#define MAX_FLASH_DEVICES       4

I am sorry but I am still confused...

This is about the QEMU Virt machine, right?

This machine was supposed to have 1 single flash, see QEMU commit
f5fdcd6e58 ("hw/arm: Add 'virt' platform") from Nov 2013:

  /* Addresses and sizes of our components.
   * 0..128MB is space for a flash device so we can run bootrom code
such as UEFI.
   ...

Due to limitations in the QEMU cfi-flash model, instead of using
a single flash device (with proper sector/bank protection), two
devices were added in QEMU commit acf82361c6 ("hw/arm/virt: Provide
flash devices for boot ROMs") Sep 2014:

    Add two flash devices to the virt board, so that it can be used for
    running guests which want a bootrom image such as UEFI. We provide
    two flash devices to make it more convenient to provide both a
    read-only UEFI image and a read-write place to store guest-set
    UEFI config variables. The '-bios' command line option is set up
    to provide an image for the first of the two flash devices.

What do you declare maximum 4 devices?

Thanks,

Phil.

> +
> +/** Macro defining the cfi-flash label describing the UEFI variable store.
> +*/
> +#define LABEL_UEFI_VAR_STORE    "System-firmware"
> +
> +STATIC NOR_FLASH_DESCRIPTION  mNorFlashDevices[MAX_FLASH_DEVICES];
> +STATIC UINTN                  mNorFlashDeviceCount = 0;
> +STATIC INT32                  mUefiVarStoreNode = MAX_INT32;
> +STATIC FDT_CLIENT_PROTOCOL    *mFdtClient;
> +
> +/** This function performs platform specific actions to initialise
> +    the NOR flash, if required.
> +
> +  @retval EFI_SUCCESS           Success.
> +**/
> +EFI_STATUS
> +NorFlashPlatformInitialization (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                  Status;
> +
> +  DEBUG ((DEBUG_INFO, "NorFlashPlatformInitialization\n"));
> +
> +  if ((mNorFlashDeviceCount > 0) && (mUefiVarStoreNode != MAX_INT32)) {
> +    //
> +    // UEFI takes ownership of the cfi-flash hardware, and exposes its
> +    // functionality through the UEFI Runtime Variable Service. This means we
> +    // need to disable it in the device tree to prevent the OS from attaching
> +    // its device driver as well.
> +    // Note: This library is loaded by the FaultTolerantWriteDxe to setup the
> +    // Ftw PCDs and later by the NorFlashDxe to provide the NorFlashPlatformLib
> +    // interfaces. Therefore the FDT node used for UEFI storage variable is
> +    // disabled here.
> +    //
> +    Status = mFdtClient->SetNodeProperty (
> +                           mFdtClient,
> +                           mUefiVarStoreNode,
> +                           "status",
> +                           "disabled",
> +                           sizeof ("disabled")
> +                           );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN, "Failed to set cfi-flash status to 'disabled'\n"));
> +    }
> +  } else {
> +    Status = EFI_NOT_FOUND;
> +    DEBUG ((DEBUG_ERROR, "Flash device for UEFI variable storage not found\n"));
> +  }
> +
> +  return Status;
> +}
> +
> +/** 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
> +  )
> +{
> +  INT32                       Node;
> +  EFI_STATUS                  Status;
> +  EFI_STATUS                  FindNodeStatus;
> +  CONST UINT32                *Reg;
> +  UINT32                      PropSize;
> +  UINT64                      Base;
> +  UINT64                      Size;
> +  UINTN                       UefiVarStoreIndex;
> +  CONST CHAR8                 *Label;
> +  UINT32                      LabelLen;
> +
> +  if (mNorFlashDeviceCount != 0) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  Status = gBS->LocateProtocol (
> +                  &gFdtClientProtocolGuid,
> +                  NULL,
> +                  (VOID **)&mFdtClient
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  UefiVarStoreIndex = MAX_UINTN;
> +  for (FindNodeStatus = mFdtClient->FindCompatibleNode (
> +                                      mFdtClient,
> +                                      "cfi-flash",
> +                                      &Node
> +                                      );
> +       !EFI_ERROR (FindNodeStatus) &&
> +         (mNorFlashDeviceCount < MAX_FLASH_DEVICES);
> +       FindNodeStatus = mFdtClient->FindNextCompatibleNode (
> +                                      mFdtClient,
> +                                      "cfi-flash",
> +                                      Node,
> +                                      &Node
> +    )) {
> +    Status = mFdtClient->GetNodeProperty (
> +                           mFdtClient,
> +                           Node,
> +                           "label",
> +                           (CONST VOID **)&Label,
> +                           &LabelLen
> +                           );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: GetNodeProperty ('label') failed (Status == %r)\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +    } else if (AsciiStrCmp (Label, LABEL_UEFI_VAR_STORE) == 0) {
> +      UefiVarStoreIndex = mNorFlashDeviceCount;
> +      mUefiVarStoreNode = Node;
> +    }
> +
> +    Status = mFdtClient->GetNodeProperty (
> +                           mFdtClient,
> +                           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_DEVICES)) {
> +      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 = KVMTOOL_NOR_BLOCK_SIZE;
> +      mNorFlashDeviceCount++;
> +    }
> +  } // for
> +
> +  // Setup the variable store in the last device
> +  if (mNorFlashDeviceCount > 0) {
> +    if (UefiVarStoreIndex == MAX_UINTN) {
> +      // We did not find a label matching the UEFI Variable store. Default to
> +      // using the last cfi-flash device as the variable store.
> +      UefiVarStoreIndex = mNorFlashDeviceCount - 1;
> +      mUefiVarStoreNode = Node;
> +    }
> +    if (mNorFlashDevices[UefiVarStoreIndex].DeviceBaseAddress != 0) {
> +      return SetupVariableStore (&mNorFlashDevices[UefiVarStoreIndex]);
> +    }
> +  }
> +
> +  return EFI_NOT_FOUND;
> +}
> +
> diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
> new file mode 100644
> index 0000000000000000000000000000000000000000..a3230e4b2be668904322103825b93e867503984e
> --- /dev/null
> +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf
> @@ -0,0 +1,49 @@
> +#/** @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
> +
>