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

Hi Ard,


Thanks,
Chao
On 2024/4/30 09:19, Chao Li wrote:
>
> Hi Ard,
>
> OK, I will submit the V5 today and make the adjustments according to 
> your suggestions.
>
> 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.
I get it, there are some logic error, I will fix in V5. I think it 
should be more better to built the HOB when the 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
> 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 (#118400): https://edk2.groups.io/g/devel/message/118400
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: 14440 bytes --]