Re: [edk2-devel] [PATCH 3/4] UefiCpuPkg/CpuTimerDxeRiscV64: Add support for Sstc

["Sunil V L" <sunilvl@ventanamicro.com>]

Hi Laszlo,

Thank you very much for the review!.

On Thu, Jan 04, 2024 at 03:38:17PM +0100, Laszlo Ersek wrote:
> On 1/3/24 14:58, Sunil V L wrote:
> > Sstc extension allows to program the timer and receive the interrupt
> > without using an SBI call. This reduces the latency to generate the timer
> > interrupt. So, detect whether Sstc extension is supported and use the
> > stimecmp register directly to program the timer interrupt.
> > 
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  .../CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf |  1 +
> >  UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h         |  2 ++
> >  UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c         | 30 +++++++++++++++++--
> >  3 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
> > index aba660186dc0..f2a2cf12caef 100644
> > --- a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
> > +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
> > @@ -41,6 +41,7 @@ [Sources.RISCV64]
> >    Timer.c
> >  
> >  [Pcd]
> > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride           ## CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency  ## CONSUMES
> >  
> >  [Protocols]
> > diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
> > index 9b3542230cb5..5e5071b3f0b2 100644
> > --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
> > +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
> > @@ -26,6 +26,8 @@
> >  //
> >  #define DEFAULT_TIMER_TICK_DURATION  100000
> >  
> > +#define RISCV_CPU_FEATURE_SSTC_BITMASK  0x2
> 
> (1) Not a bug by any means, but BIT1 might read more idiomatic.
> 
Agree. Would RISCV_CPU_FEATURE_BIT1_SSTC be better?

> > +
> >  extern VOID
> >  RiscvSetTimerPeriod (
> >    UINT32  TimerPeriod
> > diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> > index 30e48061cd06..4babfb4bfc60 100644
> > --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> > +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> > @@ -44,6 +44,19 @@ STATIC EFI_TIMER_NOTIFY  mTimerNotifyFunction;
> >  STATIC UINT64  mTimerPeriod     = 0;
> >  STATIC UINT64  mLastPeriodStart = 0;
> >  
> > +/**
> > +  Check whether Sstc is enabled in PCD.
> > +
> > +**/
> > +STATIC
> > +BOOLEAN
> > +RiscVIsSstcEnabled (
> > +  VOID
> > +  )
> > +{
> > +  return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_SSTC_BITMASK) != 0);
> > +}
> > +
> >  /**
> >    Timer Interrupt Handler.
> >  
> > @@ -94,7 +107,12 @@ TimerInterruptHandler (
> >                           ),
> >                         1000000u
> >                         );  // convert to tick
> > -  SbiSetTimer (PeriodStart);
> > +  if (RiscVIsSstcEnabled ()) {
> 
> (2) Even though the PCD is currently declared as fixed or
> patchable-in-module, seeing a PcdGet64() call on the call stack of the
> timer interrupt handler (and at a high TPL) makes me uncomfortable. It
> carries a risk that later on we relax the PCD decl to dynamic, and then
> this code would become brittle.
> 
> I propose: either replace the PcdGet64 call above with FixedPcdGet64 (so
> it can never land in the runtime / dynamic PCD protocol), or perform the
> PCD check in the entry point function of the driver, and store the
> result in a STATIC BOOLEAN variable. Then further PCD accesses (dynamic
> or otherwise) will not be needed.
> 
Ahh yes. Good point. Let me use a static variable as you suggested.

> > +    RiscVSetSupervisorTimeCompareRegister (PeriodStart);
> > +  } else {
> > +    SbiSetTimer (PeriodStart);
> > +  }
> > +
> >    RiscVEnableTimerInterrupt (); // enable SMode timer int
> >    gBS->RestoreTPL (OriginalTPL);
> >  }
> > @@ -197,7 +215,11 @@ TimerDriverSetTimerPeriod (
> >                           ),
> >                         1000000u
> >                         ); // convert to tick
> > -  SbiSetTimer (PeriodStart);
> > +  if (RiscVIsSstcEnabled ()) {
> > +    RiscVSetSupervisorTimeCompareRegister (PeriodStart);
> > +  } else {
> > +    SbiSetTimer (PeriodStart);
> > +  }
> >  
> >    mCpu->EnableInterrupt (mCpu);
> >    RiscVEnableTimerInterrupt (); // enable SMode timer int
> 
> (3) This seems like duplicated code. How about replacing the
> RiscVIsSstcEnabled() function with a more substantive function that
> incorporates both the feature check *and* the "PeriodStart" setting?
> Then you can easily call that function from both TimerInterruptHandler()
> and TimerDriverSetTimerPeriod().
>
I agree. Let me update in the next version.
 
> > @@ -282,6 +304,10 @@ TimerDriverInitialize (
> >    //
> >    mTimerNotifyFunction = NULL;
> >  
> > +  if (RiscVIsSstcEnabled ()) {
> > +    DEBUG ((DEBUG_INFO, "%a: Timer interrupt is via Sstc extension\n", __func__));
> > +  }
> > +
> 
> Right, this would be the place to fetch the PCD explicitly and to store
> the result (based on bit-masking) into the global boolean.
>
Yes!

Thanks!
Sunil 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113181): https://edk2.groups.io/g/devel/message/113181
Mute This Topic: https://groups.io/mt/103501843/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-