From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by mx.groups.io with SMTP id smtpd.web10.564.1685468292379194276 for ; Tue, 30 May 2023 10:38:12 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=nd4irfDp; spf=pass (domain: ventanamicro.com, ip: 209.85.218.46, mailfrom: tphan@ventanamicro.com) Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-96f683e8855so684435466b.2 for ; Tue, 30 May 2023 10:38:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1685468291; x=1688060291; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=zUdXBGTTNyp05vViMOVC7SMUal1l53NNZ2uD07HxdzQ=; b=nd4irfDpq4Vhqk0ZmJ6M0bkQUHLx1bfHVfl6ESNxudxs0LXQ0R6N02W7StzWFkTDMX A46OIEW1BR3d+PCo+r+jF9yXg3eTf0BHz6WCaYDN0E/IXdL1Cblv9IkOIKddtMBR+t6x wF7cj2VcjNEF/0w3xFkz6kov8TbyI8ZLI7bnx6NwLKPAtw4OKIuQKQ0GLTZTUthtjfDw cYpZnlQi3GVl/KDE2u0VN5ju6yWU0m4B9joZ7tLCDV5aZnsZAqZIPQY6SMqEvIYYlFna q02r9m69Tt9Z8GGoS3IXZONyx3TWdtUuE4OxhlL+Y0W5YZT+qCag3/uR6nKHIuH3d+B7 u1qQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685468291; x=1688060291; 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=zUdXBGTTNyp05vViMOVC7SMUal1l53NNZ2uD07HxdzQ=; b=XFbhP8tY/pdn0x8p5FTMQHFtn5U8OQnnnEyQ7SjEu2Tt5Xud/7QSEDMVILFr3yVEAU 08qEPCcE+X87vsG5EGLawZNyKtIhQhYlAgB62NPhP7UfctJPehC1uHQI3vZxPfNBIcZj Ev50w96/7+tblYpsd6aUMqyp3rzuXA+Okn660cxwDfzjbBxCfJBlG28SbXEXiBI5T6x8 Shye0ppQJU8Uy684+albvRRwzXgR7JOyLYVlxl6gJemRvBgkZMjnSTQJkd/pQj1bsDLm CTrn2EKDl4oWChMNG/oMbk2vDt3RCSc2ExdfnRt21C9bR8ieteF1GHDiSXziiX7llWxA 8sOw== X-Gm-Message-State: AC+VfDxdrFehcZSjQNr3gvDGXV6+eWXsuM4EmypohIg4uQ62cYt8t2PN 9gcBP9HYxfxoPwss1A8C8T8cD2bu7ZKcy0IaeMjwQQ== X-Google-Smtp-Source: ACHHUZ5qbysfyn+ba8rN/L0xtXT44MQgPYKhpuUOVL++3l+CJTEEZ6Z8tDhzbCEIZuWC2iNEkrD+XU7mDIAqkESR5CE= X-Received: by 2002:a17:907:94cb:b0:96f:f451:187f with SMTP id dn11-20020a17090794cb00b0096ff451187fmr2794099ejc.7.1685468290754; Tue, 30 May 2023 10:38:10 -0700 (PDT) MIME-Version: 1.0 References: <20230526231733.6755-1-tphan@ventanamicro.com> <20230526231733.6755-8-tphan@ventanamicro.com> In-Reply-To: From: "Tuan Phan" Date: Tue, 30 May 2023 10:37:58 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 7/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices To: Ard Biesheuvel Cc: devel@edk2.groups.io, 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="000000000000c0412605fcecaa37" --000000000000c0412605fcecaa37 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, May 29, 2023 at 7:07=E2=80=AFAM Ard Biesheuvel wr= ote: > 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 when someone access them. > > 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] > > ------------ > > > > > --000000000000c0412605fcecaa37 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, May 29, 2023 at 7:07=E2=80=AF= 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 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.


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