Re: [XEN v1] xen: arm: Check if timer is enabled for timer irq

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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