| Message ID | 1595584037-6877-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: arch_timer: Ensure timer is enabled before using istatus | expand |
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.
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.
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.
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. >
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.
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);