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

[Julien Grall <julien@xxxxxxx>]

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