* [edk2-devel] [PATCH v3 0/3] RISC-V: Support Svpbmt extension
@ 2024-03-01 1:29 Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 1/3] MdePkg.dec: RISC-V: Define override bit for " Tuan Phan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Tuan Phan @ 2024-03-01 1:29 UTC (permalink / raw)
To: devel
Cc: michael.d.kinney, gaoliming, zhiguang.liu, kraxel, lersek,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore, Tuan Phan
This series adds support for RISC-V Svpbmt extension.
The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will
be mapped to IO and NC mode defined in PBMT field.
v3:
- Addressed Laszlo's comments.
v2:
- Generated patch for each package.
Tuan Phan (3):
MdePkg.dec: RISC-V: Define override bit for Svpbmt extension
UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
OvmfPkg/RiscVVirt: Disable Svpbmt extension
MdePkg/MdePkg.dec | 2 +
OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 +-
.../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++++++++++++++---
.../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
4 files changed, 91 insertions(+), 15 deletions(-)
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116196): https://edk2.groups.io/g/devel/message/116196
Mute This Topic: https://groups.io/mt/104656464/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v3 1/3] MdePkg.dec: RISC-V: Define override bit for Svpbmt extension
2024-03-01 1:29 [edk2-devel] [PATCH v3 0/3] RISC-V: Support Svpbmt extension Tuan Phan
@ 2024-03-01 1:29 ` Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support " Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 3/3] OvmfPkg/RiscVVirt: Disable " Tuan Phan
2 siblings, 0 replies; 9+ messages in thread
From: Tuan Phan @ 2024-03-01 1:29 UTC (permalink / raw)
To: devel
Cc: michael.d.kinney, gaoliming, zhiguang.liu, kraxel, lersek,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore, Tuan Phan
Define the BIT 2 as the override bit for Svpbmt extension. This will
be used by RISC-V MMU library to support EFI_MEMORY_UC and
EFI_MEMORY_WC.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
MdePkg/MdePkg.dec | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 0459418906f8..6850acb96b92 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2407,6 +2407,8 @@
# previous stage has feature enabled and user wants to disable it.
# BIT 1 = Supervisor Time Compare (Sstc). This bit is relevant only if
# previous stage has feature enabled and user wants to disable it.
+ # BIT 2 = Page-Based Memory Types (Pbmt). This bit is relevant only if
+ # previous stage has feature enabled and user wants to disable it.
#
gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116197): https://edk2.groups.io/g/devel/message/116197
Mute This Topic: https://groups.io/mt/104656465/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
2024-03-01 1:29 [edk2-devel] [PATCH v3 0/3] RISC-V: Support Svpbmt extension Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 1/3] MdePkg.dec: RISC-V: Define override bit for " Tuan Phan
@ 2024-03-01 1:29 ` Tuan Phan
2024-03-01 12:14 ` Laszlo Ersek
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 3/3] OvmfPkg/RiscVVirt: Disable " Tuan Phan
2 siblings, 1 reply; 9+ messages in thread
From: Tuan Phan @ 2024-03-01 1:29 UTC (permalink / raw)
To: devel
Cc: michael.d.kinney, gaoliming, zhiguang.liu, kraxel, lersek,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore, Tuan Phan
The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
supported when Svpbmt extension available.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
.../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++++++++++++++---
.../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
2 files changed, 88 insertions(+), 14 deletions(-)
diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
index 826a1d32a1d4..f4419bb8f380 100644
--- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
+++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
@@ -36,6 +36,11 @@
#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)
+
STATIC UINTN mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48, SATP_MODE_SV39, SATP_MODE_OFF };
STATIC UINTN mMaxRootTableLevel;
STATIC UINTN mBitPerLevel;
@@ -489,32 +494,89 @@ UpdateRegionMapping (
/**
Convert GCD attribute to RISC-V page attribute.
- @param GcdAttributes The GCD attribute.
+ @param GcdAttributes The GCD attribute.
+ @param RiscVAttribtues The pointer of RISC-V page attribute.
- @return The RISC-V page attribute.
+ @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or cache type mask not valid.
+ @retval EFI_SUCCESS The operation succesfully.
**/
STATIC
-UINTN
+EFI_STATUS
GcdAttributeToPageAttribute (
- IN UINTN GcdAttributes
+ IN UINTN GcdAttributes,
+ OUT UINTN *RiscVAttributes
)
{
- UINTN RiscVAttributes;
+ UINT64 CacheTypeMask;
+ BOOLEAN PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
- RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
+ if (!RiscVAttributes) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
// Determine protection attributes
if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
- RiscVAttributes &= ~(RISCV_PG_W);
+ *RiscVAttributes &= ~(RISCV_PG_W);
}
// Process eXecute Never attribute
if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
- RiscVAttributes &= ~RISCV_PG_X;
+ *RiscVAttributes &= ~RISCV_PG_X;
+ }
+
+ CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
+ if ((CacheTypeMask != 0) &&
+ (((CacheTypeMask - 1) & CacheTypeMask) != 0))
+ {
+ DEBUG (
+ (
+ DEBUG_ERROR,
+ "%a: The cache type mask (0x%llX) should contain exactly one bit set\n",
+ __func__,
+ CacheTypeMask
+ )
+ );
+ return EFI_INVALID_PARAMETER;
}
- return RiscVAttributes;
+ switch (CacheTypeMask) {
+ case EFI_MEMORY_UC:
+ if (PmbtExtEnabled) {
+ *RiscVAttributes |= PTE_PBMT_IO;
+ } else {
+ DEBUG (
+ (
+ DEBUG_VERBOSE,
+ "%a: EFI_MEMORY_UC set but Pmbt extension not available\n",
+ __func__
+ )
+ );
+ }
+
+ break;
+ case EFI_MEMORY_WC:
+ if (PmbtExtEnabled) {
+ *RiscVAttributes |= PTE_PBMT_NC;
+ } else {
+ DEBUG (
+ (
+ DEBUG_VERBOSE,
+ "%a: EFI_MEMORY_WC set but Pmbt extension not available\n",
+ __func__
+ )
+ );
+ }
+
+ break;
+ default:
+ // Default PMA mode
+ break;
+ }
+
+ return EFI_SUCCESS;
}
/**
@@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
IN UINTN Attributes
)
{
- UINTN PageAttributesSet;
+ UINTN PageAttributesSet;
+ UINTN PageAttributesClear;
+ EFI_STATUS Status;
- PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
+ Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
if (!RiscVMmuEnabled ()) {
return EFI_SUCCESS;
}
+ PageAttributesClear = PTE_ATTRIBUTES_MASK;
+ if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
+ PageAttributesClear |= PTE_PBMT_MASK;
+ }
+
DEBUG (
(
DEBUG_VERBOSE,
- "%a: Set %llX page attribute 0x%X\n",
+ "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n",
__func__,
BaseAddress,
- PageAttributesSet
+ PageAttributesSet,
+ PageAttributesClear
)
);
@@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
BaseAddress,
Length,
PageAttributesSet,
- PTE_ATTRIBUTES_MASK,
+ PageAttributesClear,
(UINTN *)RiscVGetRootTranslateTable (),
TRUE
);
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
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116198): https://edk2.groups.io/g/devel/message/116198
Mute This Topic: https://groups.io/mt/104656466/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v3 3/3] OvmfPkg/RiscVVirt: Disable Svpbmt extension
2024-03-01 1:29 [edk2-devel] [PATCH v3 0/3] RISC-V: Support Svpbmt extension Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 1/3] MdePkg.dec: RISC-V: Define override bit for " Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support " Tuan Phan
@ 2024-03-01 1:29 ` Tuan Phan
2 siblings, 0 replies; 9+ messages in thread
From: Tuan Phan @ 2024-03-01 1:29 UTC (permalink / raw)
To: devel
Cc: michael.d.kinney, gaoliming, zhiguang.liu, kraxel, lersek,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore, Tuan Phan
Disable Svpbmt extension as QEMU not enables it in default config.
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index 6bc7c90f31dc..b8338d2eb5f5 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -203,7 +203,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
[PcdsFixedAtBuild.common]
- gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFC
+ gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFF8
gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116199): https://edk2.groups.io/g/devel/message/116199
Mute This Topic: https://groups.io/mt/104656467/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support " Tuan Phan
@ 2024-03-01 12:14 ` Laszlo Ersek
2024-03-01 23:20 ` Tuan Phan
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2024-03-01 12:14 UTC (permalink / raw)
To: devel, tphan
Cc: michael.d.kinney, gaoliming, zhiguang.liu, kraxel, rahul1.kumar,
ray.ni, sunilvl, jiewen.yao, andrei.warkentin, ardb+tianocore
On 3/1/24 02:29, Tuan Phan wrote:
> The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> supported when Svpbmt extension available.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
> .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++++++++++++++---
> .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
> 2 files changed, 88 insertions(+), 14 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> index 826a1d32a1d4..f4419bb8f380 100644
> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> @@ -36,6 +36,11 @@
> #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)
> +
> STATIC UINTN mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48, SATP_MODE_SV39, SATP_MODE_OFF };
> STATIC UINTN mMaxRootTableLevel;
> STATIC UINTN mBitPerLevel;
> @@ -489,32 +494,89 @@ UpdateRegionMapping (
> /**
> Convert GCD attribute to RISC-V page attribute.
>
> - @param GcdAttributes The GCD attribute.
> + @param GcdAttributes The GCD attribute.
> + @param RiscVAttribtues The pointer of RISC-V page attribute.
>
> - @return The RISC-V page attribute.
> + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or cache type mask not valid.
> + @retval EFI_SUCCESS The operation succesfully.
>
> **/
> STATIC
> -UINTN
> +EFI_STATUS
> GcdAttributeToPageAttribute (
> - IN UINTN GcdAttributes
> + IN UINTN GcdAttributes,
Just noticing: why is GcdAttributes *not* UINT64 in the first place?
All the bit macros we test against it, such as EFI_MEMORY_RO
(0x0000000000020000ULL) are of type unsigned long long (UINT64).
> + OUT UINTN *RiscVAttributes
> )
> {
> - UINTN RiscVAttributes;
> + UINT64 CacheTypeMask;
> + BOOLEAN PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
- Per the edk2 coding style, locals should not be initialized (separate
assignment is needed).
- Bitmask checks always need an explicit comparison, such as
((a & b) != 0)
or similar. Implicitly interpreting (a & b) as a truth value is not
appropriate.
- "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
BOOLEAN PmbtExtEnabled;
PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
>
> - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> + if (!RiscVAttributes) {
- The coding style requires an explicit nullity check:
if (RiscVAttributes == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
>
> // Determine protection attributes
> if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> - RiscVAttributes &= ~(RISCV_PG_W);
> + *RiscVAttributes &= ~(RISCV_PG_W);
> }
>
> // Process eXecute Never attribute
> if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> - RiscVAttributes &= ~RISCV_PG_X;
> + *RiscVAttributes &= ~RISCV_PG_X;
> + }
> +
My next comment is unrelated to the patch, it's just something that
catches my eye, and I think is worth fixing:
RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3 (0x00000008).
Meaning, they are of type *signed int* (INT32). Applying the bit-neg
operator on them produces a negative value (because it flips the sign
bit), which is very ugly.
I suggest a separate patch for changing these into
~(UINTN)RISCV_PG_W
~(UINTN)RISCV_PG_X
Alternatively, you could do
*RiscVAttributes = RISCV_PG_R;
if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
*RiscVAttributes |= RISCV_PG_W;
}
if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
*RiscVAttributes |= RISCV_PG_X;
}
Either way: separate patch.
> + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> + if ((CacheTypeMask != 0) &&
> + (((CacheTypeMask - 1) & CacheTypeMask) != 0))
This is not what I recommended in my previous review
<https://edk2.groups.io/g/devel/message/115243>.
Compare:
(CacheTypeMask != 0) && ...
versus
(CacheTypeMask == 0) || ...
Both of these ensure that the power-of-two check in the second
subcondition (i.e., the subtraction of 1) is avoided when CacheTypeMask
is zero. In the first variant, you get (FALSE && ...), in the second
variant, you get (TRUE || ...); therefore, the power-of-two check is
short-circuited for a zero input in both variants.
However, considering the larger CacheTypeMask validation, your variant
is incorrect, because a zero CacheTypeMask will ultimately evaluate the
condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return
EFI_INVALID_PARAMETER" statement will not be reached. Whereas (TRUE ||
...) is TRUE, and so we return EFI_INVALID_PARAMETER for CacheTypeMask==0.
> + {
> + DEBUG (
> + (
> + DEBUG_ERROR,
> + "%a: The cache type mask (0x%llX) should contain exactly one bit set\n",
- Edk2's PrintLib does not use "ll" length modifiers. %u, %x and %X are
for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore, you may
replace "l" with "L" freely.
- We generally group together the double parens for DEBUG invocations:
DEBUG ((
DEBUG_ERROR,
"%a: The cache type mask (0x%lX) ...\n",
__func__,
CacheTypeMask
));
> + __func__,
> + CacheTypeMask
> + )
> + );
The indentation of the closing parens is not correct either; please put
your patches through uncrustify first. (CI will reject these issues
anyway, in github pull requests.)
For running uncrustify locally:
- clone
<https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>
- check it out at tag 73.0.8 (the tag that edk2 CI uses on github is in
".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml")
- build it (IIRC it uses cmake)
- with nothing dirty in the working tree (i.e., everything committed, or
at least stashed to the index), run
uncrustify \
-c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
--replace \
--no-backup \
--if-changed \
-F file-list.txt
> + return EFI_INVALID_PARAMETER;
> }
>
> - return RiscVAttributes;
> + switch (CacheTypeMask) {
> + case EFI_MEMORY_UC:
> + if (PmbtExtEnabled) {
> + *RiscVAttributes |= PTE_PBMT_IO;
> + } else {
> + DEBUG (
> + (
> + DEBUG_VERBOSE,
> + "%a: EFI_MEMORY_UC set but Pmbt extension not available\n",
> + __func__
> + )
> + );
> + }
> +
> + break;
> + case EFI_MEMORY_WC:
> + if (PmbtExtEnabled) {
> + *RiscVAttributes |= PTE_PBMT_NC;
> + } else {
> + DEBUG (
> + (
> + DEBUG_VERBOSE,
> + "%a: EFI_MEMORY_WC set but Pmbt extension not available\n",
> + __func__
> + )
> + );
> + }
> +
> + break;
> + default:
> + // Default PMA mode
> + break;
> + }
> +
> + return EFI_SUCCESS;
> }
>
> /**
> @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
> IN UINTN Attributes
> )
> {
> - UINTN PageAttributesSet;
> + UINTN PageAttributesSet;
> + UINTN PageAttributesClear;
> + EFI_STATUS Status;
>
> - PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> + Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
>
> if (!RiscVMmuEnabled ()) {
> return EFI_SUCCESS;
> }
>
> + PageAttributesClear = PTE_ATTRIBUTES_MASK;
> + if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> + PageAttributesClear |= PTE_PBMT_MASK;
> + }
> +
> DEBUG (
> (
> DEBUG_VERBOSE,
> - "%a: Set %llX page attribute 0x%X\n",
> + "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n",
> __func__,
> BaseAddress,
> - PageAttributesSet
> + PageAttributesSet,
> + PageAttributesClear
> )
> );
- UINT64 should be formatted with %[Ll][uxX].
- UINT32 should be formatted with %[uxX].
- UINTN is trickier, there is no dedicated conversion specifier. The
portable solution (between 32-bit and 64-bit platforms in edk2) is to
(a) cast the UINTN value to UINT64, (b) format the latter with %[Ll][uxX].
So you need something like
DEBUG ((
DEBUG_VERBOSE,
"%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
__func__,
BaseAddress, // this is UINT64
(UINT64)PageAttributesSet, // originally UINTN
(UINT64)PageAttributesClear // originally UINTN
));
>
> @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
> BaseAddress,
> Length,
> PageAttributesSet,
> - PTE_ATTRIBUTES_MASK,
> + PageAttributesClear,
> (UINTN *)RiscVGetRootTranslateTable (),
> TRUE
> );
> 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
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116239): https://edk2.groups.io/g/devel/message/116239
Mute This Topic: https://groups.io/mt/104656466/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
2024-03-01 12:14 ` Laszlo Ersek
@ 2024-03-01 23:20 ` Tuan Phan
2024-03-04 18:01 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Tuan Phan @ 2024-03-01 23:20 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, michael.d.kinney, gaoliming, zhiguang.liu, kraxel,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore
[-- Attachment #1: Type: text/plain, Size: 11749 bytes --]
Thanks for the detailed review. Please see my comments below.
On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek <lersek@redhat.com> wrote:
> On 3/1/24 02:29, Tuan Phan wrote:
> > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> > supported when Svpbmt extension available.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > ---
> > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++++++++++++++---
> > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
> > 2 files changed, 88 insertions(+), 14 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > index 826a1d32a1d4..f4419bb8f380 100644
> > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > @@ -36,6 +36,11 @@
> > #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)
> > +
> > STATIC UINTN mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48,
> SATP_MODE_SV39, SATP_MODE_OFF };
> > STATIC UINTN mMaxRootTableLevel;
> > STATIC UINTN mBitPerLevel;
> > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> > /**
> > Convert GCD attribute to RISC-V page attribute.
> >
> > - @param GcdAttributes The GCD attribute.
> > + @param GcdAttributes The GCD attribute.
> > + @param RiscVAttribtues The pointer of RISC-V page attribute.
> >
> > - @return The RISC-V page attribute.
> > + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or cache
> type mask not valid.
> > + @retval EFI_SUCCESS The operation succesfully.
> >
> > **/
> > STATIC
> > -UINTN
> > +EFI_STATUS
> > GcdAttributeToPageAttribute (
> > - IN UINTN GcdAttributes
> > + IN UINTN GcdAttributes,
>
> Just noticing: why is GcdAttributes *not* UINT64 in the first place?
>
> All the bit macros we test against it, such as EFI_MEMORY_RO
> (0x0000000000020000ULL) are of type unsigned long long (UINT64).
>
Good catch. Will fix it.
>
> > + OUT UINTN *RiscVAttributes
> > )
> > {
> > - UINTN RiscVAttributes;
> > + UINT64 CacheTypeMask;
> > + BOOLEAN PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
>
> - Per the edk2 coding style, locals should not be initialized (separate
> assignment is needed).
>
> - Bitmask checks always need an explicit comparison, such as
>
> ((a & b) != 0)
>
> or similar. Implicitly interpreting (a & b) as a truth value is not
> appropriate.
>
> - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
>
> BOOLEAN PmbtExtEnabled;
>
> PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
>
> Will fix it.
> >
> > - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > + if (!RiscVAttributes) {
>
> - The coding style requires an explicit nullity check:
>
> if (RiscVAttributes == NULL) {
>
Will fix it.
>
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> >
> > // Determine protection attributes
> > if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > - RiscVAttributes &= ~(RISCV_PG_W);
> > + *RiscVAttributes &= ~(RISCV_PG_W);
> > }
> >
> > // Process eXecute Never attribute
> > if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > - RiscVAttributes &= ~RISCV_PG_X;
> > + *RiscVAttributes &= ~RISCV_PG_X;
> > + }
> > +
>
> My next comment is unrelated to the patch, it's just something that
> catches my eye, and I think is worth fixing:
>
> RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3 (0x00000008).
> Meaning, they are of type *signed int* (INT32). Applying the bit-neg
> operator on them produces a negative value (because it flips the sign
> bit), which is very ugly.
>
> I suggest a separate patch for changing these into
>
> ~(UINTN)RISCV_PG_W
> ~(UINTN)RISCV_PG_X
>
> Alternatively, you could do
>
Will fix it in a separate patch along with the above change.
>
> *RiscVAttributes = RISCV_PG_R;
> if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
> *RiscVAttributes |= RISCV_PG_W;
> }
> if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
> *RiscVAttributes |= RISCV_PG_X;
> }
>
> Either way: separate patch.
>
> > + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > + if ((CacheTypeMask != 0) &&
> > + (((CacheTypeMask - 1) & CacheTypeMask) != 0))
>
> This is not what I recommended in my previous review
> <https://edk2.groups.io/g/devel/message/115243>.
>
> Compare:
>
> (CacheTypeMask != 0) && ...
>
> versus
>
> (CacheTypeMask == 0) || ...
>
> Both of these ensure that the power-of-two check in the second
> subcondition (i.e., the subtraction of 1) is avoided when CacheTypeMask
> is zero. In the first variant, you get (FALSE && ...), in the second
> variant, you get (TRUE || ...); therefore, the power-of-two check is
> short-circuited for a zero input in both variants.
>
> However, considering the larger CacheTypeMask validation, your variant
> is incorrect, because a zero CacheTypeMask will ultimately evaluate the
> condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return
> EFI_INVALID_PARAMETER" statement will not be reached. Whereas (TRUE ||
> ...) is TRUE, and so we return EFI_INVALID_PARAMETER for CacheTypeMask==0.
>
Actually the EDK2 passes (CacheTypeMask == 0) to this API during my debug
session.
Given that situation, this function doesn't do anything when CacheTypeMask
== 0 so I think
it should not give the warning message.
>
> > + {
> > + DEBUG (
> > + (
> > + DEBUG_ERROR,
> > + "%a: The cache type mask (0x%llX) should contain exactly one bit
> set\n",
>
> - Edk2's PrintLib does not use "ll" length modifiers. %u, %x and %X are
> for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore, you may
> replace "l" with "L" freely.
>
Will fix it.
>
> - We generally group together the double parens for DEBUG invocations:
>
> DEBUG ((
> DEBUG_ERROR,
> "%a: The cache type mask (0x%lX) ...\n",
> __func__,
> CacheTypeMask
> ));
>
> > + __func__,
> > + CacheTypeMask
> > + )
> > + );
>
> The indentation of the closing parens is not correct either; please put
> your patches through uncrustify first. (CI will reject these issues
> anyway, in github pull requests.)
>
Actually this code is the result of uncrustify modification. Let me check
if anything
wrong with uncrustify.
>
> For running uncrustify locally:
>
> - clone
> <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>
>
> - check it out at tag 73.0.8 (the tag that edk2 CI uses on github is in
> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml")
>
> - build it (IIRC it uses cmake)
>
> - with nothing dirty in the working tree (i.e., everything committed, or
> at least stashed to the index), run
>
> uncrustify \
> -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
> --replace \
> --no-backup \
> --if-changed \
> -F file-list.txt
>
> > + return EFI_INVALID_PARAMETER;
> > }
> >
> > - return RiscVAttributes;
> > + switch (CacheTypeMask) {
> > + case EFI_MEMORY_UC:
> > + if (PmbtExtEnabled) {
> > + *RiscVAttributes |= PTE_PBMT_IO;
> > + } else {
> > + DEBUG (
> > + (
> > + DEBUG_VERBOSE,
> > + "%a: EFI_MEMORY_UC set but Pmbt extension not available\n",
> > + __func__
> > + )
> > + );
> > + }
> > +
> > + break;
> > + case EFI_MEMORY_WC:
> > + if (PmbtExtEnabled) {
> > + *RiscVAttributes |= PTE_PBMT_NC;
> > + } else {
> > + DEBUG (
> > + (
> > + DEBUG_VERBOSE,
> > + "%a: EFI_MEMORY_WC set but Pmbt extension not available\n",
> > + __func__
> > + )
> > + );
> > + }
> > +
> > + break;
> > + default:
> > + // Default PMA mode
> > + break;
> > + }
> > +
> > + return EFI_SUCCESS;
> > }
> >
> > /**
> > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
> > IN UINTN Attributes
> > )
> > {
> > - UINTN PageAttributesSet;
> > + UINTN PageAttributesSet;
> > + UINTN PageAttributesClear;
> > + EFI_STATUS Status;
> >
> > - PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> > + Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
> >
> > if (!RiscVMmuEnabled ()) {
> > return EFI_SUCCESS;
> > }
> >
> > + PageAttributesClear = PTE_ATTRIBUTES_MASK;
> > + if ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > + PageAttributesClear |= PTE_PBMT_MASK;
> > + }
> > +
> > DEBUG (
> > (
> > DEBUG_VERBOSE,
> > - "%a: Set %llX page attribute 0x%X\n",
> > + "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n",
> > __func__,
> > BaseAddress,
> > - PageAttributesSet
> > + PageAttributesSet,
> > + PageAttributesClear
> > )
> > );
>
> - UINT64 should be formatted with %[Ll][uxX].
>
> - UINT32 should be formatted with %[uxX].
>
> - UINTN is trickier, there is no dedicated conversion specifier. The
> portable solution (between 32-bit and 64-bit platforms in edk2) is to
> (a) cast the UINTN value to UINT64, (b) format the latter with %[Ll][uxX].
>
> So you need something like
>
> DEBUG ((
> DEBUG_VERBOSE,
> "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
> __func__,
> BaseAddress, // this is UINT64
> (UINT64)PageAttributesSet, // originally UINTN
> (UINT64)PageAttributesClear // originally UINTN
> ));
>
Thanks for the suggestion. Will fix it.
>
> >
> > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
> > BaseAddress,
> > Length,
> > PageAttributesSet,
> > - PTE_ATTRIBUTES_MASK,
> > + PageAttributesClear,
> > (UINTN *)RiscVGetRootTranslateTable (),
> > TRUE
> > );
> > 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
>
> Laszlo
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116286): https://edk2.groups.io/g/devel/message/116286
Mute This Topic: https://groups.io/mt/104656466/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 16521 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
2024-03-01 23:20 ` Tuan Phan
@ 2024-03-04 18:01 ` Laszlo Ersek
2024-03-07 22:00 ` Tuan Phan
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2024-03-04 18:01 UTC (permalink / raw)
To: devel, tphan
Cc: michael.d.kinney, gaoliming, zhiguang.liu, kraxel, rahul1.kumar,
ray.ni, sunilvl, jiewen.yao, andrei.warkentin, ardb+tianocore
On 3/2/24 00:20, Tuan Phan wrote:
> Thanks for the detailed review. Please see my comments below.
>
> On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
>
> On 3/1/24 02:29, Tuan Phan wrote:
> > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> > supported when Svpbmt extension available.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>
> > Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com
> <mailto:rahul1.kumar@intel.com>>
> > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com
> <mailto:tphan@ventanamicro.com>>
> > ---
> > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101
> +++++++++++++++---
> > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
> > 2 files changed, 88 insertions(+), 14 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > index 826a1d32a1d4..f4419bb8f380 100644
> > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > @@ -36,6 +36,11 @@
> > #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)
> > +
> > STATIC UINTN mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48,
> SATP_MODE_SV39, SATP_MODE_OFF };
> > STATIC UINTN mMaxRootTableLevel;
> > STATIC UINTN mBitPerLevel;
> > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> > /**
> > Convert GCD attribute to RISC-V page attribute.
> >
> > - @param GcdAttributes The GCD attribute.
> > + @param GcdAttributes The GCD attribute.
> > + @param RiscVAttribtues The pointer of RISC-V page attribute.
> >
> > - @return The RISC-V page attribute.
> > + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or
> cache type mask not valid.
> > + @retval EFI_SUCCESS The operation succesfully.
> >
> > **/
> > STATIC
> > -UINTN
> > +EFI_STATUS
> > GcdAttributeToPageAttribute (
> > - IN UINTN GcdAttributes
> > + IN UINTN GcdAttributes,
>
> Just noticing: why is GcdAttributes *not* UINT64 in the first place?
>
> All the bit macros we test against it, such as EFI_MEMORY_RO
> (0x0000000000020000ULL) are of type unsigned long long (UINT64).
>
> Good catch. Will fix it.
>
>
> > + OUT UINTN *RiscVAttributes
> > )
> > {
> > - UINTN RiscVAttributes;
> > + UINT64 CacheTypeMask;
> > + BOOLEAN PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
>
> - Per the edk2 coding style, locals should not be initialized (separate
> assignment is needed).
>
> - Bitmask checks always need an explicit comparison, such as
>
> ((a & b) != 0)
>
> or similar. Implicitly interpreting (a & b) as a truth value is not
> appropriate.
>
> - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
>
> BOOLEAN PmbtExtEnabled;
>
> PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
>
> Will fix it.
>
> >
> > - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > + if (!RiscVAttributes) {
>
> - The coding style requires an explicit nullity check:
>
> if (RiscVAttributes == NULL) {
>
> Will fix it.
>
>
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> >
> > // Determine protection attributes
> > if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > - RiscVAttributes &= ~(RISCV_PG_W);
> > + *RiscVAttributes &= ~(RISCV_PG_W);
> > }
> >
> > // Process eXecute Never attribute
> > if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > - RiscVAttributes &= ~RISCV_PG_X;
> > + *RiscVAttributes &= ~RISCV_PG_X;
> > + }
> > +
>
> My next comment is unrelated to the patch, it's just something that
> catches my eye, and I think is worth fixing:
>
> RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3 (0x00000008).
> Meaning, they are of type *signed int* (INT32). Applying the bit-neg
> operator on them produces a negative value (because it flips the sign
> bit), which is very ugly.
>
> I suggest a separate patch for changing these into
>
> ~(UINTN)RISCV_PG_W
> ~(UINTN)RISCV_PG_X
>
> Alternatively, you could do
>
> Will fix it in a separate patch along with the above change.
>
>
> *RiscVAttributes = RISCV_PG_R;
> if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
> *RiscVAttributes |= RISCV_PG_W;
> }
> if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
> *RiscVAttributes |= RISCV_PG_X;
> }
>
> Either way: separate patch.
>
> > + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > + if ((CacheTypeMask != 0) &&
> > + (((CacheTypeMask - 1) & CacheTypeMask) != 0))
>
> This is not what I recommended in my previous review
> <https://edk2.groups.io/g/devel/message/115243
> <https://edk2.groups.io/g/devel/message/115243>>.
>
> Compare:
>
> (CacheTypeMask != 0) && ...
>
> versus
>
> (CacheTypeMask == 0) || ...
>
> Both of these ensure that the power-of-two check in the second
> subcondition (i.e., the subtraction of 1) is avoided when CacheTypeMask
> is zero. In the first variant, you get (FALSE && ...), in the second
> variant, you get (TRUE || ...); therefore, the power-of-two check is
> short-circuited for a zero input in both variants.
>
> However, considering the larger CacheTypeMask validation, your variant
> is incorrect, because a zero CacheTypeMask will ultimately evaluate the
> condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return
> EFI_INVALID_PARAMETER" statement will not be reached. Whereas (TRUE ||
> ...) is TRUE, and so we return EFI_INVALID_PARAMETER for
> CacheTypeMask==0.
>
> Actually the EDK2 passes (CacheTypeMask == 0) to this API during my
> debug session.
> Given that situation, this function doesn't do anything when
> CacheTypeMask == 0 so I think
> it should not give the warning message.
I would be curious how that can happen; to me a CacheTypeMask==0 input
looks somewhat invalid.
Either way, if such an input *is* valid, then there is a different
problem with the patch: in the debug message we say that the cache type
mask should contain *exactly one* bit set. That's not correct then: it
should say *at most one* bit set. (Because the value 0 has 0 bits set,
and apparently that is valid input.)
>
>
> > + {
> > + DEBUG (
> > + (
> > + DEBUG_ERROR,
> > + "%a: The cache type mask (0x%llX) should contain exactly
> one bit set\n",
>
> - Edk2's PrintLib does not use "ll" length modifiers. %u, %x and %X are
> for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore, you may
> replace "l" with "L" freely.
>
> Will fix it.
>
>
> - We generally group together the double parens for DEBUG invocations:
>
> DEBUG ((
> DEBUG_ERROR,
> "%a: The cache type mask (0x%lX) ...\n",
> __func__,
> CacheTypeMask
> ));
>
> > + __func__,
> > + CacheTypeMask
> > + )
> > + );
>
> The indentation of the closing parens is not correct either; please put
> your patches through uncrustify first. (CI will reject these issues
> anyway, in github pull requests.)
>
> Actually this code is the result of uncrustify modification. Let me
> check if anything
> wrong with uncrustify.
It's very strange. Do you know what your original code (the input to
uncrustify) looked like? I wonder if uncrustify produces strange output
if it sees unexpected input. Normally I wouldn't expect uncrustify to
change the "((" format that I'm proposing. If it still does, then my
request is invalid of course (uncrustify has priority, whatever it does).
Thanks!
Laszlo
>
>
> For running uncrustify locally:
>
> - clone
> <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>>
>
> - check it out at tag 73.0.8 (the tag that edk2 CI uses on github is in
> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml")
>
> - build it (IIRC it uses cmake)
>
> - with nothing dirty in the working tree (i.e., everything committed, or
> at least stashed to the index), run
>
> uncrustify \
> -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
> --replace \
> --no-backup \
> --if-changed \
> -F file-list.txt
>
> > + return EFI_INVALID_PARAMETER;
> > }
> >
> > - return RiscVAttributes;
> > + switch (CacheTypeMask) {
> > + case EFI_MEMORY_UC:
> > + if (PmbtExtEnabled) {
> > + *RiscVAttributes |= PTE_PBMT_IO;
> > + } else {
> > + DEBUG (
> > + (
> > + DEBUG_VERBOSE,
> > + "%a: EFI_MEMORY_UC set but Pmbt extension not
> available\n",
> > + __func__
> > + )
> > + );
> > + }
> > +
> > + break;
> > + case EFI_MEMORY_WC:
> > + if (PmbtExtEnabled) {
> > + *RiscVAttributes |= PTE_PBMT_NC;
> > + } else {
> > + DEBUG (
> > + (
> > + DEBUG_VERBOSE,
> > + "%a: EFI_MEMORY_WC set but Pmbt extension not
> available\n",
> > + __func__
> > + )
> > + );
> > + }
> > +
> > + break;
> > + default:
> > + // Default PMA mode
> > + break;
> > + }
> > +
> > + return EFI_SUCCESS;
> > }
> >
> > /**
> > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
> > IN UINTN Attributes
> > )
> > {
> > - UINTN PageAttributesSet;
> > + UINTN PageAttributesSet;
> > + UINTN PageAttributesClear;
> > + EFI_STATUS Status;
> >
> > - PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> > + Status = GcdAttributeToPageAttribute (Attributes,
> &PageAttributesSet);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
> >
> > if (!RiscVMmuEnabled ()) {
> > return EFI_SUCCESS;
> > }
> >
> > + PageAttributesClear = PTE_ATTRIBUTES_MASK;
> > + if ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > + PageAttributesClear |= PTE_PBMT_MASK;
> > + }
> > +
> > DEBUG (
> > (
> > DEBUG_VERBOSE,
> > - "%a: Set %llX page attribute 0x%X\n",
> > + "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n",
> > __func__,
> > BaseAddress,
> > - PageAttributesSet
> > + PageAttributesSet,
> > + PageAttributesClear
> > )
> > );
>
> - UINT64 should be formatted with %[Ll][uxX].
>
> - UINT32 should be formatted with %[uxX].
>
> - UINTN is trickier, there is no dedicated conversion specifier. The
> portable solution (between 32-bit and 64-bit platforms in edk2) is to
> (a) cast the UINTN value to UINT64, (b) format the latter with
> %[Ll][uxX].
>
> So you need something like
>
> DEBUG ((
> DEBUG_VERBOSE,
> "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
> __func__,
> BaseAddress, // this is UINT64
> (UINT64)PageAttributesSet, // originally UINTN
> (UINT64)PageAttributesClear // originally UINTN
> ));
>
> Thanks for the suggestion. Will fix it.
>
>
> >
> > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
> > BaseAddress,
> > Length,
> > PageAttributesSet,
> > - PTE_ATTRIBUTES_MASK,
> > + PageAttributesClear,
> > (UINTN *)RiscVGetRootTranslateTable (),
> > TRUE
> > );
> > 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
>
> Laszlo
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116337): https://edk2.groups.io/g/devel/message/116337
Mute This Topic: https://groups.io/mt/104656466/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
2024-03-04 18:01 ` Laszlo Ersek
@ 2024-03-07 22:00 ` Tuan Phan
2024-03-08 7:53 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Tuan Phan @ 2024-03-07 22:00 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, michael.d.kinney, gaoliming, zhiguang.liu, kraxel,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore
[-- Attachment #1: Type: text/plain, Size: 15288 bytes --]
On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek <lersek@redhat.com> wrote:
> On 3/2/24 00:20, Tuan Phan wrote:
> > Thanks for the detailed review. Please see my comments below.
> >
> > On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> >
> > On 3/1/24 02:29, Tuan Phan wrote:
> > > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> > > supported when Svpbmt extension available.
> > >
> > > Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>
> > > Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> > > Cc: Rahul Kumar <rahul1.kumar@intel.com
> > <mailto:rahul1.kumar@intel.com>>
> > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> > > Signed-off-by: Tuan Phan <tphan@ventanamicro.com
> > <mailto:tphan@ventanamicro.com>>
> > > ---
> > > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101
> > +++++++++++++++---
> > > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
> > > 2 files changed, 88 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > index 826a1d32a1d4..f4419bb8f380 100644
> > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > @@ -36,6 +36,11 @@
> > > #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)
> > > +
> > > STATIC UINTN mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48,
> > SATP_MODE_SV39, SATP_MODE_OFF };
> > > STATIC UINTN mMaxRootTableLevel;
> > > STATIC UINTN mBitPerLevel;
> > > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> > > /**
> > > Convert GCD attribute to RISC-V page attribute.
> > >
> > > - @param GcdAttributes The GCD attribute.
> > > + @param GcdAttributes The GCD attribute.
> > > + @param RiscVAttribtues The pointer of RISC-V page attribute.
> > >
> > > - @return The RISC-V page attribute.
> > > + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or
> > cache type mask not valid.
> > > + @retval EFI_SUCCESS The operation succesfully.
> > >
> > > **/
> > > STATIC
> > > -UINTN
> > > +EFI_STATUS
> > > GcdAttributeToPageAttribute (
> > > - IN UINTN GcdAttributes
> > > + IN UINTN GcdAttributes,
> >
> > Just noticing: why is GcdAttributes *not* UINT64 in the first place?
> >
> > All the bit macros we test against it, such as EFI_MEMORY_RO
> > (0x0000000000020000ULL) are of type unsigned long long (UINT64).
> >
> > Good catch. Will fix it.
> >
> >
> > > + OUT UINTN *RiscVAttributes
> > > )
> > > {
> > > - UINTN RiscVAttributes;
> > > + UINT64 CacheTypeMask;
> > > + BOOLEAN PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
> >
> > - Per the edk2 coding style, locals should not be initialized
> (separate
> > assignment is needed).
> >
> > - Bitmask checks always need an explicit comparison, such as
> >
> > ((a & b) != 0)
> >
> > or similar. Implicitly interpreting (a & b) as a truth value is not
> > appropriate.
> >
> > - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
> >
> > BOOLEAN PmbtExtEnabled;
> >
> > PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
> >
> > Will fix it.
> >
> > >
> > > - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > > + if (!RiscVAttributes) {
> >
> > - The coding style requires an explicit nullity check:
> >
> > if (RiscVAttributes == NULL) {
> >
> > Will fix it.
> >
> >
> > > + return EFI_INVALID_PARAMETER;
> > > + }
> > > +
> > > + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > >
> > > // Determine protection attributes
> > > if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > > - RiscVAttributes &= ~(RISCV_PG_W);
> > > + *RiscVAttributes &= ~(RISCV_PG_W);
> > > }
> > >
> > > // Process eXecute Never attribute
> > > if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > > - RiscVAttributes &= ~RISCV_PG_X;
> > > + *RiscVAttributes &= ~RISCV_PG_X;
> > > + }
> > > +
> >
> > My next comment is unrelated to the patch, it's just something that
> > catches my eye, and I think is worth fixing:
> >
> > RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3 (0x00000008).
> > Meaning, they are of type *signed int* (INT32). Applying the bit-neg
> > operator on them produces a negative value (because it flips the sign
> > bit), which is very ugly.
> >
> > I suggest a separate patch for changing these into
> >
> > ~(UINTN)RISCV_PG_W
> > ~(UINTN)RISCV_PG_X
> >
> > Alternatively, you could do
> >
> > Will fix it in a separate patch along with the above change.
> >
> >
> > *RiscVAttributes = RISCV_PG_R;
> > if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
> > *RiscVAttributes |= RISCV_PG_W;
> > }
> > if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
> > *RiscVAttributes |= RISCV_PG_X;
> > }
> >
> > Either way: separate patch.
> >
> > > + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > > + if ((CacheTypeMask != 0) &&
> > > + (((CacheTypeMask - 1) & CacheTypeMask) != 0))
> >
> > This is not what I recommended in my previous review
> > <https://edk2.groups.io/g/devel/message/115243
> > <https://edk2.groups.io/g/devel/message/115243>>.
> >
> > Compare:
> >
> > (CacheTypeMask != 0) && ...
> >
> > versus
> >
> > (CacheTypeMask == 0) || ...
> >
> > Both of these ensure that the power-of-two check in the second
> > subcondition (i.e., the subtraction of 1) is avoided when
> CacheTypeMask
> > is zero. In the first variant, you get (FALSE && ...), in the second
> > variant, you get (TRUE || ...); therefore, the power-of-two check is
> > short-circuited for a zero input in both variants.
> >
> > However, considering the larger CacheTypeMask validation, your
> variant
> > is incorrect, because a zero CacheTypeMask will ultimately evaluate
> the
> > condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return
> > EFI_INVALID_PARAMETER" statement will not be reached. Whereas (TRUE
> ||
> > ...) is TRUE, and so we return EFI_INVALID_PARAMETER for
> > CacheTypeMask==0.
> >
> > Actually the EDK2 passes (CacheTypeMask == 0) to this API during my
> > debug session.
> > Given that situation, this function doesn't do anything when
> > CacheTypeMask == 0 so I think
> > it should not give the warning message.
>
> I would be curious how that can happen; to me a CacheTypeMask==0 input
> looks somewhat invalid.
>
> Either way, if such an input *is* valid, then there is a different
> problem with the patch: in the debug message we say that the cache type
> mask should contain *exactly one* bit set. That's not correct then: it
> should say *at most one* bit set. (Because the value 0 has 0 bits set,
> and apparently that is valid input.)
>
How about: "More than one bit set in cache type mask" ? It is a clear
message that we don't expect
more than 1 bit set if not zero.
>
>
> >
> >
> > > + {
> > > + DEBUG (
> > > + (
> > > + DEBUG_ERROR,
> > > + "%a: The cache type mask (0x%llX) should contain exactly
> > one bit set\n",
> >
> > - Edk2's PrintLib does not use "ll" length modifiers. %u, %x and %X
> are
> > for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore, you may
> > replace "l" with "L" freely.
> >
> > Will fix it.
> >
> >
> > - We generally group together the double parens for DEBUG
> invocations:
> >
> > DEBUG ((
> > DEBUG_ERROR,
> > "%a: The cache type mask (0x%lX) ...\n",
> > __func__,
> > CacheTypeMask
> > ));
> >
> > > + __func__,
> > > + CacheTypeMask
> > > + )
> > > + );
> >
> > The indentation of the closing parens is not correct either; please
> put
> > your patches through uncrustify first. (CI will reject these issues
> > anyway, in github pull requests.)
> >
> > Actually this code is the result of uncrustify modification. Let me
> > check if anything
> > wrong with uncrustify.
>
> It's very strange. Do you know what your original code (the input to
> uncrustify) looked like? I wonder if uncrustify produces strange output
> if it sees unexpected input. Normally I wouldn't expect uncrustify to
> change the "((" format that I'm proposing. If it still does, then my
> request is invalid of course (uncrustify has priority, whatever it does).
>
I checked and it comes from an un-correct uncrustify version I used before.
It should be good now.
>
> Thanks!
> Laszlo
>
> >
> >
> > For running uncrustify locally:
> >
> > - clone
> > <
> https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify <
> https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>>
> >
> > - check it out at tag 73.0.8 (the tag that edk2 CI uses on github is
> in
> > ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml")
> >
> > - build it (IIRC it uses cmake)
> >
> > - with nothing dirty in the working tree (i.e., everything
> committed, or
> > at least stashed to the index), run
> >
> > uncrustify \
> > -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
> > --replace \
> > --no-backup \
> > --if-changed \
> > -F file-list.txt
> >
> > > + return EFI_INVALID_PARAMETER;
> > > }
> > >
> > > - return RiscVAttributes;
> > > + switch (CacheTypeMask) {
> > > + case EFI_MEMORY_UC:
> > > + if (PmbtExtEnabled) {
> > > + *RiscVAttributes |= PTE_PBMT_IO;
> > > + } else {
> > > + DEBUG (
> > > + (
> > > + DEBUG_VERBOSE,
> > > + "%a: EFI_MEMORY_UC set but Pmbt extension not
> > available\n",
> > > + __func__
> > > + )
> > > + );
> > > + }
> > > +
> > > + break;
> > > + case EFI_MEMORY_WC:
> > > + if (PmbtExtEnabled) {
> > > + *RiscVAttributes |= PTE_PBMT_NC;
> > > + } else {
> > > + DEBUG (
> > > + (
> > > + DEBUG_VERBOSE,
> > > + "%a: EFI_MEMORY_WC set but Pmbt extension not
> > available\n",
> > > + __func__
> > > + )
> > > + );
> > > + }
> > > +
> > > + break;
> > > + default:
> > > + // Default PMA mode
> > > + break;
> > > + }
> > > +
> > > + return EFI_SUCCESS;
> > > }
> > >
> > > /**
> > > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
> > > IN UINTN Attributes
> > > )
> > > {
> > > - UINTN PageAttributesSet;
> > > + UINTN PageAttributesSet;
> > > + UINTN PageAttributesClear;
> > > + EFI_STATUS Status;
> > >
> > > - PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> > > + Status = GcdAttributeToPageAttribute (Attributes,
> > &PageAttributesSet);
> > > + if (EFI_ERROR (Status)) {
> > > + return Status;
> > > + }
> > >
> > > if (!RiscVMmuEnabled ()) {
> > > return EFI_SUCCESS;
> > > }
> > >
> > > + PageAttributesClear = PTE_ATTRIBUTES_MASK;
> > > + if ((PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > > + PageAttributesClear |= PTE_PBMT_MASK;
> > > + }
> > > +
> > > DEBUG (
> > > (
> > > DEBUG_VERBOSE,
> > > - "%a: Set %llX page attribute 0x%X\n",
> > > + "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n",
> > > __func__,
> > > BaseAddress,
> > > - PageAttributesSet
> > > + PageAttributesSet,
> > > + PageAttributesClear
> > > )
> > > );
> >
> > - UINT64 should be formatted with %[Ll][uxX].
> >
> > - UINT32 should be formatted with %[uxX].
> >
> > - UINTN is trickier, there is no dedicated conversion specifier. The
> > portable solution (between 32-bit and 64-bit platforms in edk2) is to
> > (a) cast the UINTN value to UINT64, (b) format the latter with
> > %[Ll][uxX].
> >
> > So you need something like
> >
> > DEBUG ((
> > DEBUG_VERBOSE,
> > "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
> > __func__,
> > BaseAddress, // this is UINT64
> > (UINT64)PageAttributesSet, // originally UINTN
> > (UINT64)PageAttributesClear // originally UINTN
> > ));
> >
> > Thanks for the suggestion. Will fix it.
> >
> >
> > >
> > > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
> > > BaseAddress,
> > > Length,
> > > PageAttributesSet,
> > > - PTE_ATTRIBUTES_MASK,
> > > + PageAttributesClear,
> > > (UINTN *)RiscVGetRootTranslateTable (),
> > > TRUE
> > > );
> > > 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
> >
> > Laszlo
> >
> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116486): https://edk2.groups.io/g/devel/message/116486
Mute This Topic: https://groups.io/mt/104656466/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 21704 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
2024-03-07 22:00 ` Tuan Phan
@ 2024-03-08 7:53 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2024-03-08 7:53 UTC (permalink / raw)
To: Tuan Phan
Cc: devel, michael.d.kinney, gaoliming, zhiguang.liu, kraxel,
rahul1.kumar, ray.ni, sunilvl, jiewen.yao, andrei.warkentin,
ardb+tianocore
On 3/7/24 23:00, Tuan Phan wrote:
>
>
> On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
>
> On 3/2/24 00:20, Tuan Phan wrote:
> > Thanks for the detailed review. Please see my comments below.
> >
> > On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>
> > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
> >
> > On 3/1/24 02:29, Tuan Phan wrote:
> > > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes
> will be
> > > supported when Svpbmt extension available.
> > >
> > > Cc: Gerd Hoffmann <kraxel@redhat.com
> <mailto:kraxel@redhat.com> <mailto:kraxel@redhat.com
> <mailto:kraxel@redhat.com>>>
> > > Cc: Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com> <mailto:lersek@redhat.com
> <mailto:lersek@redhat.com>>>
> > > Cc: Rahul Kumar <rahul1.kumar@intel.com
> <mailto:rahul1.kumar@intel.com>
> > <mailto:rahul1.kumar@intel.com <mailto:rahul1.kumar@intel.com>>>
> > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>
> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>
> > > Signed-off-by: Tuan Phan <tphan@ventanamicro.com
> <mailto:tphan@ventanamicro.com>
> > <mailto:tphan@ventanamicro.com <mailto:tphan@ventanamicro.com>>>
> > > ---
> > > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101
> > +++++++++++++++---
> > > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 +
> > > 2 files changed, 88 insertions(+), 14 deletions(-)
> > >
> > > diff --git
> a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > index 826a1d32a1d4..f4419bb8f380 100644
> > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > @@ -36,6 +36,11 @@
> > > #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)
> > > +
> > > STATIC UINTN mModeSupport[] = { SATP_MODE_SV57,
> SATP_MODE_SV48,
> > SATP_MODE_SV39, SATP_MODE_OFF };
> > > STATIC UINTN mMaxRootTableLevel;
> > > STATIC UINTN mBitPerLevel;
> > > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> > > /**
> > > Convert GCD attribute to RISC-V page attribute.
> > >
> > > - @param GcdAttributes The GCD attribute.
> > > + @param GcdAttributes The GCD attribute.
> > > + @param RiscVAttribtues The pointer of RISC-V page attribute.
> > >
> > > - @return The RISC-V page attribute.
> > > + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is
> NULL or
> > cache type mask not valid.
> > > + @retval EFI_SUCCESS The operation succesfully.
> > >
> > > **/
> > > STATIC
> > > -UINTN
> > > +EFI_STATUS
> > > GcdAttributeToPageAttribute (
> > > - IN UINTN GcdAttributes
> > > + IN UINTN GcdAttributes,
> >
> > Just noticing: why is GcdAttributes *not* UINT64 in the first
> place?
> >
> > All the bit macros we test against it, such as EFI_MEMORY_RO
> > (0x0000000000020000ULL) are of type unsigned long long (UINT64).
> >
> > Good catch. Will fix it.
> >
> >
> > > + OUT UINTN *RiscVAttributes
> > > )
> > > {
> > > - UINTN RiscVAttributes;
> > > + UINT64 CacheTypeMask;
> > > + BOOLEAN PmbtExtEnabled = (PcdGet64
> (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
> >
> > - Per the edk2 coding style, locals should not be initialized
> (separate
> > assignment is needed).
> >
> > - Bitmask checks always need an explicit comparison, such as
> >
> > ((a & b) != 0)
> >
> > or similar. Implicitly interpreting (a & b) as a truth value
> is not
> > appropriate.
> >
> > - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
> >
> > BOOLEAN PmbtExtEnabled;
> >
> > PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
> >
> > Will fix it.
> >
> > >
> > > - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > > + if (!RiscVAttributes) {
> >
> > - The coding style requires an explicit nullity check:
> >
> > if (RiscVAttributes == NULL) {
> >
> > Will fix it.
> >
> >
> > > + return EFI_INVALID_PARAMETER;
> > > + }
> > > +
> > > + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > >
> > > // Determine protection attributes
> > > if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > > - RiscVAttributes &= ~(RISCV_PG_W);
> > > + *RiscVAttributes &= ~(RISCV_PG_W);
> > > }
> > >
> > > // Process eXecute Never attribute
> > > if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > > - RiscVAttributes &= ~RISCV_PG_X;
> > > + *RiscVAttributes &= ~RISCV_PG_X;
> > > + }
> > > +
> >
> > My next comment is unrelated to the patch, it's just something
> that
> > catches my eye, and I think is worth fixing:
> >
> > RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3
> (0x00000008).
> > Meaning, they are of type *signed int* (INT32). Applying the
> bit-neg
> > operator on them produces a negative value (because it flips
> the sign
> > bit), which is very ugly.
> >
> > I suggest a separate patch for changing these into
> >
> > ~(UINTN)RISCV_PG_W
> > ~(UINTN)RISCV_PG_X
> >
> > Alternatively, you could do
> >
> > Will fix it in a separate patch along with the above change.
> >
> >
> > *RiscVAttributes = RISCV_PG_R;
> > if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
> > *RiscVAttributes |= RISCV_PG_W;
> > }
> > if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
> > *RiscVAttributes |= RISCV_PG_X;
> > }
> >
> > Either way: separate patch.
> >
> > > + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > > + if ((CacheTypeMask != 0) &&
> > > + (((CacheTypeMask - 1) & CacheTypeMask) != 0))
> >
> > This is not what I recommended in my previous review
> > <https://edk2.groups.io/g/devel/message/115243
> <https://edk2.groups.io/g/devel/message/115243>
> > <https://edk2.groups.io/g/devel/message/115243
> <https://edk2.groups.io/g/devel/message/115243>>>.
> >
> > Compare:
> >
> > (CacheTypeMask != 0) && ...
> >
> > versus
> >
> > (CacheTypeMask == 0) || ...
> >
> > Both of these ensure that the power-of-two check in the second
> > subcondition (i.e., the subtraction of 1) is avoided when
> CacheTypeMask
> > is zero. In the first variant, you get (FALSE && ...), in the
> second
> > variant, you get (TRUE || ...); therefore, the power-of-two
> check is
> > short-circuited for a zero input in both variants.
> >
> > However, considering the larger CacheTypeMask validation, your
> variant
> > is incorrect, because a zero CacheTypeMask will ultimately
> evaluate the
> > condition to FALSE -- (FALSE && ...) is FALSE --, and so the
> "return
> > EFI_INVALID_PARAMETER" statement will not be reached. Whereas
> (TRUE ||
> > ...) is TRUE, and so we return EFI_INVALID_PARAMETER for
> > CacheTypeMask==0.
> >
> > Actually the EDK2 passes (CacheTypeMask == 0) to this API during my
> > debug session.
> > Given that situation, this function doesn't do anything when
> > CacheTypeMask == 0 so I think
> > it should not give the warning message.
>
> I would be curious how that can happen; to me a CacheTypeMask==0 input
> looks somewhat invalid.
>
> Either way, if such an input *is* valid, then there is a different
> problem with the patch: in the debug message we say that the cache type
> mask should contain *exactly one* bit set. That's not correct then: it
> should say *at most one* bit set. (Because the value 0 has 0 bits set,
> and apparently that is valid input.)
>
> How about: "More than one bit set in cache type mask" ? It is a clear
> message that we don't expect
> more than 1 bit set if not zero.
Sure, that works too.
Laszlo
>
>
>
> >
> >
> > > + {
> > > + DEBUG (
> > > + (
> > > + DEBUG_ERROR,
> > > + "%a: The cache type mask (0x%llX) should contain exactly
> > one bit set\n",
> >
> > - Edk2's PrintLib does not use "ll" length modifiers. %u, %x
> and %X are
> > for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore,
> you may
> > replace "l" with "L" freely.
> >
> > Will fix it.
> >
> >
> > - We generally group together the double parens for DEBUG
> invocations:
> >
> > DEBUG ((
> > DEBUG_ERROR,
> > "%a: The cache type mask (0x%lX) ...\n",
> > __func__,
> > CacheTypeMask
> > ));
> >
> > > + __func__,
> > > + CacheTypeMask
> > > + )
> > > + );
> >
> > The indentation of the closing parens is not correct either;
> please put
> > your patches through uncrustify first. (CI will reject these
> issues
> > anyway, in github pull requests.)
> >
> > Actually this code is the result of uncrustify modification. Let me
> > check if anything
> > wrong with uncrustify.
>
> It's very strange. Do you know what your original code (the input to
> uncrustify) looked like? I wonder if uncrustify produces strange output
> if it sees unexpected input. Normally I wouldn't expect uncrustify to
> change the "((" format that I'm proposing. If it still does, then my
> request is invalid of course (uncrustify has priority, whatever it
> does).
>
> I checked and it comes from an un-correct uncrustify version I used
> before. It should be good now.
>
>
> Thanks!
> Laszlo
>
> >
> >
> > For running uncrustify locally:
> >
> > - clone
> >
> <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify> <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify <https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>>>
> >
> > - check it out at tag 73.0.8 (the tag that edk2 CI uses on
> github is in
> > ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml")
> >
> > - build it (IIRC it uses cmake)
> >
> > - with nothing dirty in the working tree (i.e., everything
> committed, or
> > at least stashed to the index), run
> >
> > uncrustify \
> > -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
> > --replace \
> > --no-backup \
> > --if-changed \
> > -F file-list.txt
> >
> > > + return EFI_INVALID_PARAMETER;
> > > }
> > >
> > > - return RiscVAttributes;
> > > + switch (CacheTypeMask) {
> > > + case EFI_MEMORY_UC:
> > > + if (PmbtExtEnabled) {
> > > + *RiscVAttributes |= PTE_PBMT_IO;
> > > + } else {
> > > + DEBUG (
> > > + (
> > > + DEBUG_VERBOSE,
> > > + "%a: EFI_MEMORY_UC set but Pmbt extension not
> > available\n",
> > > + __func__
> > > + )
> > > + );
> > > + }
> > > +
> > > + break;
> > > + case EFI_MEMORY_WC:
> > > + if (PmbtExtEnabled) {
> > > + *RiscVAttributes |= PTE_PBMT_NC;
> > > + } else {
> > > + DEBUG (
> > > + (
> > > + DEBUG_VERBOSE,
> > > + "%a: EFI_MEMORY_WC set but Pmbt extension not
> > available\n",
> > > + __func__
> > > + )
> > > + );
> > > + }
> > > +
> > > + break;
> > > + default:
> > > + // Default PMA mode
> > > + break;
> > > + }
> > > +
> > > + return EFI_SUCCESS;
> > > }
> > >
> > > /**
> > > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
> > > IN UINTN Attributes
> > > )
> > > {
> > > - UINTN PageAttributesSet;
> > > + UINTN PageAttributesSet;
> > > + UINTN PageAttributesClear;
> > > + EFI_STATUS Status;
> > >
> > > - PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> > > + Status = GcdAttributeToPageAttribute (Attributes,
> > &PageAttributesSet);
> > > + if (EFI_ERROR (Status)) {
> > > + return Status;
> > > + }
> > >
> > > if (!RiscVMmuEnabled ()) {
> > > return EFI_SUCCESS;
> > > }
> > >
> > > + PageAttributesClear = PTE_ATTRIBUTES_MASK;
> > > + if ((PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > > + PageAttributesClear |= PTE_PBMT_MASK;
> > > + }
> > > +
> > > DEBUG (
> > > (
> > > DEBUG_VERBOSE,
> > > - "%a: Set %llX page attribute 0x%X\n",
> > > + "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n",
> > > __func__,
> > > BaseAddress,
> > > - PageAttributesSet
> > > + PageAttributesSet,
> > > + PageAttributesClear
> > > )
> > > );
> >
> > - UINT64 should be formatted with %[Ll][uxX].
> >
> > - UINT32 should be formatted with %[uxX].
> >
> > - UINTN is trickier, there is no dedicated conversion
> specifier. The
> > portable solution (between 32-bit and 64-bit platforms in
> edk2) is to
> > (a) cast the UINTN value to UINT64, (b) format the latter with
> > %[Ll][uxX].
> >
> > So you need something like
> >
> > DEBUG ((
> > DEBUG_VERBOSE,
> > "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
> > __func__,
> > BaseAddress, // this is UINT64
> > (UINT64)PageAttributesSet, // originally UINTN
> > (UINT64)PageAttributesClear // originally UINTN
> > ));
> >
> > Thanks for the suggestion. Will fix it.
> >
> >
> > >
> > > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
> > > BaseAddress,
> > > Length,
> > > PageAttributesSet,
> > > - PTE_ATTRIBUTES_MASK,
> > > + PageAttributesClear,
> > > (UINTN *)RiscVGetRootTranslateTable (),
> > > TRUE
> > > );
> > > 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
> >
> > Laszlo
> >
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116491): https://edk2.groups.io/g/devel/message/116491
Mute This Topic: https://groups.io/mt/104656466/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-08 7:53 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-01 1:29 [edk2-devel] [PATCH v3 0/3] RISC-V: Support Svpbmt extension Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 1/3] MdePkg.dec: RISC-V: Define override bit for " Tuan Phan
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support " Tuan Phan
2024-03-01 12:14 ` Laszlo Ersek
2024-03-01 23:20 ` Tuan Phan
2024-03-04 18:01 ` Laszlo Ersek
2024-03-07 22:00 ` Tuan Phan
2024-03-08 7:53 ` Laszlo Ersek
2024-03-01 1:29 ` [edk2-devel] [PATCH v3 3/3] OvmfPkg/RiscVVirt: Disable " Tuan Phan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox