Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

["Laszlo Ersek" <lersek@redhat.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-