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

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 12402 bytes --]

Hi Ard,


Thanks,
Chao
On 2024/4/25 21:02, Ard Biesheuvel wrote:
> 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?

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 14340 bytes --]