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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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,