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

[Peter Maydell]

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