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 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 >> Cc: Jiewen Yao >> Cc: Gerd Hoffmann >> Co-authored-by: Xianglai Li >> Signed-off-by: Chao Li >> --- >> .../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.
>> + (C) Copyright 2021 Hewlett Packard Enterprise Development LP
>> + Copyright (c) 2024 Loongson Technology Corporation Limited. All rights reserved.
>> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> +**/ >> + >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#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.
>> +# Copyright (c) 2024 Loongson Technology Corporation Limited. All rights reserved.
>> +# >> +# 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] -=-=-=-=-=-=-=-=-=-=-=-