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);
+    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 */
-- 
2.17.1

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,

-- 
Julien Grall

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

-- 
Julien Grall

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,

-- 
Julien Grall

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