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

["Michael Brown" <mcb30@ipxe.org>]

> +/**
> +  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.


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