| Message ID | 20230607043943.1837186-13-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed: fixes and extensions | expand |
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 >
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 >>
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
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 > > >
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 >> >> >> >
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
> 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
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 --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;
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(+)