diff mbox series

[v2,12/12] target/arm: Allow users to set the number of VFP registers

Message ID 20230607043943.1837186-13-clg@kaod.org
State New
Headers show
Series aspeed: fixes and extensions | expand

Commit Message

Cédric Le Goater June 7, 2023, 4:39 a.m. UTC
Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
have 16 64-bit FPU registers and not 32 registers. Let users set the
number of VFP registers with a CPU property.

The primary use case of this property is for the Cortex A7 of the
Aspeed AST2600 SoC.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/arm/cpu.h        |  2 ++
 hw/arm/aspeed_ast2600.c |  2 ++
 target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

Comments

Joel Stanley June 7, 2023, 11:06 a.m. UTC | #1
On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
>
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

You saw a crash with a buildroot image without this change, as I recall?

The logic is a bit hard to follow but it is good to see a fix.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  target/arm/cpu.h        |  2 ++
>  hw/arm/aspeed_ast2600.c |  2 ++
>  target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d469a2637b32..79f1a96ddf39 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -916,6 +916,8 @@ struct ArchCPU {
>      bool has_pmu;
>      /* CPU has VFP */
>      bool has_vfp;
> +    /* CPU has 32 VFP registers */
> +    bool has_vfp_d32;
>      /* CPU has Neon */
>      bool has_neon;
>      /* CPU has M-profile DSP extension */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 1bf12461481c..a8b3a8065a11 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>                                  &error_abort);
>          object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>                                  &error_abort);
> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
> +                                &error_abort);
>          object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>                                   OBJECT(s->memory), &error_abort);
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5182ed0c9113..74fe6ae78192 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
>  static Property arm_cpu_has_vfp_property =
>              DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>
> +static Property arm_cpu_has_vfp_d32_property =
> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
> +
>  static Property arm_cpu_has_neon_property =
>              DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>
> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>          }
>      }
>
> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> +        cpu->has_vfp_d32 = true;
> +        if (!kvm_enabled()) {
> +            /*
> +             * The permitted values of the SIMDReg bits [3:0] on
> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> +             * make sure that has_vfp_d32 can not be set to false.
> +             */
> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> +                qdev_property_add_static(DEVICE(obj),
> +                                         &arm_cpu_has_vfp_d32_property);
> +            }
> +        }
> +    }
> +
>      if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>          cpu->has_neon = true;
>          if (!kvm_enabled()) {
> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
> +        return;
> +    }
> +
> +   if (!cpu->has_vfp_d32) {
> +        uint32_t u;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
> +        cpu->isar.mvfr0 = u;
> +    }
> +
>      if (!cpu->has_vfp) {
>          uint64_t t;
>          uint32_t u;
> --
> 2.40.1
>
Cédric Le Goater June 7, 2023, 2:19 p.m. UTC | #2
On 6/7/23 13:06, Joel Stanley wrote:
> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
>> have 16 64-bit FPU registers and not 32 registers. Let users set the
>> number of VFP registers with a CPU property.
>>
>> The primary use case of this property is for the Cortex A7 of the
>> Aspeed AST2600 SoC.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> You saw a crash with a buildroot image without this change, as I recall?

yes, when compiled with VFPv4d32 support, user space crashes on real HW.

Thanks,

C.

> 
> The logic is a bit hard to follow but it is good to see a fix.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>> ---
>>   target/arm/cpu.h        |  2 ++
>>   hw/arm/aspeed_ast2600.c |  2 ++
>>   target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
>>   3 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index d469a2637b32..79f1a96ddf39 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -916,6 +916,8 @@ struct ArchCPU {
>>       bool has_pmu;
>>       /* CPU has VFP */
>>       bool has_vfp;
>> +    /* CPU has 32 VFP registers */
>> +    bool has_vfp_d32;
>>       /* CPU has Neon */
>>       bool has_neon;
>>       /* CPU has M-profile DSP extension */
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1bf12461481c..a8b3a8065a11 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>                                   &error_abort);
>>           object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>>                                   &error_abort);
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
>> +                                &error_abort);
>>           object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>>                                    OBJECT(s->memory), &error_abort);
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5182ed0c9113..74fe6ae78192 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
>>   static Property arm_cpu_has_vfp_property =
>>               DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>>
>> +static Property arm_cpu_has_vfp_d32_property =
>> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
>> +
>>   static Property arm_cpu_has_neon_property =
>>               DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>>
>> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>>           }
>>       }
>>
>> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
>> +        cpu->has_vfp_d32 = true;
>> +        if (!kvm_enabled()) {
>> +            /*
>> +             * The permitted values of the SIMDReg bits [3:0] on
>> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
>> +             * make sure that has_vfp_d32 can not be set to false.
>> +             */
>> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
>> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
>> +                qdev_property_add_static(DEVICE(obj),
>> +                                         &arm_cpu_has_vfp_d32_property);
>> +            }
>> +        }
>> +    }
>> +
>>       if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>>           cpu->has_neon = true;
>>           if (!kvm_enabled()) {
>> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           return;
>>       }
>>
>> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
>> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
>> +        return;
>> +    }
>> +
>> +   if (!cpu->has_vfp_d32) {
>> +        uint32_t u;
>> +
>> +        u = cpu->isar.mvfr0;
>> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
>> +        cpu->isar.mvfr0 = u;
>> +    }
>> +
>>       if (!cpu->has_vfp) {
>>           uint64_t t;
>>           uint32_t u;
>> --
>> 2.40.1
>>
Peter Maydell June 8, 2023, 10:19 a.m. UTC | #3
On Wed, 7 Jun 2023 at 05:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
>
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

thanks
-- PMM
Mads Ynddal June 19, 2023, 12:47 p.m. UTC | #4
Sorry, if this has already been acknowledged, but I couldn't find it on the
mailinglist.

This commit seems to break compatibility with macOS accelerator hvf when
virtualizing ARM CPUs.

It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
same VM worked on earlier versions of QEMU.

It can be reproduced with the following:

qemu-system-aarch64 \
  -nodefaults \
  -display "none" \
  -machine "virt" \
  -accel "hvf" \
  -cpu "host" \
  -serial "mon:stdio"
qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither


If you fix/work on this issue in a separate thread/patch, you can add
reported-by, so I'll automatically follow and help test it:

Reported-by: Mads Ynddal <mads@ynddal.dk>

—
Mads Ynddal

> On 7 Jun 2023, at 06.39, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
> 
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> target/arm/cpu.h        |  2 ++
> hw/arm/aspeed_ast2600.c |  2 ++
> target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
> 3 files changed, 36 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d469a2637b32..79f1a96ddf39 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -916,6 +916,8 @@ struct ArchCPU {
>     bool has_pmu;
>     /* CPU has VFP */
>     bool has_vfp;
> +    /* CPU has 32 VFP registers */
> +    bool has_vfp_d32;
>     /* CPU has Neon */
>     bool has_neon;
>     /* CPU has M-profile DSP extension */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 1bf12461481c..a8b3a8065a11 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>                                 &error_abort);
>         object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>                                 &error_abort);
> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
> +                                &error_abort);
>         object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>                                  OBJECT(s->memory), &error_abort);
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5182ed0c9113..74fe6ae78192 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
> static Property arm_cpu_has_vfp_property =
>             DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
> 
> +static Property arm_cpu_has_vfp_d32_property =
> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
> +
> static Property arm_cpu_has_neon_property =
>             DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
> 
> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>         }
>     }
> 
> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> +        cpu->has_vfp_d32 = true;
> +        if (!kvm_enabled()) {
> +            /*
> +             * The permitted values of the SIMDReg bits [3:0] on
> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> +             * make sure that has_vfp_d32 can not be set to false.
> +             */
> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> +                qdev_property_add_static(DEVICE(obj),
> +                                         &arm_cpu_has_vfp_d32_property);
> +            }
> +        }
> +    }
> +
>     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>         cpu->has_neon = true;
>         if (!kvm_enabled()) {
> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>         return;
>     }
> 
> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
> +        return;
> +    }
> +
> +   if (!cpu->has_vfp_d32) {
> +        uint32_t u;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
> +        cpu->isar.mvfr0 = u;
> +    }
> +
>     if (!cpu->has_vfp) {
>         uint64_t t;
>         uint32_t u;
> -- 
> 2.40.1
> 
> 
>
Cédric Le Goater June 19, 2023, 1:09 p.m. UTC | #5
On 6/19/23 14:47, Mads Ynddal wrote:
> Sorry, if this has already been acknowledged, but I couldn't find it on the
> mailinglist.
> 
> This commit seems to break compatibility with macOS accelerator hvf when
> virtualizing ARM CPUs.
> 
> It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
> and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
> same VM worked on earlier versions of QEMU.
> 
> It can be reproduced with the following:
> 
> qemu-system-aarch64 \
>    -nodefaults \
>    -display "none" \
>    -machine "virt" \
>    -accel "hvf" \
>    -cpu "host" \
>    -serial "mon:stdio"
> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither

ARM's "Vector Floating Point" unit has many implementation with different
features: VFPv3-D16/D32, *FP16, VFPv4-D16/D32, Neon, etc. The test might
be too strict and could possibly be removed.

Could you send us the result of 'cat /proc/cpuinfo' on the host ?

Thanks,

C.

> If you fix/work on this issue in a separate thread/patch, you can add
> reported-by, so I'll automatically follow and help test it:
> 
> Reported-by: Mads Ynddal <mads@ynddal.dk>
> 
> —
> Mads Ynddal
> 
>> On 7 Jun 2023, at 06.39, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
>> have 16 64-bit FPU registers and not 32 registers. Let users set the
>> number of VFP registers with a CPU property.
>>
>> The primary use case of this property is for the Cortex A7 of the
>> Aspeed AST2600 SoC.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> target/arm/cpu.h        |  2 ++
>> hw/arm/aspeed_ast2600.c |  2 ++
>> target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
>> 3 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index d469a2637b32..79f1a96ddf39 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -916,6 +916,8 @@ struct ArchCPU {
>>      bool has_pmu;
>>      /* CPU has VFP */
>>      bool has_vfp;
>> +    /* CPU has 32 VFP registers */
>> +    bool has_vfp_d32;
>>      /* CPU has Neon */
>>      bool has_neon;
>>      /* CPU has M-profile DSP extension */
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1bf12461481c..a8b3a8065a11 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>                                  &error_abort);
>>          object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>>                                  &error_abort);
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
>> +                                &error_abort);
>>          object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>>                                   OBJECT(s->memory), &error_abort);
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5182ed0c9113..74fe6ae78192 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
>> static Property arm_cpu_has_vfp_property =
>>              DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>>
>> +static Property arm_cpu_has_vfp_d32_property =
>> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
>> +
>> static Property arm_cpu_has_neon_property =
>>              DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>>
>> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>>          }
>>      }
>>
>> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
>> +        cpu->has_vfp_d32 = true;
>> +        if (!kvm_enabled()) {
>> +            /*
>> +             * The permitted values of the SIMDReg bits [3:0] on
>> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
>> +             * make sure that has_vfp_d32 can not be set to false.
>> +             */
>> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
>> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
>> +                qdev_property_add_static(DEVICE(obj),
>> +                                         &arm_cpu_has_vfp_d32_property);
>> +            }
>> +        }
>> +    }
>> +
>>      if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>>          cpu->has_neon = true;
>>          if (!kvm_enabled()) {
>> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          return;
>>      }
>>
>> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
>> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
>> +        return;
>> +    }
>> +
>> +   if (!cpu->has_vfp_d32) {
>> +        uint32_t u;
>> +
>> +        u = cpu->isar.mvfr0;
>> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
>> +        cpu->isar.mvfr0 = u;
>> +    }
>> +
>>      if (!cpu->has_vfp) {
>>          uint64_t t;
>>          uint32_t u;
>> -- 
>> 2.40.1
>>
>>
>>
>
Peter Maydell June 19, 2023, 1:41 p.m. UTC | #6
On Mon, 19 Jun 2023 at 13:47, Mads Ynddal <mads@ynddal.dk> wrote:
>
> Sorry, if this has already been acknowledged, but I couldn't find it on the
> mailinglist.
>
> This commit seems to break compatibility with macOS accelerator hvf when
> virtualizing ARM CPUs.
>
> It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
> and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
> same VM worked on earlier versions of QEMU.
>
> It can be reproduced with the following:
>
> qemu-system-aarch64 \
>   -nodefaults \
>   -display "none" \
>   -machine "virt" \
>   -accel "hvf" \
>   -cpu "host" \
>   -serial "mon:stdio"
> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
>
>
> If you fix/work on this issue in a separate thread/patch, you can add
> reported-by, so I'll automatically follow and help test it:
>
> Reported-by: Mads Ynddal <mads@ynddal.dk>
>


> > @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
> >         }
> >     }
> >
> > +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> > +        cpu->has_vfp_d32 = true;
> > +        if (!kvm_enabled()) {

Probably this should be "if (!kvm_enabled() && !hvf_enabled())".
Is that sufficient to fix the regression ? (I have a feeling it
isn't, but we might as well test...)

> > +            /*
> > +             * The permitted values of the SIMDReg bits [3:0] on
> > +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> > +             * make sure that has_vfp_d32 can not be set to false.
> > +             */
> > +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> > +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> > +                qdev_property_add_static(DEVICE(obj),
> > +                                         &arm_cpu_has_vfp_d32_property);
> > +            }
> > +        }
> > +    }
> > +
> >     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
> >         cpu->has_neon = true;
> >         if (!kvm_enabled()) {
> > @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >         return;
> >     }
> >
> > +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> > +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
> > +        return;
> > +    }

The other thing I see looking again at this code is that it
doesn't account for CPUs which don't have AArch32 support
at all. The MVFR0 register which the aa32_simd_r32 feature
test is looking at is an AArch32 register, and the test
will not return a sensible answer on an AArch64-only CPU.

On the other side of this, target/arm/hvf/hvf.c always
sets ARM_FEATURE_NEON, which I think is probably not
correct given that Neon is also an AArch32-only thing.

thanks
-- PMM
Mads Ynddal June 19, 2023, 1:46 p.m. UTC | #7
> ARM's "Vector Floating Point" unit has many implementation with different
> features: VFPv3-D16/D32, *FP16, VFPv4-D16/D32, Neon, etc. The test might
> be too strict and could possibly be removed.
> 
> Could you send us the result of 'cat /proc/cpuinfo' on the host ?
> 
> Thanks,
> 
> C.

The host is macOS, so there's no '/proc/cpuinfo'. I can get this from sysctl:

$ sysctl -a | grep hw.optional
hw.optional.arm.FEAT_FlagM: 1
hw.optional.arm.FEAT_FlagM2: 1
hw.optional.arm.FEAT_FHM: 1
hw.optional.arm.FEAT_DotProd: 1
hw.optional.arm.FEAT_SHA3: 1
hw.optional.arm.FEAT_RDM: 1
hw.optional.arm.FEAT_LSE: 1
hw.optional.arm.FEAT_SHA256: 1
hw.optional.arm.FEAT_SHA512: 1
hw.optional.arm.FEAT_SHA1: 1
hw.optional.arm.FEAT_AES: 1
hw.optional.arm.FEAT_PMULL: 1
hw.optional.arm.FEAT_SPECRES: 0
hw.optional.arm.FEAT_SB: 1
hw.optional.arm.FEAT_FRINTTS: 1
hw.optional.arm.FEAT_LRCPC: 1
hw.optional.arm.FEAT_LRCPC2: 1
hw.optional.arm.FEAT_FCMA: 1
hw.optional.arm.FEAT_JSCVT: 1
hw.optional.arm.FEAT_PAuth: 1
hw.optional.arm.FEAT_PAuth2: 0
hw.optional.arm.FEAT_FPAC: 0
hw.optional.arm.FEAT_DPB: 1
hw.optional.arm.FEAT_DPB2: 1
hw.optional.arm.FEAT_BF16: 0
hw.optional.arm.FEAT_I8MM: 0
hw.optional.arm.FEAT_ECV: 1
hw.optional.arm.FEAT_LSE2: 1
hw.optional.arm.FEAT_CSV2: 1
hw.optional.arm.FEAT_CSV3: 1
hw.optional.arm.FEAT_DIT: 1
hw.optional.arm.FEAT_FP16: 1
hw.optional.arm.FEAT_SSBS: 1
hw.optional.arm.FEAT_BTI: 0
hw.optional.arm.FP_SyncExceptions: 1
hw.optional.floatingpoint: 1
hw.optional.neon: 1
hw.optional.neon_hpfp: 1
hw.optional.neon_fp16: 1
hw.optional.armv8_1_atomics: 1
hw.optional.armv8_2_fhm: 1
hw.optional.armv8_2_sha512: 1
hw.optional.armv8_2_sha3: 1
hw.optional.armv8_3_compnum: 1
hw.optional.watchpoint: 4
hw.optional.breakpoint: 6
hw.optional.armv8_crc32: 1
hw.optional.armv8_gpi: 1
hw.optional.AdvSIMD: 1
hw.optional.AdvSIMD_HPFPCvt: 1
hw.optional.ucnormal_mem: 1
hw.optional.arm64: 1


If it's any help, the Linux guest looks like this:

$ cat /proc/cpuinfo
processor : 0
BogoMIPS : 48.00
Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 asimddp sha512 asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp flagm2 frint
CPU implementer : 0x00
CPU architecture: 8
CPU variant : 0x0
CPU part : 0x000
CPU revision : 0

—
Mads Ynddal
Richard Henderson June 19, 2023, 1:54 p.m. UTC | #8
On 6/19/23 15:41, Peter Maydell wrote:
> On Mon, 19 Jun 2023 at 13:47, Mads Ynddal <mads@ynddal.dk> wrote:
>>
>> Sorry, if this has already been acknowledged, but I couldn't find it on the
>> mailinglist.
>>
>> This commit seems to break compatibility with macOS accelerator hvf when
>> virtualizing ARM CPUs.
>>
>> It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
>> and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
>> same VM worked on earlier versions of QEMU.
>>
>> It can be reproduced with the following:
>>
>> qemu-system-aarch64 \
>>    -nodefaults \
>>    -display "none" \
>>    -machine "virt" \
>>    -accel "hvf" \
>>    -cpu "host" \
>>    -serial "mon:stdio"
>> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
>>
>>
>> If you fix/work on this issue in a separate thread/patch, you can add
>> reported-by, so I'll automatically follow and help test it:
>>
>> Reported-by: Mads Ynddal <mads@ynddal.dk>
>>
> 
> 
>>> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>>>          }
>>>      }
>>>
>>> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
>>> +        cpu->has_vfp_d32 = true;
>>> +        if (!kvm_enabled()) {
> 
> Probably this should be "if (!kvm_enabled() && !hvf_enabled())".
> Is that sufficient to fix the regression ? (I have a feeling it
> isn't, but we might as well test...)

Yes, insufficient.  But I'm also changing these to tcg || qtest.

> 
>>> +            /*
>>> +             * The permitted values of the SIMDReg bits [3:0] on
>>> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
>>> +             * make sure that has_vfp_d32 can not be set to false.
>>> +             */
>>> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
>>> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
>>> +                qdev_property_add_static(DEVICE(obj),
>>> +                                         &arm_cpu_has_vfp_d32_property);
>>> +            }
>>> +        }
>>> +    }
>>> +
>>>      if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>>>          cpu->has_neon = true;
>>>          if (!kvm_enabled()) {
>>> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>          return;
>>>      }
>>>
>>> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
>>> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
>>> +        return;
>>> +    }
> 
> The other thing I see looking again at this code is that it
> doesn't account for CPUs which don't have AArch32 support
> at all. The MVFR0 register which the aa32_simd_r32 feature
> test is looking at is an AArch32 register, and the test
> will not return a sensible answer on an AArch64-only CPU.

This is the problem.  The code needs restructuring (which I am about to test).

> On the other side of this, target/arm/hvf/hvf.c always
> sets ARM_FEATURE_NEON, which I think is probably not
> correct given that Neon is also an AArch32-only thing.

At one time NEON also meant AdvSIMD, though we have now changed aa64 to the isar test.  We 
could probably get rid of NEON now too, with just a little more cleanup.


r~
diff mbox series

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d469a2637b32..79f1a96ddf39 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -916,6 +916,8 @@  struct ArchCPU {
     bool has_pmu;
     /* CPU has VFP */
     bool has_vfp;
+    /* CPU has 32 VFP registers */
+    bool has_vfp_d32;
     /* CPU has Neon */
     bool has_neon;
     /* CPU has M-profile DSP extension */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1bf12461481c..a8b3a8065a11 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -316,6 +316,8 @@  static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
                                 &error_abort);
         object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
                                 &error_abort);
+        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
+                                &error_abort);
         object_property_set_link(OBJECT(&s->cpu[i]), "memory",
                                  OBJECT(s->memory), &error_abort);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5182ed0c9113..74fe6ae78192 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1275,6 +1275,9 @@  static Property arm_cpu_cfgend_property =
 static Property arm_cpu_has_vfp_property =
             DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
 
+static Property arm_cpu_has_vfp_d32_property =
+            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
+
 static Property arm_cpu_has_neon_property =
             DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
 
@@ -1406,6 +1409,22 @@  void arm_cpu_post_init(Object *obj)
         }
     }
 
+    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
+        cpu->has_vfp_d32 = true;
+        if (!kvm_enabled()) {
+            /*
+             * The permitted values of the SIMDReg bits [3:0] on
+             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
+             * make sure that has_vfp_d32 can not be set to false.
+             */
+            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
+                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
+                qdev_property_add_static(DEVICE(obj),
+                                         &arm_cpu_has_vfp_d32_property);
+            }
+        }
+    }
+
     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
         cpu->has_neon = true;
         if (!kvm_enabled()) {
@@ -1672,6 +1691,19 @@  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (cpu->has_vfp_d32 != cpu->has_neon) {
+        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
+        return;
+    }
+
+   if (!cpu->has_vfp_d32) {
+        uint32_t u;
+
+        u = cpu->isar.mvfr0;
+        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
+        cpu->isar.mvfr0 = u;
+    }
+
     if (!cpu->has_vfp) {
         uint64_t t;
         uint32_t u;