Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 05/20] UefiCpuPkg: Add CpuTimerDxe module

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

On Thu, Feb 09, 2023 at 10:17:56AM +0000, Michael Brown wrote:
> > +/**
> > +  Timer Interrupt Handler.
> > +
> > +  @param InterruptType    The type of interrupt that occured
> > +  @param SystemContext    A pointer to the system context when the interrupt occured
> > +**/
> > +VOID
> > +EFIAPI
> > +TimerInterruptHandler (
> > +  IN EFI_EXCEPTION_TYPE  InterruptType,
> > +  IN EFI_SYSTEM_CONTEXT  SystemContext
> > +  )
> > +{
> > +  EFI_TPL  OriginalTPL;
> > +  UINT64   RiscvTimer;
> > +
> > +  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> > +  if (mTimerNotifyFunction != NULL) {
> > +    mTimerNotifyFunction (mTimerPeriod);
> > +  }
> > +
> > +  RiscVDisableTimerInterrupt (); // Disable SMode timer int
> > +  RiscVClearPendingTimerInterrupt ();
> > +  if (mTimerPeriod == 0) {
> > +    gBS->RestoreTPL (OriginalTPL);
> > +    RiscVDisableTimerInterrupt (); // Disable SMode timer int
> > +    return;
> > +  }
> > +
> > +  RiscvTimer               = RiscVReadTimer ();
> > +  SbiSetTimer (RiscvTimer += mTimerPeriod);
> > +  gBS->RestoreTPL (OriginalTPL);
> > +  RiscVEnableTimerInterrupt (); // enable SMode timer int
> > +}
> 
> This design looks as though it does not support nested timer interrupts.
> The call to RestoreTPL() may invoke callbacks that may themselves include
> delay loops that wait upon further timer interrupts.  With the above code,
> those timer interrupts will never arrive since the timer interrupt is
> disabled at the point that you call RestoreTPL().
> 
> This will break device drivers such as those for USB network devices that
> rely on nested timer interrupts.
> 
> 
Hi Michael!,

Thanks a lot for this feedback and background. We are aware of few issues
in this module. Currently, it is mostly porting what exists today in
edk2-platforms repo. We want to add all these additional fixes after
this basic thing is merged. That way we will have git history instead of
combining all fixes single commit. Andrei has a patch ready and waiting
for this to get merged. We can either combine this with his patch or
create one more.

Would that strategy be fine with you?

Thanks!
Sunil

> The easiest fix is to use NestedTimerInterruptLib and change the code as
> follows:
> 
> Add at the start of the function:
> 
>   STATIC NESTED_INTERRUPT_STATE NestedInterruptState;
> 
> Change:
> 
>   OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> 
> to
> 
>   OriginalTPL = NestedInterruptRaiseTPL ();
> 
> Change:
> 
>   gBS->RestoreTPL (OriginalTPL);
>   RiscVEnableTimerInterrupt (); // enable SMode timer int
> 
> to
> 
>   RiscVEnableTimerInterrupt (); // enable SMode timer int
>   NestedInterruptRestoreTPL (OriginalTpl, SystemContext,
> &NestedInterruptState);
> 
> Note that the timer interrupt is then correctly re-enabled before the call
> to NestedInterruptRestoreTPL().  NestedInterruptTplLib takes care of the
> mess required behind the scenes to make this all provably safe.
> 
> 
> You'll also need to extend Library/NestedInterruptTplLib/Iret.c to support
> MDE_CPU_RISCV64: it should be fairly obvious what's needed there.
> 
> 
> See commit https://github.com/tianocore/edk2/commit/a086f4a63b for the
> equivalent fix being applied to OVMF.
> 
> 
> You can find the background leading up to the creation of
> NestedInterruptTplLib at:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=4162
>   https://github.com/tianocore/edk2/commit/9bf473da4c
>   https://github.com/tianocore/edk2/commit/a24fbd6061
>   https://github.com/tianocore/edk2/commit/a086f4a63b
> 
> Thanks,
> 
> Michael
>