Re: [PULL 09/11] target/arm/kvm: host cpu: Add support for sve<N> properties

[Andrew Jones <drjones@redhat.com>]

On Thu, Feb 03, 2022 at 04:46:21PM +0000, Peter Maydell wrote:
> On Fri, 1 Nov 2019 at 08:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > From: Andrew Jones <drjones@redhat.com>
> >
> > Allow cpu 'host' to enable SVE when it's available, unless the
> > user chooses to disable it with the added 'sve=off' cpu property.
> > Also give the user the ability to select vector lengths with the
> > sve<N> properties. We don't adopt 'max' cpu's other sve property,
> > sve-max-vq, because that property is difficult to use with KVM.
> > That property assumes all vector lengths in the range from 1 up
> > to and including the specified maximum length are supported, but
> > there may be optional lengths not supported by the host in that
> > range. With KVM one must be more specific when enabling vector
> > lengths.
> 
> Hi; I've been looking at the '-cpu max' vs '-cpu host' code
> as part of trying to sort out the 'hvf' accelerator doing
> oddly different things with them. I noticed an oddity
> introduced in this patch. In the commit message you say that
> we deliberately leave the 'sve-max-vq' property out of the
> new aarch64_add_sve_properties(), because it is difficult to
> use with KVM. But in the code for handling '-cpu max' in
> aarch64_max_initfn():
> 
> > @@ -602,17 +617,11 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> >  static void aarch64_max_initfn(Object *obj)
> >  {
> >      ARMCPU *cpu = ARM_CPU(obj);
> > -    uint32_t vq;
> > -    uint64_t t;
> >
> >      if (kvm_enabled()) {
> >          kvm_arm_set_cpu_features_from_host(cpu);
> > -        if (kvm_arm_sve_supported(CPU(cpu))) {
> > -            t = cpu->isar.id_aa64pfr0;
> > -            t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
> > -            cpu->isar.id_aa64pfr0 = t;
> > -        }
> 
> because this 'if' doesn't exit the function early...
> 
> >      } else {
> > +        uint64_t t;
> >          uint32_t u;
> >          aarch64_a57_initfn(obj);
> >
> > @@ -712,17 +721,9 @@ static void aarch64_max_initfn(Object *obj)
> >  #endif
> >      }
> 
> ...all this code at the tail of the function runs for both KVM
> and TCG, and it still sets the sve-max-vq property, even for
> using '-cpu max' with KVM.
> 
> > -    object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> > -                        cpu_arm_set_sve, NULL, NULL, &error_fatal);
> > +    aarch64_add_sve_properties(obj);
> >      object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> >                          cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
> > -
> > -    for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> > -        char name[8];
> > -        sprintf(name, "sve%d", vq * 128);
> > -        object_property_add(obj, name, "bool", cpu_arm_get_sve_vq,
> > -                            cpu_arm_set_sve_vq, NULL, NULL, &error_fatal);
> > -    }
> >  }
> 
> Was this intentional?

No, darn. I don't know how many times I rebased that series and was always
careful to ensure sve-max-vq was left in the non-kvm part of the above
condition. I guess the final rebase finally got me...

> 
> I'd like to fix up the weird divergence between -cpu host and
> -cpu max, either by moving sve-max-vq into aarch64_add_sve_properties()
> so it's present on both, or by changing the aarch64_max_initfn() so
> it only adds the property when using TCG.

The later, please. sve-max-vq won't work for any of the machines that
support SVE that I know of, so I think it's a bad idea for KVM.

> 
> (I think also this code may get the '-cpu max,aarch64=off' case wrong,
> as it doesn't guard the calls to add the sve and pauth properties
> with the "if aarch64" feature check.)

Yes, but these property dependencies may need to be checked at property
finalize time. That means that the properties may get added, but then
they will error out if the user tried to enable them. Otherwise, they'll
be disabled and the QMP query will inform the user that they cannot be
enabled.

Thanks,
drew