From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B33CF22135D59 for ; Tue, 6 Mar 2018 06:33:22 -0800 (PST) Received: by mail-it0-x241.google.com with SMTP id c11so13991816ith.4 for ; Tue, 06 Mar 2018 06:39:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=0iOJL+5f79FPAChzUqUOY+AcC/UBkOJ8uVz1rSba40c=; b=jq8Nn1BZfkzV35QsEe9bJrPnnYcqQuEV6jV1ar1TGT26iVZImOI0tzN2ASZpRQQ+Jv uSnIqgxE+FhiAAla+jkTmRe1asSLizBFfui3pwvgCS5OKpfP0mKMoTob8zz6/zXpxuBR Xvc3tdov/Q4EgaQyP+797XW59s8/1S5dApHw8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=0iOJL+5f79FPAChzUqUOY+AcC/UBkOJ8uVz1rSba40c=; b=pUJ8elKlNhxCOub7lsgeR0mXhDvIGrPIMbUqYWUwgKAryzPk6mII+Gx/svntI0zdkV PttavzzA5mq6OCBjXvYoWAPFYcz7yTTJYoP1t40fupWYrEtK0O1ya2GZsYv2Z1smJMX1 Ld4x5Z+fn21FfWNqZv1vhOk1keQ11AbdexgvUVBG2t+6ZonDOeerCsUy9yoY9ckUr6Tg AMhVFjE2kYmzuCCwUoZT1R+M/yV3+6sBCRtz9cJTWrH/Tfe+LXItRpcZZGMBymSO98MT IgM8IFWd506dYxwTs8RdLuVLLICSm3cCAOUL+di7Bb5vgEk6ySlRho8WkX/CfSLvk2yM EwiQ== X-Gm-Message-State: AElRT7FIc2EtgeMvKJbH4JfjeUuGSSCvQQma9ViFcnaXr01nXuh8frcE bWpqWWb1b60k48St8x21UEZSRJETrrI8ru09P47p/Q== X-Google-Smtp-Source: AG47ELtkpYFG0w1et9IuxaxL/UPBo/7LrO3B54fFquNd5QeNQO+N/+yBOjWY8shl021jcedMr5YCsk1oe3wvqopZMHc= X-Received: by 10.36.91.138 with SMTP id g132mr18341736itb.50.1520347175712; Tue, 06 Mar 2018 06:39:35 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Tue, 6 Mar 2018 06:39:35 -0800 (PST) In-Reply-To: <70195198-34e4-5711-8534-abccad6068dd@redhat.com> References: <20180306132424.25961-1-ard.biesheuvel@linaro.org> <70195198-34e4-5711-8534-abccad6068dd@redhat.com> From: Ard Biesheuvel Date: Tue, 6 Mar 2018 14:39:35 +0000 Message-ID: To: Laszlo Ersek Cc: Marc Zyngier , "edk2-devel@lists.01.org" , Leif Lindholm , Drew Jones , Wei Huang 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:33:23 -0000 Content-Type: text/plain; charset="UTF-8" On 6 March 2018 at 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 > <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 > : > > [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 > Thanks all Pushed as 5e3719aeaef1