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

From: Andrew Jones <drjones@redhat.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org,
	armbru@redhat.com, eric.auger@redhat.com, qemu-arm@nongnu.org,
	imammedo@redhat.com, alex.bennee@linaro.org, Dave.Martin@arm.com
Subject: Re: [Qemu-devel] [PATCH v2 06/14] target/arm: Allow SVE to be disabled via a CPU property
Date: Wed, 26 Jun 2019 15:52:44 +0200	[thread overview]
Message-ID: <20190626135244.2jt6alr7lh2v43nu@kamzik.brq.redhat.com> (raw)
In-Reply-To: <c794d810-fbaf-ff81-9833-3e40fb5a951d@linaro.org>

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].

> 
> 
> >  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >                        uint64_t value)
> >  {
> > +    ARMCPU *cpu = env_archcpu(env);
> >      int cur_el = arm_current_el(env);
> > -    int old_len = sve_zcr_len_for_el(env, cur_el);
> > -    int new_len;
> > +    int old_len, new_len;
> > +
> > +    assert(cpu->sve_max_vq);
> 
> Certainly there's no reason for this assert, given the above.

Yeah, I'll remove for v3.

Thanks,
drew