* [XEN v1] xen: arm: Check if timer is enabled for timer irq
@ 2022-08-10 10:58 Ayan Kumar Halder
2022-08-10 12:16 ` Bertrand Marquis
2022-08-10 13:34 ` Julien Grall
0 siblings, 2 replies; 9+ messages in thread
From: Ayan Kumar Halder @ 2022-08-10 10:58 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, stefanos, julien, Volodymyr_Babchuk,
bertrand.marquis, Ayan Kumar Halder
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 10:58 [XEN v1] xen: arm: Check if timer is enabled for timer irq Ayan Kumar Halder
@ 2022-08-10 12:16 ` Bertrand Marquis
2022-08-10 12:54 ` Ayan Kumar Halder
2022-08-10 13:34 ` Julien Grall
1 sibling, 1 reply; 9+ messages in thread
From: Bertrand Marquis @ 2022-08-10 12:16 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 12:16 ` Bertrand Marquis
@ 2022-08-10 12:54 ` Ayan Kumar Halder
2022-08-10 12:58 ` Bertrand Marquis
0 siblings, 1 reply; 9+ messages in thread
From: Ayan Kumar Halder @ 2022-08-10 12:54 UTC (permalink / raw)
To: Bertrand Marquis
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 12:54 ` Ayan Kumar Halder
@ 2022-08-10 12:58 ` Bertrand Marquis
0 siblings, 0 replies; 9+ messages in thread
From: Bertrand Marquis @ 2022-08-10 12:58 UTC (permalink / raw)
To: Ayan Kumar Halder
Cc: xen-devel, sstabellini, stefanos, julien, Volodymyr_Babchuk
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 10:58 [XEN v1] xen: arm: Check if timer is enabled for timer irq Ayan Kumar Halder
2022-08-10 12:16 ` Bertrand Marquis
@ 2022-08-10 13:34 ` Julien Grall
2022-08-10 14:00 ` Ayan Kumar Halder
1 sibling, 1 reply; 9+ messages in thread
From: Julien Grall @ 2022-08-10 13:34 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 13:34 ` Julien Grall
@ 2022-08-10 14:00 ` Ayan Kumar Halder
2022-08-10 14:51 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Ayan Kumar Halder @ 2022-08-10 14:00 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis
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,
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 14:00 ` Ayan Kumar Halder
@ 2022-08-10 14:51 ` Julien Grall
2022-08-10 16:44 ` Ayan Kumar Halder
0 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2022-08-10 14:51 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 14:51 ` Julien Grall
@ 2022-08-10 16:44 ` Ayan Kumar Halder
2022-08-10 17:45 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Ayan Kumar Halder @ 2022-08-10 16:44 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq
2022-08-10 16:44 ` Ayan Kumar Halder
@ 2022-08-10 17:45 ` Julien Grall
0 siblings, 0 replies; 9+ messages in thread
From: Julien Grall @ 2022-08-10 17:45 UTC (permalink / raw)
To: Ayan Kumar Halder, xen-devel
Cc: sstabellini, stefanos, Volodymyr_Babchuk, bertrand.marquis
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-08-10 17:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-10 10:58 [XEN v1] xen: arm: Check if timer is enabled for timer irq Ayan Kumar Halder
2022-08-10 12:16 ` Bertrand Marquis
2022-08-10 12:54 ` Ayan Kumar Halder
2022-08-10 12:58 ` Bertrand Marquis
2022-08-10 13:34 ` Julien Grall
2022-08-10 14:00 ` Ayan Kumar Halder
2022-08-10 14:51 ` Julien Grall
2022-08-10 16:44 ` Ayan Kumar Halder
2022-08-10 17:45 ` Julien Grall
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.