On 6 March 2018 at 14:25, Laszlo Ersek <ler...@redhat.com> wrote:
> On 03/06/18 14:24, Ard Biesheuvel wrote:
>> From: Marc Zyngier <marc.zyng...@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.zyng...@arm.com>
>> Reviewed-by: Ard Biesheuvel <ard.biesheu...@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.1040...@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.d...@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 <ler...@redhat.com>
>
Thanks all
Pushed as 5e3719aeaef1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
