| Message ID | 20220810105822.18404-1-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v1] xen: arm: Check if timer is enabled for timer irq | expand |
Hi Ayan, > On 10 Aug 2022, at 11:58, Ayan Kumar Halder <ayankuma@amd.com> wrote: > > Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, > CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates > whether the timer condition is met." > > Also similar description applies to CNTP_CTL as well. > > One should always check that the timer is enabled and status is set, to > determine if the timer interrupt has been generated. > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > --- > xen/arch/arm/time.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c > index dec53b5f7d..f220586c52 100644 > --- a/xen/arch/arm/time.c > +++ b/xen/arch/arm/time.c > @@ -222,8 +222,13 @@ int reprogram_timer(s_time_t timeout) > /* Handle the firing timer */ > static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) > { > - if ( irq == (timer_irq[TIMER_HYP_PPI]) && > - READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING ) > + uint8_t timer_en_mask = (CNTx_CTL_PENDING | CNTx_CTL_ENABLE); This should either be a macro or be added directly into the condition. But here seeing the rest of the code, I would suggest to create a macro for the whole condition and use it directly into the ifs as I find that this solution using boolean variable is making the code unclear. May I suggest the following: #define CNTx_CTL_IS_PENDING(reg) (READ_SYSREG(reg) & (CNTx_CTL_PENDING | CNTx_CTL_ENABLE)) Or in fact just adding CNTx_CTL_ENABLE in the if directly. > + bool timer_cond_el2 = (READ_SYSREG(CNTHP_CTL_EL2) & timer_en_mask) == > + timer_en_mask ? true : false; ? True:false is redundant here and not needed. > + bool timer_cond_el0 = (READ_SYSREG(CNTP_CTL_EL0) & timer_en_mask) == > + timer_en_mask ? true : false; Same here > + > + if ( irq == (timer_irq[TIMER_HYP_PPI]) && timer_cond_el2 ) > { > perfc_incr(hyp_timer_irqs); > /* Signal the generic timer code to do its work */ > @@ -232,8 +237,7 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) > WRITE_SYSREG(0, CNTHP_CTL_EL2); > } > > - if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && > - READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING ) > + if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && timer_cond_el0 ) > { > perfc_incr(phys_timer_irqs); > /* Signal the generic timer code to do its work */ > -- > 2.17.1 > Cheers Bertrand
On 10/08/2022 13:16, Bertrand Marquis wrote: > Hi Ayan, Hi Bertrand, > >> On 10 Aug 2022, at 11:58, Ayan Kumar Halder <ayankuma@amd.com> wrote: >> >> Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, >> CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates >> whether the timer condition is met." >> >> Also similar description applies to CNTP_CTL as well. >> >> One should always check that the timer is enabled and status is set, to >> determine if the timer interrupt has been generated. >> >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> --- >> xen/arch/arm/time.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c >> index dec53b5f7d..f220586c52 100644 >> --- a/xen/arch/arm/time.c >> +++ b/xen/arch/arm/time.c >> @@ -222,8 +222,13 @@ int reprogram_timer(s_time_t timeout) >> /* Handle the firing timer */ >> static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) >> { >> - if ( irq == (timer_irq[TIMER_HYP_PPI]) && >> - READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING ) >> + uint8_t timer_en_mask = (CNTx_CTL_PENDING | CNTx_CTL_ENABLE); > This should either be a macro or be added directly into the condition. > > But here seeing the rest of the code, I would suggest to create a macro for the > whole condition and use it directly into the ifs as I find that this solution using > boolean variable is making the code unclear. > > May I suggest the following: > #define CNTx_CTL_IS_PENDING(reg) (READ_SYSREG(reg) & (CNTx_CTL_PENDING | CNTx_CTL_ENABLE)) This will return true even if either CNTx_CTL_PENDING or CNTx_CTL_ENABLE is set. > > Or in fact just adding CNTx_CTL_ENABLE in the if directly. We want to check that both are set. So this should be :- #define CNTx_CTL_IS_PENDING(reg) ( (READ_SYSREG(reg) & (CNTx_CTL_PENDING | CNTx_CTL_ENABLE)) == (CNTx_CTL_PENDING | CNTx_CTL_ENABLE) ) Let me know if you agree. I will prefer using a macro rather putting this in 'if' condition as it might make readability difficult. - Ayan > >> + bool timer_cond_el2 = (READ_SYSREG(CNTHP_CTL_EL2) & timer_en_mask) == >> + timer_en_mask ? true : false; > ? True:false is redundant here and not needed. > >> + bool timer_cond_el0 = (READ_SYSREG(CNTP_CTL_EL0) & timer_en_mask) == >> + timer_en_mask ? true : false; > Same here > >> + >> + if ( irq == (timer_irq[TIMER_HYP_PPI]) && timer_cond_el2 ) >> { >> perfc_incr(hyp_timer_irqs); >> /* Signal the generic timer code to do its work */ >> @@ -232,8 +237,7 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) >> WRITE_SYSREG(0, CNTHP_CTL_EL2); >> } >> >> - if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && >> - READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING ) >> + if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && timer_cond_el0 ) >> { >> perfc_incr(phys_timer_irqs); >> /* Signal the generic timer code to do its work */ >> -- >> 2.17.1 >> > Cheers > Bertrand
Hi Ayan, > On 10 Aug 2022, at 13:54, Ayan Kumar Halder <ayankuma@amd.com> wrote: > > > On 10/08/2022 13:16, Bertrand Marquis wrote: >> Hi Ayan, > > Hi Bertrand, > >> >>> On 10 Aug 2022, at 11:58, Ayan Kumar Halder <ayankuma@amd.com> wrote: >>> >>> Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, >>> CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates >>> whether the timer condition is met." >>> >>> Also similar description applies to CNTP_CTL as well. >>> >>> One should always check that the timer is enabled and status is set, to >>> determine if the timer interrupt has been generated. >>> >>> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >>> --- >>> xen/arch/arm/time.c | 12 ++++++++---- >>> 1 file changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c >>> index dec53b5f7d..f220586c52 100644 >>> --- a/xen/arch/arm/time.c >>> +++ b/xen/arch/arm/time.c >>> @@ -222,8 +222,13 @@ int reprogram_timer(s_time_t timeout) >>> /* Handle the firing timer */ >>> static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) >>> { >>> - if ( irq == (timer_irq[TIMER_HYP_PPI]) && >>> - READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING ) >>> + uint8_t timer_en_mask = (CNTx_CTL_PENDING | CNTx_CTL_ENABLE); >> This should either be a macro or be added directly into the condition. >> >> But here seeing the rest of the code, I would suggest to create a macro for the >> whole condition and use it directly into the ifs as I find that this solution using >> boolean variable is making the code unclear. >> >> May I suggest the following: >> #define CNTx_CTL_IS_PENDING(reg) (READ_SYSREG(reg) & (CNTx_CTL_PENDING | CNTx_CTL_ENABLE)) > This will return true even if either CNTx_CTL_PENDING or CNTx_CTL_ENABLE is set. Yes this is missing the comparison part >> >> Or in fact just adding CNTx_CTL_ENABLE in the if directly. > We want to check that both are set. > > So this should be :- > #define CNTx_CTL_IS_PENDING(reg) ( (READ_SYSREG(reg) & (CNTx_CTL_PENDING | CNTx_CTL_ENABLE)) == > (CNTx_CTL_PENDING | CNTx_CTL_ENABLE) ) > > Let me know if you agree. I will prefer using a macro rather putting this in 'if' condition as it might make readability difficult. Yes I agree Bertrand > > - Ayan > >> >>> + bool timer_cond_el2 = (READ_SYSREG(CNTHP_CTL_EL2) & timer_en_mask) == >>> + timer_en_mask ? true : false; >> ? True:false is redundant here and not needed. >> >>> + bool timer_cond_el0 = (READ_SYSREG(CNTP_CTL_EL0) & timer_en_mask) == >>> + timer_en_mask ? true : false; >> Same here >> >>> + >>> + if ( irq == (timer_irq[TIMER_HYP_PPI]) && timer_cond_el2 ) >>> { >>> perfc_incr(hyp_timer_irqs); >>> /* Signal the generic timer code to do its work */ >>> @@ -232,8 +237,7 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) >>> WRITE_SYSREG(0, CNTHP_CTL_EL2); >>> } >>> >>> - if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && >>> - READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING ) >>> + if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && timer_cond_el0 ) >>> { >>> perfc_incr(phys_timer_irqs); >>> /* Signal the generic timer code to do its work */ >>> -- >>> 2.17.1 >>> >> Cheers >> Bertrand
Hi Ayan, Bertrand already made some comments. I will try to avoid repeating the same comments. On 10/08/2022 11:58, Ayan Kumar Halder wrote: > Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, You are modifying code that is common between AArch64 and AArch32. So I would mention this behavior is common. Also, please specify the version of the spec. This helps in case the behavior has changed. Also, NIT: I would prefer if you quote the Arm Arm rather than auxiliary specifications. > CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates > whether the timer condition is met." I think the key point here is the field ISTATUS is "unknown" when the ENABLE bit is 0. > > Also similar description applies to CNTP_CTL as well. > > One should always check that the timer is enabled and status is set, to > determine if the timer interrupt has been generated. I understand the theory. In practice, I am not sure this could ever happen because the timer interrupt is level and by clearing *_CTL you will lower the line and therefore you should not receive the interrupt again. Checking the 'enable' is not going to add too much overhead. So I am fine if this is added. That said, would you be able to provide more details on how this was spotted? Cheers,
On 10/08/2022 14:34, Julien Grall wrote: > Hi Ayan, Hi Julien, > > Bertrand already made some comments. I will try to avoid repeating the > same comments. > > On 10/08/2022 11:58, Ayan Kumar Halder wrote: >> Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, > > You are modifying code that is common between AArch64 and AArch32. So > I would mention this behavior is common. Also, please specify the version > of the spec. This helps in case the behavior has changed. ack > > Also, NIT: I would prefer if you quote the Arm Arm rather than auxiliary > specifications. ack > >> CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates >> whether the timer condition is met." > > I think the key point here is the field ISTATUS is "unknown" when the > ENABLE bit is 0. yes, this is the key thing. > >> >> Also similar description applies to CNTP_CTL as well. >> >> One should always check that the timer is enabled and status is set, to >> determine if the timer interrupt has been generated. > > I understand the theory. In practice, I am not sure this could ever > happen because the timer interrupt is level and by clearing *_CTL you > will lower the line and therefore you should not receive the interrupt > again. > > Checking the 'enable' is not going to add too much overhead. So I am > fine if this is added. That said, would you be able to provide more > details on how this was spotted? This was spotted while debugging an unrelated problem while porting Xen on R52. For a different reason, I was not able to get context switch to work correctly. When I was scrutinizing the timer_interrupt() with the documentation, I found that we are not checking ENABLE. Although the code works fine today (on aarch32 or aarch64), I thought it is better to add the check for the sake of compliance with the documentation. I can send the v2 patch (addressing yours and Bertrand's comment) if you think it is fine. - Ayan > > Cheers, >
Hi Ayan, On 10/08/2022 15:00, Ayan Kumar Halder wrote: > On 10/08/2022 14:34, Julien Grall wrote: >> On 10/08/2022 11:58, Ayan Kumar Halder wrote: >>> Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, >> Checking the 'enable' is not going to add too much overhead. So I am >> fine if this is added. That said, would you be able to provide more >> details on how this was spotted? > > This was spotted while debugging an unrelated problem while porting Xen > on R52. For a different reason, I was not able to get context switch to > work correctly. > > When I was scrutinizing the timer_interrupt() with the documentation, I > found that we are not checking ENABLE. > > Although the code works fine today (on aarch32 or aarch64), I thought it > is better to add the check for the sake of compliance with the > documentation. Thanks for the clarification. I am quite curious to know why you think our code is not compliant. As I wrote before, when ENABLE is cleared, you should never have an interrupt because the timer interrupt is level. So I believe our code is compliant with the Arm Arm. The only reason I am OK with checking ENABLE is because the overhead is limited. If this wasn't the case, then I think I would have wanted clear justification in the commit message *why* this is not compliant. FWIW, Linux seems to use the same approach as us (see [1]). So, if you think this is not compliant, then maybe this is something you also want to consider to fix there? Cheers, [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n644
On 10/08/2022 15:51, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 10/08/2022 15:00, Ayan Kumar Halder wrote: >> On 10/08/2022 14:34, Julien Grall wrote: >>> On 10/08/2022 11:58, Ayan Kumar Halder wrote: >>>> Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, >>> Checking the 'enable' is not going to add too much overhead. So I am >>> fine if this is added. That said, would you be able to provide more >>> details on how this was spotted? >> >> This was spotted while debugging an unrelated problem while porting >> Xen on R52. For a different reason, I was not able to get context >> switch to work correctly. >> >> When I was scrutinizing the timer_interrupt() with the documentation, >> I found that we are not checking ENABLE. >> >> Although the code works fine today (on aarch32 or aarch64), I thought >> it is better to add the check for the sake of compliance with the >> documentation. > > Thanks for the clarification. I am quite curious to know why you think > our code is not compliant. > > As I wrote before, when ENABLE is cleared, you should never have an > interrupt because the timer interrupt is level. So I believe our code > is compliant with the Arm Arm. > > The only reason I am OK with checking ENABLE is because the overhead > is limited. If this wasn't the case, then I think I would have wanted > clear justification in the commit message *why* this is not compliant. Sorry, I think I misunderstood this part of the documentation "When the value of the ENABLE bit is 1, ISTATUS indicates whether the timer condition is met." I understood this as "ENABLE" need to be checked before "ISTATUS" is checked. - Ayan > > FWIW, Linux seems to use the same approach as us (see [1]). So, if you > think this is not compliant, then maybe this is something you also > want to consider to fix there? > > Cheers, > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n644 >
Hi Ayan, On 10/08/2022 17:44, Ayan Kumar Halder wrote: > > On 10/08/2022 15:51, Julien Grall wrote: >> Hi Ayan, > Hi Julien, >> >> On 10/08/2022 15:00, Ayan Kumar Halder wrote: >>> On 10/08/2022 14:34, Julien Grall wrote: >>>> On 10/08/2022 11:58, Ayan Kumar Halder wrote: >>>>> Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, >>>> Checking the 'enable' is not going to add too much overhead. So I am >>>> fine if this is added. That said, would you be able to provide more >>>> details on how this was spotted? >>> >>> This was spotted while debugging an unrelated problem while porting >>> Xen on R52. For a different reason, I was not able to get context >>> switch to work correctly. >>> >>> When I was scrutinizing the timer_interrupt() with the documentation, >>> I found that we are not checking ENABLE. >>> >>> Although the code works fine today (on aarch32 or aarch64), I thought >>> it is better to add the check for the sake of compliance with the >>> documentation. >> >> Thanks for the clarification. I am quite curious to know why you think >> our code is not compliant. >> >> As I wrote before, when ENABLE is cleared, you should never have an >> interrupt because the timer interrupt is level. So I believe our code >> is compliant with the Arm Arm. >> >> The only reason I am OK with checking ENABLE is because the overhead >> is limited. If this wasn't the case, then I think I would have wanted >> clear justification in the commit message *why* this is not compliant. > > Sorry, I think I misunderstood this part of the documentation > > "When the value of the ENABLE bit is 1, ISTATUS indicates whether the > timer condition is met." > > I understood this as "ENABLE" need to be checked before "ISTATUS" is > checked. Sorry I should have been clearer. I wasn't suggesting your understanding of the spec were wrong. In fact, it is correct and in theory we should check it. I was pointing out that in practice I believe this is not necessary and our code is still compliant. I also agree, this is not obvious from the code why we are compliant there are usually two ways to approach it: 1) Add the extra check/barriers. The pros is the code is strictly compliant the cons is this could add overhead. 2) Document it. The cons is we may be wrong and therefore end up in a unknown territory. In this case, this is mitigated by the fact the bit is "unknown" (and therefore I believe can never have a different meaning) and this will only trigger a "walk the list". That list would be empty if the timer is disabled. You went with 1) and I am fine with that (I explained why before). Cheers,
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index dec53b5f7d..f220586c52 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -222,8 +222,13 @@ int reprogram_timer(s_time_t timeout) /* Handle the firing timer */ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) { - if ( irq == (timer_irq[TIMER_HYP_PPI]) && - READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING ) + uint8_t timer_en_mask = (CNTx_CTL_PENDING | CNTx_CTL_ENABLE); + bool timer_cond_el2 = (READ_SYSREG(CNTHP_CTL_EL2) & timer_en_mask) == + timer_en_mask ? true : false; + bool timer_cond_el0 = (READ_SYSREG(CNTP_CTL_EL0) & timer_en_mask) == + timer_en_mask ? true : false; + + if ( irq == (timer_irq[TIMER_HYP_PPI]) && timer_cond_el2 ) { perfc_incr(hyp_timer_irqs); /* Signal the generic timer code to do its work */ @@ -232,8 +237,7 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) WRITE_SYSREG(0, CNTHP_CTL_EL2); } - if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && - READ_SYSREG(CNTP_CTL_EL0) & CNTx_CTL_PENDING ) + if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) && timer_cond_el0 ) { perfc_incr(phys_timer_irqs); /* Signal the generic timer code to do its work */
Refer "Arm Architecture Registers DDI 0595", AArch32 system registers, CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates whether the timer condition is met." Also similar description applies to CNTP_CTL as well. One should always check that the timer is enabled and status is set, to determine if the timer interrupt has been generated. Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> --- xen/arch/arm/time.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)