* [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus
@ 2020-07-24 9:47 Shaokun Zhang
2020-07-24 10:22 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Shaokun Zhang @ 2020-07-24 9:47 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Mark Rutland, Marc Zyngier, Daniel Lezcano, Shaokun Zhang, Nianyao Tang
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);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus
2020-07-24 9:47 [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus Shaokun Zhang
@ 2020-07-24 10:22 ` Marc Zyngier
2020-07-25 8:49 ` Shaokun Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2020-07-24 10:22 UTC (permalink / raw)
To: Shaokun Zhang
Cc: Mark Rutland, Daniel Lezcano, Nianyao Tang, linux-arm-kernel
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?
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus
2020-07-24 10:22 ` Marc Zyngier
@ 2020-07-25 8:49 ` Shaokun Zhang
2020-07-25 9:23 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Shaokun Zhang @ 2020-07-25 8:49 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Mark Rutland, Daniel Lezcano, Nianyao Tang, linux-arm-kernel
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
Thanks,
Shaokun
>
> M.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus
2020-07-25 8:49 ` Shaokun Zhang
@ 2020-07-25 9:23 ` Marc Zyngier
2020-07-28 2:18 ` Shaokun Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2020-07-25 9:23 UTC (permalink / raw)
To: Shaokun Zhang
Cc: Mark Rutland, Daniel Lezcano, Nianyao Tang, linux-arm-kernel
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
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.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus
2020-07-25 9:23 ` Marc Zyngier
@ 2020-07-28 2:18 ` Shaokun Zhang
2020-07-28 7:59 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Shaokun Zhang @ 2020-07-28 2:18 UTC (permalink / raw)
To: Marc Zyngier
Cc: Mark Rutland, Daniel Lezcano, jiayanlei, Nianyao Tang, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus
2020-07-28 2:18 ` Shaokun Zhang
@ 2020-07-28 7:59 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2020-07-28 7:59 UTC (permalink / raw)
To: Shaokun Zhang
Cc: Mark Rutland, Daniel Lezcano, jiayanlei, Nianyao Tang, linux-arm-kernel
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@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.
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...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-28 8:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 9:47 [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus Shaokun Zhang
2020-07-24 10:22 ` Marc Zyngier
2020-07-25 8:49 ` Shaokun Zhang
2020-07-25 9:23 ` Marc Zyngier
2020-07-28 2:18 ` Shaokun Zhang
2020-07-28 7:59 ` Marc Zyngier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.