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: 12922 bytes --]

Hi Ard,


Thanks,
Chao
On 2024/4/26 09:20, Chao Li wrote:
>
> Hi Ard,
>
> 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)".
>
I will save the real HOB data in the next version.
>
>>> +    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 (#118309): https://edk2.groups.io/g/devel/message/118309
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: 15032 bytes --]