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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan,

> On 10 Aug 2022, at 13:54, Ayan Kumar Halder <ayankuma@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
>>> ---
>>> 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