diff mbox series

[v2,06/14] target/arm: Allow SVE to be disabled via a CPU property

Message ID 20190621163422.6127-7-drjones@redhat.com
State New
Headers show
Series target/arm/kvm: enable SVE in guests | expand

Commit Message

Andrew Jones June 21, 2019, 4:34 p.m. UTC
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.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu.c         | 10 +++++-
 target/arm/cpu64.c       | 72 ++++++++++++++++++++++++++++++++++------
 target/arm/helper.c      |  8 +++--
 target/arm/monitor.c     |  2 +-
 tests/arm-cpu-features.c |  1 +
 5 files changed, 78 insertions(+), 15 deletions(-)

Comments

Philippe Mathieu-Daudé June 21, 2019, 4:55 p.m. UTC | #1
Hi Drew,

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.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.c         | 10 +++++-
>  target/arm/cpu64.c       | 72 ++++++++++++++++++++++++++++++++++------
>  target/arm/helper.c      |  8 +++--
>  target/arm/monitor.c     |  2 +-
>  tests/arm-cpu-features.c |  1 +
>  5 files changed, 78 insertions(+), 15 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 858f668d226e..f08e178fc84b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -198,7 +198,7 @@ static void arm_cpu_reset(CPUState *s)
>          env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
>          env->cp15.cptr_el[3] |= CPTR_EZ;
>          /* with maximum vector length */
> -        env->vfp.zcr_el[1] = cpu->sve_max_vq - 1;
> +        env->vfp.zcr_el[1] = cpu->sve_max_vq ? cpu->sve_max_vq - 1 : 0;
>          env->vfp.zcr_el[2] = env->vfp.zcr_el[1];
>          env->vfp.zcr_el[3] = env->vfp.zcr_el[1];
>          /*
> @@ -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;
> +    }
> +
>      if (arm_feature(env, ARM_FEATURE_M) && !cpu->has_dsp) {
>          uint32_t u;
>  
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 946994838d8a..02ada65f240c 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -257,27 +257,75 @@ static void aarch64_a72_initfn(Object *obj)
>      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
>  }
>  
> -static void cpu_max_get_sve_vq(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> +static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      visit_type_uint32(v, name, &cpu->sve_max_vq, errp);
>  }
>  
> -static void cpu_max_set_sve_vq(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> +static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      Error *err = NULL;
> +    uint32_t value;
>  
> -    visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
> +    visit_type_uint32(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>  
> -    if (!err && (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ)) {
> -        error_setg(&err, "unsupported SVE vector length");
> -        error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
> +    if (!cpu->sve_max_vq) {
> +        error_setg(errp, "cannot set sve-max-vq");
> +        error_append_hint(errp, "SVE has been disabled with sve=off\n");
> +        return;
> +    }
> +
> +    cpu->sve_max_vq = value;
> +
> +    if (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ) {
> +        error_setg(errp, "unsupported SVE vector length");
> +        error_append_hint(errp, "Valid sve-max-vq in range [1-%d]\n",
>                            ARM_MAX_VQ);
>      }
> -    error_propagate(errp, err);
> +}
> +
> +static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    bool value = !!cpu->sve_max_vq;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    Error *err = NULL;
> +    bool value;
> +
> +    visit_type_bool(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    if (value) {
> +        /*
> +         * We handle the -cpu <cpu>,sve=off,sve=on case by reinitializing,
> +         * but otherwise we don't do anything as an sve=on could come after
> +         * a sve-max-vq setting.

I don't understand why would someone use that...

For the rest:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +         */
> +        if (!cpu->sve_max_vq) {
> +            cpu->sve_max_vq = ARM_MAX_VQ;
> +        }
> +    } else {
> +        cpu->sve_max_vq = 0;
> +    }
>  }
>  
>  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> @@ -373,8 +421,10 @@ static void aarch64_max_initfn(Object *obj)
>  #endif
>  
>          cpu->sve_max_vq = ARM_MAX_VQ;
> -        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> -                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> +        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> +                            cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
> +        object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> +                            cpu_arm_set_sve, NULL, NULL, &error_fatal);
>      }
>  }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index edba94004e0b..f500ccb6d31b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5314,9 +5314,13 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
>  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);
> +
> +    old_len = sve_zcr_len_for_el(env, cur_el);
>  
>      /* Bits other than [3:0] are RAZ/WI.  */
>      QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 19e3120eef95..157c487a1551 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -90,7 +90,7 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
>  }
>  
>  static const char *cpu_model_advertised_features[] = {
> -    "aarch64", "pmu",
> +    "aarch64", "pmu", "sve",
>      NULL
>  };
>  
> diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> index 31b1c15bb979..509e458e9c2f 100644
> --- a/tests/arm-cpu-features.c
> +++ b/tests/arm-cpu-features.c
> @@ -158,6 +158,7 @@ static void test_query_cpu_model_expansion(const void *data)
>  
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          assert_has_feature(qts, "max", "aarch64");
> +        assert_has_feature(qts, "max", "sve");
>          assert_has_feature(qts, "cortex-a57", "pmu");
>          assert_has_feature(qts, "cortex-a57", "aarch64");
>  
>
Andrew Jones June 21, 2019, 5:11 p.m. UTC | #2
On Fri, Jun 21, 2019 at 06:55:02PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Drew,
> 
> 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.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu.c         | 10 +++++-
> >  target/arm/cpu64.c       | 72 ++++++++++++++++++++++++++++++++++------
> >  target/arm/helper.c      |  8 +++--
> >  target/arm/monitor.c     |  2 +-
> >  tests/arm-cpu-features.c |  1 +
> >  5 files changed, 78 insertions(+), 15 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 858f668d226e..f08e178fc84b 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -198,7 +198,7 @@ static void arm_cpu_reset(CPUState *s)
> >          env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
> >          env->cp15.cptr_el[3] |= CPTR_EZ;
> >          /* with maximum vector length */
> > -        env->vfp.zcr_el[1] = cpu->sve_max_vq - 1;
> > +        env->vfp.zcr_el[1] = cpu->sve_max_vq ? cpu->sve_max_vq - 1 : 0;
> >          env->vfp.zcr_el[2] = env->vfp.zcr_el[1];
> >          env->vfp.zcr_el[3] = env->vfp.zcr_el[1];
> >          /*
> > @@ -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;
> > +    }
> > +
> >      if (arm_feature(env, ARM_FEATURE_M) && !cpu->has_dsp) {
> >          uint32_t u;
> >  
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 946994838d8a..02ada65f240c 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -257,27 +257,75 @@ static void aarch64_a72_initfn(Object *obj)
> >      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
> >  }
> >  
> > -static void cpu_max_get_sve_vq(Object *obj, Visitor *v, const char *name,
> > -                               void *opaque, Error **errp)
> > +static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> > +                                   void *opaque, Error **errp)
> >  {
> >      ARMCPU *cpu = ARM_CPU(obj);
> >      visit_type_uint32(v, name, &cpu->sve_max_vq, errp);
> >  }
> >  
> > -static void cpu_max_set_sve_vq(Object *obj, Visitor *v, const char *name,
> > -                               void *opaque, Error **errp)
> > +static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
> > +                                   void *opaque, Error **errp)
> >  {
> >      ARMCPU *cpu = ARM_CPU(obj);
> >      Error *err = NULL;
> > +    uint32_t value;
> >  
> > -    visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
> > +    visit_type_uint32(v, name, &value, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> >  
> > -    if (!err && (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ)) {
> > -        error_setg(&err, "unsupported SVE vector length");
> > -        error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
> > +    if (!cpu->sve_max_vq) {
> > +        error_setg(errp, "cannot set sve-max-vq");
> > +        error_append_hint(errp, "SVE has been disabled with sve=off\n");
> > +        return;
> > +    }
> > +
> > +    cpu->sve_max_vq = value;
> > +
> > +    if (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ) {
> > +        error_setg(errp, "unsupported SVE vector length");
> > +        error_append_hint(errp, "Valid sve-max-vq in range [1-%d]\n",
> >                            ARM_MAX_VQ);
> >      }
> > -    error_propagate(errp, err);
> > +}
> > +
> > +static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
> > +                            void *opaque, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    bool value = !!cpu->sve_max_vq;
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> > +                            void *opaque, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    Error *err = NULL;
> > +    bool value;
> > +
> > +    visit_type_bool(v, name, &value, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +
> > +    if (value) {
> > +        /*
> > +         * We handle the -cpu <cpu>,sve=off,sve=on case by reinitializing,
> > +         * but otherwise we don't do anything as an sve=on could come after
> > +         * a sve-max-vq setting.
> 
> I don't understand why would someone use that...

Command line generators can append something like 'feat1=off,feat2=off,...'
to the cpu feature property list by default, and then, based on user input,
rather than parsing and modifying the default command line they may just
append the new value, like 'feat1=off,feat2=off,...,feat1=on' to change it.

> 
> For the rest:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!
drew

> 
> > +         */
> > +        if (!cpu->sve_max_vq) {
> > +            cpu->sve_max_vq = ARM_MAX_VQ;
> > +        }
> > +    } else {
> > +        cpu->sve_max_vq = 0;
> > +    }
> >  }
> >  
> >  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> > @@ -373,8 +421,10 @@ static void aarch64_max_initfn(Object *obj)
> >  #endif
> >  
> >          cpu->sve_max_vq = ARM_MAX_VQ;
> > -        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> > -                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> > +        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> > +                            cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
> > +        object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> > +                            cpu_arm_set_sve, NULL, NULL, &error_fatal);
> >      }
> >  }
> >  
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index edba94004e0b..f500ccb6d31b 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -5314,9 +5314,13 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> >  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);
> > +
> > +    old_len = sve_zcr_len_for_el(env, cur_el);
> >  
> >      /* Bits other than [3:0] are RAZ/WI.  */
> >      QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index 19e3120eef95..157c487a1551 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -90,7 +90,7 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> >  }
> >  
> >  static const char *cpu_model_advertised_features[] = {
> > -    "aarch64", "pmu",
> > +    "aarch64", "pmu", "sve",
> >      NULL
> >  };
> >  
> > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > index 31b1c15bb979..509e458e9c2f 100644
> > --- a/tests/arm-cpu-features.c
> > +++ b/tests/arm-cpu-features.c
> > @@ -158,6 +158,7 @@ static void test_query_cpu_model_expansion(const void *data)
> >  
> >      if (g_str_equal(qtest_get_arch(), "aarch64")) {
> >          assert_has_feature(qts, "max", "aarch64");
> > +        assert_has_feature(qts, "max", "sve");
> >          assert_has_feature(qts, "cortex-a57", "pmu");
> >          assert_has_feature(qts, "cortex-a57", "aarch64");
> >  
> > 
>
Eric Auger June 26, 2019, 10 a.m. UTC | #3
Hi Drew,

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.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.c         | 10 +++++-
>  target/arm/cpu64.c       | 72 ++++++++++++++++++++++++++++++++++------
>  target/arm/helper.c      |  8 +++--
>  target/arm/monitor.c     |  2 +-
>  tests/arm-cpu-features.c |  1 +
>  5 files changed, 78 insertions(+), 15 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 858f668d226e..f08e178fc84b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -198,7 +198,7 @@ static void arm_cpu_reset(CPUState *s)
>          env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
>          env->cp15.cptr_el[3] |= CPTR_EZ;
>          /* with maximum vector length */
> -        env->vfp.zcr_el[1] = cpu->sve_max_vq - 1;
> +        env->vfp.zcr_el[1] = cpu->sve_max_vq ? cpu->sve_max_vq - 1 : 0;
>          env->vfp.zcr_el[2] = env->vfp.zcr_el[1];
>          env->vfp.zcr_el[3] = env->vfp.zcr_el[1];
>          /*
> @@ -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;
> +    }
> +
>      if (arm_feature(env, ARM_FEATURE_M) && !cpu->has_dsp) {
>          uint32_t u;
>  
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 946994838d8a..02ada65f240c 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -257,27 +257,75 @@ static void aarch64_a72_initfn(Object *obj)
>      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
>  }
>  
> -static void cpu_max_get_sve_vq(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> +static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      visit_type_uint32(v, name, &cpu->sve_max_vq, errp);
>  }
>  
> -static void cpu_max_set_sve_vq(Object *obj, Visitor *v, const char *name,
> -                               void *opaque, Error **errp)
> +static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      Error *err = NULL;
> +    uint32_t value;
>  
> -    visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
> +    visit_type_uint32(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>  
> -    if (!err && (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ)) {
> -        error_setg(&err, "unsupported SVE vector length");
> -        error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
> +    if (!cpu->sve_max_vq) {
> +        error_setg(errp, "cannot set sve-max-vq");
> +        error_append_hint(errp, "SVE has been disabled with sve=off\n");
> +        return;
> +    }
> +
> +    cpu->sve_max_vq = value;
> +
> +    if (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ) {
> +        error_setg(errp, "unsupported SVE vector length");
> +        error_append_hint(errp, "Valid sve-max-vq in range [1-%d]\n",
>                            ARM_MAX_VQ);
>      }
> -    error_propagate(errp, err);
> +}
> +
> +static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    bool value = !!cpu->sve_max_vq;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    Error *err = NULL;
> +    bool value;
> +
> +    visit_type_bool(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    if (value) {
> +        /*
> +         * We handle the -cpu <cpu>,sve=off,sve=on case by reinitializing,
> +         * but otherwise we don't do anything as an sve=on could come after
> +         * a sve-max-vq setting.
> +         */
> +        if (!cpu->sve_max_vq) {
> +            cpu->sve_max_vq = ARM_MAX_VQ;> +        }
> +    } else {
> +        cpu->sve_max_vq = 0;
> +    }
>  }
>  
>  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> @@ -373,8 +421,10 @@ static void aarch64_max_initfn(Object *obj)
>  #endif
>  
>          cpu->sve_max_vq = ARM_MAX_VQ;
> -        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> -                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> +        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> +                            cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
> +        object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> +                            cpu_arm_set_sve, NULL, NULL, &error_fatal);
Wouldn't it be possible to allow 0 as a valid value for sve-max-vq and
interpret this as sve=off?
>      }
>  }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index edba94004e0b..f500ccb6d31b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5314,9 +5314,13 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
>  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);
> +
> +    old_len = sve_zcr_len_for_el(env, cur_el);
The comment
    /*
     * Because we arrived here, we know both FP and SVE are enabled;
     * otherwise we would have trapped access to the ZCR_ELn register.
     */
gives the impression we are sure SVE is enabled. So is it mandated to
test sve_max_vq? Otherwise adapt the comment?

Thanks

Eric
>  
>      /* Bits other than [3:0] are RAZ/WI.  */
>      QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 19e3120eef95..157c487a1551 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -90,7 +90,7 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
>  }
>  
>  static const char *cpu_model_advertised_features[] = {
> -    "aarch64", "pmu",
> +    "aarch64", "pmu", "sve",
>      NULL
>  };
>  
> diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> index 31b1c15bb979..509e458e9c2f 100644
> --- a/tests/arm-cpu-features.c
> +++ b/tests/arm-cpu-features.c
> @@ -158,6 +158,7 @@ static void test_query_cpu_model_expansion(const void *data)
>  
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          assert_has_feature(qts, "max", "aarch64");
> +        assert_has_feature(qts, "max", "sve");
>          assert_has_feature(qts, "cortex-a57", "pmu");
>          assert_has_feature(qts, "cortex-a57", "aarch64");
>  
>
Richard Henderson June 26, 2019, 10:20 a.m. UTC | #4
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.


> @@ -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?


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


r~
Andrew Jones June 26, 2019, 1:38 p.m. UTC | #5
On Wed, Jun 26, 2019 at 12:00:54PM +0200, Auger Eric wrote:
> Hi Drew,
> 
> 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.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu.c         | 10 +++++-
> >  target/arm/cpu64.c       | 72 ++++++++++++++++++++++++++++++++++------
> >  target/arm/helper.c      |  8 +++--
> >  target/arm/monitor.c     |  2 +-
> >  tests/arm-cpu-features.c |  1 +
> >  5 files changed, 78 insertions(+), 15 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 858f668d226e..f08e178fc84b 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -198,7 +198,7 @@ static void arm_cpu_reset(CPUState *s)
> >          env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
> >          env->cp15.cptr_el[3] |= CPTR_EZ;
> >          /* with maximum vector length */
> > -        env->vfp.zcr_el[1] = cpu->sve_max_vq - 1;
> > +        env->vfp.zcr_el[1] = cpu->sve_max_vq ? cpu->sve_max_vq - 1 : 0;
> >          env->vfp.zcr_el[2] = env->vfp.zcr_el[1];
> >          env->vfp.zcr_el[3] = env->vfp.zcr_el[1];
> >          /*
> > @@ -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;
> > +    }
> > +
> >      if (arm_feature(env, ARM_FEATURE_M) && !cpu->has_dsp) {
> >          uint32_t u;
> >  
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 946994838d8a..02ada65f240c 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -257,27 +257,75 @@ static void aarch64_a72_initfn(Object *obj)
> >      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
> >  }
> >  
> > -static void cpu_max_get_sve_vq(Object *obj, Visitor *v, const char *name,
> > -                               void *opaque, Error **errp)
> > +static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
> > +                                   void *opaque, Error **errp)
> >  {
> >      ARMCPU *cpu = ARM_CPU(obj);
> >      visit_type_uint32(v, name, &cpu->sve_max_vq, errp);
> >  }
> >  
> > -static void cpu_max_set_sve_vq(Object *obj, Visitor *v, const char *name,
> > -                               void *opaque, Error **errp)
> > +static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
> > +                                   void *opaque, Error **errp)
> >  {
> >      ARMCPU *cpu = ARM_CPU(obj);
> >      Error *err = NULL;
> > +    uint32_t value;
> >  
> > -    visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
> > +    visit_type_uint32(v, name, &value, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> >  
> > -    if (!err && (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ)) {
> > -        error_setg(&err, "unsupported SVE vector length");
> > -        error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
> > +    if (!cpu->sve_max_vq) {
> > +        error_setg(errp, "cannot set sve-max-vq");
> > +        error_append_hint(errp, "SVE has been disabled with sve=off\n");
> > +        return;
> > +    }
> > +
> > +    cpu->sve_max_vq = value;
> > +
> > +    if (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ) {
> > +        error_setg(errp, "unsupported SVE vector length");
> > +        error_append_hint(errp, "Valid sve-max-vq in range [1-%d]\n",
> >                            ARM_MAX_VQ);
> >      }
> > -    error_propagate(errp, err);
> > +}
> > +
> > +static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
> > +                            void *opaque, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    bool value = !!cpu->sve_max_vq;
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> > +                            void *opaque, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    Error *err = NULL;
> > +    bool value;
> > +
> > +    visit_type_bool(v, name, &value, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +
> > +    if (value) {
> > +        /*
> > +         * We handle the -cpu <cpu>,sve=off,sve=on case by reinitializing,
> > +         * but otherwise we don't do anything as an sve=on could come after
> > +         * a sve-max-vq setting.
> > +         */
> > +        if (!cpu->sve_max_vq) {
> > +            cpu->sve_max_vq = ARM_MAX_VQ;> +        }
> > +    } else {
> > +        cpu->sve_max_vq = 0;
> > +    }
> >  }
> >  
> >  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> > @@ -373,8 +421,10 @@ static void aarch64_max_initfn(Object *obj)
> >  #endif
> >  
> >          cpu->sve_max_vq = ARM_MAX_VQ;
> > -        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> > -                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> > +        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
> > +                            cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
> > +        object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> > +                            cpu_arm_set_sve, NULL, NULL, &error_fatal);
> Wouldn't it be possible to allow 0 as a valid value for sve-max-vq and
> interpret this as sve=off?

No, because it wouldn't be a very nice interface considering all other cpu
features are enabled/disabled with booleans, all the sve<vl-bits>
properties are in vl-bits rather than vq's - so it'd be nicer to just
forget about sve-max-vq altogether, and indeed it wouldn't work for cpu
'host', because that cpu type doesn't have the sve-max-vq property at all.

> >      }
> >  }
> >  
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index edba94004e0b..f500ccb6d31b 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -5314,9 +5314,13 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> >  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);
> > +
> > +    old_len = sve_zcr_len_for_el(env, cur_el);
> The comment
>     /*
>      * Because we arrived here, we know both FP and SVE are enabled;
>      * otherwise we would have trapped access to the ZCR_ELn register.
>      */
> gives the impression we are sure SVE is enabled. So is it mandated to
> test sve_max_vq? Otherwise adapt the comment?

Nope, it's not mandatory, that was just some extra paranoia I can remove
now.

Thanks,
drew

> 
> Thanks
> 
> Eric
> >  
> >      /* Bits other than [3:0] are RAZ/WI.  */
> >      QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index 19e3120eef95..157c487a1551 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -90,7 +90,7 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> >  }
> >  
> >  static const char *cpu_model_advertised_features[] = {
> > -    "aarch64", "pmu",
> > +    "aarch64", "pmu", "sve",
> >      NULL
> >  };
> >  
> > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > index 31b1c15bb979..509e458e9c2f 100644
> > --- a/tests/arm-cpu-features.c
> > +++ b/tests/arm-cpu-features.c
> > @@ -158,6 +158,7 @@ static void test_query_cpu_model_expansion(const void *data)
> >  
> >      if (g_str_equal(qtest_get_arch(), "aarch64")) {
> >          assert_has_feature(qts, "max", "aarch64");
> > +        assert_has_feature(qts, "max", "sve");
> >          assert_has_feature(qts, "cortex-a57", "pmu");
> >          assert_has_feature(qts, "cortex-a57", "aarch64");
> >  
> > 
>
Andrew Jones June 26, 2019, 1:52 p.m. UTC | #6
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
Andrew Jones July 17, 2019, 3:43 p.m. UTC | #7
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
diff mbox series

Patch

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 858f668d226e..f08e178fc84b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -198,7 +198,7 @@  static void arm_cpu_reset(CPUState *s)
         env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
         env->cp15.cptr_el[3] |= CPTR_EZ;
         /* with maximum vector length */
-        env->vfp.zcr_el[1] = cpu->sve_max_vq - 1;
+        env->vfp.zcr_el[1] = cpu->sve_max_vq ? cpu->sve_max_vq - 1 : 0;
         env->vfp.zcr_el[2] = env->vfp.zcr_el[1];
         env->vfp.zcr_el[3] = env->vfp.zcr_el[1];
         /*
@@ -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;
+    }
+
     if (arm_feature(env, ARM_FEATURE_M) && !cpu->has_dsp) {
         uint32_t u;
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 946994838d8a..02ada65f240c 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -257,27 +257,75 @@  static void aarch64_a72_initfn(Object *obj)
     define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
 }
 
-static void cpu_max_get_sve_vq(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
+static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
 {
     ARMCPU *cpu = ARM_CPU(obj);
     visit_type_uint32(v, name, &cpu->sve_max_vq, errp);
 }
 
-static void cpu_max_set_sve_vq(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
+static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
 {
     ARMCPU *cpu = ARM_CPU(obj);
     Error *err = NULL;
+    uint32_t value;
 
-    visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
+    visit_type_uint32(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 
-    if (!err && (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ)) {
-        error_setg(&err, "unsupported SVE vector length");
-        error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
+    if (!cpu->sve_max_vq) {
+        error_setg(errp, "cannot set sve-max-vq");
+        error_append_hint(errp, "SVE has been disabled with sve=off\n");
+        return;
+    }
+
+    cpu->sve_max_vq = value;
+
+    if (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ) {
+        error_setg(errp, "unsupported SVE vector length");
+        error_append_hint(errp, "Valid sve-max-vq in range [1-%d]\n",
                           ARM_MAX_VQ);
     }
-    error_propagate(errp, err);
+}
+
+static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    bool value = !!cpu->sve_max_vq;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    Error *err = NULL;
+    bool value;
+
+    visit_type_bool(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    if (value) {
+        /*
+         * We handle the -cpu <cpu>,sve=off,sve=on case by reinitializing,
+         * but otherwise we don't do anything as an sve=on could come after
+         * a sve-max-vq setting.
+         */
+        if (!cpu->sve_max_vq) {
+            cpu->sve_max_vq = ARM_MAX_VQ;
+        }
+    } else {
+        cpu->sve_max_vq = 0;
+    }
 }
 
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
@@ -373,8 +421,10 @@  static void aarch64_max_initfn(Object *obj)
 #endif
 
         cpu->sve_max_vq = ARM_MAX_VQ;
-        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
-                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
+        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq,
+                            cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal);
+        object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
+                            cpu_arm_set_sve, NULL, NULL, &error_fatal);
     }
 }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index edba94004e0b..f500ccb6d31b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5314,9 +5314,13 @@  uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
 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);
+
+    old_len = sve_zcr_len_for_el(env, cur_el);
 
     /* Bits other than [3:0] are RAZ/WI.  */
     QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 19e3120eef95..157c487a1551 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -90,7 +90,7 @@  GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
 }
 
 static const char *cpu_model_advertised_features[] = {
-    "aarch64", "pmu",
+    "aarch64", "pmu", "sve",
     NULL
 };
 
diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 31b1c15bb979..509e458e9c2f 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -158,6 +158,7 @@  static void test_query_cpu_model_expansion(const void *data)
 
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         assert_has_feature(qts, "max", "aarch64");
+        assert_has_feature(qts, "max", "sve");
         assert_has_feature(qts, "cortex-a57", "pmu");
         assert_has_feature(qts, "cortex-a57", "aarch64");