Re: [PATCH] target/arm/helper: Fix timer interrupt masking when HCR_EL2.E2H == 0

[Florian Lugou]

On Thu, Jun 20, 2024 at 11:43:17AM +0100, Peter Maydell wrote:
> On Sat, 15 Jun 2024 at 19:56, Florian Lugou <florian.lugou@provenrun.com> 
> wrote:
> >
> > CNTHCTL_EL2 based masking of timer interrupts was introduced in
> > f6fc36deef6abcee406211f3e2f11ff894b87fa4. This masking was however
> > effective no matter whether EL2 was enabled in the current security
> > state or not, contrary to arm specification.
> >
> > Signed-off-by: Florian Lugou <florian.lugou@provenrun.com>
> > ---
> >  target/arm/helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index ce31957235..60e2344c68 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2684,7 +2684,8 @@ static void gt_update_irq(ARMCPU *cpu, int timeridx)
> >       * If bit CNTHCTL_EL2.CNT[VP]MASK is set, it overrides IMASK.
> >       * It is RES0 in Secure and NonSecure state.
> >       */
> > -    if ((ss == ARMSS_Root || ss == ARMSS_Realm) &&
> > +    if ((arm_hcr_el2_eff(env) & HCR_E2H) &&
> > +        (ss == ARMSS_Root || ss == ARMSS_Realm) &&
> 
> When the architecture says "is EL2 enabled in the current security state"
> it doesn't mean "is HCR_EL2.E2H set?", it means "is this either 
> NonSecure/Realm
> or else is SCR_EL2.EEL2 set?". Compare the pseudocode EL2Enabled()
> and QEMU's arm_is_el2_enabled() and arm_is_el2_enabled_secstate() functions.
> This doesn't mean much in Root state, and for Realm state EL2 is always
> enabled (assuming it is implemented).
> 
> For this timer check, we're doing I think the same thing as the
> pseudocode AArch64.CheckTimerConditions(), which does:
> 
>   if (IsFeatureImplemented(FEAT_RME) && ss IN {SS_Root, SS_Realm} &&
>       CNTHCTL_EL2.CNTPMASK == '1') then
>      imask = '1';
> 
> so I'm inclined to say that our current implementation in QEMU is correct.

Indeed. I got confused with the specification, my apologies.

I am facing an issue with QEMU freezing waiting for a timer interrupt when
running with -icount shift=0,sleep=off. Bissection has shown that the issue
appeared with f6fc36deef6abcee406211f3e2f11ff894b87fa4.

Further testing suggests that the issue may come from gt_recalc_timer. Calling
gt_update_irq before timer_mod (as it was done before f6fc36deef6a) rather than
at the end of the function solves the issue. Is it possible that timer_mod
relies on cpu->gt_timer_outputs, which has not been modified at this point to
reflect the timer triggering?

> 
> >          ((timeridx == GTIMER_VIRT && (cnthctl & R_CNTHCTL_CNTVMASK_MASK)) 
> > ||
> >           (timeridx == GTIMER_PHYS && (cnthctl & 
> > R_CNTHCTL_CNTPMASK_MASK)))) {
> >          irqstate = 0;
> > --
> 
> thanks
> -- PMM

Best,

-- 
Florian

signature.asc
Description: PGP signature