| Message ID | 20241206160239.3229094-4-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: implement SEL2 physical and virtual timers | expand |
On Fri, 6 Dec 2024 at 16:02, 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] > --- > include/hw/arm/bsa.h | 2 + > target/arm/cpu.h | 2 + > target/arm/gtimer.h | 4 +- > hw/arm/sbsa-ref.c | 2 + > hw/arm/virt.c | 2 + I would put the board changes in their own patch(es). > diff --git a/target/arm/helper.c b/target/arm/helper.c > index cd147b717a..f82503304e 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; > + } These should generally be using CP_ACCESS_TRAP_UNCATEGORIZED, not CP_ACCESS_TRAP. The pseudocode uses "UNDEF", which means it wants ESR to be reported as an uncategorized-exception (classic UNDEF), not as a "trapped system register access". Almost always a trapped-sysreg-access is directed to a specific EL; an UNDEF is never directed to a specific EL but always to the usual destination for exceptions. I should probably check whether the other uses of CP_ACCESS_TRAP are correct or just bugs we haven't noticed yet... > + default: > + g_assert_not_reached(); > + } > +} thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 6 Dec 2024 at 16:02, 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] >> --- >> include/hw/arm/bsa.h | 2 + >> target/arm/cpu.h | 2 + >> target/arm/gtimer.h | 4 +- >> hw/arm/sbsa-ref.c | 2 + >> hw/arm/virt.c | 2 + > > I would put the board changes in their own patch(es). Won't that break bisection?
On Mon, 16 Dec 2024 at 19:32, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Fri, 6 Dec 2024 at 16:02, 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] > >> --- > >> include/hw/arm/bsa.h | 2 + > >> target/arm/cpu.h | 2 + > >> target/arm/gtimer.h | 4 +- > >> hw/arm/sbsa-ref.c | 2 + > >> hw/arm/virt.c | 2 + > > > > I would put the board changes in their own patch(es). > > Won't that break bisection? Any guest code attempting to use this timer currently is not going to work because the registers don't even exist. So there's no previous working state that would be broken. thanks -- PMM
On Fri, 6 Dec 2024 at 16:02, 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 > 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 Can we call this ARM_TIMER_S_EL2_VIRT_IRQ please? We currently have ARCH_TIMER_NS_EL2_VIRT_IRQ so we should be consistent about where in the name we put the "VIRT" bit. > +#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 -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 6 Dec 2024 at 16:02, 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 > >> 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 > > Can we call this ARM_TIMER_S_EL2_VIRT_IRQ please? I'm going to assume you mean ARCH_TIMER_S_EL2_VIRT_IRQ ;-) > We currently have ARCH_TIMER_NS_EL2_VIRT_IRQ > so we should be consistent about where in > the name we put the "VIRT" bit. > >> +#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 > > -- PMM
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 de016e6da3..f8f7425a5f 100644 --- a/target/arm/gtimer.h +++ b/target/arm/gtimer.h @@ -15,7 +15,9 @@ enum { GTIMER_HYP = 2, /* EL2 physical timer */ GTIMER_SEC = 3, /* EL3 physical timer */ GTIMER_HYPVIRT = 4, /* EL2 virtual timer */ -#define NUM_GTIMERS 5 + GTIMER_SEC_PEL2 = 5, /* Secure EL2 physical timer */ + GTIMER_SEC_VEL2 = 6, /* Secure EL2 virtual timer */ +#define NUM_GTIMERS 7 }; #endif diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index e3195d5449..5dc36b3cee 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -484,6 +484,8 @@ static void create_gic(SBSAMachineState *sms, 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 (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) { 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++) { diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 6938161b95..d15916c436 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -2078,6 +2078,10 @@ 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); + 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); } #endif diff --git a/target/arm/helper.c b/target/arm/helper.c index cd147b717a..f82503304e 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); @@ -2744,6 +2779,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) switch (timeridx) { case GTIMER_VIRT: case GTIMER_HYPVIRT: + case GTIMER_SEC_VEL2: offset = cpu->env.cp15.cntvoff_el2; break; default: @@ -2858,6 +2894,7 @@ static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri, switch (timeridx) { case GTIMER_VIRT: case GTIMER_HYPVIRT: + case GTIMER_SEC_VEL2: offset = gt_virt_cnt_offset(env); break; case GTIMER_PHYS: @@ -2878,6 +2915,7 @@ static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri, switch (timeridx) { case GTIMER_VIRT: case GTIMER_HYPVIRT: + case GTIMER_SEC_VEL2: offset = gt_virt_cnt_offset(env); break; case GTIMER_PHYS: @@ -3186,6 +3224,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); +} + +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); @@ -3242,6 +3336,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; @@ -6624,6 +6732,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 }; static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
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] --- include/hw/arm/bsa.h | 2 + target/arm/cpu.h | 2 + target/arm/gtimer.h | 4 +- hw/arm/sbsa-ref.c | 2 + hw/arm/virt.c | 2 + target/arm/cpu.c | 4 ++ target/arm/helper.c | 158 +++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 173 insertions(+), 1 deletion(-)