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

["Tuan Phan" <tphan@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 7092 bytes --]

Hi Sunil,

On Mon, Mar 18, 2024 at 6:00 AM Sunil V L <sunilvl@ventanamicro.com> wrote:

> Hi Tuan,
>
> On Thu, Mar 14, 2024 at 01:19:16PM -0700, 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 | 106 ++++++++++++++----
> >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |   1 +
> >  2 files changed, 86 insertions(+), 21 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > index 46ba4b4709b1..34300dca5c34 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;
> > @@ -487,32 +492,82 @@ UpdateRegionMapping (
> >  /**
> >    Convert GCD attribute to RISC-V page attribute.
> >
> > -  @param  GcdAttributes The GCD attribute.
> > +  @param  GcdAttributes   The GCD attribute.
> > +  @param  RiscVAttributes The pointer of RISC-V page attribute.
> >
> > -  @return               The RISC-V page attribute.
> > +  @retval EFI_INVALID_PARAMETER   The RiscVAttributes is NULL or cache
> type mask not valid.
> > +  @retval EFI_SUCCESS             The operation succesfully.
> >
> >  **/
> >  STATIC
> > -UINT64
> > +EFI_STATUS
> >  GcdAttributeToPageAttribute (
> > -  IN UINT64  GcdAttributes
> > +  IN UINT64   GcdAttributes,
> > +  OUT UINT64  *RiscVAttributes
> >    )
> >  {
> > -  UINT64  RiscVAttributes;
> > +  UINT64   CacheTypeMask;
> > +  BOOLEAN  PmbtExtEnabled;
> >
> Why not read the PCD once and save in a static variable?
>
I can put it into a static variable if you think it is more clean.

>
> > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > +  if (RiscVAttributes == NULL) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > +
> > +  PmbtExtEnabled = FALSE;
> > +  if ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > +    PmbtExtEnabled = TRUE;
> > +  }
> >
> >    // Determine protection attributes
> >    if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > -    RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> > +    *RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> >    }
> >
> >    // Process eXecute Never attribute
> >    if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > -    RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> > +    *RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> > +  }
> > +
> > +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > +  if ((CacheTypeMask != 0) &&
> > +      (((CacheTypeMask - 1) & CacheTypeMask) != 0))
> > +  {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "%a: More than one bit set in cache type mask (0x%LX)\n",
> > +      __func__,
> > +      CacheTypeMask
> > +      ));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  switch (CacheTypeMask) {
> > +    case EFI_MEMORY_UC:
> > +      if (PmbtExtEnabled) {
> > +        *RiscVAttributes |= PTE_PBMT_IO;
> > +      }
> > +
> > +      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 RiscVAttributes;
> > +  return EFI_SUCCESS;
> >  }
> >
> >  /**
> > @@ -535,29 +590,38 @@ RiscVSetMemoryAttributes (
> >    IN UINT64                Attributes
> >    )
> >  {
> > -  UINT64  PageAttributesSet;
> > +  UINT64      PageAttributesSet;
> > +  UINT64      PageAttributesClear;
> > +  EFI_STATUS  Status;
> >
> > -  PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> > +  Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> >
> Is there a reason to do this prior to checking RiscVMmuEnabled()?
>
Only reason is return error due to invalid parameter before return
EFI_SUCCESS if Mmu not enabled.

>
> >    if (!RiscVMmuEnabled ()) {
> >      return EFI_SUCCESS;
> >    }
> >
> > -  DEBUG (
> > -    (
> > -     DEBUG_VERBOSE,
> > -     "%a: Set %llX page attribute 0x%X\n",
> > -     __func__,
> > -     BaseAddress,
> > -     PageAttributesSet
> > -    )
> > -    );
> > +  PageAttributesClear = PTE_ATTRIBUTES_MASK;
> > +  if ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > +    PageAttributesClear |= PTE_PBMT_MASK;
> > +  }
> > +
> I think static variable would be better.
>
> > +  DEBUG ((
> > +    DEBUG_VERBOSE,
> > +    "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
> > +    __func__,
> > +    BaseAddress,
> > +    PageAttributesSet,
> > +    PageAttributesClear
> > +    ));
> >
> >    return UpdateRegionMapping (
> >             BaseAddress,
> >             Length,
> >             PageAttributesSet,
> > -           PTE_ATTRIBUTES_MASK,
> > +           PageAttributesClear,
> >             (UINT64 *)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 (#116901): https://edk2.groups.io/g/devel/message/116901
Mute This Topic: https://groups.io/mt/104934719/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: 9826 bytes --]