From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) by mx.groups.io with SMTP id smtpd.web10.19682.1687459318358012534 for ; Thu, 22 Jun 2023 11:41:58 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=c1iVFh9t; spf=pass (domain: ventanamicro.com, ip: 209.85.208.44, mailfrom: tphan@ventanamicro.com) Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-51beb527a89so1109680a12.1 for ; Thu, 22 Jun 2023 11:41:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1687459316; x=1690051316; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=XqTGcwsjWaBPXcIDngrWvdir6dB6PZh4Z6rOHTWnSsE=; b=c1iVFh9t20s3B/yFs4F4/hzGmWPjefBsMpqtrZgVy+WhpTs4M1L3r26DJ0tP+9tx7s caWtEbPXPP18kvYS15a48HDDNbLpfjIFXQzcKTKpPN8BHC76mbBzOAcxEiM9H2YsCD8V ytxbOw4z5roRpHQD29k2Dsi06jC8M7J5sN1bJsEbjhmMzJGHkpBxongkBLAsVIG+m968 OkUn7hemY515wgH4lSAVoEeei27vTHZcZpbhj7FIFJzDRLwztJIYlRxesov+1TfauR3Y 7HASyJUOaPLggUZqwy9xsF97cj2QvF3kxSVD0vQUEzW0JExyL6QGuAt3BtfdyjMdntmh /IuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687459316; x=1690051316; 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=XqTGcwsjWaBPXcIDngrWvdir6dB6PZh4Z6rOHTWnSsE=; b=VaD3D8obhY7XtsCKzWFSEFjIYy5pZ0GKTSx0UPOkbig5VdfIkXUXnhBtp62ZWhQT/v jFPouAfnVsqbYJdcCUsnsrXgKFDTj9Bmg5utROMnP37lYF9OYTg3gffqJn+AGz76HXrb HjSjC5qIHr2nAr6tL2DyYLjkrzsqos1bbPaQSVvy7BMbIQl/jQIdR322HkPjgmI9oL2R 4mITOsdyEdP04bwbvZOAvXBIHV5dxPk3/iP9yN2UFDNgXKeuf55u0PZ28lNxTlJeJOxb ILPzMyXwd4f/czt9URT9t5IiD2qWtxpbvOvZrIm+ojSFaCFKiunGJp36d4xGgXXUH5CV G/gQ== X-Gm-Message-State: AC+VfDxgt8IyM1XqmNvf/lNcZRqhTvvNTK2JAaKWV50NXPmA3BQn222a l0c/xhBdluPAWbSBln5Dy2rECMqH/BIFGLq4GmiZ7WKZk9n+8Pnhqxc= X-Google-Smtp-Source: ACHHUZ51pGPniPtY52zTD7OYZP3sXb+SQyk7P/fLjn1Hvds6rKZNdhzSOSXl0D1LeVn0jJQ+2IeC/K2tUY+ODZ+06PM= X-Received: by 2002:a17:907:86a4:b0:988:fea3:ae5a with SMTP id qa36-20020a17090786a400b00988fea3ae5amr8825602ejc.47.1687459316162; Thu, 22 Jun 2023 11:41:56 -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: <1763FC7B83488280.11718@groups.io> From: "Tuan Phan" Date: Thu, 22 Jun 2023 11:41:44 -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="0000000000001cf58e05febc3db0" --0000000000001cf58e05febc3db0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, May 30, 2023 at 10:38=E2=80=AFAM Tuan Phan via 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 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? > >> > 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 > > --0000000000001cf58e05febc3db0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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


On Mon, May 29, 2023 at 7:07=E2=80=AFAM = Ard Biesheuvel <ard= b@kernel.org> wrote:
On Sat, 27 May 2023 at 01:18, Tuan Phan <tphan@ventanamicro.com> wro= te:
>
> 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

> 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]
> ------------
>
>

--0000000000001cf58e05febc3db0--