From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by mx.groups.io with SMTP id smtpd.web11.22040.1687465675638557860 for ; Thu, 22 Jun 2023 13:27:56 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=KlyNeiN4; spf=pass (domain: ventanamicro.com, ip: 209.85.208.47, mailfrom: tphan@ventanamicro.com) Received: by mail-ed1-f47.google.com with SMTP id 4fb4d7f45d1cf-5149aafef44so8841281a12.0 for ; Thu, 22 Jun 2023 13:27:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1687465673; x=1690057673; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Iemz5l/Nd0gVjsIXD4j9e01PCInQtD3G8jca74pHvJ4=; b=KlyNeiN4tde1DEbfc4zNX1PaoFqHi47ezozL2xiyObvhlagE7+DECGHtJAAMwuDxvP wcShn+qeNU7MIXKboTUBlYlQjrfSUbBb2HcpEJW2DJbkEtv/ACjwxlhHfR9tx4rAGCJW A4pPwWTrDrEz8B7VV7xLMozB4Mv07XDeePZ9+TTp4ZxarvCXf3pH19t+SDj2yE4IpuUx KWN6jONdRXCs+7A9lnozb+i1wrt1f51dhZRraI9B8QRrF/SUXf5NvNIbOS1dbr/pXyzm Wky8Oy1WWaYrvTq8QgXRwqDq0sdx/nIeH6DEIdjfymu7HJ3hGy+6pzJiLW77LlDEs3NP sKTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687465673; x=1690057673; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Iemz5l/Nd0gVjsIXD4j9e01PCInQtD3G8jca74pHvJ4=; b=MeLN7InBK2hVGetj+IrlXe2UFnxr3WWMg0q1ETOPnmt+7kwRDOr43g11ZJ/TVjiQ2x 8Vcm4UaG1CIMhoJQDNAQhYBVfePewCwqBaIsH2PuxslC7eRP2PLbxCZIYcV9pKHIXXer fhSAC2tFWPo4O3s6qyAMlY9ZiZgMO9fuC3rLLO1NtHkgi3h+Y1CMnyfIWq6p1nTAdObb RQhMW70NZr5bbHQpq6z86qWFlvC6usy1P/puH+Y+/OWXkqdqsTaOWdtLERqRjxOF4FR1 QF4uPuh874MH9iIDH05otMmEu+U79EiAZkxTjUCOyX0KctZpPp7NksIb1mhxe6B9ELuw u8bw== X-Gm-Message-State: AC+VfDy95xtf22AcirCz9VlENVV1b6uQjBgOVVZz+qpCqIN0ck3e7Ses EvmQQIaiKtuuyQDk+n0HgMB7o2MGdeCvFeS5T/8cIPGt5zkhhI6l8oY= X-Google-Smtp-Source: ACHHUZ5ZobV78pT+Xdzg+CsRfOxVQmuAXnrutndHWlPx2yRsbk5DwKP5+vKI2gf3nNEDFzQEIEgEztZr2CQyZhwbrfI= X-Received: by 2002:aa7:c9c1:0:b0:514:8e6f:66f5 with SMTP id i1-20020aa7c9c1000000b005148e6f66f5mr13421192edt.19.1687465673511; Thu, 22 Jun 2023 13:27:53 -0700 (PDT) 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, 22 Jun 2023 13:27:42 -0700 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, tphan@ventanamicro.com Cc: Ard Biesheuvel , michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, sunilvl@ventanamicro.com, git@danielschaefer.me, andrei.warkentin@intel.com Content-Type: multipart/alternative; boundary="0000000000000a67ee05febdb8b4" --0000000000000a67ee05febdb8b4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 >>> 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 ar= e >> registered with MMU. If not when MMU enabled, illegal access exception w= hen >> 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 >>> > 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, Compatibl= e); >>> > + } >>> > +} >>> > + >>> > /** >>> > @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 >> >> --0000000000000a67ee05febdb8b4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Jun 22, 2023 at 11:41=E2=80= =AFAM Tuan Phan <tphan@ventana= micro.com> wrote:


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


On Mon, May 29, 2023 at 7:07=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> wrote:
On Sat, 27 May 202= 3 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 addresses and s= ize are registered with MMU. If not when MMU enabled, illegal access except= ion when someone access them.

=
Hi Ard,
Do you still have concerns about this patch?=C2=A0
BTW, I will drop this patch and put VirtN= orFlashDxe in APRIORI DXE list to make sure it runs before VariableRuntimeD= xe.

> 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.i= o/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]
> ------------
>
>

--0000000000000a67ee05febdb8b4--