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 <kra...@redhat.com>
> > Cc: Rahul Kumar <rahul1.ku...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Cc: Andrei Warkentin <andrei.warken...@intel.com>
> > Signed-off-by: Sunil V L <suni...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
