On Fri, 6 Dec 2024 at 16:02, 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: qemu-sta...@nongnu.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>
>
> ---
> 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
