From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 0D07FAC16F4 for ; Thu, 14 Dec 2023 18:59:29 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=l4qhqh/mRoGCBtfVEd0YaaqY5tzysLWZ75pSMj9My5k=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1702580368; v=1; b=jD2AGBr5iNtKBMsVxmbl6Kz8d1tOndFmwtqHTFcjqWM+FYc/xD6r1JXOTqF84hUK9AWIaA8p G8qPfIacy6J47Rv5puP0FrVROobnUU+KQq92Ol14TMz7YkJ2R1G6CU07pRtk4hVohKZK7QhcOgH vlJaFTzsxgkLmH+wTkHL0ysQ= X-Received: by 127.0.0.2 with SMTP id RZG2YY7687511xoqtBvsbHeh; Thu, 14 Dec 2023 10:59:28 -0800 X-Received: from mail-vs1-f50.google.com (mail-vs1-f50.google.com [209.85.217.50]) by mx.groups.io with SMTP id smtpd.web11.31590.1702580367923020964 for ; Thu, 14 Dec 2023 10:59:28 -0800 X-Received: by mail-vs1-f50.google.com with SMTP id ada2fe7eead31-4662a125b64so1204689137.3 for ; Thu, 14 Dec 2023 10:59:27 -0800 (PST) X-Gm-Message-State: TZUeumbXQZYRjuxG18jpPmCQx7686176AA= X-Google-Smtp-Source: AGHT+IGm+jqPJhS0DYdRGxbQ5C12k5v7oUPFbB4I9SGMrdara5X4S1w/TcRTr1ffzR4UZ0/AVV1IUv1CI989VaPQ41k= X-Received: by 2002:a05:6102:1624:b0:464:4891:cce9 with SMTP id cu36-20020a056102162400b004644891cce9mr9876385vsb.16.1702580366773; Thu, 14 Dec 2023 10:59:26 -0800 (PST) MIME-Version: 1.0 References: <20230526231733.6755-1-tphan@ventanamicro.com> <20230526231733.6755-8-tphan@ventanamicro.com> <1763FC7B83488280.11718@groups.io> In-Reply-To: From: "Tuan Phan" Date: Thu, 14 Dec 2023 10:59:16 -0800 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 7/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices To: devel@edk2.groups.io, andrei.warkentin@intel.com Cc: Ard Biesheuvel , "Kinney, Michael D" , "Gao, Liming" , "Liu, Zhiguang" , "sunilvl@ventanamicro.com" , "git@danielschaefer.me" Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,tphan@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000f6ad32060c7ce1a7" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=jD2AGBr5; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --000000000000f6ad32060c7ce1a7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Dec 7, 2023 at 11:36=E2=80=AFPM Andrei Warkentin 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/MemoryInitP= eiLib.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=E2=80=A6 > > > > 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 duri= ng > OS use of RT services. It=E2=80=99s probably a good idea to avoid using M= MIO > regions for all I/O used by Boot Services. > > > Agree. Will post a patch to fix it. > A > > > > > > *From:* Tuan Phan > *Sent:* Thursday, June 22, 2023 3:28 PM > *To:* devel@edk2.groups.io; tphan@ventanamicro.com > *Cc:* Ard Biesheuvel ; Kinney, Michael D < > michael.d.kinney@intel.com>; Gao, Liming ; Liu, > Zhiguang ; sunilvl@ventanamicro.com; > git@danielschaefer.me; Warkentin, Andrei > *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=E2=80=AFAM Tuan Phan wrote: > > > > > > On Tue, May 30, 2023 at 10:38=E2=80=AFAM Tuan Phan via groups.io ventanamicro.com@groups.io> wrote: > > > > > > On Mon, May 29, 2023 at 7:07=E2=80=AFAM Ard Biesheuvel = wrote: > > On Sat, 27 May 2023 at 01:18, Tuan Phan wrote: > > > > Normally, DXE driver would add device resource to GCD before start usin= g. > > 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 wh= en > 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 t= o > make sure it runs before VariableRuntimeDxe. > > > > Signed-off-by: Tuan Phan > > Reviewed-by: Andrei Warkentin > > > --- > > 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 > > #include > > > > +/** > > + 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 =3D 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 =3D fdt_node_offset_by_compatible (FdtBase, -1, Compatible); > > + while (Node !=3D -FDT_ERR_NOTFOUND) { > > + Reg =3D (UINT64 *)fdt_getprop (FdtBase, Node, "reg", &LenP); > > + if (Reg) { > > + ASSERT (LenP =3D=3D (2 * sizeof (UINT64))); > > + AddIoMemoryBaseSizeHob (SwapBytes64 (Reg[0]), SwapBytes64 > (Reg[1])); > > + } > > + Node =3D 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] > > ------------ > > > > > >=20 > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000f6ad32060c7ce1a7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Dec 7, 2023 at 11:36=E2=80=AF= PM Andrei Warkentin <andre= i.warkentin@intel.com> wrote:

Hi Tuan,

=C2=A0

I noticed that the OvmfPkg RV Sec uses PopulateIoRes= ources by adding entries to GCD of type EFI_RESOURCE_MEMORY_MAPPED_IO. Cont= rast this with edk2-platforms/Platforms/RaspberryPi/Library/MemoryInitPeiLi= b/MemoryInitPeiLib.c, which adds these as memory and then allocates them away as EfiReservedMemoryType. I remembe= r this came up during the upstreaming of the Raspberry Pi port=E2=80=A6<= /u>

=C2=A0

Anything we add as MMIO will end up growing the Runt= ime Services mappings, as MMIO are specifically non-memory mappings that ne= ed to be present during OS use of RT services. It=E2=80=99s probably a good= idea to avoid using MMIO regions for all I/O used by Boot Services.

=C2=A0

Agree. Will post a patch to fix it.
=C2=A0

A

=C2=A0

=C2=A0

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&= gt;; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>= ; sunilvl@ven= tanamicro.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

=C2=A0

=C2=A0

=C2=A0

On Thu, Jun 22, 2023 at 11:41=E2=80=AFAM Tuan Phan &= lt;tphan@ventan= amicro.com> wrote:

=C2=A0

=C2=A0

On Tue, May 30, 2023 at 10:38=E2=80=AFAM Tuan Phan v= ia groups.io <tphan=3Dventanamicro.com@groups.io> wrote:

=C2=A0

=C2=A0

On Mon, May 29, 2023 at 7:07=E2=80=AFAM Ard Biesheuv= el <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 usi= ng.
> But some key resources such as uart, flash base address are being acce= ssing
> 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?

=C2=A0

These resources should be in memory space so their a= ddresses and size are registered with MMU. If not when MMU enabled, illegal= access exception when someone access them.

=C2=A0

Hi Ard,

Do you still have concerns about this patch?=C2=A0

BTW, I will drop this patch and put VirtNorFlashDxe = in APRIORI DXE list to make sure it runs before VariableRuntimeDxe.<= u>


> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> Reviewed-by: Andrei Warkentin <andrei.warkentin@intel.com>

> ---
>=C2=A0 OvmfPkg/RiscVVirt/Sec/Platform.c=C2=A0 | 62 ++++++++++++++++++++= +++++++++++
>=C2=A0 OvmfPkg/RiscVVirt/Sec/SecMain.inf |=C2=A0 1 +
>=C2=A0 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
>=C2=A0 #include <libfdt.h>
>=C2=A0 #include <Guid/FdtHob.h>
>
> +/**
> +=C2=A0 Build memory map I/O range resource HOB using the
> +=C2=A0 base address and size.
> +
> +=C2=A0 @param=C2=A0 MemoryBase=C2=A0 =C2=A0 =C2=A0Memory map I/O base= .
> +=C2=A0 @param=C2=A0 MemorySize=C2=A0 =C2=A0 =C2=A0Memory map I/O size= .
> +
> +**/
> +STATIC
> +VOID
> +AddIoMemoryBaseSizeHob (
> +=C2=A0 EFI_PHYSICAL_ADDRESS=C2=A0 MemoryBase,
> +=C2=A0 UINT64=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = MemorySize
> +=C2=A0 )
> +{
> +=C2=A0 /* Align to EFI_PAGE_SIZE */
> +=C2=A0 MemorySize =3D ALIGN_VALUE (MemorySize, EFI_PAGE_SIZE);
> +=C2=A0 BuildResourceDescriptorHob (
> +=C2=A0 =C2=A0 EFI_RESOURCE_MEMORY_MAPPED_IO,
> +=C2=A0 =C2=A0 EFI_RESOURCE_ATTRIBUTE_PRESENT=C2=A0 =C2=A0 =C2=A0|
> +=C2=A0 =C2=A0 EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +=C2=A0 =C2=A0 EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
> +=C2=A0 =C2=A0 EFI_RESOURCE_ATTRIBUTE_TESTED,
> +=C2=A0 =C2=A0 MemoryBase,
> +=C2=A0 =C2=A0 MemorySize
> +=C2=A0 =C2=A0 );
> +}
> +
> +/**
> +=C2=A0 Populate IO resources from FDT that not added to GCD by its > +=C2=A0 driver in the DXE phase.
> +
> +=C2=A0 @param=C2=A0 FdtBase=C2=A0 =C2=A0 =C2=A0 =C2=A0Fdt base addres= s
> +=C2=A0 @param=C2=A0 Compatible=C2=A0 =C2=A0 Compatible string
> +
> +**/
> +STATIC
> +VOID
> +PopulateIoResources (
> +=C2=A0 VOID=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *FdtBase,
> +=C2=A0 CONST CHAR8*=C2=A0 Compatible
> +=C2=A0 )
> +{
> +=C2=A0 UINT64=C2=A0 *Reg;
> +=C2=A0 INT32=C2=A0 =C2=A0Node, LenP;
> +
> +=C2=A0 Node =3D fdt_node_offset_by_compatible (FdtBase, -1, Compatibl= e);
> +=C2=A0 while (Node !=3D -FDT_ERR_NOTFOUND) {
> +=C2=A0 =C2=A0 Reg =3D (UINT64 *)fdt_getprop (FdtBase, Node, "reg= ", &LenP);
> +=C2=A0 =C2=A0 if (Reg) {
> +=C2=A0 =C2=A0 =C2=A0 ASSERT (LenP =3D=3D (2 * sizeof (UINT64)));
> +=C2=A0 =C2=A0 =C2=A0 AddIoMemoryBaseSizeHob (SwapBytes64 (Reg[0]), Sw= apBytes64 (Reg[1]));
> +=C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 Node =3D fdt_node_offset_by_compatible (FdtBase, Node, = Compatible);
> +=C2=A0 }
> +}
> +
>=C2=A0 /**
>=C2=A0 =C2=A0 @retval EFI_SUCCESS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 The address of FDT is passed in HOB.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 EFI_UNSUPPORTED=C2=A0 =C2=A0 = =C2=A0 =C2=A0 Can't locate FDT.
> @@ -80,5 +137,10 @@ PlatformPeimInitialization (
>
>=C2=A0 =C2=A0 BuildFvHob (PcdGet32 (PcdOvmfDxeMemFvBase), PcdGet32 (Pcd= OvmfDxeMemFvSize));
>
> +=C2=A0 PopulateIoResources (Base, "ns16550a");
> +=C2=A0 PopulateIoResources (Base, "qemu,fw-cfg-mmio");
> +=C2=A0 PopulateIoResources (Base, "virtio,mmio");
> +=C2=A0 AddIoMemoryBaseSizeHob (PcdGet32 (PcdOvmfFdBaseAddress), PcdGe= t32 (PcdOvmfFirmwareFdSize));
> +
>=C2=A0 =C2=A0 return EFI_SUCCESS;
>=C2=A0 }
> 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 @@
>=C2=A0 =C2=A0 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
>=C2=A0 =C2=A0 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>=C2=A0 =C2=A0 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
> +=C2=A0 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
>
>=C2=A0 [Guids]
>=C2=A0 =C2=A0 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:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#112547) | =20 | Mute = This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000f6ad32060c7ce1a7--