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

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

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.

> +          //
> +          // 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?

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

> +[Sources]
> +  QemuFwCfgLibMmio.c
> +  QemuFwCfgMmioPei.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +

Please put in alphabetical order

> +[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 (#118388): https://edk2.groups.io/g/devel/message/118388
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]
-=-=-=-=-=-=-=-=-=-=-=-