Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus

[Shaokun Zhang <zhangshaokun@hisilicon.com>]

Hi Marc,

在 2020/7/25 17:23, Marc Zyngier 写道:
> On Sat, 25 Jul 2020 09:49:55 +0100,
> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>
>> Hi Marc,
>>
>> 在 2020/7/24 18:22, Marc Zyngier 写道:
>>> On 2020-07-24 10:47, Shaokun Zhang wrote:
>>>> From: Nianyao Tang <tangnianyao@huawei.com>
>>>>
>>>> In Arm ARM spec, there is a description for timer control register, when
>>>> the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN. We shall
>>>> only read and use ISTATUS when ENABLE is 1, otherwise ISTATUS may be
>>>> invalid.
>>>>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>
>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>> ---
>>>>  drivers/clocksource/arm_arch_timer.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clocksource/arm_arch_timer.c
>>>> b/drivers/clocksource/arm_arch_timer.c
>>>> index 6c3e84180146..0bbc2715de79 100644
>>>> --- a/drivers/clocksource/arm_arch_timer.c
>>>> +++ b/drivers/clocksource/arm_arch_timer.c
>>>> @@ -641,7 +641,8 @@ static __always_inline irqreturn_t
>>>> timer_handler(const int access,
>>>>      unsigned long ctrl;
>>>>
>>>>      ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
>>>> -    if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
>>>> +    if ((ctrl & ARCH_TIMER_CTRL_ENABLE) &&
>>>> +        (ctrl & ARCH_TIMER_CTRL_IT_STAT)) {
>>>>          ctrl |= ARCH_TIMER_CTRL_IT_MASK;
>>>>          arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
>>>>          evt->event_handler(evt);
>>>
>>> Interesting. A question for you though:
>>>
>>> How do you think we made it in the interrupt handler if the timer
>>> was disabled?
>>
>> Let's assume this scenario as follow:
>> a. Mask timer interrupt by PSTATE.I
>> b. Timer interrupt is set and pending in GICC
>> c. Disable timer by CNT{P,V}_CTL_EL0.ENABLE and the clear operation will consume
>> much more time when GIC is very busy.
>> d. Unmask timer interrupt by PSTATE.I, but timer interrupt is not clear in time
>> and forward to cpu.
>> e. We receive a timer interrupt with ENABLE=0
> 
> And that's a spurious interrupt. Big deal. Should we care? No, because

Let's assume this scenario for guest:
1. Guest masks timer interrupt by PSTATE.I in EL1(VHE ON)
2. Guest enable vtimer by CNTV_CTL_EL0.ENABLE
3. Vtimer phy interrupt is forwarded to kvm, and vtimer virtual interrupt
   is set pending in LR
4. Back to guest, disable vtimer by CNTV_CTL_EL0.ENABLE
5. Guest unmasks timer interrupt by PSTATE.I
6. Guest receives a timer interrupt with ENABLE=0
[From 4 to 6, vtimer virtual is pending in LR and no more guest-exit]

Thanks,
Shaokun

> this can happen for any device, in any situation. If the GIC cannot
> retire a level PPI quickly enough, that's a GIC quality of
> implementation issue, and I don't plan to paper over it in all
> existing drivers.
> 
> Thanks,
> 
> 	M.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel