On Thu, Jun 20, 2024 at 11:43:17AM +0100, Peter Maydell wrote:
> On Sat, 15 Jun 2024 at 19:56, Florian Lugou <florian.lu...@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.lu...@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
