Re: [PATCH 4/4] target/arm/cpu64: Validate sve vector lengths are supported

[Andrew Jones]

On Thu, Aug 19, 2021 at 09:37:58PM +0200, Andrew Jones wrote:
> Future CPU types may specify which vector lengths are supported.
> We can apply nearly the same logic to validate those lengths
> as we do for KVM's supported vector lengths. We merge the code
> where we can, but unfortunately can't completely merge it because
> KVM requires all vector lengths, power-of-two or not, smaller than
> the maximum enabled length to also be enabled. The architecture
> only requires all the power-of-two lengths, though, so TCG will
> only enforce that.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu64.c | 101 ++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 56 deletions(-)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 557fd4757740..9cb41c442600 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -329,35 +329,26 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>                      break;
>                  }
>              }
> -            max_vq = vq <= ARM_MAX_VQ ? vq - 1 : ARM_MAX_VQ;
> -            bitmap_andnot(cpu->sve_vq_map, cpu->sve_vq_supported,
> -                          cpu->sve_vq_init, max_vq);
> -            if (max_vq == 0 || bitmap_empty(cpu->sve_vq_map, max_vq)) {
> -                error_setg(errp, "cannot disable sve%d", vq * 128);
> -                error_append_hint(errp, "Disabling sve%d results in all "
> -                                  "vector lengths being disabled.\n",
> -                                  vq * 128);
> -                error_append_hint(errp, "With SVE enabled, at least one "
> -                                  "vector length must be enabled.\n");
> -                return;
> -            }
>          } else {
>              /* 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) {
> +            for (vq = 1; 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;
> -            bitmap_complement(cpu->sve_vq_map, cpu->sve_vq_init, max_vq);
> +        }
> +
> +        max_vq = vq <= ARM_MAX_VQ ? vq - 1 : ARM_MAX_VQ;
> +        bitmap_andnot(cpu->sve_vq_map, cpu->sve_vq_supported,
> +                      cpu->sve_vq_init, max_vq);
> +        if (max_vq == 0 || bitmap_empty(cpu->sve_vq_map, max_vq)) {
> +            error_setg(errp, "cannot disable sve%d", vq * 128);
> +            error_append_hint(errp, "Disabling sve%d results in all "
> +                              "vector lengths being disabled.\n",
> +                              vq * 128);
> +            error_append_hint(errp, "With SVE enabled, at least one "
> +                              "vector length must be enabled.\n");
> +            return;
>          }
>  
>          max_vq = find_last_bit(cpu->sve_vq_map, max_vq) + 1;
> @@ -393,46 +384,44 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>      assert(max_vq != 0);
>      bitmap_clear(cpu->sve_vq_map, max_vq, ARM_MAX_VQ - max_vq);
>  
> -    if (kvm_enabled()) {
> -        /* Ensure the set of lengths matches what KVM supports. */
> -        bitmap_xor(tmp, cpu->sve_vq_map, cpu->sve_vq_supported, max_vq);
> -        if (!bitmap_empty(tmp, max_vq)) {
> -            vq = find_last_bit(tmp, max_vq) + 1;
> -            if (test_bit(vq - 1, cpu->sve_vq_map)) {
> -                if (cpu->sve_max_vq) {
> -                    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<N> properties.\n");
> -                } else {
> -                    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);
> -                }
> +    /* Ensure the set of lengths matches what is supported. */
> +    bitmap_xor(tmp, cpu->sve_vq_map, cpu->sve_vq_supported, max_vq);
> +    if (!bitmap_empty(tmp, max_vq)) {
> +        vq = find_last_bit(tmp, max_vq) + 1;
> +        if (test_bit(vq - 1, cpu->sve_vq_map)) {
> +            if (cpu->sve_max_vq) {
> +                error_setg(errp, "cannot set sve-max-vq=%d", 
> cpu->sve_max_vq);
> +                error_append_hint(errp, "This CPU 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 CPU. Try "
> +                                  "using only sve<N> properties.\n");
>              } else {
> +                error_setg(errp, "cannot enable sve%d", vq * 128);
> +                error_append_hint(errp, "This CPU does not support "
> +                                  "the vector length %d-bits.\n", vq * 128);
> +            }
> +            return;
> +        } else {
> +            if (kvm_enabled()) {
>                  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);
> -            }
> -            return;
> -        }
> -    } else {
> -        /* 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-two length smaller than "
> -                                  "the maximum, sve%d\n",
> -                                  vq * 128, max_vq * 128);
>                  return;
> +            } else {
> +                /* 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-two length smaller "
> +                                          " than the maximum, sve%d\n",

When moving 'than' down to avoid going over 80 characters I somehow added
a leading space, making two spaces between 'smaller' and 'than'. I'll send
a v2 to remove it.

Thanks,
drew

> +                                          vq * 128, max_vq * 128);
> +                        return;
> +                    }
> +                }
>              }
>          }
>      }
> -- 
> 2.31.1
>