On 3/7/24 23:00, Tuan Phan wrote:
>
>
> On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek <ler...@redhat.com
> <mailto: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>
> > <mailto: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> <mailto:kra...@redhat.com
> <mailto:kra...@redhat.com>>>
> > > Cc: Laszlo Ersek <ler...@redhat.com
> <mailto:ler...@redhat.com> <mailto:ler...@redhat.com
> <mailto:ler...@redhat.com>>>
> > > Cc: Rahul Kumar <rahul1.ku...@intel.com
> <mailto:rahul1.ku...@intel.com>
> > <mailto:rahul1.ku...@intel.com <mailto:rahul1.ku...@intel.com>>>
> > > Cc: Ray Ni <ray...@intel.com <mailto:ray...@intel.com>
> <mailto:ray...@intel.com <mailto:ray...@intel.com>>>
> > > Signed-off-by: Tuan Phan <tp...@ventanamicro.com
> <mailto:tp...@ventanamicro.com>
> > <mailto: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>
> > <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://projec...@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify
> <https://projec...@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>
> <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 (#116491): https://edk2.groups.io/g/devel/message/116491
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]
-=-=-=-=-=-=-=-=-=-=-=-
