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::242; helo=mail-it0-x242.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (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 74C252202E4B8 for ; Thu, 15 Mar 2018 00:05:15 -0700 (PDT) Received: by mail-it0-x242.google.com with SMTP id e98-v6so2612634itd.4 for ; Thu, 15 Mar 2018 00:11:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=dlf9PfPPFOCkI11IAxbpjDID0GGiJVpyIsNWOTheJ4o=; b=QcMltYunAgFJw1UDY3moOWVHF0/ORYdjevWQKctOKqnEv5psJoLsFNOasa2Y39Qpq0 UzSvkxL9RPvCNQsY0oghqVSaap0WLkIHMHST+URu5SajgKRtNSAhIegVG9eH4P1X2rBE GpQ06Hp7O8Lu1yy/VcURkWarcTJdYpL8k+Un0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=dlf9PfPPFOCkI11IAxbpjDID0GGiJVpyIsNWOTheJ4o=; b=FOrqAn35VMJ+Ss5liU27U1TWscD8w6BO5aZ56B7x4YuZzZANGUR8jMROJq9SXpiIrN SQCWRytSjkjrWUt/VP0dT3dlALrJnUb6LyN1PG3PopasnUNk101QLMV06TSAfDCwh/uN 0SKIend2/lAP0ORRw19/t+vzxgmDlmDTL7mVFSFwxDAH2fYMH+tWJY6GzxSasPXxlWgq sb01oVaPx013laH6WnXPZesMpitHLrw4fw6p29InqYPpZ3PaY8NJ+88c8n0Go7U1Uzhd d46Qq6+wWsp4nV+aCsxr6UionmYQ3skq8JxAxZlGkb3yZi4hYAvrJn4blFwELKzZiybb PQNA== X-Gm-Message-State: AElRT7FrpTcM4UzneH15WEpW/dDR7g1OkGXmFbjAPNwGTazS/AUaeJvG QgyCaJrqlmtVwtLvMXBg2OyNYQ== X-Google-Smtp-Source: AG47ELtqRyHcZAq7W6dyae8h6CZef8tQp6DHiEyXXbllnXgfav6Hcff/Q/cTQxMfjKFZysfSseAJ8w== X-Received: by 2002:a24:f803:: with SMTP id a3-v6mr2473216ith.13.1521097898306; Thu, 15 Mar 2018 00:11:38 -0700 (PDT) Received: from SZX1000114654 ([45.56.152.100]) by smtp.gmail.com with ESMTPSA id 12-v6sm2812018itm.0.2018.03.15.00.11.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Mar 2018 00:11:37 -0700 (PDT) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Thu, 15 Mar 2018 15:11:33 +0800 To: Ard Biesheuvel Cc: Marc Zyngier , Guo Heyi , "edk2-devel@lists.01.org" , Yi Li , Leif Lindholm Message-ID: <20180315071133.GD108227@SZX1000114654> References: <1520901090-96694-1-git-send-email-heyi.guo@linaro.org> <1520901090-96694-2-git-send-email-heyi.guo@linaro.org> <0403f2bf-d6a8-b101-73a2-949946f71e46@arm.com> <20180314002509.GE96299@SZX1000114654> <86a7vbaynt.wl-marc.zyngier@arm.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload 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: Thu, 15 Mar 2018 07:05:15 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Marc and Ard, I found the timer re-enable code was added by Ard for special reason: commit b1a633434ddc5fc28de817debd963f7845fb78c7 Author: Ard Biesheuvel Date: Thu Sep 18 21:16:47 2014 +0000 ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling So this line of code cannot be removed and I will add an ISB after enabling timer. Another strange thing is that we got lots of "Spurious GIC interrupt" error messages when timer enable code was removed, though at last it could boot to EFI shell. Please let me know if you have any idea about the possible reason of this issue. Regards, Heyi On Wed, Mar 14, 2018 at 02:50:41PM +0000, Ard Biesheuvel wrote: > On 14 March 2018 at 07:45, Marc Zyngier wrote: > > On Wed, 14 Mar 2018 00:25:09 +0000, > > Guo Heyi wrote: > >> > >> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote: > >> > On 13/03/18 00:31, Heyi Guo wrote: > >> > > If timer interrupt is level sensitive, reloading timer compare > >> > > register has a side effect of clearing GIC pending status, so a "ISB" > >> > > is needed to make sure this instruction is executed before enabling > >> > > CPU IRQ, or else we may get spurious timer interrupts. > >> > > > >> > > Contributed-under: TianoCore Contribution Agreement 1.1 > >> > > Signed-off-by: Heyi Guo > >> > > Signed-off-by: Yi Li > >> > > Cc: Leif Lindholm > >> > > Cc: Ard Biesheuvel > >> > > Cc: Marc Zyngier > >> > > --- > >> > > > >> > > Notes: > >> > > v2: > >> > > - Use ISB instead of DSB [Marc] > >> > > - Update commit message accordingly. > >> > > > >> > > ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 + > >> > > 1 file changed, 1 insertion(+) > >> > > > >> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > >> > > index 33d7c922221f..32abee8726a0 100644 > >> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > >> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > >> > > @@ -337,6 +337,7 @@ TimerInterruptHandler ( > >> > > > >> > > // Set next compare value > >> > > ArmGenericTimerSetCompareVal (CompareValue); > >> > > + ArmInstructionSynchronizationBarrier (); > >> > > ArmGenericTimerEnableTimer (); > >> > > } > >> > > >> > Sorry for being pedantic here, but it would make more sense if ISB was > >> > placed after the enabling of the timer. Otherwise, you only force the > >> > update of the comparator. But on the other hand, the timer was never > >> > disabled the first place, in which case you'd wonder why you're trying > >> > to enable it again. > >> Yes, I also had such question and hesitated at this place :) > >> > > >> > So either you leave the ISB here and remove the enable call, or move the > >> > ISB after the enable. > >> > >> If we are going to remove the enable call, is it better to be changed in a > >> separate patch? It seems not related with adding ISB, though it is only a > >> one-line change. > > > > I guess a separate patch doesn't hurt, but that's for Ard and Leif to > > decide. > > > > Yes, please.