Hi Sunil,
On Mon, Mar 18, 2024 at 6:00 AM Sunil V L <suni...@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 <kra...@redhat.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Rahul Kumar <rahul1.ku...@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Signed-off-by: Tuan Phan <tp...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
