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

[Philippe Mathieu-Daudé]

On 18/12/24 19:15, Alex Bennée 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
> ---
>   include/hw/arm/bsa.h |   2 +
>   target/arm/cpu.h     |   2 +
>   target/arm/gtimer.h  |   4 +-
>   target/arm/cpu.c     |   4 ++
>   target/arm/helper.c  | 158 +++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 169 insertions(+), 1 deletion(-)


> 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 */

Should we rename as GTIMER_SEC_PEL3 for consistency?

>       GTIMER_HYPVIRT  = 4, /* EL2 virtual timer */

Also GTIMER_HYP     -> GTIMER_PEL2,
     GTIMER_HYPVIRT -> GTIMER_VEL2?

> -#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|