From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <marc.zyngier@arm.com>
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 3F9EA224872AE
 for <edk2-devel@lists.01.org>; Tue, 13 Mar 2018 02:27:14 -0700 (PDT)
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 F402A1596;
 Tue, 13 Mar 2018 02:33:35 -0700 (PDT)
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
 0BE2B3F487; Tue, 13 Mar 2018 02:33:34 -0700 (PDT)
To: Heyi Guo <heyi.guo@linaro.org>, edk2-devel@lists.01.org
Cc: Yi Li <phoenix.liyi@huawei.com>, Leif Lindholm
 <leif.lindholm@linaro.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>
References: <1520901090-96694-1-git-send-email-heyi.guo@linaro.org>
 <1520901090-96694-2-git-send-email-heyi.guo@linaro.org>
From: Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <0403f2bf-d6a8-b101-73a2-949946f71e46@arm.com>
Date: Tue, 13 Mar 2018 09:33:33 +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: <1520901090-96694-2-git-send-email-heyi.guo@linaro.org>
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  <edk2-devel.lists.01.org>
List-Unsubscribe: <https://lists.01.org/mailman/options/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=unsubscribe>
List-Archive: <http://lists.01.org/pipermail/edk2-devel/>
List-Post: <mailto:edk2-devel@lists.01.org>
List-Help: <mailto:edk2-devel-request@lists.01.org?subject=help>
List-Subscribe: <https://lists.01.org/mailman/listinfo/edk2-devel>,
 <mailto:edk2-devel-request@lists.01.org?subject=subscribe>
X-List-Received-Date: Tue, 13 Mar 2018 09:27:15 -0000
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit

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 <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 
> 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.

So either you leave the ISB here and remove the enable call, or move the
ISB after the enable.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...