Re: [edk2-devel] [PATCH v3 2/7] OvmfPkg: Add the way of HOBs in QemuFwCfgLibMmio

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 25 Apr 2024 at 14:13, Chao Li <lichao@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+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Signed-off-by: Chao Li <lichao@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?

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

resource

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

> +  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 (#118300): https://edk2.groups.io/g/devel/message/118300
Mute This Topic: https://groups.io/mt/105728768/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-