Re: [PATCH v2 05/17] target/arm: Improve ID_AA64PFR0 FP/SIMD validation

[Peter Maydell]

On Mon, 24 Feb 2020 at 22:22, Richard Henderson
<address@hidden> wrote:
>
> When sanity checking id_aa64pfr0, use the raw FP and SIMD fields,
> because the values must match.  Delay the test until we've finished
> modifying the id_aa64pfr0 register.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/cpu.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5be4c25809..f10f34b655 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1427,17 +1427,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>
> -    if (arm_feature(env, ARM_FEATURE_AARCH64) &&
> -        cpu->has_vfp != cpu->has_neon) {
> -        /*
> -         * This is an architectural requirement for AArch64; AArch32 is
> -         * more flexible and permits VFP-no-Neon and Neon-no-VFP.
> -         */
> -        error_setg(errp,
> -                   "AArch64 CPUs must have both VFP and Neon or neither");
> -        return;
> -    }
> -
>      if (!cpu->has_vfp) {
>          uint64_t t;
>          uint32_t u;
> @@ -1537,6 +1526,18 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          cpu->isar.mvfr0 = u;
>      }
>
> +    if (arm_feature(env, ARM_FEATURE_AARCH64) &&
> +        FIELD_EX64(cpu->isar.id_aa64pfr0, ID_AA64PFR0, FP) !=
> +        FIELD_EX64(cpu->isar.id_aa64pfr0, ID_AA64PFR0, ADVSIMD)) {
> +        /*
> +         * This is an architectural requirement for AArch64.  Not only
> +         * both vfp and advsimd or neither, but further both must
> +         * support fp16 or neither.
> +         */
> +        error_setg(errp, "AArch64 CPUs must match VFP and NEON");
> +        return;
> +    }
> +
>      if (arm_feature(env, ARM_FEATURE_M) && !cpu->has_dsp) {
>          uint32_t u;

This check is supposed to be "did the user accidentally specify
some incompatible settings on their '-cpu,+this,-that' option?".
By making it check the actual ID register values, you're turning
it into also a check on "does the implementation specify sane
ID register values", which (a) is useful for TCG but ought to
be an assert and (b) we shouldn't be checking for KVM in case
the h/w is giving us dubious ID values.

thanks
-- PMM