Re: [PATCH 20/71] target/arm: Add ID_AA64SMFR0_EL1

[Peter Maydell]

On Thu, 2 Jun 2022 at 23:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This register is allocated from the existing block of id registers,
> so it is already RES0 for cpus that do not implement SME.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -682,13 +682,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
> *ahcf)
>          ahcf->isar.id_aa64pfr0 = t;
>
>          /*
> -         * Before v5.1, KVM did not support SVE and did not expose
> -         * ID_AA64ZFR0_EL1 even as RAZ.  After v5.1, KVM still does
> -         * not expose the register to "user" requests like this
> -         * unless the host supports SVE.
> +         * KVM began exposing the unallocated ID registers as RAZ in 4.15.
> +         * Using SVE supported is an easy way to tell if these registers
> +         * are exposed, since both of these depend on SVE anyway.
>           */

This slightly loses context described in the old comment, though the
old comment isn't quite correct either. Between kernel commits 73433762fcae
(part of the initial implementation of SVE support) and f81cb2c3ad41 (which
fixed this bug), the kernel did indeed not expose ID_AA64ZFR0_EL1 as RAZ if
SVE was not implemented. So there's a range of kernels that had this
bug and for which we need to guard the access to ID_AA64ZFR0_EL1.
This isn't the case for ID_AA64SMFR0_EL1, though, which all kernels
should handle correctly (ignoring the pre-4.15 case).

So I think:
 (1) we should read ID_AA64SMFR0_EL1 further up in the same code
block where we read all the other ID registers like id_aa64mmfr0 etc.

 (2) separately, we should update this comment to read something like:

/*
 * There is a range of kernels between kernel commit 73433762fcae
 * and f81cb2c3ad41 which have a bug where the kernel doesn't expose
 * SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has enabled
 * SVE support, so we only read it here, rather than together with all
 * the other ID registers earlier.
 */

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM