Hi Tuan, I noticed that the OvmfPkg RV Sec uses PopulateIoResources by adding entries to GCD of type EFI_RESOURCE_MEMORY_MAPPED_IO. Contrast this with edk2-platforms/Platforms/RaspberryPi/Library/MemoryInitPeiLib/MemoryInitPeiLib.c, which adds these as memory and then allocates them away as EfiReservedMemoryType. I remember this came up during the upstreaming of the Raspberry Pi port…
Anything we add as MMIO will end up growing the Runtime Services mappings, as MMIO are specifically non-memory mappings that need to be present during OS use of RT services. It’s probably a good idea to avoid using MMIO regions for all I/O used by Boot Services. A From: Tuan Phan <tp...@ventanamicro.com> Sent: Thursday, June 22, 2023 3:28 PM To: devel@edk2.groups.io; tp...@ventanamicro.com Cc: Ard Biesheuvel <a...@kernel.org>; Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com>; suni...@ventanamicro.com; g...@danielschaefer.me; Warkentin, Andrei <andrei.warken...@intel.com> Subject: Re: [edk2-devel] [PATCH v3 7/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices On Thu, Jun 22, 2023 at 11:41 AM Tuan Phan <tp...@ventanamicro.com<mailto:tp...@ventanamicro.com>> wrote: On Tue, May 30, 2023 at 10:38 AM Tuan Phan via groups.io<http://groups.io> <tphan=ventanamicro....@groups.io<mailto:ventanamicro....@groups.io>> wrote: On Mon, May 29, 2023 at 7:07 AM Ard Biesheuvel <a...@kernel.org<mailto:a...@kernel.org>> wrote: On Sat, 27 May 2023 at 01:18, Tuan Phan <tp...@ventanamicro.com<mailto:tp...@ventanamicro.com>> wrote: > > Normally, DXE driver would add device resource to GCD before start using. > But some key resources such as uart, flash base address are being accessing > directly in some core modules. > > Those resources should be populated to HOB in SEC phase so they are > added to GCD before anyone can access them. > Why should these be in the GCD to begin with? These resources should be in memory space so their addresses and size are registered with MMU. If not when MMU enabled, illegal access exception when someone access them. Hi Ard, Do you still have concerns about this patch? BTW, I will drop this patch and put VirtNorFlashDxe in APRIORI DXE list to make sure it runs before VariableRuntimeDxe. > Signed-off-by: Tuan Phan > <tp...@ventanamicro.com<mailto:tp...@ventanamicro.com>> > Reviewed-by: Andrei Warkentin > <andrei.warken...@intel.com<mailto:andrei.warken...@intel.com>> > --- > OvmfPkg/RiscVVirt/Sec/Platform.c | 62 +++++++++++++++++++++++++++++++ > OvmfPkg/RiscVVirt/Sec/SecMain.inf | 1 + > 2 files changed, 63 insertions(+) > > diff --git a/OvmfPkg/RiscVVirt/Sec/Platform.c > b/OvmfPkg/RiscVVirt/Sec/Platform.c > index 3645c27b0b12..944b82c84a6e 100644 > --- a/OvmfPkg/RiscVVirt/Sec/Platform.c > +++ b/OvmfPkg/RiscVVirt/Sec/Platform.c > @@ -21,6 +21,63 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include <libfdt.h> > #include <Guid/FdtHob.h> > > +/** > + Build memory map I/O range resource HOB using the > + base address and size. > + > + @param MemoryBase Memory map I/O base. > + @param MemorySize Memory map I/O size. > + > +**/ > +STATIC > +VOID > +AddIoMemoryBaseSizeHob ( > + EFI_PHYSICAL_ADDRESS MemoryBase, > + UINT64 MemorySize > + ) > +{ > + /* Align to EFI_PAGE_SIZE */ > + MemorySize = ALIGN_VALUE (MemorySize, EFI_PAGE_SIZE); > + BuildResourceDescriptorHob ( > + EFI_RESOURCE_MEMORY_MAPPED_IO, > + EFI_RESOURCE_ATTRIBUTE_PRESENT | > + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | > + EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE | > + EFI_RESOURCE_ATTRIBUTE_TESTED, > + MemoryBase, > + MemorySize > + ); > +} > + > +/** > + Populate IO resources from FDT that not added to GCD by its > + driver in the DXE phase. > + > + @param FdtBase Fdt base address > + @param Compatible Compatible string > + > +**/ > +STATIC > +VOID > +PopulateIoResources ( > + VOID *FdtBase, > + CONST CHAR8* Compatible > + ) > +{ > + UINT64 *Reg; > + INT32 Node, LenP; > + > + Node = fdt_node_offset_by_compatible (FdtBase, -1, Compatible); > + while (Node != -FDT_ERR_NOTFOUND) { > + Reg = (UINT64 *)fdt_getprop (FdtBase, Node, "reg", &LenP); > + if (Reg) { > + ASSERT (LenP == (2 * sizeof (UINT64))); > + AddIoMemoryBaseSizeHob (SwapBytes64 (Reg[0]), SwapBytes64 (Reg[1])); > + } > + Node = fdt_node_offset_by_compatible (FdtBase, Node, Compatible); > + } > +} > + > /** > @retval EFI_SUCCESS The address of FDT is passed in HOB. > EFI_UNSUPPORTED Can't locate FDT. > @@ -80,5 +137,10 @@ PlatformPeimInitialization ( > > BuildFvHob (PcdGet32 (PcdOvmfDxeMemFvBase), PcdGet32 > (PcdOvmfDxeMemFvSize)); > > + PopulateIoResources (Base, "ns16550a"); > + PopulateIoResources (Base, "qemu,fw-cfg-mmio"); > + PopulateIoResources (Base, "virtio,mmio"); > + AddIoMemoryBaseSizeHob (PcdGet32 (PcdOvmfFdBaseAddress), PcdGet32 > (PcdOvmfFirmwareFdSize)); > + > return EFI_SUCCESS; > } > diff --git a/OvmfPkg/RiscVVirt/Sec/SecMain.inf > b/OvmfPkg/RiscVVirt/Sec/SecMain.inf > index 0e2a5785e8a4..75d5b74b3d3f 100644 > --- a/OvmfPkg/RiscVVirt/Sec/SecMain.inf > +++ b/OvmfPkg/RiscVVirt/Sec/SecMain.inf > @@ -62,6 +62,7 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize > > [Guids] > gFdtHobGuid > -- > 2.25.1 > > > > ------------ > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#105346): https://edk2.groups.io/g/devel/message/105346 > Mute This Topic: https://groups.io/mt/99160000/1131722 > Group Owner: devel+ow...@edk2.groups.io<mailto:devel%2bow...@edk2.groups.io> > Unsubscribe: https://edk2.groups.io/g/devel/unsub > [a...@kernel.org<mailto:a...@kernel.org>] > ------------ > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112214): https://edk2.groups.io/g/devel/message/112214 Mute This Topic: https://groups.io/mt/99160000/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
