On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek <ler...@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 <ler...@redhat.com > > <mailto:ler...@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 <kra...@redhat.com <mailto:kra...@redhat.com>> > > > Cc: Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com>> > > > Cc: Rahul Kumar <rahul1.ku...@intel.com > > <mailto:rahul1.ku...@intel.com>> > > > Cc: Ray Ni <ray...@intel.com <mailto:ray...@intel.com>> > > > Signed-off-by: Tuan Phan <tp...@ventanamicro.com > > <mailto:tp...@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://projec...@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify < > https://projec...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
