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 CF12DD8095A for ; Wed, 7 Feb 2024 18:15:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=cxf/BFm8SSDhCykNnYMCpf+6r1UMsfaTJnQrSfr3G1s=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1707329726; v=1; b=G4Hqig5Z0VtXVWQka/jypMw4bh8KknG3ZUPaJsrRJPrs9lSl2O+fE7RpWNrcB1semgv/fNbx y+hTL/8IlsAbVR8z5EZcfIgwci8GYMa/z3jJMJopwAwTvcez+5n/sVMafNcNhTTTYbtcbsaA8Ml A2V6OAbl5J6oIFYmgMUs2ogI= X-Received: by 127.0.0.2 with SMTP id dmaGYY7687511xNSehI8ELkN; Wed, 07 Feb 2024 10:15:26 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.5260.1707329725802710039 for ; Wed, 07 Feb 2024 10:15:26 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-588-5V1OKfQOPqOLjntnhYqtxQ-1; Wed, 07 Feb 2024 13:15:21 -0500 X-MC-Unique: 5V1OKfQOPqOLjntnhYqtxQ-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7C75A85A58B; Wed, 7 Feb 2024 18:15:20 +0000 (UTC) X-Received: from [10.39.195.126] (unknown [10.39.195.126]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5CE6D40C9444; Wed, 7 Feb 2024 18:15:18 +0000 (UTC) Message-ID: Date: Wed, 7 Feb 2024 19:15:17 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension To: devel@edk2.groups.io, tphan@ventanamicro.com Cc: 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 References: <20240207012910.2133-1-tphan@ventanamicro.com> <20240207012910.2133-3-tphan@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: <20240207012910.2133-3-tphan@ventanamicro.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 67GQgRsr6h7wMQEiTPxlGKnZx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=G4Hqig5Z; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) 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. >=20 > Signed-off-by: Tuan Phan > --- > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 25 ++++++++++++++++++- > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 + > 2 files changed, 25 insertions(+), 1 deletion(-) >=20 > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/UefiC= puPkg/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 > =20 > +#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. > 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; > } > =20 > + if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMA= SK) !=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; > } > =20 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? (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. > @@ -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. > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf b/Uef= iCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > index 51ebe1750e97..1dbaa81f3608 100644 > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > @@ -28,3 +28,4 @@ > =20 > [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 (#115243): https://edk2.groups.io/g/devel/message/115243 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-