public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Sunil V L" <sunilvl@ventanamicro.com>
To: Tuan Phan <tphan@ventanamicro.com>
Cc: devel@edk2.groups.io, michael.d.kinney@intel.com,
	gaoliming@byosoft.com.cn, zhiguang.liu@intel.com,
	kraxel@redhat.com, lersek@redhat.com, rahul1.kumar@intel.com,
	ray.ni@intel.com, jiewen.yao@intel.com,
	andrei.warkentin@intel.com, ardb+tianocore@kernel.org
Subject: Re: [edk2-devel] [PATCH v4 3/4] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
Date: Mon, 8 Apr 2024 10:17:18 +0530	[thread overview]
Message-ID: <ZhN21tuSQjqD+b4w@sunil-laptop> (raw)
In-Reply-To: <CABYABGTDEEvzU7cbrEM0r60Ok8LWRKU_Yw9GzKmvv-=Y9WBXow@mail.gmail.com>

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.com@groups.io> wrote:
> 
> > 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.
> >
> 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 <sunilvl@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-04-08  4:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-14 20:19 [edk2-devel] [PATCH v4 0/4] RISC-V: Support Svpbmt extension Tuan Phan
2024-03-14 20:19 ` [edk2-devel] [PATCH v4 1/4] MdePkg.dec: RISC-V: Define override bit for " Tuan Phan
2024-03-18 13:02   ` Sunil V L
2024-03-14 20:19 ` [edk2-devel] [PATCH v4 2/4] UefiCpuPkg: RISC-V: MMU: Explictly use UINT64 instead of UINTN Tuan Phan
2024-03-14 20:19 ` [edk2-devel] [PATCH v4 3/4] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension Tuan Phan
2024-03-18 13:00   ` Sunil V L
2024-03-19 16:44     ` Tuan Phan
     [not found]     ` <17BE38330B281CA5.29196@groups.io>
2024-04-02 22:42       ` Tuan Phan
2024-04-08  4:47         ` Sunil V L [this message]
2024-03-14 20:19 ` [edk2-devel] [PATCH v4 4/4] OvmfPkg/RiscVVirt: Disable " Tuan Phan
2024-03-18 13:01   ` Sunil V L
2024-04-08  5:45 ` [edk2-devel] [PATCH v4 0/4] RISC-V: Support " Sunil V L

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZhN21tuSQjqD+b4w@sunil-laptop \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox