Re: [PATCH v2 3/5] target/arm: implement SEL2 physical and virtual timers

[Peter Maydell]

On Wed, 18 Dec 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> When FEAT_SEL2 was implemented the SEL2 timers where missed. This
> shows up when building the latest Hafnium with SPMC_AT_EL=2. The
> actual implementation utilises the same logic as the rest of the
> timers so all we need to do is:
>
>   - define the timers and their access functions
>   - conditionally add the correct system registers
>   - create a new accessfn as the rules are subtly different to the
>     existing secure timer
>
> Fixes: e9152ee91c (target/arm: add ARMv8.4-SEL2 system registers)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.org
> Cc: Andrei Homescu <ahomescu@google.com>
> Cc: Arve Hjønnevåg <arve@google.com>
> Cc: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>
> ---
> v1
>   - add better comments to GTIMER descriptions
>   - also define new timers for sbsa-ref
>   - don't conditionally gate qemu_timer creation on the feature
>   - take cntvoff_el2 int account for SEC_VEL2 in gt_recalc/g_tval_[read|write]
> v2
>   - rename IRQ to ARCH_TIMER_S_EL2_VIRT_IRQ
>   - split machine enablement into separate patches
>   - return CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
> ---

> +static CPAccessResult gt_sel2timer_access(CPUARMState *env,
> +                                          const ARMCPRegInfo *ri,
> +                                          bool isread)
> +{
> +    /*
> +     * The AArch64 register view of the secure EL2 timers are mostly
> +     * accessible from EL3 and EL2 although can also be trapped to EL2
> +     * from EL1 depending on nested virt config.
> +     */
> +    switch (arm_current_el(env)) {
> +    case 0:
> +        return CP_ACCESS_TRAP;
> +    case 1:
> +        if (!arm_is_secure(env)) {
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        } else if (arm_hcr_el2_eff(env) & HCR_NV) {
> +            return CP_ACCESS_TRAP_EL2;
> +        }
> +        return CP_ACCESS_TRAP;
> +    case 2:
> +        if (!arm_is_secure(env)) {
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        }
> +        return CP_ACCESS_OK;
> +    case 3:
> +        if (env->cp15.scr_el3 & SCR_EEL2) {
> +            return CP_ACCESS_OK;
> +        } else {
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        }
> +    default:
> +        g_assert_not_reached();
> +    }
> +}

This code is still using CP_ACCESS_TRAP in some codepaths, which
isn't correct. Either:
 * you want an UNDEF: that's CP_ACCESS_TRAP_UNCATEGORIZED
 * you want to trap to some specific EL: that's CP_ACCESS_TRAP_EL2
   or CP_ACCESS_TRAP_EL3 depending on where you need to trap to.

thanks
-- PMM