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 78C2F7803D0 for ; Wed, 28 Feb 2024 18:01:02 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=0EYPwVEpATCT/okByvryRq7QFLLhnyQDBr6wUErE6hA=; 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=1709143261; v=1; b=q7Ldp6cRd7pcYdVqv4wk7vmazE3iwP7gbNmI0FjIHxHvUfnFRH5l6gvkvmxfXMGEFUribmGk nUsWeCPrv7GfCx9Ubz+jh5jor/LaNeMAX31j8PROVfKUC51ErNQn4Z+tOeufjE+clvv85d2xxid zTPiMEk23fg5H0DJWSqnrP/s= X-Received: by 127.0.0.2 with SMTP id oGNpYY7687511xEkEuvLatwh; Wed, 28 Feb 2024 10:01:01 -0800 X-Received: from mail-ua1-f47.google.com (mail-ua1-f47.google.com [209.85.222.47]) by mx.groups.io with SMTP id smtpd.web10.3693.1709143260164592734 for ; Wed, 28 Feb 2024 10:01:00 -0800 X-Received: by mail-ua1-f47.google.com with SMTP id a1e0cc1a2514c-7d5c2502ea2so3106026241.1 for ; Wed, 28 Feb 2024 10:01:00 -0800 (PST) X-Gm-Message-State: uid2HmVmgrJRrRNt7uTAZambx7686176AA= X-Google-Smtp-Source: AGHT+IHytYkGO3bSeQQIgSG+qf5ZlTjfsryKVvQcoQgvENHlfbiZ6y6yhQKBgD7rXh4G1YX8sSbl78rzj8QJ4PoFU60= X-Received: by 2002:a1f:db44:0:b0:4c0:2abe:d585 with SMTP id s65-20020a1fdb44000000b004c02abed585mr302923vkg.6.1709143258886; Wed, 28 Feb 2024 10:00:58 -0800 (PST) MIME-Version: 1.0 References: <20240207012910.2133-1-tphan@ventanamicro.com> <20240207012910.2133-3-tphan@ventanamicro.com> In-Reply-To: From: "Tuan Phan" Date: Wed, 28 Feb 2024 10:00:48 -0800 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension To: Laszlo Ersek Cc: devel@edk2.groups.io, michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, kraxel@redhat.com, rahul1.kumar@intel.com, ray.ni@intel.com, sunilvl@ventanamicro.com, jiewen.yao@intel.com, andrei.warkentin@intel.com, ardb+tianocore@kernel.org 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="000000000000d125ad061274ec7a" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=q7Ldp6cR; 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 --000000000000d125ad061274ec7a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Feb 7, 2024 at 10:15=E2=80=AFAM Laszlo Ersek wr= ote: > On 2/7/24 02:29, Tuan Phan wrote: > > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will be > > supported when Svpbmt extension available. > > > > Signed-off-by: Tuan Phan > > --- > > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 25 ++++++++++++++++++- > > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 + > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > > index 826a1d32a1d4..c50a28e97e4b 100644 > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > > @@ -36,6 +36,15 @@ > > #define PTE_PPN_SHIFT 10 > > #define RISCV_MMU_PAGE_SHIFT 12 > > > > +#define RISCV_CPU_FEATURE_PBMT_BITMASK BIT2 > > +#define PTE_PBMT_NC BIT61 > > +#define PTE_PBMT_IO BIT62 > > +#define PTE_PBMT_MASK (PTE_PBMT_NC | PTE_PBMT_IO) > > + > > +#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | \ > > + EFI_MEMORY_WT | EFI_MEMORY_WB | \ > > + EFI_MEMORY_UCE) > > + > > (1) I've stated this elsewhere -- introducing such a macro is justified, > but calling it EFI_* is not. The EFI_ prefix is reserved for the spec. > Will fix it. > > > STATIC UINTN mModeSupport[] =3D { SATP_MODE_SV57, SATP_MODE_SV48, > SATP_MODE_SV39, SATP_MODE_OFF }; > > STATIC UINTN mMaxRootTableLevel; > > STATIC UINTN mBitPerLevel; > > @@ -514,6 +523,20 @@ GcdAttributeToPageAttribute ( > > RiscVAttributes &=3D ~RISCV_PG_X; > > } > > > > + if ((PcdGet64 (PcdRiscVFeatureOverride) & > RISCV_CPU_FEATURE_PBMT_BITMASK) !=3D 0) { > > + switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) { > > + case EFI_MEMORY_UC: > > + RiscVAttributes |=3D PTE_PBMT_IO; > > + break; > > + case EFI_MEMORY_WC: > > + RiscVAttributes |=3D PTE_PBMT_NC; > > + break; > > + default: > > + // Default PMA mode > > + break; > > + } > > + } > > + > > return RiscVAttributes; > > } > > > > Several questions / observations: > > (2) If the feature is cleared in the PCD, does it deserve a warning that > the attribute setting request cannot be honored? > Sure, I will add a warning if the feature has not been enabled. > > (3) The memory cacheability attributes are expressed as distinct bits of > a bitmask because, for expressing *capabilities*, they must be possible > to OR together. However, when setting actual attributes, I think the > bitmask should contain *exactly* one bit set -- in other words, the > value of the bitmask should be an integral power of two (that's not hard > to check). > > Do you agree about this? If so, I'd suggest rejecting the request (with > an appropriate status code) if zero bits, or multiple bits, are set. > > UINT64 CacheTypeMask; > > CacheType =3D GcdAttributes & MEMORY_CACHETYPE_MASK; > if ((CacheType =3D=3D 0) || > (((CacheType - 1) & CacheType) !=3D 0)) { > return EFI_INVALID_PARAMETER; > } > switch (CacheType) { > ... > } > > This would of course require changing the GcdAttributeToPageAttribute() > prototype, because right now the function cannot return an error. > > That makes sense. Will fix it. Thanks > > > @@ -559,7 +582,7 @@ RiscVSetMemoryAttributes ( > > BaseAddress, > > Length, > > PageAttributesSet, > > - PTE_ATTRIBUTES_MASK, > > + PTE_ATTRIBUTES_MASK | PTE_PBMT_MASK, > > (UINTN *)RiscVGetRootTranslateTable (), > > TRUE > > ); > > (4) I feel we shouldn't try to clear PTE_PBMT_MASK if > PcdRiscVFeatureOverride tells us that Svpbmt is not available. Just a > thought. > Sure. > > > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > > index 51ebe1750e97..1dbaa81f3608 100644 > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > > @@ -28,3 +28,4 @@ > > > > [Pcd] > > gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode ## CONSUMES > > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES > > Thanks > Laszlo > > -=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 (#116128): https://edk2.groups.io/g/devel/message/116128 Mute This Topic: https://groups.io/mt/104211195/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- --000000000000d125ad061274ec7a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Feb 7, 2024 at 10:15=E2=80=AF= AM Laszlo Ersek <lersek@redhat.com<= /a>> wrote:
On 2/7/24 02:29, Tuan Phan wrote:<= br> > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will be
> supported when Svpbmt extension available.
>
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
>=C2=A0 .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 25 +++++++++++++= +++++-
>=C2=A0 .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf=C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 1 +
>=C2=A0 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/Ue= fiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> index 826a1d32a1d4..c50a28e97e4b 100644
> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> @@ -36,6 +36,15 @@
>=C2=A0 #define PTE_PPN_SHIFT=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A010
>=C2=A0 #define RISCV_MMU_PAGE_SHIFT=C2=A0 12
>=C2=A0
> +#define RISCV_CPU_FEATURE_PBMT_BITMASK=C2=A0 BIT2
> +#define PTE_PBMT_NC=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0BIT61
> +#define PTE_PBMT_IO=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0BIT62
> +#define PTE_PBMT_MASK=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0(PTE_PBMT_NC | PTE_PBMT_IO)
> +
> +#define EFI_MEMORY_CACHETYPE_MASK=C2=A0 (EFI_MEMORY_UC | EFI_MEMORY_W= C |=C2=A0 \
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0EFI_MEMORY_W= T | EFI_MEMORY_WB | \
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0EFI_MEMORY_U= CE)
> +

(1) I've stated this elsewhere -- introducing such a macro is justified= ,
but calling it EFI_* is not. The EFI_ prefix is reserved for the spec.
<= /blockquote>
Will fix it.
=C2=A0

>=C2=A0 STATIC UINTN=C2=A0 mModeSupport[] =3D { SATP_MODE_SV57, SATP_MOD= E_SV48, SATP_MODE_SV39, SATP_MODE_OFF };
>=C2=A0 STATIC UINTN=C2=A0 mMaxRootTableLevel;
>=C2=A0 STATIC UINTN=C2=A0 mBitPerLevel;
> @@ -514,6 +523,20 @@ GcdAttributeToPageAttribute (
>=C2=A0 =C2=A0 =C2=A0 RiscVAttributes &=3D ~RISCV_PG_X;
>=C2=A0 =C2=A0 }
>=C2=A0
> +=C2=A0 if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATUR= E_PBMT_BITMASK) !=3D 0) {
> +=C2=A0 =C2=A0 switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) = {
> +=C2=A0 =C2=A0 =C2=A0 case EFI_MEMORY_UC:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVAttributes |=3D PTE_PBMT_IO;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 case EFI_MEMORY_WC:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVAttributes |=3D PTE_PBMT_NC;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 default:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 // Default PMA mode
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 }
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return RiscVAttributes;
>=C2=A0 }
>=C2=A0

Several questions / observations:

(2) If the feature is cleared in the PCD, does it deserve a warning that the attribute setting request cannot be honored?
Sure,= I will add a warning if the feature has not been enabled.

(3) The memory cacheability attributes are expressed as distinct bits of a bitmask because, for expressing *capabilities*, they must be possible
to OR together. However, when setting actual attributes, I think the
bitmask should contain *exactly* one bit set -- in other words, the
value of the bitmask should be an integral power of two (that's not har= d
to check).

Do you agree about this? If so, I'd suggest rejecting the request (with=
an appropriate status code) if zero bits, or multiple bits, are set.

=C2=A0 UINT64=C2=A0 CacheTypeMask;

=C2=A0 CacheType =3D GcdAttributes & MEMORY_CACHETYPE_MASK;
=C2=A0 if ((CacheType =3D=3D 0) ||
=C2=A0 =C2=A0 =C2=A0 (((CacheType - 1) & CacheType) !=3D 0)) {
=C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
=C2=A0 }
=C2=A0 switch (CacheType) {
=C2=A0 =C2=A0 ...
=C2=A0 }

This would of course require changing the GcdAttributeToPageAttribute()
prototype, because right now the function cannot return an error.

That makes sense. Will fix it. Thanks=C2=A0

> @@ -559,7 +582,7 @@ RiscVSetMemoryAttributes (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BaseAddress,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Length,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PageAttributesSet,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PTE_ATTRIBUTES_MASK,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PTE_ATTRIBUTES_MASK | PTE_PB= MT_MASK,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(UINTN *)RiscVGetRootTr= anslateTable (),
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0TRUE
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0);

(4) I feel we shouldn't try to clear PTE_PBMT_MASK if
PcdRiscVFeatureOverride tells us that Svpbmt is not available. Just a
thought.
Sure.=C2=A0

> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf b/= UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf
> index 51ebe1750e97..1dbaa81f3608 100644
> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf
> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf
> @@ -28,3 +28,4 @@
>=C2=A0
>=C2=A0 [Pcd]
>=C2=A0 =C2=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode=C2=A0= ## CONSUMES
> +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride=C2=A0 =C2=A0 = =C2=A0## CONSUMES

Thanks
Laszlo

_._,_._,_
Groups.io Links:

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

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

_._,_._,_
--000000000000d125ad061274ec7a--