Re: [edk2-devel] [PATCH v3 7/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices

["Tuan Phan" <tphan@ventanamicro.com>]

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

On Thu, Dec 7, 2023 at 11:36 PM Andrei Warkentin <andrei.warkentin@intel.com>
wrote:

> 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.
>
>
>
Agree. Will post a patch to fix it.


> A
>
>
>
>
>
> *From:* Tuan Phan <tphan@ventanamicro.com>
> *Sent:* Thursday, June 22, 2023 3:28 PM
> *To:* devel@edk2.groups.io; tphan@ventanamicro.com
> *Cc:* Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <
> michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; sunilvl@ventanamicro.com;
> git@danielschaefer.me; Warkentin, Andrei <andrei.warkentin@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 <tphan@ventanamicro.com> wrote:
>
>
>
>
>
> On Tue, May 30, 2023 at 10:38 AM Tuan Phan via groups.io <tphan=
> ventanamicro.com@groups.io> wrote:
>
>
>
>
>
> On Mon, May 29, 2023 at 7:07 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sat, 27 May 2023 at 01:18, Tuan Phan <tphan@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 <tphan@ventanamicro.com>
> > Reviewed-by: Andrei Warkentin <andrei.warkentin@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+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> > ------------
> >
> >
>
> 
>
>


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