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

[Marc Zyngier]

On 2020-07-28 03:18, Shaokun Zhang wrote:
> Hi Marc,
> 
> 在 2020/7/25 17:23, Marc Zyngier 写道:
>> On Sat, 25 Jul 2020 09:49:55 +0100,
>> Shaokun Zhang <zhangshaokun at 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 at 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 at arm.com>
>>>>> Cc: Marc Zyngier <maz at kernel.org>
>>>>> Cc: Daniel Lezcano <daniel.lezcano at linaro.org>
>>>>> Signed-off-by: Nianyao Tang <tangnianyao at huawei.com>
>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun at 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.

As I said, this is just a spurious interrupt, which can happen
at any time.

This just outlines a limitation of the VGIC (an interrupt queued
in a LR cannot be retired without causing an exit).

Can we fix it? No. Are we going to sprinkle these checks all over
the place? Neither. As I said, this is a quality of implementation
issue, and drivers already cope with this.

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