Re: [Qemu-arm] [Qemu-devel] [PATCH v2 06/14] target/arm: Allow SVE to be disabled via a CPU property

[Andrew Jones]

On Wed, Jun 26, 2019 at 03:52:44PM +0200, Andrew Jones wrote:
> On Wed, Jun 26, 2019 at 12:20:29PM +0200, Richard Henderson wrote:
> > On 6/21/19 6:34 PM, Andrew Jones wrote:
> > > Since 97a28b0eeac14 ("target/arm: Allow VFP and Neon to be disabled via
> > > a CPU property") we can disable the 'max' cpu model's VFP and neon
> > > features, but there's no way to disable SVE. Add the 'sve=on|off'
> > > property to give it that flexibility. We also rename
> > > cpu_max_get/set_sve_vq to cpu_max_get/set_sve_max_vq in order for them
> > > to follow the typical *_get/set_<property-name> pattern.
> > 
> > I think perhaps the new property should not be overloaded on 
> > cpu->sve_max_vq.
> > 
> > At present you are generating an error for
> > 
> >     -cpu max,sve=off,sve_max_vq=2
> > 
> > but not for
> > 
> >     -cpu max,sve_max_vq=2,sve=off
> > 
> > and then there's the issue of
> > 
> >     -cpu max,sve_max_vq=2,sve=off,sve=on
> > 
> > discarding the earlier sve_max_vq setting.
> 
> Yeah, it might be best to add a new boolean in order for that last example
> to work as expected. At least my expectation would be that we'd set the
> max vq to 2, when sve is disabled nothing happens to it, but then when sve
> is reenabled we'll still have that max vq 2. I'm guessing you're expecting
> that too, since you brought it up. I think the other two examples above
> are behaving as I'd expect them to.
> 
> > 
> > 
> > > @@ -1129,6 +1129,14 @@ static void arm_cpu_realizefn(DeviceState *dev, 
> > > Error **errp)
> > >          cpu->isar.mvfr0 = u;
> > >      }
> > >  
> > > +    if (!cpu->sve_max_vq) {
> > > +        uint64_t t;
> > > +
> > > +        t = cpu->isar.id_aa64pfr0;
> > > +        t = FIELD_DP64(t, ID_AA64PFR0, SVE, 0);
> > > +        cpu->isar.id_aa64pfr0 = t;
> > > +    }
> > 
> > 
> > I suppse the isar bits are initialized too late for you to be able to re-use
> > the ID_AA64PFR0.SVE field *as* the property?
> 
> Hmm, that's probably worth trying. Also reworking vfp and neon to use the
> fields as the properties, putting the sanity checks directly in the
> property set accessor may work too. pmu and aarch64 could work like that
> too, but those still have ARM_FEATURE_* bits, so they're not really the
> same [yet].

I played with this for the PMU, i.e. I removed 'has_pmu' and used its bits
in id_aa64dfr0. Everything worked, so it's possible. I think the changes
to convert the PMU, VFP, and NEON are too far outside the scope of this
series to do them now, but I'll attempt to add a has-sve boolean without
adding a "has_sve" boolean in this series by using id_aa64pfr0.

Thanks,
drew