On 10/08/2022 13:16, Bertrand Marquis wrote:
Hi Ayan,
Hi Bertrand,
On 10 Aug 2022, at 11:58, Ayan Kumar Halder <[email protected]> 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 <[email protected]>
---
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.
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.
- 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
