From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=marc.zyngier@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id AFEEB22135D5E for ; Tue, 6 Mar 2018 06:42:02 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1D55214; Tue, 6 Mar 2018 06:48:16 -0800 (PST) Received: from [10.1.207.62] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0CAEF3F25C; Tue, 6 Mar 2018 06:48:14 -0800 (PST) To: Laszlo Ersek , Ard Biesheuvel Cc: edk2-devel@lists.01.org, leif.lindholm@linaro.org, Drew Jones , Wei Huang References: <20180306132424.25961-1-ard.biesheuvel@linaro.org> <70195198-34e4-5711-8534-abccad6068dd@redhat.com> From: Marc Zyngier Organization: ARM Ltd Message-ID: <87064fbb-e8f7-5d01-7a19-e731fafe6ef7@arm.com> Date: Tue, 6 Mar 2018 14:48:13 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <70195198-34e4-5711-8534-abccad6068dd@redhat.com> Subject: Re: [PATCH] ArmPkg/TimerDxe: Always perform an EOI, even for spurious interrupts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Mar 2018 14:42:03 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit On 06/03/18 14:25, Laszlo Ersek wrote: > On 03/06/18 14:24, Ard Biesheuvel wrote: >> From: Marc Zyngier >> >> 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 >> Reviewed-by: Ard Biesheuvel >> --- >> 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. to 3.19.. 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 Yup. In general, not reporting bugs on the ML ends up in missing fixes and duplicated work. Not the best use of an already limited resource. > <54D3796E.1040907@redhat.com>, subject "virtual timer interrupt stuck > across guest firmware reboot on KVM", in your 2015 archives :) I only have a single trace of this as Christoffer cc'd me on on reply. I wasn't cc'd on the previous emails, nor on the follow-up if it ever existed. > 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 > : > > [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 Not only timer interrupts. Any interrupt can be spurious, depending on how quickly your interrupt controller can retire an interrupt that hasn't been acknowledged already. Propagation delays and all that, and assuming a level interrupt. For an edge interrupt, you cannot retire it, full stop (because you cannot "unwrite" something). > 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. > > Quoting my downstream patch under my sig for reference. > > For this patch: > > Reviewed-by: Laszlo Ersek Thanks, M. -- Jazz is not dead. It just smells funny...