On Wed, Feb 7, 2024 at 10:15 AM Laszlo Ersek <ler...@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 <tp...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
