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

[Richard Henderson]

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~