* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).