On Wed, 27 Nov 2024 at 17:01, Alex Bennée <alex.ben...@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.ben...@linaro.org>
> Cc: Andrei Homescu <ahome...@google.com>
> Cc: Arve Hjønnevåg <a...@google.com>
> Cc: Rémi Denis-Courmont <remi.denis.courm...@huawei.com>
> ---
> include/hw/arm/bsa.h | 2 +
> target/arm/cpu.h | 2 +
> target/arm/gtimer.h | 4 +-
> hw/arm/virt.c | 2 +
> target/arm/cpu.c | 8 +++
> target/arm/helper.c | 155 +++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 172 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
> index 8eaab603c0..b4ecca1b1c 100644
> --- a/include/hw/arm/bsa.h
> +++ b/include/hw/arm/bsa.h
> @@ -22,6 +22,8 @@
> #define QEMU_ARM_BSA_H
>
> /* These are architectural INTID values */
> +#define ARCH_TIMER_S_VIRT_EL2_IRQ 19
> +#define ARCH_TIMER_S_EL2_IRQ 20
> #define VIRTUAL_PMU_IRQ 23
> #define ARCH_GIC_MAINT_IRQ 25
> #define ARCH_TIMER_NS_EL2_IRQ 26
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d86e641280..10b5354d6f 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1139,6 +1139,8 @@ void arm_gt_vtimer_cb(void *opaque);
> void arm_gt_htimer_cb(void *opaque);
> void arm_gt_stimer_cb(void *opaque);
> void arm_gt_hvtimer_cb(void *opaque);
> +void arm_gt_sel2timer_cb(void *opaque);
> +void arm_gt_sel2vtimer_cb(void *opaque);
>
> unsigned int gt_cntfrq_period_ns(ARMCPU *cpu);
> void gt_rme_post_el_change(ARMCPU *cpu, void *opaque);
> diff --git a/target/arm/gtimer.h b/target/arm/gtimer.h
> index b992941bef..3c097c59c7 100644
> --- a/target/arm/gtimer.h
> +++ b/target/arm/gtimer.h
> @@ -15,7 +15,9 @@ enum {
> GTIMER_HYP = 2,
> GTIMER_SEC = 3,
> GTIMER_HYPVIRT = 4,
> -#define NUM_GTIMERS 5
> + GTIMER_SEC_PEL2 = 5,
> + GTIMER_SEC_VEL2 = 6,
> +#define NUM_GTIMERS 7
Maybe we should add comments to this enum giving the architectural
names of these timers?
GTIMER_PHYS = 0, /* EL1 physical timer */
GTIMER_VIRT = 1, /* EL1 virtual timer */
GTIMER_HYP = 2, /* EL2 physical timer */
GTIMER_SEC = 3, /* EL3 physical timer */
GTIMER_HYPVIRT = 4, /* EL2 virtual timer */
GTIMER_SEC_PEL2 = 5, /* Secure EL2 physical timer */
GTIMER_SEC_VEL2 = 6, /* Secure EL2 virtual timer */
> };
>
> #endif
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 1a381e9a2b..451d154459 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -873,6 +873,8 @@ static void create_gic(VirtMachineState *vms,
> MemoryRegion *mem)
> [GTIMER_HYP] = ARCH_TIMER_NS_EL2_IRQ,
> [GTIMER_SEC] = ARCH_TIMER_S_EL1_IRQ,
> [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
> + [GTIMER_SEC_PEL2] = ARCH_TIMER_S_EL2_IRQ,
> + [GTIMER_SEC_VEL2] = ARCH_TIMER_S_VIRT_EL2_IRQ,
> };
>
> for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
We probably need to also update at least sbsa-ref for this new
pair of interrupts, and perhaps others (if they allow CPUs
with FEAT_SEL2).
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6938161b95..e42ab8ce8b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2078,6 +2078,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
> **errp)
> arm_gt_stimer_cb, cpu);
> cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> arm_gt_hvtimer_cb, cpu);
> +
> + /* FEAT_SEL2 also has physical and virtual timers */
> + if (cpu_isar_feature(aa64_sel2, cpu)) {
> + cpu->gt_timer[GTIMER_SEC_PEL2] = timer_new(QEMU_CLOCK_VIRTUAL,
> scale,
> + arm_gt_sel2timer_cb,
> cpu);
> + cpu->gt_timer[GTIMER_SEC_VEL2] = timer_new(QEMU_CLOCK_VIRTUAL,
> scale,
> + arm_gt_sel2vtimer_cb,
> cpu);
> + }
We create all the other gt_timer[] entries regardless of whether the
feature that uses them exists, and I think we should do the same
here. It's pretty harmless and it means code elsewhere doesn't
have to be super-careful about "is cpu->gt_timer[n] valid or is
it zeroed out memory?".
> }
> #endif
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f38eb054c0..b5a8a5846e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2668,6 +2668,41 @@ static CPAccessResult gt_stimer_access(CPUARMState
> *env,
> }
> }
>
> +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;
> + } 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;
> + }
> + return CP_ACCESS_OK;
> + case 3:
> + if (env->cp15.scr_el3 & SCR_EEL2) {
> + return CP_ACCESS_OK;
> + } else {
> + return CP_ACCESS_TRAP;
> + }
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> uint64_t gt_get_countervalue(CPUARMState *env)
> {
> ARMCPU *cpu = env_archcpu(env);
> @@ -3175,6 +3210,62 @@ static void gt_sec_ctl_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> gt_ctl_write(env, ri, GTIMER_SEC, value);
> }
>
> +static void gt_sec_pel2_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + gt_timer_reset(env, ri, GTIMER_SEC_PEL2);
> +}
> +
> +static void gt_sec_pel2_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + gt_cval_write(env, ri, GTIMER_SEC_PEL2, value);
> +}
> +
> +static uint64_t gt_sec_pel2_tval_read(CPUARMState *env, const ARMCPRegInfo
> *ri)
> +{
> + return gt_tval_read(env, ri, GTIMER_SEC_PEL2);
> +}
> +
> +static void gt_sec_pel2_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + gt_tval_write(env, ri, GTIMER_SEC_PEL2, value);
> +}
gt_tval_read() and gt_tval_write() have a switch which
determines whether they obey the physical offset (CNTPOFF_EL2)
or the virtual offset (CNTVOFF_EL2) or neither. For GTIMER_SEC_PEL2
the correct answer is "neither", which is what you get by default,
but for GTIMER_SEC_VEL2 we should be honouring CNTVOFF_EL2,
so you need to add a case to the switch for that.
We also need to fix a pre-existing bug in gt_recalc_timer(),
which does
uint64_t offset = timeridx == GTIMER_VIRT ?
cpu->env.cp15.cntvoff_el2 : gt_phys_raw_cnt_offset(&cpu->env);
and so fails to include CNTVOFF_EL2 when working with
GTIMER_HYPVIRT; then you can also add GTIMER_SEC_VEL2 to that.
> +
> +static void gt_sec_pel2_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + gt_ctl_write(env, ri, GTIMER_SEC_PEL2, value);
> +}
> +
> +static void gt_sec_vel2_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> + gt_timer_reset(env, ri, GTIMER_SEC_VEL2);
> +}
> +
> +static void gt_sec_vel2_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + gt_cval_write(env, ri, GTIMER_SEC_VEL2, value);
> +}
> +
> +static uint64_t gt_sec_vel2_tval_read(CPUARMState *env, const ARMCPRegInfo
> *ri)
> +{
> + return gt_tval_read(env, ri, GTIMER_SEC_VEL2);
> +}
> +
> +static void gt_sec_vel2_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + gt_tval_write(env, ri, GTIMER_SEC_VEL2, value);
> +}
> +
> +static void gt_sec_vel2_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + gt_ctl_write(env, ri, GTIMER_SEC_VEL2, value);
> +}
> +
> static void gt_hv_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> {
> gt_timer_reset(env, ri, GTIMER_HYPVIRT);
> @@ -3231,6 +3322,20 @@ void arm_gt_stimer_cb(void *opaque)
> gt_recalc_timer(cpu, GTIMER_SEC);
> }
>
> +void arm_gt_sel2timer_cb(void *opaque)
> +{
> + ARMCPU *cpu = opaque;
> +
> + gt_recalc_timer(cpu, GTIMER_SEC_PEL2);
> +}
> +
> +void arm_gt_sel2vtimer_cb(void *opaque)
> +{
> + ARMCPU *cpu = opaque;
> +
> + gt_recalc_timer(cpu, GTIMER_SEC_VEL2);
> +}
> +
> void arm_gt_hvtimer_cb(void *opaque)
> {
> ARMCPU *cpu = opaque;
> @@ -6613,6 +6718,56 @@ static const ARMCPRegInfo el2_sec_cp_reginfo[] = {
> .access = PL2_RW, .accessfn = sel2_access,
> .nv2_redirect_offset = 0x48,
> .fieldoffset = offsetof(CPUARMState, cp15.vstcr_el2) },
> +#ifndef CONFIG_USER_ONLY
> + /* Secure EL2 Physical Timer */
> + { .name = "CNTHPS_TVAL_EL2", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 5, .opc2 = 0,
> + .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL2_RW,
> + .accessfn = gt_sel2timer_access,
> + .readfn = gt_sec_pel2_tval_read,
> + .writefn = gt_sec_pel2_tval_write,
> + .resetfn = gt_sec_pel2_timer_reset,
> + },
> + { .name = "CNTHPS_CTL_EL2", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 5, .opc2 = 1,
> + .type = ARM_CP_IO, .access = PL2_RW,
> + .accessfn = gt_sel2timer_access,
> + .fieldoffset = offsetof(CPUARMState,
> cp15.c14_timer[GTIMER_SEC_PEL2].ctl),
> + .resetvalue = 0,
> + .writefn = gt_sec_pel2_ctl_write, .raw_writefn = raw_write,
> + },
> + { .name = "CNTHPS_CVAL_EL2", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 5, .opc2 = 2,
> + .type = ARM_CP_IO, .access = PL2_RW,
> + .accessfn = gt_sel2timer_access,
> + .fieldoffset = offsetof(CPUARMState,
> cp15.c14_timer[GTIMER_SEC_PEL2].cval),
> + .writefn = gt_sec_pel2_cval_write, .raw_writefn = raw_write,
> + },
> + /* Secure EL2 Virtual Timer */
> + { .name = "CNTHVS_TVAL_EL2", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 4, .opc2 = 0,
> + .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL2_RW,
> + .accessfn = gt_sel2timer_access,
> + .readfn = gt_sec_vel2_tval_read,
> + .writefn = gt_sec_vel2_tval_write,
> + .resetfn = gt_sec_vel2_timer_reset,
> + },
> + { .name = "CNTHVS_CTL_EL2", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 4, .opc2 = 1,
> + .type = ARM_CP_IO, .access = PL2_RW,
> + .accessfn = gt_sel2timer_access,
> + .fieldoffset = offsetof(CPUARMState,
> cp15.c14_timer[GTIMER_SEC_VEL2].ctl),
> + .resetvalue = 0,
> + .writefn = gt_sec_vel2_ctl_write, .raw_writefn = raw_write,
> + },
> + { .name = "CNTHVS_CVAL_EL2", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 4, .opc2 = 2,
> + .type = ARM_CP_IO, .access = PL2_RW,
> + .accessfn = gt_sel2timer_access,
> + .fieldoffset = offsetof(CPUARMState,
> cp15.c14_timer[GTIMER_SEC_VEL2].cval),
> + .writefn = gt_sec_vel2_cval_write, .raw_writefn = raw_write,
> + },
> +#endif
> };
thanks
-- PMM
