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

["Laszlo Ersek" <lersek@redhat.com>]

On 1/4/24 16:01, Sunil V L wrote:
> 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?

Sorry, I was unclear: the macro *name* was fine, IMO; my proposal was to
change the *replacement text* from 0x2 to BIT1. (Because BIT1 is another
macro, from "MdePkg/Include/Base.h"; those are frequently used all over
edk2.)

Thanks!
Laszlo

> 
>>> +
>>>  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 (#113284): https://edk2.groups.io/g/devel/message/113284
Mute This Topic: https://groups.io/mt/103501843/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-