Hi Ard,

Thanks,
Chao
On 2024/4/25 21:02, Ard Biesheuvel wrote:
On Thu, 25 Apr 2024 at 14:13, Chao Li<lic...@loongson.cn>  wrote:
Added the HOB methods to load and store the QEMU firmware configure
address, data address and DMA address, which are not enabled during the
DXE stage.

Build-tested only (with "ArmVirtQemu.dsc and RiscVVirtQemu.dsc").

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4755

Cc: Ard Biesheuvel<ardb+tianoc...@kernel.org>
Cc: Jiewen Yao<jiewen....@intel.com>
Cc: Gerd Hoffmann<kra...@redhat.com>
Cc: Leif Lindholm<quic_llind...@quicinc.com>
Cc: Sami Mujawar<sami.muja...@arm.com>
Cc: Sunil V L<suni...@ventanamicro.com>
Cc: Andrei Warkentin<andrei.warken...@intel.com>
Signed-off-by: Chao Li<lic...@loongson.cn>
---
  .../Library/QemuFwCfgLib/QemuFwCfgLibMmio.c   | 81 +++++++++++++++--
  .../Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf |  1 +
  .../QemuFwCfgLib/QemuFwCfgLibMmioInternal.h   | 74 +++++++++++++++
  .../Library/QemuFwCfgLib/QemuFwCfgMmioDxe.c   | 91 ++++++++++++++++---
  4 files changed, 226 insertions(+), 21 deletions(-)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.c 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.c
index dc949c8e26..b5dbc5e4b5 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.c
@@ -8,11 +8,16 @@
    SPDX-License-Identifier: BSD-2-Clause-Patent
  **/

+#include <Base.h>
  #include <Uefi.h>

+#include <Pi/PiBootMode.h>
+#include <Pi/PiHob.h>
+
  #include <Library/BaseLib.h>
  #include <Library/BaseMemoryLib.h>
  #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
  #include <Library/IoLib.h>
  #include <Library/QemuFwCfgLib.h>
  #include <Library/UefiBootServicesTableLib.h>
@@ -21,6 +26,62 @@

  #include "QemuFwCfgLibMmioInternal.h"

+EFI_GUID  mFwCfgResourceGuid = FW_CONFIG_RESOURCE_HOB_GUID;
+
+/**
+  Build firmware configure resource address HOB.
+
+  @param[in]   FwCfgResource  A pointer to firmware configure resource.
+
+  @retval  NULL
+**/
+VOID
+QemuBuildFwCfgResourceHob (
+  IN QEMU_FW_CFG_RESOURCE  *FwCfgResource
+  )
+{
+  UINT64  Data64;
+
+  Data64 = (UINT64)(UINTN)FwCfgResource;
+
+  BuildGuidDataHob (
+    &mFwCfgResourceGuid,
+    (VOID *)&Data64,
This looks wrong: why are you taking the address of the stack variable
rather than the address of the resource descriptor?

It only saves the pointer of FwCfgResource, and the memory space has been created in the PEI constructor.  Do you mean saving all contents of FwCfgResource in the HOB?

The following line is indeed wrong. if only save the pointer, the size should be "sizeof (UINT64)".


+    sizeof (QEMU_FW_CFG_RESOURCE)
+    );
+}
+
+/**
+  Get firmware configure resource in HOB.
+
+  @param VOID
+
+  @retval  FwCfgResource    The firmware configure resouce in HOB.
resource
All right.

+           NULL             The firmware configure resouce not found.
+**/
+QEMU_FW_CFG_RESOURCE *
+QemuGetFwCfgResourceHob (
+  VOID
+  )
+{
+  EFI_HOB_GUID_TYPE     *GuidHob;
+  VOID                  *DataInHob;
+  QEMU_FW_CFG_RESOURCE  *FwCfgResource;
+
+  GuidHob   = NULL;
+  DataInHob = NULL;
+
+  GuidHob = GetFirstGuidHob (&mFwCfgResourceGuid);
Please define this GUID in the package .DEC file and add it to the
[Guids] section in the .INF so that you can refer to its name
directly.
OK.

+  if (GuidHob == NULL) {
+    return NULL;
+  }
+
+  DataInHob     = GET_GUID_HOB_DATA (GuidHob);
+  FwCfgResource = (QEMU_FW_CFG_RESOURCE *)(*(UINTN *)DataInHob);
+
+  return FwCfgResource;
+}
+
  /**
    Returns a boolean indicating if the firmware configuration interface
    is available or not.
@@ -37,7 +98,7 @@ QemuFwCfgIsAvailable (
    VOID
    )
  {
-  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
+  return (BOOLEAN)(QemuGetFwCfgSelectorAddress () != 0 && 
QemuGetFwCfgDataAddress () != 0);
  }

  /**
@@ -56,7 +117,7 @@ QemuFwCfgSelectItem (
    )
  {
    if (QemuFwCfgIsAvailable ()) {
-    MmioWrite16 (mFwCfgSelectorAddress, SwapBytes16 ((UINT16)QemuFwCfgItem));
+    MmioWrite16 (QemuGetFwCfgSelectorAddress (), SwapBytes16 
((UINT16)QemuFwCfgItem));
    }
  }

@@ -86,30 +147,30 @@ MmioReadBytes (

   #if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_RISCV64) || defined 
(MDE_CPU_LOONGARCH64)
    while (Ptr < End) {
-    *(UINT64 *)Ptr = MmioRead64 (mFwCfgDataAddress);
+    *(UINT64 *)Ptr = MmioRead64 (QemuGetFwCfgDataAddress ());
      Ptr           += 8;
    }

    if (Left & 4) {
-    *(UINT32 *)Ptr = MmioRead32 (mFwCfgDataAddress);
+    *(UINT32 *)Ptr = MmioRead32 (QemuGetFwCfgDataAddress ());
      Ptr           += 4;
    }

   #else
    while (Ptr < End) {
-    *(UINT32 *)Ptr = MmioRead32 (mFwCfgDataAddress);
+    *(UINT32 *)Ptr = MmioRead32 (QemuGetFwCfgDataAddress ());
      Ptr           += 4;
    }

   #endif

    if (Left & 2) {
-    *(UINT16 *)Ptr = MmioRead16 (mFwCfgDataAddress);
+    *(UINT16 *)Ptr = MmioRead16 (QemuGetFwCfgDataAddress ());
      Ptr           += 2;
    }

    if (Left & 1) {
-    *Ptr = MmioRead8 (mFwCfgDataAddress);
+    *Ptr = MmioRead8 (QemuGetFwCfgDataAddress ());
    }
  }

@@ -162,9 +223,9 @@ DmaTransferBytes (
    // This will fire off the transfer.
    //
   #if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_RISCV64) || defined 
(MDE_CPU_LOONGARCH64)
-  MmioWrite64 (mFwCfgDmaAddress, SwapBytes64 ((UINT64)&Access));
+  MmioWrite64 (QemuGetFwCfgDmaAddress (), SwapBytes64 ((UINT64)&Access));
   #else
-  MmioWrite32 ((UINT32)(mFwCfgDmaAddress + 4), SwapBytes32 ((UINT32)&Access));
+  MmioWrite32 ((UINT32)(QemuGetFwCfgDmaAddress () + 4), SwapBytes32 
((UINT32)&Access));
   #endif

    //
@@ -233,7 +294,7 @@ MmioWriteBytes (
    UINTN  Idx;

    for (Idx = 0; Idx < Size; ++Idx) {
-    MmioWrite8 (mFwCfgDataAddress, ((UINT8 *)Buffer)[Idx]);
+    MmioWrite8 (QemuGetFwCfgDataAddress (), ((UINT8 *)Buffer)[Idx]);
    }
  }

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf
index f2596f270e..8e191f2d22 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf
@@ -40,6 +40,7 @@ [LibraryClasses]
    BaseLib
    BaseMemoryLib
    DebugLib
+  HobLib
    IoLib
    UefiBootServicesTableLib

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmioInternal.h 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmioInternal.h
index d7d645f700..6101e15b21 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmioInternal.h
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmioInternal.h
@@ -16,6 +16,17 @@ extern UINTN  mFwCfgSelectorAddress;
  extern UINTN  mFwCfgDataAddress;
  extern UINTN  mFwCfgDmaAddress;

+#define FW_CONFIG_RESOURCE_HOB_GUID \
+  { \
+    0x3cc47b04, 0x0d3e, 0xaa64, { 0x06, 0xa6, 0x4b, 0xdc, 0x9a, 0x2c, 0x61, 
0x19 } \
+  }
+
+typedef struct {
+  UINTN  FwCfgSelectorAddress;
+  UINTN  FwCfgDataAddress;
+  UINTN  FwCfgDmaAddress;
+} QEMU_FW_CFG_RESOURCE;
+
  /**
    Reads firmware configuration bytes into a buffer

@@ -96,6 +107,69 @@ EFIAPI
    IN UINTN  Size
    );

+/**
+  Build firmware configure resource HOB.
+
+  @param[in]   FwCfgResource  A pointer to firmware configure resource.
+
+  @retval  NULL
+**/
+VOID
+QemuBuildFwCfgResourceHob (
+  IN QEMU_FW_CFG_RESOURCE  *FwCfgResource
+  );
+
+/**
+  Get firmware configure resource HOB.
+
+  @param VOID
+
+  @retval  FwCfgResource    The firmware configure resouce in HOB.
+**/
+QEMU_FW_CFG_RESOURCE *
+QemuGetFwCfgResourceHob (
+  VOID
+  );
+
+/**
+  To get firmware configure selector address.
+
+  @param VOID
+
+  @retval  firmware configure selector address
+**/
+UINTN
+EFIAPI
+QemuGetFwCfgSelectorAddress (
+  VOID
+  );
+
+/**
+  To get firmware configure Data address.
+
+  @param VOID
+
+  @retval  firmware configure data address
+**/
+UINTN
+EFIAPI
+QemuGetFwCfgDataAddress (
+  VOID
+  );
+
+/**
+  To get firmware DMA address.
+
+  @param VOID
+
+  @retval  firmware DMA address
+**/
+UINTN
+EFIAPI
+QemuGetFwCfgDmaAddress (
+  VOID
+  );
+
  /**
    Slow READ_BYTES_FUNCTION.
  **/
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioDxe.c 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioDxe.c
index 4844a42a36..2d5055a76e 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioDxe.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioDxe.c
@@ -32,23 +32,92 @@ READ_BYTES_FUNCTION   *InternalQemuFwCfgReadBytes  = 
MmioReadBytes;
  WRITE_BYTES_FUNCTION  *InternalQemuFwCfgWriteBytes = MmioWriteBytes;
  SKIP_BYTES_FUNCTION   *InternalQemuFwCfgSkipBytes  = MmioSkipBytes;

+/**
+  To get firmware configure selector address.
+
+  @param VOID
+
+  @retval  firmware configure selector address
+**/
+UINTN
+EFIAPI
+QemuGetFwCfgSelectorAddress (
+  VOID
+  )
+{
+  return mFwCfgSelectorAddress;
+}
+
+/**
+  To get firmware configure Data address.
+
+  @param VOID
+
+  @retval  firmware configure data address
+**/
+UINTN
+EFIAPI
+QemuGetFwCfgDataAddress (
+  VOID
+  )
+{
+  return mFwCfgDataAddress;
+}
+
+/**
+  To get firmware DMA address.
+
+  @param VOID
+
+  @retval  firmware DMA address
+**/
+UINTN
+EFIAPI
+QemuGetFwCfgDmaAddress (
+  VOID
+  )
+{
+  return mFwCfgDmaAddress;
+}
+
+
  RETURN_STATUS
  EFIAPI
  QemuFwCfgInitialize (
    VOID
    )
  {
-  EFI_STATUS           Status;
-  FDT_CLIENT_PROTOCOL  *FdtClient;
-  CONST UINT64         *Reg;
-  UINT32               RegSize;
-  UINTN                AddressCells, SizeCells;
-  UINT64               FwCfgSelectorAddress;
-  UINT64               FwCfgSelectorSize;
-  UINT64               FwCfgDataAddress;
-  UINT64               FwCfgDataSize;
-  UINT64               FwCfgDmaAddress;
-  UINT64               FwCfgDmaSize;
+  EFI_STATUS            Status;
+  FDT_CLIENT_PROTOCOL   *FdtClient;
+  CONST UINT64          *Reg;
+  UINT32                RegSize;
+  UINTN                 AddressCells, SizeCells;
+  UINT64                FwCfgSelectorAddress;
+  UINT64                FwCfgSelectorSize;
+  UINT64                FwCfgDataAddress;
+  UINT64                FwCfgDataSize;
+  UINT64                FwCfgDmaAddress;
+  UINT64                FwCfgDmaSize;
+  QEMU_FW_CFG_RESOURCE  *FwCfgResource;
+
+  //
+  // Check whether the Qemu firmware configure resources HOB has been created,
+  // if so use the resources in the HOB.
+  //
+  FwCfgResource = QemuGetFwCfgResourceHob ();
+  if (FwCfgResource != NULL) {
+    mFwCfgSelectorAddress = FwCfgResource->FwCfgSelectorAddress;
+    mFwCfgDataAddress     = FwCfgResource->FwCfgDataAddress;
+    mFwCfgDmaAddress      = FwCfgResource->FwCfgDmaAddress;
+
+    if (mFwCfgDmaAddress != 0) {
+      InternalQemuFwCfgReadBytes  = DmaReadBytes;
+      InternalQemuFwCfgWriteBytes = DmaWriteBytes;
+      InternalQemuFwCfgSkipBytes  = DmaSkipBytes;
+    }
+
+    return RETURN_SUCCESS;
+  }

    Status = gBS->LocateProtocol (
                    &gFdtClientProtocolGuid,
--
2.27.0











-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118307): https://edk2.groups.io/g/devel/message/118307
Mute This Topic: https://groups.io/mt/105728768/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to