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

[Julien Grall <julien@xxxxxxx>]

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