Re: [PATCH] ArmPkg/TimerDxe: Always perform an EOI, even for spurious interrupts

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 6 March 2018 at 14:25, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/06/18 14:24, Ard Biesheuvel wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> The generic timer driver only EOIs the timer interrupt if
>> the ISTATUS bit is set. This is completely fine if you pretend
>> that spurious interrupts do not exist. But as a matter of fact,
>> they do, and the first one will leave the interrupt activated
>> at the GIC level, making sure that no other interrupt can make
>> it anymore.
>>
>> Making sure that each interrupt Ack is paired with an EOI is the
>> way to go. Oh, and enabling the interrupt each time it is taken
>> is completely pointless. We entered this function for a good
>> reason...
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> index 2416c90f5545..33d7c922221f 100644
>> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> @@ -306,12 +306,13 @@ TimerInterruptHandler (
>>    //
>>    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
>>
>> +  // Signal end of interrupt early to help avoid losing subsequent ticks
>> +  // from long duration handlers
>> +  gInterrupt->EndOfInterrupt (gInterrupt, Source);
>> +
>>    // Check if the timer interrupt is active
>>    if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {
>>
>> -    // Signal end of interrupt early to help avoid losing subsequent ticks from long duration handlers
>> -    gInterrupt->EndOfInterrupt (gInterrupt, Source);
>> -
>>      if (mTimerNotifyFunction) {
>>        mTimerNotifyFunction (mTimerPeriod * mElapsedPeriod);
>>      }
>> @@ -339,9 +340,6 @@ TimerInterruptHandler (
>>      ArmGenericTimerEnableTimer ();
>>    }
>>
>> -  // Enable timer interrupts
>> -  gInterrupt->EnableInterruptSource (gInterrupt, Source);
>> -
>>    gBS->RestoreTPL (OriginalTPL);
>>  }
>>
>>
>
> It is spooky to see this patch on edk2-devel :)
>

:-)

> In RHEL-7 ArmVirtQemu (aka "AAVMF"), we had carried a somewhat similar
> patch, for a long time.
>
> I originally added it in Feb 2015, after we rebased our aarch64 host
> kernel from 3.18.<whatever> to 3.19.<somethingelse>. That host kernel
> (incl. KVM) rebase broke guest UEFI *reboot*. We figured it was a KVM
> regression -- but, because we couldn't figure out the exact KVM problem,
> we plastered it over in the guest firmware, a bit similarly to the
> above. This was done for RHBZ#1188054.
>
> We reported the issue to several KVM people (off-list -- maybe that
> wasn't a great idea, sorry!); you might find the thread under msgid
> <54D3796E.1040907@redhat.com>, subject "virtual timer interrupt stuck
> across guest firmware reboot on KVM", in your 2015 archives :)
>
> At the same time, we filed RHBZ#1189429 for tracking the KVM problem, so
> that once the host kernel got fixed, we could backport those patches,
> and revert the guest firmware kludge.
>
> The host kernel (KVM) fix was
> <https://marc.info/?i=1440942866-23802-1-git-send-email-christoffer.dall@linaro.org>:
>
>   [PATCH 0/9] Rework architected timer and fix UEFI reset
>
> And, indeed we dropped the "AAVMF" kludge (which revert was separately
> tracked under RHBZ#1259395).
>
> So it looks like spurious timer interrupts exists on phys hardware as
> well -- I guess I should have attempted to upstream the guest fw patch
> back then, after all. We might have ended up with an improved version of
> it that could have now covered this recent symptom too, perhaps.
>

I was never aware of any such issues at any point, so a head's up
would have been nice, yes :-)

> Quoting my downstream patch under my sig for reference.
>
> For this patch:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks all

Pushed as 5e3719aeaef1