On Tue, Apr 02, 2024 at 03:42:19PM -0700, Tuan Phan wrote:
> On Tue, Mar 19, 2024 at 9:45 AM Tuan Phan via groups.io <tphan=
> ventanamicro....@groups.io> wrote:
>
> > 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.
> >
> Looks like PcdRiscVFeatureOverride can be a patchable PCD so putting it to
> a static variable may not work.
>
I don't think that will be an issue for this use case. But I don't have
major issue keeping like this itself.
> >
> >> > - 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.
> >
Okay. Thanks!
> >>
> >> > 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
> >> > --
Reviewed-by: Sunil V L <suni...@ventanamicro.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117490): https://edk2.groups.io/g/devel/message/117490
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]
-=-=-=-=-=-=-=-=-=-=-=-
