Re: [PATCH v2 6/6] target/arm/helper: Implement CNTHCTL_EL2.CNT[VP]MASK

[Peter Maydell]

On Wed, 2 Aug 2023 at 18:02, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> When FEAT_RME is implemented, these bits override the value of
> CNT[VP]_CTL_EL0.IMASK in Realm and Root state. Move the IRQ state update
> into a new gt_update_irq() function and test those bits every time we
> recompute the IRQ state.
>
> Since we're removing the IRQ state from some trace events, add a new
> trace event for gt_update_irq().
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  target/arm/cpu.h        |  3 +++
>  target/arm/helper.c     | 54 ++++++++++++++++++++++++++++++++---------
>  target/arm/trace-events |  7 +++---
>  3 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bcd65a63ca..bedc7ec6dc 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1743,6 +1743,9 @@ static inline void xpsr_write(CPUARMState *env, 
> uint32_t val, uint32_t mask)
>  #define HSTR_TTEE (1 << 16)
>  #define HSTR_TJDBX (1 << 17)
>
> +#define CNTHCTL_CNTVMASK      (1 << 18)
> +#define CNTHCTL_CNTPMASK      (1 << 19)
> +
>  /* Return the current FPSCR value.  */
>  uint32_t vfp_get_fpscr(CPUARMState *env);
>  void vfp_set_fpscr(CPUARMState *env, uint32_t val);
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 77dd80ad28..68e915ddda 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2608,6 +2608,28 @@ static uint64_t gt_get_countervalue(CPUARMState *env)
>      return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(cpu);
>  }
>
> +static void gt_update_irq(ARMCPU *cpu, int timeridx)
> +{
> +    CPUARMState *env = &cpu->env;
> +    uint64_t cnthctl = env->cp15.cnthctl_el2;
> +    ARMSecuritySpace ss = arm_security_space(env);
> +    /* ISTATUS && !IMASK */
> +    int irqstate = (env->cp15.c14_timer[timeridx].ctl & 6) == 4;
> +
> +    /*
> +     * 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) &&
> +        ((timeridx == GTIMER_VIRT && (cnthctl & CNTHCTL_CNTVMASK)) ||
> +         (timeridx == GTIMER_PHYS && (cnthctl & CNTHCTL_CNTPMASK)))) {
> +        irqstate = 0;
> +    }
> +
> +    qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
> +    trace_arm_gt_update_irq(timeridx, irqstate);
> +}

> @@ -2888,6 +2904,21 @@ static void gt_virt_ctl_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      gt_ctl_write(env, ri, GTIMER_VIRT, value);
>  }
>
> +static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                             uint64_t value)
> +{
> +    ARMCPU *cpu = env_archcpu(env);
> +    uint32_t oldval = env->cp15.cnthctl_el2;
> +
> +    raw_write(env, ri, value);
> +
> +    if ((oldval ^ value) & CNTHCTL_CNTVMASK) {
> +        gt_update_irq(cpu, GTIMER_VIRT);
> +    } else if ((oldval ^ value) & CNTHCTL_CNTPMASK) {
> +        gt_update_irq(cpu, GTIMER_PHYS);
> +    }
> +}
> +
>  static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                                uint64_t value)
>  {
> @@ -6200,7 +6231,8 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>         * reset values as IMPDEF. We choose to reset to 3 to comply with
>         * both ARMv7 and ARMv8.
>         */
> -      .access = PL2_RW, .resetvalue = 3,
> +      .access = PL2_RW, .type = ARM_CP_IO, .resetvalue = 3,
> +      .writefn = gt_cnthctl_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.cnthctl_el2) },

You also need to add ".raw_writefn = raw_write". Otherwise
on an inbound migration we will use the writefn to update
the register value from the inbound data, which will
update the irq line and potentially mess up the state of
the device on the other end of it.

>      { .name = "CNTVOFF_EL2", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 0, .opc2 = 3,

The other missing thing here is that we now need to recalculate
the interrupt state when the security state changes, because
a transition from (for instance) EL1 NonSecure to EL3 Root
means the CNTVMASK and CNTPMASK bits should now take effect.
The security state can only change on a transition either
into or out of EL3, so you can do this with an el_change_hook:
call arm_register_el_change_hook() in the same way we do
already to set up the pmu_post_el_change function. Then in
that function you can call gt_update_irq(). (The hook gets
called after the exception return has updated the CPU state
to the target exception level.)

thanks
-- PMM