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
>