* [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
* 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
* [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
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