From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22a.google.com (mail-it0-x22a.google.com [IPv6:2607:f8b0:4001:c0b::22a]) (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 0492B1A1E18 for ; Mon, 8 Aug 2016 04:51:22 -0700 (PDT) Received: by mail-it0-x22a.google.com with SMTP id u186so71464131ita.0 for ; Mon, 08 Aug 2016 04:51:21 -0700 (PDT) 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=utm6CeCI5T5BvMFxcFFDpFw+EfnGwaCxPH1nvLdmQmg=; b=OYoABYM/wfeioVDLGyvHmQWL1yu+p59npjzuCx8vn9NfZDHFyfL0a8n0VdvRcjWHc0 qF3BR5/Og1zRTzzDP6sh1ycCYbto3rQoExc3PVtPltCIUN3jjFt++UipJy4lWf4GMrCc bsJ98KKkGBmDi7r/+U77/OcmrzFCFNlLDofkY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=utm6CeCI5T5BvMFxcFFDpFw+EfnGwaCxPH1nvLdmQmg=; b=jZMffiNATFFZHWEvb3kPJrjsTDtSE9e1PzDew3i3kPsYPAnc5TLLzBydrEPhTvTeVG qyFExVrZw3akwuBnAr4ntJ2NfUXZL+5cpO+/vmE59RT4mQX6Xv0xSI+nzYt1oiT5VMy9 RU9bT9MhE43kxyI3yKBZWNYDImeiG+fRmh5x9fgBs27qCYuxRmqF9ZuhsGd12UyrUdoK S04FUBT03c7QyDWSBjPjloH0vvcaj8Qm9Vw6oUL3eB5kXn0MBUGcUppCXgQwCCrl304H 0HXTcda73tHT42HWj1/oV2WUZdgp0Ef+tXTaMBuUvk8R0ZvYh9ShphnPxvZkVbC0CCBS SjXw== X-Gm-Message-State: AEkoouupJaiLkEnScGfYNH5FPJx2ncxayESAGfx3pIs+uNcGTqkxJjPn+e7x+NUvk1oyEwF3+CFUZPisndVKbQM0 X-Received: by 10.36.57.215 with SMTP id l206mr18420893ita.5.1470657081240; Mon, 08 Aug 2016 04:51:21 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 8 Aug 2016 04:51:20 -0700 (PDT) In-Reply-To: References: <20160805165911.14744-1-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Mon, 8 Aug 2016 13:51:20 +0200 Message-ID: To: Alexei Fedorov Cc: Evan Lloyd , "Cohen, Eugene" , "edk2-devel@lists.01.org" , Heyi Guo , Leif Lindholm Subject: Re: [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Aug 2016 11:51:22 -0000 Content-Type: text/plain; charset=UTF-8 On 8 August 2016 at 13:08, Ard Biesheuvel wrote: > On 8 August 2016 at 13:06, Alexei Fedorov wrote: >> Timer's pending interrupt is cleared HARDWARE_INTERRUPT_HANDLER TimerInterruptHandler() in when timer is re-programmed for the next shot. > > Yes, that is the timer side. > >> Does it mean that TimerDxe driver is part EFI_HARDWARE_INTERRUPT_PROTOCOL? >> > > No. The peripheral and the GIC each have their own mask and enable > registers for the interrupt. That is exactly why the comment describes > in detail which part is the responsibility of the GIC, and which is > the responsibility of the peripheral. > ... and actually, looking at TimerInterruptHandler (), I don't see the point of re-enabling the interrupt early, given that 308: OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); disables interrupts globally, and only re-enables them on line 346, at which point the mTimerNotifyFunction() has already returned. So I propose we simply do diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c index 1169d426b255..f0fcb05757ac 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c @@ -308,10 +308,7 @@ TimerInterruptHandler ( OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); // 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); + while ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) { if (mTimerNotifyFunction) { mTimerNotifyFunction (mTimerPeriod * mElapsedPeriod); so that if the condition exists that we know will trigger the interrupt immediately as soon as we unmask it, we simply enter the loop again just like we would when taking the [nested] interrupt. @Heyi: any thoughts? -- Ard.