Re: [Qemu-devel] [PATCH] HACK: Centralize sve property checks

[Andrew Jones]

On Fri, Aug 09, 2019 at 06:31:12PM -0700, Richard Henderson wrote:
> As promised, an experiment in unifying the checks.
> 
> I believe that I've tested for all of the same conditions, modulo the
> timing at which they are emitted.  As before, only one error is reported,
> so if multiple errors exist you can't rely on which error is seen.
> 
> I think there's value in splitting the sve_map_vq bitmap such that the
> initialized state can be easily manipulated with bitmap functions.
> 
> The diffstat is promising.  Lemme know what you think.

Hi Richard,

Thanks for this!

I'm back after a long context switch away from this work. There is indeed
a nice number of lines reduced. I applied this (with a couple minor fixups
for kvm code) and lightly tested it (primarily with tcg, other than the
sve-not-supported-with-this-kvm path). It works for me. I wouldn't say
this approach is less complex, but with the LOC reduction, then I'm happy
to get started on integrating it into the v4 series. How would you suggest
I apply credit for it? I can give you authorship for any patches that
mainly contain your code. Or I can keep authorship and add mentions of
your contributions in the commit messages. Just let me know your
preference.

I also have a couple comments below on the patch.

> 
> 
> r~
> 
> ---
>  target/arm/cpu.h     |  16 +-
>  target/arm/cpu.c     |  12 +-
>  target/arm/cpu64.c   | 502 ++++++++++++++-----------------------------
>  target/arm/monitor.c |   7 +
>  4 files changed, 190 insertions(+), 347 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 7993085bea..924476480b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -184,11 +184,11 @@ typedef struct {
>  
>  #ifdef TARGET_AARCH64
>  # define ARM_MAX_VQ    16
> -void arm_cpu_sve_finalize(ARMCPU *cpu);
> +void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
>  uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq);
>  #else
>  # define ARM_MAX_VQ    1
> -static inline void arm_cpu_sve_finalize(ARMCPU *cpu) { }
> +static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
>  static inline uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
>  { return 0; }
>  #endif
> @@ -924,12 +924,14 @@ struct ARMCPU {
>      /*
>       * In sve_vq_map each set bit is a supported vector length of
>       * (bit-number + 1) * 16 bytes, i.e. each bit number + 1 is the vector
> -     * length in quadwords. We need a map size twice the maximum
> -     * quadword length though because we use two bits for each vector
> -     * length in order to track four states: uninitialized, uninitialized
> -     * but supported by KVM, off, and on.
> +     * length in quadwords.
> +     *
> +     * While processing properties during initialization, corresponding
> +     * sve_vq_init bits are set for bits in sve_vq_map that have been
> +     * set by properties.
>       */
> -    DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ * 2);
> +    DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ);
> +    DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ);
>  };
>  
>  void arm_cpu_post_init(Object *obj);
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 604f5c3651..a113c30def 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1255,8 +1255,12 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>  
> -    if (cpu_isar_feature(aa64_sve, cpu)) {
> -        arm_cpu_sve_finalize(cpu);
> +    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> +        arm_cpu_sve_finalize(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
>  
>      if (arm_feature(env, ARM_FEATURE_AARCH64) &&
> @@ -2656,7 +2660,9 @@ static void arm_host_initfn(Object *obj)
>      ARMCPU *cpu = ARM_CPU(obj);
>  
>      kvm_arm_set_cpu_features_from_host(cpu);
> -    aarch64_add_sve_properties(obj);
> +    if (arm_feature(env, ARM_FEATURE_AARCH64)) {

&cpu->env

> +        aarch64_add_sve_properties(obj);
> +    }
>      arm_cpu_post_init(obj);
>  }
>  
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 0313eec88a..7c7e54ad78 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -257,165 +257,168 @@ static void aarch64_a72_initfn(Object *obj)
>      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
>  }
>  
> -/*
> - * While we eventually use cpu->sve_vq_map as a typical bitmap, where each vq
> - * has only two states (off/on), until we've finalized the map at realize 
> time
> - * we use an extra bit, at the vq - 1 + ARM_MAX_VQ bit number, to also allow
> - * tracking of the uninitialized state and the uninitialized but supported by
> - * KVM state. The arm_vq_state typedef and following functions allow us to 
> more
> - * easily work with the bitmap.
> - */
> -typedef enum arm_vq_state {
> -    ARM_VQ_OFF,
> -    ARM_VQ_ON,
> -    ARM_VQ_UNINITIALIZED,
> -    ARM_VQ_UNINITIALIZED_KVM_SUPPORTED
> -    /*
> -     * More states cannot be added without adding bits to sve_vq_map
> -     * and modifying its supporting functions.
> -     */
> -} arm_vq_state;
> -
> -static arm_vq_state arm_cpu_vq_map_get(ARMCPU *cpu, uint32_t vq)
> +void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>  {
> -    assert(vq <= ARM_MAX_VQ);
> -
> -    return test_bit(vq - 1, cpu->sve_vq_map) |
> -           test_bit(vq - 1 + ARM_MAX_VQ, cpu->sve_vq_map) << 1;
> -}
> -
> -static void arm_cpu_vq_map_set(ARMCPU *cpu, uint32_t vq, arm_vq_state state)
> -{
> -    assert(vq <= ARM_MAX_VQ);
> -    assert(state == ARM_VQ_OFF || state == ARM_VQ_ON);
> -
> -    clear_bit(vq - 1 + ARM_MAX_VQ, cpu->sve_vq_map);
> -
> -    if (state == ARM_VQ_ON) {
> -        set_bit(vq - 1, cpu->sve_vq_map);
> -    } else {
> -        clear_bit(vq - 1, cpu->sve_vq_map);
> -    }
> -}
> -
> -static bool arm_cpu_vq_kvm_supported(ARMCPU *cpu, uint32_t vq)
> -{
> -    return arm_cpu_vq_map_get(cpu, vq) != ARM_VQ_UNINITIALIZED;
> -}
> -
> -static bool arm_cpu_vq_uninitialized(ARMCPU *cpu, uint32_t vq)
> -{
> -    arm_vq_state vq_state = arm_cpu_vq_map_get(cpu, vq);
> -
> -    if (kvm_enabled()) {
> -        return vq_state == ARM_VQ_UNINITIALIZED_KVM_SUPPORTED;
> -    }
> -
> -    return vq_state == ARM_VQ_UNINITIALIZED;
> -}
> -
> -/*
> - * Uninitialized vector lengths need a default value to use in case we need
> - * to query their value prior to map finalization. Additionally map 
> finalizing
> - * itself needs to determine what value to assign uninitialized vector 
> lengths.
> - * The default is determined as follows:
> - *
> - *  * When no vector lengths have been explicitly enabled, i.e. either no
> - *    input has been provided by the user at all, or vector lengths have
> - *    only been disabled, then all uninitialized vector lengths default 'ON'.
> - *
> - *  * When one or more vector lengths have been enabled, then all 
> uninitialized
> - *    vector lengths default 'OFF'. Note, when checking for enabled vector
> - *    lengths we do not discriminate between user-enabled vector lengths and
> - *    auto-enabled vector lengths (which were auto-enabled in order to 
> satisfy
> - *    the user-enabled vector lengths). This implies the default can never
> - *    transition back to 'ON', even if the user-enabled and auto-enabled 
> vector
> - *    lengths that initially transitioned it to 'OFF' are later disabled, as 
> at
> - *    least one vector length must remain enabled unless the SVE feature is
> - *    completely disabled. If SVE is completely disabled then all vector
> - *    lengths are effectively 'OFF'.
> - */
> -static bool arm_cpu_vq_map_get_default(ARMCPU *cpu)
> -{
> -    uint32_t vq;
> -
> -    for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> -        if (arm_cpu_vq_map_get(cpu, vq) == ARM_VQ_ON) {
> -            return false;
> -        }
> -    }
> -
> -    return true;
> -}
> -
> -/*
> - * We need to be able to track the number of enabled (or will-be enabled)
> - * vector lengths in order to ensure we never drop to zero. If the default
> - * is 'ON', then we count enabled and uninitialized vector lengths. 
> Otherwise,
> - * if the default is 'OFF', then we only count enabled vector lengths.
> - */
> -static int arm_cpu_num_vqs_available(ARMCPU *cpu)
> -{
> -    uint32_t vq;
> -    bool defval;
> -    int num = 0;
> -
> -    defval = arm_cpu_vq_map_get_default(cpu);
> -
> -    for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> -        if (arm_cpu_vq_map_get(cpu, vq) == ARM_VQ_ON) {
> -            ++num;
> -        } else if (defval && arm_cpu_vq_uninitialized(cpu, vq)) {
> -            ++num;
> -        }
> -    }
> -
> -    return num;
> -}
> -
> -static void arm_cpu_vq_map_init(ARMCPU *cpu)
> -{
> -    /* Set all vq's to 0b10 (ARM_VQ_UNINITIALIZED) */
> -    bitmap_clear(cpu->sve_vq_map, 0, ARM_MAX_VQ);
> -    bitmap_set(cpu->sve_vq_map, ARM_MAX_VQ, ARM_MAX_VQ);
> -
> -    if (kvm_enabled()) {
> -        /*
> -         * As the upper bits of the ARM_VQ_UNINITIALIZED_KVM_SUPPORTED (0b11)
> -         * states have already been set with the bitmap_set() above, we only
> -         * need to OR in the lower bits.
> -         */
> -        DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
> +    DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
> +    DECLARE_BITMAP(test, ARM_MAX_VQ);
> +    uint32_t vq, max_vq = 0;
>  
> +    /* Collect the set of VQ supported by KVM. */
> +    if (kvm_enabled() && kvm_arm_sve_supported(CPU(cpu))) {
>          kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
> -        bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, kvm_supported, 
> ARM_MAX_VQ);
> +    } else {
> +        bitmap_zero(kvm_supported, ARM_MAX_VQ);
>      }
> -}
>  
> -void arm_cpu_sve_finalize(ARMCPU *cpu)
> -{
> -    bool defval = arm_cpu_vq_map_get_default(cpu);
> -    uint32_t vq, max_vq;
> +    /*
> +     * Process explicit sve<N> properties.
> +     * From the properties, sve_vq_map<N> implies sve_vq_init<N>.
> +     * Check first for any sve<N> enabled.
> +     */
> +    if (!bitmap_empty(cpu->sve_vq_map, ARM_MAX_VQ)) {
> +        max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
>  
> -    for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> -        if (kvm_enabled() && !arm_cpu_vq_kvm_supported(cpu, vq)) {
> -            arm_cpu_vq_map_set(cpu, vq, ARM_VQ_OFF);
> -        } else if (arm_cpu_vq_uninitialized(cpu, vq)) {
> -            if (defval) {
> -                arm_cpu_vq_map_set(cpu, vq, ARM_VQ_ON);
> -            } else {
> -                arm_cpu_vq_map_set(cpu, vq, ARM_VQ_OFF);
> +        if (cpu->sve_max_vq && max_vq > cpu->sve_max_vq) {
> +            error_setg(errp, "cannot enable sve%d", max_vq * 128);
> +            error_append_hint(errp, "sve%d is larger than the maximum vector 
> "
> +                              "length, sve-max-vq=%d (%d bits)\n",
> +                              max_vq * 128, cpu->sve_max_vq,
> +                              cpu->sve_max_vq * 128);
> +            return;
> +        }
> +
> +        /* Propagate enabled bits down through required powers-of-two. */
> +        for (vq = pow2floor(max_vq); vq >= 1; vq >>= 1) {
> +            if (!test_bit(vq - 1, cpu->sve_vq_init)) {
> +                set_bit(vq - 1, cpu->sve_vq_map);
> +                set_bit(vq - 1, cpu->sve_vq_init);
>              }
>          }

While the architecture requires this, so all hardware should also
require it / depend on it, I would only do this propagation in the
non-KVM path. When KVM is enabled I prefer giving KVM full authority
over what vector lengths may be enabled. The reasoning is for the
unlikely case of KVM possibly trying to workaround some hardware
errata involving power-of-2 vector lengths.

> +
> +        /*
> +         * For KVM we have to automatically enable all supported unitialized
> +         * lengths, even when the smaller lengths are not all powers-of-two.
> +         */
> +        if (kvm_enabled()) {
> +            if (!kvm_arm_sve_supported(CPU(cpu))) {
> +                error_setg(errp, "cannot enable sve%d", max_vq);

max_vq * 128

> +                error_append_hint(errp, "SVE not supported by "
> +                                  "KVM on this host\n");
> +                return;
> +            }
> +            bitmap_andnot(test, kvm_supported, cpu->sve_vq_init, max_vq);
> +            bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, test, max_vq);
> +            bitmap_or(cpu->sve_vq_init, cpu->sve_vq_init, test, max_vq);
> +        }
> +    } else if (cpu->sve_max_vq == 0) {
> +        /*
> +         * No explicit bits enabled, and no implicit bits from sve-max-vq.
> +         */
> +        if (!cpu_isar_feature(aa64_sve, cpu)) {
> +            /* SVE is disabled and so are all vector lengths.  Good. */
> +            return;
> +        }
> +
> +        /* Disabling a power-of-two disables all larger lengths. */
> +        if (test_bit(0, cpu->sve_vq_init)) {
> +            error_setg(errp, "cannot disable sve128");
> +            error_append_hint(errp, "Disabling sve128 results in all vector "
> +                              "lengths being disabled.\n");
> +            error_append_hint(errp, "With SVE enabled, at least one vector "
> +                                  "length must be enabled.\n");
> +
> +            return;
> +        }
> +        for (vq = 2; vq <= ARM_MAX_VQ; vq <<= 1) {
> +            if (test_bit(vq - 1, cpu->sve_vq_init)) {
> +                break;
> +            }
> +        }
> +        max_vq = (vq <= ARM_MAX_VQ ? vq - 1 : ARM_MAX_VQ);
> +
> +        /* Enable all supported bits other than those explicit disabled. */
> +        if (kvm_enabled()) {
> +            bitmap_andnot(cpu->sve_vq_map, kvm_supported,
> +                          cpu->sve_vq_init, max_vq);
> +        } else {
> +            bitmap_complement(cpu->sve_vq_map, cpu->sve_vq_init, max_vq);
> +        }
> +        bitmap_set(cpu->sve_vq_init, 0, max_vq);
> +        max_vq = find_last_bit(cpu->sve_vq_map, max_vq) + 1;
>      }
>  
> -    max_vq = arm_cpu_vq_map_next_smaller(cpu, ARM_MAX_VQ + 1);
> +    /*
> +     * Process the sve-max-vq property.
> +     * Note that we know from the above that no bit above
> +     * sve-max-vq is currently set.
> +     */
> +    if (cpu->sve_max_vq != 0) {
> +        max_vq = cpu->sve_max_vq;
>  
> -    if (!cpu->sve_max_vq) {
> -        cpu->sve_max_vq = max_vq;
> +        if (!test_bit(max_vq - 1, cpu->sve_vq_map) &&
> +            test_bit(max_vq - 1, cpu->sve_vq_init)) {
> +            error_setg(errp, "cannot disable sve%d", max_vq * 128);
> +            error_append_hint(errp, "The maximum vector length must be "
> +                              "enabled, sve-max-vq=%d (%d bits)\n",
> +                              max_vq, max_vq * 128);
> +            return;
> +        }
> +
> +        /* Set all bits not explicitly set within sve-max-vq.  */
> +        bitmap_complement(test, cpu->sve_vq_init, max_vq);
> +        bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, test, max_vq);
>      }
>  
> -    assert(max_vq && cpu->sve_max_vq == max_vq);
> +    /*
> +     * At this point we will have found max_vq.  All bits implied from
> +     * other properties are set or clear in cpu->sve_vq_map.
> +     * All bits implied from other sve<N> are set in cpu->sve_vq_init.
> +     * If sve_vq_map<N> & !sve_vq_init<N>, then the bit must have come
> +     * sve-max-vq.
> +     */
> +    assert(max_vq != 0);
> +
> +    /* Ensure all required powers-of-two are enabled. */
> +    for (vq = pow2floor(max_vq); vq >= 1; vq >>= 1) {
> +        if (!test_bit(vq - 1, cpu->sve_vq_map)) {
> +            error_setg(errp, "cannot disable sve%d", vq * 128);
> +            error_append_hint(errp, "sve%d is required as it "
> +                              "is a power-of-2 length smaller than "
> +                              "the maximum, sve%d\n",
> +                              vq * 128, max_vq * 128);
> +            return;
> +        }
> +    }
> +
> +    /* Ensure the set of lengths matches what KVM supports. */
> +    if (kvm_enabled()) {
> +        bitmap_xor(test, cpu->sve_vq_map, kvm_supported, max_vq);
> +        if (!bitmap_empty(test, max_vq)) {
> +            vq = find_last_bit(test, max_vq);
> +            if (!test_bit(vq - 1, cpu->sve_vq_map)) {
> +                error_setg(errp, "cannot disable sve%d", vq * 128);
> +                error_append_hint(errp, "The KVM host requires all "
> +                                  "supported vector lengths smaller "
> +                                  "than %d bits to also be enabled.\n",
> +                                  max_vq * 128);
> +            } else if (test_bit(vq - 1, cpu->sve_vq_init)) {
> +                error_setg(errp, "cannot enable sve%d", vq * 128);
> +                error_append_hint(errp, "This KVM host does not support "
> +                                  "the vector length %d-bits.\n", vq * 128);
> +            } else {
> +                error_setg(errp, "cannot set sve-max-vq=%d", 
> cpu->sve_max_vq);
> +                error_append_hint(errp, "This KVM host does not support "
> +                                  "the vector length %d-bits.\n", vq * 128);
> +                error_append_hint(errp, "It may not be possible to use "
> +                                  "sve-max-vq with this KVM host. Try "
> +                                  "using only sve<vl-bits> properties.\n");
> +            }
> +            return;
> +        }
> +    }
> +
> +    /* From now on sve_max_vq is the actual maximum supported length. */
> +    cpu->sve_max_vq = max_vq;
>  }
>  
>  uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq)
> @@ -427,6 +430,7 @@ uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, 
> uint32_t vq)
>       * to find the maximum vq enabled, which may be ARM_MAX_VQ, but this
>       * function always returns the next smaller than the input.
>       */
> +    assert(vq > 1);
>      assert(vq <= ARM_MAX_VQ + 1);
>  
>      bitnum = find_last_bit(cpu->sve_vq_map, vq - 1);
> @@ -437,7 +441,15 @@ 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);
> +    uint32_t value;
> +
> +    /* All vector lengths are disabled when SVE is off. */
> +    if (!cpu_isar_feature(aa64_sve, cpu)) {
> +        value = 0;
> +    } else {
> +        value = cpu->sve_max_vq;
> +    }
> +    visit_type_uint32(v, name, &value, errp);
>  }
>  
>  static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
> @@ -445,7 +457,6 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor 
> *v, const char *name,
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      Error *err = NULL;
> -    uint32_t vq;
>  
>      visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
>      if (err) {
> @@ -465,27 +476,6 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor 
> *v, const char *name,
>                            ARM_MAX_VQ);
>          return;
>      }
> -
> -    for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> -        if (vq <= cpu->sve_max_vq) {
> -            char tmp[8];
> -
> -            sprintf(tmp, "sve%d", vq * 128);
> -            object_property_set_bool(obj, true, tmp, &err);
> -            if (err) {
> -                if (kvm_enabled()) {
> -                    error_append_hint(&err, "It is not possible to use "
> -                                      "sve-max-vq with this KVM host. Try "
> -                                      "using only sve<vl-bits> "
> -                                      "properties.\n");
> -                }
> -                error_propagate(errp, err);
> -                return;
> -            }
> -        } else if (arm_cpu_vq_map_get(cpu, vq) == ARM_VQ_ON) {
> -            arm_cpu_vq_map_set(cpu, vq, ARM_VQ_OFF);
> -        }
> -    }
>  }
>  
>  static void cpu_arm_get_sve_vq(Object *obj, Visitor *v, const char *name,
> @@ -493,24 +483,14 @@ static void cpu_arm_get_sve_vq(Object *obj, Visitor *v, 
> const char *name,
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      uint32_t vq = atoi(&name[3]) / 128;
> -    arm_vq_state vq_state = arm_cpu_vq_map_get(cpu, vq);
>      bool value;
>  
> +    /* All vector lengths are disabled when SVE is off. */
>      if (!cpu_isar_feature(aa64_sve, cpu)) {
> -        /* All vector lengths are disabled when SVE is off. */
>          value = false;
> -    } else if (vq_state == ARM_VQ_ON) {
> -        value = true;
> -    } else if (vq_state == ARM_VQ_OFF) {
> -        value = false;
> -    } else if (kvm_enabled() && !arm_cpu_vq_kvm_supported(cpu, vq)) {
> -        value = false;
> -    } else if (arm_cpu_vq_map_get_default(cpu)) {
> -        value = true;
>      } else {
> -        value = false;
> +        value = test_bit(vq - 1, cpu->sve_vq_map);
>      }
> -
>      visit_type_bool(v, name, &value, errp);
>  }
>  
> @@ -519,7 +499,6 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v, 
> const char *name,
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>      uint32_t vq = atoi(&name[3]) / 128;
> -    uint32_t max_vq = 0;
>      Error *err = NULL;
>      bool value;
>  
> @@ -529,146 +508,12 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor 
> *v, const char *name,
>          return;
>      }
>  
> -    if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
> -        error_setg(errp, "cannot enable %s", name);
> -        error_append_hint(errp, "SVE not supported by KVM on this host\n");
> -        return;
> -    }
> -
> -    /*
> -     * We need to know the maximum vector length, which may just currently
> -     * be the maximum length, in order to validate the enabling/disabling
> -     * of this vector length.
> -     */
> -    if (!cpu->sve_max_vq) {
> -        for (max_vq = ARM_MAX_VQ; max_vq >= 1; --max_vq) {
> -            if (arm_cpu_vq_map_get(cpu, max_vq) == ARM_VQ_ON) {
> -                break;
> -            }
> -        }
> -    }
> -
> -    if (cpu->sve_max_vq && value && vq > cpu->sve_max_vq) {
> -        error_setg(errp, "cannot enable %s", name);
> -        error_append_hint(errp, "vq=%d (%d bits) is larger than the "
> -                          "maximum vector length, sve-max-vq=%d "
> -                          "(%d bits)\n", vq, vq * 128,
> -                          cpu->sve_max_vq, cpu->sve_max_vq * 128);
> -    } else if (cpu->sve_max_vq && !value && vq == cpu->sve_max_vq) {
> -        error_setg(errp, "cannot disable %s", name);
> -        error_append_hint(errp, "The maximum vector length must be "
> -                          "enabled, sve-max-vq=%d (%d bits)\n",
> -                          cpu->sve_max_vq, cpu->sve_max_vq * 128);
> -    } else if (!kvm_enabled() && cpu->sve_max_vq && !value &&
> -               vq < cpu->sve_max_vq && is_power_of_2(vq)) {
> -        error_setg(errp, "cannot disable %s", name);
> -        error_append_hint(errp, "vq=%d (%d bits) is required as it is a "
> -                          "power-of-2 length smaller than the maximum, "
> -                          "sve-max-vq=%d (%d bits)\n", vq, vq * 128,
> -                          cpu->sve_max_vq, cpu->sve_max_vq * 128);
> -    } else if (!kvm_enabled() && max_vq && !value && vq < max_vq &&
> -               is_power_of_2(vq)) {
> -        error_setg(errp, "cannot disable %s", name);
> -        error_append_hint(errp, "Vector length %d-bits is required as it "
> -                          "is a power-of-2 length smaller than another "
> -                          "enabled vector length. Disable all larger vector "
> -                          "lengths first.\n", vq * 128);
> -    } else if (kvm_enabled() && !value && vq < max_vq &&
> -                arm_cpu_vq_kvm_supported(cpu, vq)) {
> -        error_setg(errp, "cannot disable %s", name);
> -        error_append_hint(errp, "Vector length %d-bits is a KVM supported "
> -                          "length smaller than another enabled vector "
> -                          "length. Disable all larger vector lengths "
> -                          "first.\n", vq * 128);
> -    } else if (kvm_enabled() && value && !arm_cpu_vq_kvm_supported(cpu, vq)) 
> {
> -        error_setg(errp, "cannot enable %s", name);
> -        error_append_hint(errp, "This KVM host does not support "
> -                          "the vector length %d-bits.\n", vq * 128);
> +    if (value) {
> +        set_bit(vq - 1, cpu->sve_vq_map);
>      } else {
> -        uint32_t s;
> -
> -        if (value) {
> -            bool fail = false;
> -
> -            /*
> -             * Enabling a vector length automatically enables all
> -             * uninitialized power-of-2 lengths smaller than it, as
> -             * per the architecture.
> -             *
> -             * For KVM we have to automatically enable all supported,
> -             * uninitialized lengths smaller than this length, even
> -             * when the smaller lengths are not power-of-2's.
> -             */
> -            for (s = 1; s < vq; ++s) {
> -                if (kvm_enabled() || is_power_of_2(s)) {
> -                    if (arm_cpu_vq_uninitialized(cpu, s)) {
> -                        arm_cpu_vq_map_set(cpu, s, ARM_VQ_ON);
> -                    } else if (arm_cpu_vq_map_get(cpu, s) == ARM_VQ_OFF) {
> -                        fail = true;
> -                        break;
> -                    }
> -                }
> -            }
> -
> -            if (!kvm_enabled() && fail) {
> -                error_setg(errp, "cannot enable %s", name);
> -                error_append_hint(errp, "Vector length %d-bits is disabled "
> -                                  "and is a power-of-2 length smaller than "
> -                                  "%s. All power-of-2 vector lengths smaller 
> "
> -                                  "than the maximum length are required.\n",
> -                                  s * 128, name);
> -
> -            } else if (fail) {
> -                error_setg(errp, "cannot enable %s", name);
> -                error_append_hint(errp, "Vector length %d-bits is disabled "
> -                                  "and the KVM host requires all supported "
> -                                  "vector lengths smaller than %s to also be 
> "
> -                                  "enabled.\n", s * 128, name);
> -            } else {
> -                arm_cpu_vq_map_set(cpu, vq, ARM_VQ_ON);
> -            }
> -        } else {
> -            /*
> -             * When disabling vector lengths with KVM enabled if the vq 
> wasn't
> -             * supported then we leave it in the ARM_VQ_UNINITIALIZED state 
> in
> -             * order to keep that unsupported information. It'll be set to 
> OFF
> -             * later when we finalize the map.
> -             *
> -             * We would have errored-out already if we were attempting to
> -             * disable a power-of-2 vector length less than another enabled
> -             * vector length, but there may be uninitialized vector lengths
> -             * larger than a power-of-2 vector length that we're disabling.
> -             * We disable all of those lengths now too, as they can no longer
> -             * be enabled. Additionally, for KVM, we have to automatically
> -             * disable all supported, uninitialized lengths larger than this
> -             * length, even when this length is not a power-of-2.
> -             */
> -            if (kvm_enabled() || is_power_of_2(vq)) {
> -                for (s = vq + 1; s <= ARM_MAX_VQ; ++s) {
> -                    if (!kvm_enabled() || arm_cpu_vq_kvm_supported(cpu, vq)) 
> {
> -                        arm_cpu_vq_map_set(cpu, s, ARM_VQ_OFF);
> -                    }
> -                }
> -            }
> -
> -            if (!kvm_enabled() || arm_cpu_vq_kvm_supported(cpu, vq)) {
> -                arm_cpu_vq_map_set(cpu, vq, ARM_VQ_OFF);
> -            }
> -
> -            /*
> -             * We just disabled one or more vector lengths. We need to make
> -             * sure we didn't disable them all when SVE is enabled.
> -             */
> -            if (cpu_isar_feature(aa64_sve, cpu) &&
> -                !arm_cpu_num_vqs_available(cpu)) {
> -                error_setg(errp, "cannot disable %s", name);
> -                error_append_hint(errp, "Disabling %s results in all vector "
> -                                  "lengths being disabled.\n", name);
> -                error_append_hint(errp, "With SVE enabled, at least one 
> vector "
> -                                  "length must be enabled.\n");
> -            }
> -        }
> +        clear_bit(vq - 1, cpu->sve_vq_map);
>      }
> +    set_bit(vq - 1, cpu->sve_vq_init);
>  }
>  
>  static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
> @@ -702,32 +547,15 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, 
> const char *name,
>      t = cpu->isar.id_aa64pfr0;
>      t = FIELD_DP64(t, ID_AA64PFR0, SVE, value);
>      cpu->isar.id_aa64pfr0 = t;
> -
> -    /*
> -     * When SVE is enabled ensure that we have at least one vector
> -     * length available.
> -     */
> -    if (cpu_isar_feature(aa64_sve, cpu) && !arm_cpu_num_vqs_available(cpu)) {
> -        error_setg(errp, "cannot enable SVE");
> -        error_append_hint(errp, "All possible SVE vector lengths have "
> -                          "been disabled.\n");
> -    }
>  }
>  
>  void aarch64_add_sve_properties(Object *obj)
>  {
> -    ARMCPU *cpu = ARM_CPU(obj);
>      uint32_t vq;
>  
>      object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
>                          cpu_arm_set_sve, NULL, NULL, &error_fatal);
>  
> -    /*
> -     * sve_vq_map uses a special state while setting properties, so
> -     * we initialize it here with its init function and finalize it
> -     * in arm_cpu_realizefn().
> -     */
> -    arm_cpu_vq_map_init(cpu);
>      for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
>          char name[8];
>          sprintf(name, "sve%d", vq * 128);
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index e2d559c230..433cbe9b53 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -195,6 +195,9 @@ CpuModelExpansionInfo 
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>          if (!err) {
>              visit_check_struct(visitor, &err);
>          }
> +        if (!err) {
> +            arm_cpu_sve_finalize(ARM_CPU(obj), &err);
> +        }
>          visit_end_struct(visitor, NULL);
>          visit_free(visitor);
>          if (err) {
> @@ -202,6 +205,10 @@ CpuModelExpansionInfo 
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>              error_propagate(errp, err);
>              return NULL;
>          }
> +    } else {
> +        Error *err = NULL;
> +        arm_cpu_sve_finalize(ARM_CPU(obj), &err);
> +        assert(err == NULL);
>      }

Should we already introduce a function that will collect all
finalizers together now, rather than sprinkling around 
arm_cpu_sve_finalize() calls? Something like

void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
{
  arm_cpu_sve_finalize(cpu, errp);
}

Of course we can introduce it when/if we add other finalizers
later, but I guess the vfp-neon finalizer should be coming
soon anyway.

>  
>      expansion_info = g_new0(CpuModelExpansionInfo, 1);
> -- 
> 2.17.1
> 
> 

Thanks,
drew