From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c00::244; helo=mail-pf0-x244.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf0-x244.google.com (mail-pf0-x244.google.com [IPv6:2607:f8b0:400e:c00::244]) (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 1B53421E4904A for ; Tue, 13 Mar 2018 17:18:51 -0700 (PDT) Received: by mail-pf0-x244.google.com with SMTP id u5so641309pfh.6 for ; Tue, 13 Mar 2018 17:25:14 -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=Og2wMWLi4aIrONIoho8gbbNAdLDJ7dG7hNSIqGCLd4M=; b=TFvyl0t+/J+7mNbulRGkztnEhYdPDDsQOiayE0OX7YDG9qi5k6zsFYO0oOC+b4TONQ p3rXKkITKAx2MzhM0JVoxCbDiP/RUVcid37deNEY/D/2j1EmXu+gyO8yF4VutmZi5ENz NwZy+bZiZSBQoW/X2bsbV2Q529rW0Js5rpr6c= 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=Og2wMWLi4aIrONIoho8gbbNAdLDJ7dG7hNSIqGCLd4M=; b=m2M/7gdWD3ao6kVM6uKKFmPkNfoPxg/tm/ePqMTaagoEdWrwOGEYoLYD9aEQNrT+pI qagjm7uq5ZO5lyYrJT1cI53xh2WpQHUbwbMvg4aN/lqypHJhWt0TJ9iS3NMN6rg7lKzN 9G+hT5AiLH4Rw//5QaN8QKQfyfGvt4Yg18eKyQ7kvpxJ3hZ1FmZm5Qoo9Ry4u4+qGPcl H0Q6BtxYT+RkYCatteumCn8O1y/sdiD9+CTplisW+PwcPtRDRp1CFFa7lnN/KXc+jEQJ vhZ7t9aQc183CQ8e5hE42WEqkwMqkLCiwHgrbMhYCQ1F5HhjweafYoVj0Dgi4mAYHmXQ qIMg== X-Gm-Message-State: AElRT7FsJLqtrI28hae5eWtfsF2Pulm2x6jtBLUNdOl26YL8k8H6zmrA 0+Ls6WkKfXx2c0i4Cnoq2MgJLg== X-Google-Smtp-Source: AG47ELvllAKbk326vu9O+hA2QGdX8RusTHsm9LdM+FM5iunjyLAgSH9N+0gVEi915XHYRr/fhaTFxQ== X-Received: by 10.98.147.156 with SMTP id r28mr2386312pfk.204.1520987113974; Tue, 13 Mar 2018 17:25:13 -0700 (PDT) Received: from SZX1000114654 ([45.56.152.76]) by smtp.gmail.com with ESMTPSA id t63sm2201127pfj.44.2018.03.13.17.25.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 13 Mar 2018 17:25:13 -0700 (PDT) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Wed, 14 Mar 2018 08:25:09 +0800 To: Marc Zyngier Cc: Heyi Guo , edk2-devel@lists.01.org, Yi Li , Leif Lindholm , Ard Biesheuvel Message-ID: <20180314002509.GE96299@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> MIME-Version: 1.0 In-Reply-To: <0403f2bf-d6a8-b101-73a2-949946f71e46@arm.com> 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: Wed, 14 Mar 2018 00:18:52 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. Thanks and regards, Heyi > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...