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

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

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

On Wed, Feb 7, 2024 at 10:15 AM Laszlo Ersek <lersek@redhat.com> wrote:

> On 2/7/24 02:29, Tuan Phan wrote:
> > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will be
> > supported when Svpbmt extension available.
> >
> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > ---
> >  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 25 ++++++++++++++++++-
> >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |  1 +
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > index 826a1d32a1d4..c50a28e97e4b 100644
> > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > @@ -36,6 +36,15 @@
> >  #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)
> > +
> > +#define EFI_MEMORY_CACHETYPE_MASK  (EFI_MEMORY_UC | EFI_MEMORY_WC |  \
> > +                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
> > +                                     EFI_MEMORY_UCE)
> > +
>
> (1) I've stated this elsewhere -- introducing such a macro is justified,
> but calling it EFI_* is not. The EFI_ prefix is reserved for the spec.
>
Will fix it.


>
> >  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48,
> SATP_MODE_SV39, SATP_MODE_OFF };
> >  STATIC UINTN  mMaxRootTableLevel;
> >  STATIC UINTN  mBitPerLevel;
> > @@ -514,6 +523,20 @@ GcdAttributeToPageAttribute (
> >      RiscVAttributes &= ~RISCV_PG_X;
> >    }
> >
> > +  if ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > +    switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
> > +      case EFI_MEMORY_UC:
> > +        RiscVAttributes |= PTE_PBMT_IO;
> > +        break;
> > +      case EFI_MEMORY_WC:
> > +        RiscVAttributes |= PTE_PBMT_NC;
> > +        break;
> > +      default:
> > +        // Default PMA mode
> > +        break;
> > +    }
> > +  }
> > +
> >    return RiscVAttributes;
> >  }
> >
>
> Several questions / observations:
>
> (2) If the feature is cleared in the PCD, does it deserve a warning that
> the attribute setting request cannot be honored?
>
Sure, I will add a warning if the feature has not been enabled.

>
> (3) The memory cacheability attributes are expressed as distinct bits of
> a bitmask because, for expressing *capabilities*, they must be possible
> to OR together. However, when setting actual attributes, I think the
> bitmask should contain *exactly* one bit set -- in other words, the
> value of the bitmask should be an integral power of two (that's not hard
> to check).
>
> Do you agree about this? If so, I'd suggest rejecting the request (with
> an appropriate status code) if zero bits, or multiple bits, are set.
>
>   UINT64  CacheTypeMask;
>
>   CacheType = GcdAttributes & MEMORY_CACHETYPE_MASK;
>   if ((CacheType == 0) ||
>       (((CacheType - 1) & CacheType) != 0)) {
>     return EFI_INVALID_PARAMETER;
>   }
>   switch (CacheType) {
>     ...
>   }
>
> This would of course require changing the GcdAttributeToPageAttribute()
> prototype, because right now the function cannot return an error.
>
> That makes sense. Will fix it. Thanks

>
> > @@ -559,7 +582,7 @@ RiscVSetMemoryAttributes (
> >             BaseAddress,
> >             Length,
> >             PageAttributesSet,
> > -           PTE_ATTRIBUTES_MASK,
> > +           PTE_ATTRIBUTES_MASK | PTE_PBMT_MASK,
> >             (UINTN *)RiscVGetRootTranslateTable (),
> >             TRUE
> >             );
>
> (4) I feel we shouldn't try to clear PTE_PBMT_MASK if
> PcdRiscVFeatureOverride tells us that Svpbmt is not available. Just a
> thought.
>
Sure.

>
> > 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
>
> Thanks
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116128): https://edk2.groups.io/g/devel/message/116128
Mute This Topic: https://groups.io/mt/104211195/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: 7202 bytes --]