Re: [PATCH v5 4/9] target/arm/cpu64: max cpu: Introduce sve<N> properties

[Andrew Jones]

On Mon, Oct 07, 2019 at 09:35:35AM +0100, Alex Bennée wrote:
> 
> Andrew Jones <address@hidden> writes:
> 
> > Introduce cpu properties to give fine control over SVE vector lengths.
> > We introduce a property for each valid length up to the current
> > maximum supported, which is 2048-bits. The properties are named, e.g.
> > sve128, sve256, sve384, sve512, ..., where the number is the number of
> > bits. See the updates to docs/arm-cpu-features.rst for a description
> > of the semantics and for example uses.
> >
> > Note, as sve-max-vq is still present and we'd like to be able to
> > support qmp_query_cpu_model_expansion with guests launched with e.g.
> > -cpu max,sve-max-vq=8 on their command lines, then we do allow
> > sve-max-vq and sve<N> properties to be provided at the same time, but
> > this is not recommended, and is why sve-max-vq is not mentioned in the
> > document.  If sve-max-vq is provided then it enables all lengths smaller
> > than and including the max and disables all lengths larger. It also has
> > the side-effect that no larger lengths may be enabled and that the max
> > itself cannot be disabled. Smaller non-power-of-two lengths may,
> > however, be disabled, e.g. -cpu max,sve-max-vq=4,sve384=off provides a
> > guest the vector lengths 128, 256, and 512 bits.
> >
> > This patch has been co-authored with Richard Henderson, who reworked
> > the target/arm/cpu64.c changes in order to push all the validation and
> > auto-enabling/disabling steps into the finalizer, resulting in a nice
> > LOC reduction.
> >
> > Signed-off-by: Andrew Jones <address@hidden>
> > ---
> >  docs/arm-cpu-features.rst | 168 +++++++++++++++++++++++++++++++--
> >  include/qemu/bitops.h     |   1 +
> >  target/arm/cpu.c          |  19 ++++
> >  target/arm/cpu.h          |  19 ++++
> >  target/arm/cpu64.c        | 192 ++++++++++++++++++++++++++++++++++++-
> >  target/arm/helper.c       |  10 +-
> >  target/arm/monitor.c      |  12 +++
> >  tests/arm-cpu-features.c  | 194 ++++++++++++++++++++++++++++++++++++++
> >  8 files changed, 606 insertions(+), 9 deletions(-)
> >
> > diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
> > index c79dcffb5556..2ea4d6e90c02 100644
> > --- a/docs/arm-cpu-features.rst
> > +++ b/docs/arm-cpu-features.rst
> > @@ -48,18 +48,31 @@ block in the script for usage) is used to issue the QMP 
> > commands.
> >        (QEMU) query-cpu-model-expansion type=full model={"name":"max"}
> >        { "return": {
> >          "model": { "name": "max", "props": {
> > -        "pmu": true, "aarch64": true
> > +        "sve1664": true, "pmu": true, "sve1792": true, "sve1920": true,
> > +        "sve128": true, "aarch64": true, "sve1024": true, "sve": true,
> > +        "sve640": true, "sve768": true, "sve1408": true, "sve256": true,
> > +        "sve1152": true, "sve512": true, "sve384": true, "sve1536": true,
> > +        "sve896": true, "sve1280": true, "sve2048": true
> 
> Does having a list of VL's not have implications for the versioning of
> the API? Do we need to tick a version each time a new vector length is
> added?
>

Versioning the set of cpu features a versioned machine type may enable is
a good idea, but I don't think we should add versioning to this API. Since
it doesn't matter what gets returned by the expansion call as much as
whether or not it may be enabled, we can use both existence and permission
to control the enabling of features. And, we don't have to do it the same
way for all features. Here are some examples:

* The 'aarch64' feature doesn't exist on armv7 CPUs. So it can't be
  enabled there.

* The 'pmu' feature does exist on KVM armv7 CPUs, but KVM doesn't
  support it, so it can't be enabled there.

If we add features that could be enabled, but we don't want them to be
available to older machine types, then we can either ensure the CPU
property isn't added, or is removed, from the CPU model when that machine
type is in use, or we can add code to the enable function that fails
the enable when a machine type dependent condition check fails.

Does that answer the question?

Thanks,
drew