Re: [edk2-devel] [PATCH v4 4/8] OvmfPkg: Add the QemuFwCfgMmioLib PEI stage version

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

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

Hi Ard,

OK, I will submit the V5 today and make the adjustments according to 
your suggestions.


Thanks,
Chao
On 2024/4/29 21:11, Ard Biesheuvel wrote:
> On Fri, 26 Apr 2024 at 10:29, Chao Li<lichao@loongson.cn>  wrote:
>> Added the PEI stage library for QemuFwCfgMmioLib, which uses the FDT to
>> find the fw_cfg and parse it.
>>
>> 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>
>> Co-authored-by: Xianglai Li<lixianglai@loongson.cn>
>> Signed-off-by: Chao Li<lichao@loongson.cn>
>> ---
>>   .../Library/QemuFwCfgLib/QemuFwCfgMmioPei.c   | 235 ++++++++++++++++++
>>   .../QemuFwCfgLib/QemuFwCfgMmioPeiLib.inf      |  52 ++++
>>   2 files changed, 287 insertions(+)
>>   create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPei.c
>>   create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPeiLib.inf
>>
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPei.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPei.c
>> new file mode 100644
>> index 0000000000..055148de8e
>> --- /dev/null
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPei.c
>> @@ -0,0 +1,235 @@
>> +/** @file
>> +
>> +  Stateful and implicitly initialized fw_cfg library implementation.
>> +
>> +  Copyright (C) 2013 - 2014, Red Hat, Inc.
>> +  Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.<BR>
>> +  (C) Copyright 2021 Hewlett Packard Enterprise Development LP<BR>
>> +  Copyright (c) 2024 Loongson Technology Corporation Limited. All rights reserved.<BR>
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#include <Uefi.h>
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/QemuFwCfgLib.h>
>> +
>> +#include <libfdt.h>
>> +
>> +#include "QemuFwCfgLibMmioInternal.h"
>> +
>> +/**
>> +  To get firmware configure selector address.
>> +
>> +  @param VOID
>> +
>> +  @retval  firmware configure selector address
>> +**/
>> +UINTN
>> +EFIAPI
>> +QemuGetFwCfgSelectorAddress (
>> +  VOID
>> +  )
>> +{
>> +  QEMU_FW_CFG_RESOURCE  *FwCfgResource;
>> +
>> +  FwCfgResource = QemuGetFwCfgResourceHob ();
>> +  ASSERT (FwCfgResource != NULL);
>> +
>> +  return FwCfgResource->FwCfgSelectorAddress;
>> +}
>> +
>> +/**
>> +  To get firmware configure Data address.
>> +
>> +  @param VOID
>> +
>> +  @retval  firmware configure data address
>> +**/
>> +UINTN
>> +EFIAPI
>> +QemuGetFwCfgDataAddress (
>> +  VOID
>> +  )
>> +{
>> +  QEMU_FW_CFG_RESOURCE  *FwCfgResource;
>> +
>> +  FwCfgResource = QemuGetFwCfgResourceHob ();
>> +  ASSERT (FwCfgResource != NULL);
>> +
>> +  return FwCfgResource->FwCfgDataAddress;
>> +}
>> +
>> +/**
>> +  To get firmware DMA address.
>> +
>> +  @param VOID
>> +
>> +  @retval  firmware DMA address
>> +**/
>> +UINTN
>> +EFIAPI
>> +QemuGetFwCfgDmaAddress (
>> +  VOID
>> +  )
>> +{
>> +  QEMU_FW_CFG_RESOURCE  *FwCfgResource;
>> +
>> +  FwCfgResource = QemuGetFwCfgResourceHob ();
>> +  ASSERT (FwCfgResource != NULL);
>> +
>> +  return FwCfgResource->FwCfgDmaAddress;
>> +}
>> +
>> +RETURN_STATUS
>> +EFIAPI
>> +QemuFwCfgInitialize (
>> +  VOID
>> +  )
>> +{
>> +  VOID                  *DeviceTreeBase;
>> +  INT32                 Node;
>> +  INT32                 Prev;
>> +  CONST CHAR8           *Type;
>> +  INT32                 Len;
>> +  CONST UINT64          *Reg;
>> +  UINT64                FwCfgSelectorAddress;
>> +  UINT64                FwCfgSelectorSize;
>> +  UINT64                FwCfgDataAddress;
>> +  UINT64                FwCfgDataSize;
>> +  UINT64                FwCfgDmaAddress;
>> +  UINT64                FwCfgDmaSize;
>> +  QEMU_FW_CFG_RESOURCE  *FwCfgResource;
>> +  VOID                  *Buffer;
>> +
>> +  //
>> +  // Check whether the Qemu firmware configure resources HOB has been created,
>> +  // if so use the resources in the HOB.
>> +  //
>> +  FwCfgResource = QemuGetFwCfgResourceHob ();
>> +  if (FwCfgResource != NULL) {
>> +    return RETURN_SUCCESS;
>> +  }
>> +
>> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
>> +  ASSERT (DeviceTreeBase != NULL);
>> +  //
>> +  // Make sure we have a valid device tree blob
>> +  //
>> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
>> +
>> +  //
>> +  // Create resouce memory
>> +  //
>> +  Buffer = AllocatePages(EFI_SIZE_TO_PAGES (sizeof (QEMU_FW_CFG_RESOURCE)));
>> +  ASSERT (Buffer != NULL);
>> +  ZeroMem (Buffer, sizeof (QEMU_FW_CFG_RESOURCE));
>> +
>> +  FwCfgResource = (QEMU_FW_CFG_RESOURCE *)Buffer;
>> +
> You will need to respin after all, so please incorporate the fixes I
> proposed on v4
>
>> +  for (Prev = 0; ; Prev = Node) {
>> +    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
>> +    if (Node < 0) {
>> +      break;
>> +    }
>> +
>> +    //
>> +    // Check for memory node
>> +    //
>> +    Type = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
>> +    if ((Type) &&
> and here
>
>> +        (AsciiStrnCmp (Type, "qemu,fw-cfg-mmio", Len) == 0))
>> +    {
>> +      //
>> +      // Get the 'reg' property of this node. For now, we will assume
>> +      // two 8 byte quantities for base and size, respectively.
>> +      //
>> +      Reg = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>> +      if ((Reg != 0) && (Len == (2 * sizeof (UINT64)))) {
>> +        FwCfgDataAddress     = SwapBytes64 (Reg[0]);
>> +        FwCfgDataSize        = 8;
>> +        FwCfgSelectorAddress = FwCfgDataAddress + FwCfgDataSize;
>> +        FwCfgSelectorSize    = 2;
>> +
>> +        //
>> +        // The following ASSERT()s express
>> +        //
>> +        //   Address + Size - 1 <= MAX_UINTN
>> +        //
>> +        // for both registers, that is, that the last byte in each MMIO range is
>> +        // expressible as a MAX_UINTN. The form below is mathematically
>> +        // equivalent, and it also prevents any unsigned overflow before the
>> +        // comparison.
>> +        //
>> +        ASSERT (FwCfgSelectorAddress <= MAX_UINTN - FwCfgSelectorSize + 1);
>> +        ASSERT (FwCfgDataAddress     <= MAX_UINTN - FwCfgDataSize     + 1);
>> +
>> +        FwCfgResource->FwCfgSelectorAddress = FwCfgSelectorAddress;
>> +        FwCfgResource->FwCfgDataAddress     = FwCfgDataAddress;
>> +
>> +        DEBUG ((
>> +          DEBUG_INFO,
>> +          "Found FwCfg @ 0x%Lx/0x%Lx\n",
>> +          FwCfgSelectorAddress,
>> +          FwCfgDataAddress
>> +          ));
>> +
>> +        if (SwapBytes64 (Reg[1]) >= 0x18) {
>> +          FwCfgDmaAddress = FwCfgDataAddress + 0x10;
>> +          FwCfgDmaSize    = 0x08;
>> +
>> +          //
>> +          // See explanation above.
>> +          //
>> +          ASSERT (FwCfgDmaAddress <= MAX_UINTN - FwCfgDmaSize + 1);
>> +
>> +          DEBUG ((DEBUG_INFO, "Found FwCfg DMA @ 0x%Lx\n", FwCfgDmaAddress));
>> +          FwCfgResource->FwCfgDmaAddress  = FwCfgDmaAddress;
>> +        } else {
>> +          FwCfgDmaAddress = 0;
>> +        }
>> +
>> +        if ((FwCfgSelectorAddress != 0) && (FwCfgDataAddress != 0)) {
>> +          UINT32  Signature;
>> +
> Please move this declaration to the function scope.
OK
>
>> +          //
>> +          // Select Item Signature
>> +          //
>> +          MmioWrite16 (FwCfgSelectorAddress, SwapBytes16 ((UINT16)QemuFwCfgItemSignature));
>> +
>> +          //
>> +          // Readout the Signature.
>> +          //
>> +          Signature = MmioRead32 (FwCfgDataAddress);
>> +
>> +          if (Signature != SIGNATURE_32 ('Q', 'E', 'M', 'U')) {
>> +            FwCfgResource->FwCfgDataAddress     = 0;
>> +            FwCfgResource->FwCfgSelectorAddress = 0;
>> +            FwCfgResource->FwCfgDmaAddress      = 0;
>> +            QemuBuildFwCfgResourceHob (FwCfgResource);
>> +          }
>> +
>> +          //
>> +          // Build the firmware configure resource HOB.
>> +          //
>> +          QemuBuildFwCfgResourceHob (FwCfgResource);
> This logic does not look right at all. What is the intent here? Should
> the HOB only be built if the signature check passes?
I think it should check passes and then built the HOB, after all this is 
a QEMU lib, if you don't think so, I can move the built HOB out of this 
check.
>
>> +        }
>> +
>> +        break;
>> +      } else {
>> +        DEBUG ((
>> +          DEBUG_ERROR,
>> +          "%a: Failed to parse FDT QemuCfg node\n",
>> +          __func__
>> +          ));
>> +        break;
>> +      }
>> +    }
>> +  }
>> +
>> +  return RETURN_SUCCESS;
>> +}
>> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPeiLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPeiLib.inf
>> new file mode 100644
>> index 0000000000..a3dc9a03da
>> --- /dev/null
>> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgMmioPeiLib.inf
>> @@ -0,0 +1,52 @@
>> +## @file
>> +#
>> +#  Stateful, implicitly initialized fw_cfg library.
>> +#
>> +#  Copyright (C) 2013 - 2014, Red Hat, Inc.
>> +#  Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
>> +#  Copyright (c) 2024 Loongson Technology Corporation Limited. All rights reserved.<BR>
>> +#
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 1.29
>> +  BASE_NAME                      = QemuFwCfgPeiLib
>> +  FILE_GUID                      = CDF9A9D5-7422-4DCB-B41D-607151AD320B
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = QemuFwCfgLib|PEIM
>> +
>> +  CONSTRUCTOR                    = QemuFwCfgInitialize
>> +
>> +#
>> +# The following information is for reference only and not required by the build
>> +# tools.
>> +#
>> +#  VALID_ARCHITECTURES           = LOONGARCH64
>> +#
>> +
> You can drop the comment block above
OK
>
>> +[Sources]
>> +  QemuFwCfgLibMmio.c
>> +  QemuFwCfgMmioPei.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +
> Please put in alphabetical order
OK
>
>> +[LibraryClasses]
>> +  BaseLib
>> +  BaseMemoryLib
>> +  DebugLib
>> +  HobLib
>> +  IoLib
>> +  MemoryAllocationLib
>> +  PcdLib
>> +
>> +[Pcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
>> +
>> +[Guids]
>> +  gQemuFirmwareResourceHobGuid
>> --
>> 2.27.0
>>
>
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118396): https://edk2.groups.io/g/devel/message/118396
Mute This Topic: https://groups.io/mt/105746793/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: 13816 bytes --]