Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH 25/29] OvmfPkg: Add NorFlashQemuLib library

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

Hi Ard,

Please see my query inline marked [SAMI].

Regards,

Sami Mujawar

On 19/10/2022 01:19 pm, Ard Biesheuvel via groups.io wrote:
> On Mon, 10 Oct 2022 at 12:13, Sunil V L <sunilvl@ventanamicro.com> wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4076
>>
>> This is copied from ArmVirtPkg since it is required for
>> other architectures also.
>>
>> It also adds the instance for single flash drive which has
>> both code and variables. This is copied from SbsaQemu.
>>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Daniel Schaefer <git@danielschaefer.me>
>> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Let's call these
>
> QemuNorFlashPlatformLib [for the library class]
>
> QemuNorFlashDeviceTreeLib
> QemuNorFlashStaticLib
>
> and for the driver
>
> QemuNorFlashDxe

[SAMI] We use the NorFlashDxe for the Kvmtool guest firmware, see 
https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/ArmVirtKvmTool.dsc#L294

Considering this, should QemuNorFlashDxe be called OvmfNorFlashDxe?

[/SAMI]

>
>
> i sent out some patches for edk2-platforms to eliminate the dependency
> on ArmPlatformPkg's NorFlashDxe and NorFlashPlatformLib definitions.
> Once we move everything in OvmfPkg over to these ones, we can drop the
> old one altogether.
>
>
>
>> ---
>>   OvmfPkg/OvmfPkg.dec                           |   8 ++
>>   .../NorFlashQemuLib/NorFlashQemuLib.inf       |  40 ++++++
>>   .../NorFlashQemuUnifiedLib.inf                |  30 ++++
>>   OvmfPkg/Include/Library/NorFlashPlatformLib.h |  30 ++++
>>   .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 136 ++++++++++++++++++
>>   .../NorFlashQemuLib/NorFlashQemuUnifiedLib.c  |  40 ++++++
>>   6 files changed, 284 insertions(+)
>>   create mode 100644 OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
>>   create mode 100644 OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.inf
>>   create mode 100644 OvmfPkg/Include/Library/NorFlashPlatformLib.h
>>   create mode 100644 OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
>>   create mode 100644 OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.c
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 7d2acc5ea0e0..0697c91c6836 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -129,6 +129,10 @@ [LibraryClasses]
>>     #
>>     HardwareInfoLib|Include/Library/HardwareInfoLib.h
>>
>> +  ##  @libraryclass  NorFlashQemuLib
>> +  #
>> +  NorFlashPlatformLib|Include/Library/NorFlashPlatformLib.h
>> +
>>   [Guids]
>>     gUefiOvmfPkgTokenSpaceGuid            = {0x93bb96af, 0xb9f2, 0x4eb8, {0x94, 0x62, 0xe0, 0xba, 0x74, 0x56, 0x42, 0x36}}
>>     gEfiXenInfoGuid                       = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
>> @@ -405,6 +409,10 @@ [PcdsFixedAtBuild]
>>     #  check to decide whether to abort dispatch of the driver it is linked into.
>>     gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|""|VOID*|0x68
>>
>> +  ## The base address and size of the FVMAIN
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFvBaseAddress|0|UINT64|0x71
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFvSize|0|UINT32|0x72
>> +
>>   [PcdsDynamic, PcdsDynamicEx]
>>     gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>> diff --git a/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
>> new file mode 100644
>> index 000000000000..ecd8059b3508
>> --- /dev/null
>> +++ b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
>> @@ -0,0 +1,40 @@
>> +#/** @file
>> +#
>> +#  Component description file for NorFlashQemuLib module
>> +#
>> +#  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
>> +#
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +#**/
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = NorFlashQemuLib
>> +  FILE_GUID                      = 42C30D8E-BFAD-4E77-9041-E7DAAE88DF7A
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = NorFlashPlatformLib
>> +
>> +[Sources.common]
>> +  NorFlashQemuLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  DebugLib
>> +  UefiBootServicesTableLib
>> +
>> +[Protocols]
>> +  gFdtClientProtocolGuid          ## CONSUMES
>> +
>> +[Depex]
>> +  gFdtClientProtocolGuid
>> +
>> +[Pcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFvBaseAddress
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFvSize
>> diff --git a/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.inf b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.inf
>> new file mode 100644
>> index 000000000000..91d1406fc3e7
>> --- /dev/null
>> +++ b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.inf
>> @@ -0,0 +1,30 @@
>> +#/** @file
>> +#
>> +#  Component description file for NorFlashQemuLib module
>> +#
>> +#  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
>> +#
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +#**/
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = NorFlashQemuUnifiedLib
>> +  FILE_GUID                      = 064742F1-E531-4D7D-A154-22315889CC23
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = NorFlashPlatformLib
>> +
>> +[Sources.common]
>> +  NorFlashQemuUnifiedLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[Pcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>> diff --git a/OvmfPkg/Include/Library/NorFlashPlatformLib.h b/OvmfPkg/Include/Library/NorFlashPlatformLib.h
>> new file mode 100644
>> index 000000000000..6ef5b70e9948
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Library/NorFlashPlatformLib.h
>> @@ -0,0 +1,30 @@
>> +/** @file
>> +
>> + Copyright (c) 2011-2012, ARM Ltd. All rights reserved.<BR>
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> + **/
>> +
>> +#ifndef _NORFLASHPLATFORMLIB_H_
>> +#define _NORFLASHPLATFORMLIB_H_
>> +
>> +typedef struct {
>> +  UINTN    DeviceBaseAddress;       // Start address of the Device Base Address (DBA)
>> +  UINTN    RegionBaseAddress;       // Start address of one single region
>> +  UINTN    Size;
>> +  UINTN    BlockSize;
>> +} NOR_FLASH_DESCRIPTION;
>> +
>> +EFI_STATUS
>> +NorFlashPlatformInitialization (
>> +  VOID
>> +  );
>> +
>> +EFI_STATUS
>> +NorFlashPlatformGetDevices (
>> +  OUT NOR_FLASH_DESCRIPTION  **NorFlashDescriptions,
>> +  OUT UINT32                 *Count
>> +  );
>> +
>> +#endif /* _NORFLASHPLATFORMLIB_H_ */
>> diff --git a/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
>> new file mode 100644
>> index 000000000000..3632fa9e7a98
>> --- /dev/null
>> +++ b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
>> @@ -0,0 +1,136 @@
>> +/** @file
>> +
>> + Copyright (c) 2014-2018, Linaro 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>
>> +
>> +#define QEMU_NOR_BLOCK_SIZE  SIZE_256KB
>> +
>> +#define MAX_FLASH_BANKS  4
>> +
>> +EFI_STATUS
>> +NorFlashPlatformInitialization (
>> +  VOID
>> +  )
>> +{
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +NOR_FLASH_DESCRIPTION  mNorFlashDevices[MAX_FLASH_BANKS];
>> +
>> +EFI_STATUS
>> +NorFlashPlatformGetDevices (
>> +  OUT NOR_FLASH_DESCRIPTION  **NorFlashDescriptions,
>> +  OUT UINT32                 *Count
>> +  )
>> +{
>> +  FDT_CLIENT_PROTOCOL  *FdtClient;
>> +  INT32                Node;
>> +  EFI_STATUS           Status;
>> +  EFI_STATUS           FindNodeStatus;
>> +  CONST UINT32         *Reg;
>> +  UINT32               PropSize;
>> +  UINT32               Num;
>> +  UINT64               Base;
>> +  UINT64               Size;
>> +
>> +  Status = gBS->LocateProtocol (
>> +                  &gFdtClientProtocolGuid,
>> +                  NULL,
>> +                  (VOID **)&FdtClient
>> +                  );
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  Num = 0;
>> +  for (FindNodeStatus = FdtClient->FindCompatibleNode (
>> +                                     FdtClient,
>> +                                     "cfi-flash",
>> +                                     &Node
>> +                                     );
>> +       !EFI_ERROR (FindNodeStatus) && Num < 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)) && Num < 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 (PcdOvmfFvBaseAddress) + PcdGet32 (PcdOvmfFvSize) > Base) &&
>> +          ((Base + Size) > PcdGet64 (PcdOvmfFvBaseAddress)))
>> +      {
>> +        continue;
>> +      }
>> +
>> +      mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
>> +      mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
>> +      mNorFlashDevices[Num].Size              = (UINTN)Size;
>> +      mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
>> +      Num++;
>> +    }
>> +
>> +    //
>> +    // UEFI takes ownership of the NOR flash, and exposes its functionality
>> +    // through the UEFI Runtime Services GetVariable, SetVariable, etc. This
>> +    // means we need to disable it in the device tree to prevent the OS from
>> +    // attaching its device driver as well.
>> +    // Note that this also hides other flash banks, but the only other flash
>> +    // bank we expect to encounter is the one that carries the UEFI executable
>> +    // code, which is not intended to be guest updatable, and is usually backed
>> +    // in a readonly manner by QEMU anyway.
>> +    //
>> +    Status = FdtClient->SetNodeProperty (
>> +                          FdtClient,
>> +                          Node,
>> +                          "status",
>> +                          "disabled",
>> +                          sizeof ("disabled")
>> +                          );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN, "Failed to set NOR flash status to 'disabled'\n"));
>> +    }
>> +  }
>> +
>> +  *NorFlashDescriptions = mNorFlashDevices;
>> +  *Count                = Num;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.c b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.c
>> new file mode 100644
>> index 000000000000..1420fb5b596c
>> --- /dev/null
>> +++ b/OvmfPkg/Library/NorFlashQemuLib/NorFlashQemuUnifiedLib.c
>> @@ -0,0 +1,40 @@
>> +/** @file
>> +
>> + Copyright (c) 2019, Linaro Ltd. All rights reserved
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> + **/
>> +
>> +#include <Base.h>
>> +#include <PiDxe.h>
>> +#include <Library/NorFlashPlatformLib.h>
>> +
>> +#define QEMU_NOR_BLOCK_SIZE  SIZE_256KB
>> +
>> +EFI_STATUS
>> +NorFlashPlatformInitialization (
>> +  VOID
>> +  )
>> +{
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +NOR_FLASH_DESCRIPTION  mNorFlashDevice =
>> +{
>> +  FixedPcdGet32 (PcdOvmfFdBaseAddress),
>> +  FixedPcdGet64 (PcdFlashNvStorageVariableBase),
>> +  FixedPcdGet32 (PcdOvmfFirmwareFdSize),
>> +  QEMU_NOR_BLOCK_SIZE
>> +};
>> +
>> +EFI_STATUS
>> +NorFlashPlatformGetDevices (
>> +  OUT NOR_FLASH_DESCRIPTION  **NorFlashDescriptions,
>> +  OUT UINT32                 *Count
>> +  )
>> +{
>> +  *NorFlashDescriptions = &mNorFlashDevice;
>> +  *Count                = 1;
>> +  return EFI_SUCCESS;
>> +}
>> --
>> 2.25.1
>>
>
> 
>
>