[PATCH v2 0/3] target/arm: Add sve-default-vector-length cpu property

[PATCH v2 0/3] target/arm: Add sve-default-vector-length cpu property

[Richard Henderson]

This is intended to resolve #482.

Changes for v2:
  * Split out length bounding fix to new patch.
  * Use byte units for sve-default-vector-length.
  * Support undocumented -1 "maximum".
  * Add documentation.


r~


Richard Henderson (3):
  target/arm: Correctly bound length in sve_zcr_get_valid_len
  target/arm: Export aarch64_sve_zcr_get_valid_len
  target/arm: Add sve-default-vector-length cpu property

 docs/system/arm/cpu-features.rst | 11 ++++++
 target/arm/cpu.h                 |  5 +++
 target/arm/internals.h           | 10 ++++++
 target/arm/cpu.c                 | 14 ++++++--
 target/arm/cpu64.c               | 60 ++++++++++++++++++++++++++++++++
 target/arm/helper.c              |  8 +++--
 6 files changed, 103 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] target/arm: Correctly bound length in sve_zcr_get_valid_len

[Richard Henderson]

Currently, our only caller is sve_zcr_len_for_el, which has
already masked the length extracted from ZCR_ELx, so the
masking done here is a nop.  But we will shortly have uses
from other locations, where the length will be unmasked.

Saturate the length to ARM_MAX_VQ instead of truncating to
the low 4 bits.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0c07ca9837..8c1d8dbce3 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6461,7 +6461,9 @@ static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
 {
     uint32_t end_len;
 
-    end_len = start_len &= 0xf;
+    start_len = MIN(start_len, ARM_MAX_VQ - 1);
+    end_len = start_len;
+
     if (!test_bit(start_len, cpu->sve_vq_map)) {
         end_len = find_last_bit(cpu->sve_vq_map, start_len);
         assert(end_len < start_len);
-- 
2.25.1

Re: [PATCH v2 1/3] target/arm: Correctly bound length in sve_zcr_get_valid_len

[Peter Maydell]

On Fri, 23 Jul 2021 at 21:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Currently, our only caller is sve_zcr_len_for_el, which has
> already masked the length extracted from ZCR_ELx, so the
> masking done here is a nop.  But we will shortly have uses
> from other locations, where the length will be unmasked.
>
> Saturate the length to ARM_MAX_VQ instead of truncating to
> the low 4 bits.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 2/3] target/arm: Export aarch64_sve_zcr_get_valid_len

[Richard Henderson]

Rename from sve_zcr_get_valid_len and make accessible
from outside of helper.c.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h | 10 ++++++++++
 target/arm/helper.c    |  4 ++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 11a72013f5..cd2ea8a388 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -177,6 +177,16 @@ void arm_translate_init(void);
 void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
 #endif /* CONFIG_TCG */
 
+/**
+ * aarch64_sve_zcr_get_valid_len:
+ * @cpu: cpu context
+ * @start_len: maximum len to consider
+ *
+ * Return the maximum supported sve vector length <= @start_len.
+ * Note that both @start_len and the return value are in units
+ * of ZCR_ELx.LEN, so the vector bit length is (x + 1) * 128.
+ */
+uint32_t aarch64_sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len);
 
 enum arm_fprounding {
     FPROUNDING_TIEEVEN,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8c1d8dbce3..155d8bf239 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6457,7 +6457,7 @@ int sve_exception_el(CPUARMState *env, int el)
     return 0;
 }
 
-static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
+uint32_t aarch64_sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len)
 {
     uint32_t end_len;
 
@@ -6489,7 +6489,7 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
         zcr_len = MIN(zcr_len, 0xf & (uint32_t)env->vfp.zcr_el[3]);
     }
 
-    return sve_zcr_get_valid_len(cpu, zcr_len);
+    return aarch64_sve_zcr_get_valid_len(cpu, zcr_len);
 }
 
 static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
2.25.1

Re: [PATCH v2 2/3] target/arm: Export aarch64_sve_zcr_get_valid_len

[Peter Maydell]

On Fri, 23 Jul 2021 at 21:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Rename from sve_zcr_get_valid_len and make accessible
> from outside of helper.c.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h | 10 ++++++++++

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 3/3] target/arm: Add sve-default-vector-length cpu property

[Richard Henderson]

Mirror the behavour of /proc/sys/abi/sve_default_vector_length
under the real linux kernel.  We have no way of passing along
a real default across exec like the kernel can, but this is a
decent way of adjusting the startup vector length of a process.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/482
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 docs/system/arm/cpu-features.rst | 11 ++++++
 target/arm/cpu.h                 |  5 +++
 target/arm/cpu.c                 | 14 ++++++--
 target/arm/cpu64.c               | 60 ++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index c455442eaf..4ff36cc83f 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -376,3 +376,14 @@ verbose command lines.  However, the recommended way to select vector
 lengths is to explicitly enable each desired length.  Therefore only
 example's (1), (4), and (6) exhibit recommended uses of the properties.
 
+SVE User-mode Default Vector Length Property
+--------------------------------------------
+
+For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
+defined to mirror the Linux kernel parameter file
+`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
+is in units of bytes and must be between 16 and 8192.  
+If not specified, the default vector length is 64.
+
+If the default length is larger than the maximum vector length enabled
+with `sve<N>` properties, the actual vector length will be reduced.
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index be9a4dceae..9f0a5f84d5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1006,6 +1006,11 @@ struct ARMCPU {
     /* Used to set the maximum vector length the cpu will support.  */
     uint32_t sve_max_vq;
 
+#ifdef CONFIG_USER_ONLY
+    /* Used to set the default vector length at process start. */
+    uint32_t sve_default_vq;
+#endif
+
     /*
      * 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
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 752b15bb79..2866dd7658 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -201,7 +201,8 @@ static void arm_cpu_reset(DeviceState *dev)
         env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
         /* with reasonable vector length */
         if (cpu_isar_feature(aa64_sve, cpu)) {
-            env->vfp.zcr_el[1] = MIN(cpu->sve_max_vq - 1, 3);
+            env->vfp.zcr_el[1] =
+                aarch64_sve_zcr_get_valid_len(cpu, cpu->sve_default_vq - 1);
         }
         /*
          * Enable TBI0 but not TBI1.
@@ -1051,7 +1052,16 @@ static void arm_cpu_initfn(Object *obj)
     QLIST_INIT(&cpu->pre_el_change_hooks);
     QLIST_INIT(&cpu->el_change_hooks);
 
-#ifndef CONFIG_USER_ONLY
+#ifdef CONFIG_USER_ONLY
+# ifdef TARGET_AARCH64
+    /*
+     * The linux kernel defaults to 512-bit vectors, when sve is supported.
+     * See documentation for /proc/sys/abi/sve_default_vector_length, and
+     * our corresponding sve-default-vector-length cpu property.
+     */
+    cpu->sve_default_vq = 4;
+# endif
+#else
     /* Our inbound IRQ and FIQ lines */
     if (kvm_enabled()) {
         /* VIRQ and VFIQ are unused with KVM but we add them to maintain
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index c7a1626bec..c690318a9b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -559,6 +559,59 @@ static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
     cpu->isar.id_aa64pfr0 = t;
 }
 
+#ifdef CONFIG_USER_ONLY
+/* Mirror linux /proc/sys/abi/sve_default_vector_length. */
+static void cpu_arm_set_sve_default_vec_len(Object *obj, Visitor *v,
+                                            const char *name, void *opaque,
+                                            Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    int32_t default_len, default_vq, remainder;
+
+    if (!visit_type_int32(v, name, &default_len, errp)) {
+        return;
+    }
+
+    /* Undocumented, but the kernel allows -1 to indicate "maximum". */
+    if (default_len == -1) {
+        cpu->sve_default_vq = ARM_MAX_VQ;
+        return;
+    }
+
+    default_vq = default_len / 16;
+    remainder = default_len % 16;
+
+    /*
+     * Note that the 512 max comes from include/uapi/asm/sve_context.h
+     * and is the maximum architectural width of ZCR_ELx.LEN.
+     */
+    if (remainder || default_vq < 1 || default_vq > 512) {
+        error_setg(errp, "cannot set sve-default-vector-length");
+        if (remainder) {
+            error_append_hint(errp, "Vector length not a multiple of 16\n");
+        } else if (default_vq < 1) {
+            error_append_hint(errp, "Vector length smaller than 16\n");
+        } else {
+            error_append_hint(errp, "Vector length larger than %d\n",
+                              512 * 16);
+        }
+        return;
+    }
+
+    cpu->sve_default_vq = default_vq;
+}
+
+static void cpu_arm_get_sve_default_vec_len(Object *obj, Visitor *v,
+                                            const char *name, void *opaque,
+                                            Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    int32_t value = cpu->sve_default_vq * 16;
+
+    visit_type_int32(v, name, &value, errp);
+}
+#endif
+
 void aarch64_add_sve_properties(Object *obj)
 {
     uint32_t vq;
@@ -571,6 +624,13 @@ void aarch64_add_sve_properties(Object *obj)
         object_property_add(obj, name, "bool", cpu_arm_get_sve_vq,
                             cpu_arm_set_sve_vq, NULL, NULL);
     }
+
+#ifdef CONFIG_USER_ONLY
+    /* Mirror linux /proc/sys/abi/sve_default_vector_length. */
+    object_property_add(obj, "sve-default-vector-length", "int32",
+                        cpu_arm_get_sve_default_vec_len,
+                        cpu_arm_set_sve_default_vec_len, NULL, NULL);
+#endif
 }
 
 void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
-- 
2.25.1

Re: [PATCH v2 3/3] target/arm: Add sve-default-vector-length cpu property

[Peter Maydell]

On Fri, 23 Jul 2021 at 21:36, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Mirror the behavour of /proc/sys/abi/sve_default_vector_length
> under the real linux kernel.  We have no way of passing along
> a real default across exec like the kernel can, but this is a
> decent way of adjusting the startup vector length of a process.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/482
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  docs/system/arm/cpu-features.rst | 11 ++++++
>  target/arm/cpu.h                 |  5 +++
>  target/arm/cpu.c                 | 14 ++++++--
>  target/arm/cpu64.c               | 60 ++++++++++++++++++++++++++++++++
>  4 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index c455442eaf..4ff36cc83f 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -376,3 +376,14 @@ verbose command lines.  However, the recommended way to select vector
>  lengths is to explicitly enable each desired length.  Therefore only
>  example's (1), (4), and (6) exhibit recommended uses of the properties.
>
> +SVE User-mode Default Vector Length Property
> +--------------------------------------------
> +
> +For qemu-aarch64, the cpu property `sve-default-vector-length=N` is

You probably don't want single-backticks. In rST this means
"interpreted text", which can be handled as a bunch of different
things if tagged with a specific "role":
https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#interpreted-text
The default "role" if none is specified is "title_reference", intended for
references to book or article titles, and it renders into the HTML
as <cite>...</cite> (usually comes out as italics).

If you want fixed-width-font text, that's double backtick: ``text``.
If you want italics, that's *emphasised text*.
If you want bold, that's **strong text**.

> +defined to mirror the Linux kernel parameter file
> +`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
> +is in units of bytes and must be between 16 and 8192.
> +If not specified, the default vector length is 64.
> +
> +If the default length is larger than the maximum vector length enabled
> +with `sve<N>` properties, the actual vector length will be reduced.

We should document the -1 case too, something like:

If this property is set to ``-1`` then the default vector length
is set to the maximum possible length.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 3/3] target/arm: Add sve-default-vector-length cpu property

[Andrew Jones]

On Fri, Jul 23, 2021 at 10:33:44AM -1000, Richard Henderson wrote:
> Mirror the behavour of /proc/sys/abi/sve_default_vector_length
> under the real linux kernel.  We have no way of passing along
> a real default across exec like the kernel can, but this is a
> decent way of adjusting the startup vector length of a process.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/482
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  docs/system/arm/cpu-features.rst | 11 ++++++
>  target/arm/cpu.h                 |  5 +++
>  target/arm/cpu.c                 | 14 ++++++--
>  target/arm/cpu64.c               | 60 ++++++++++++++++++++++++++++++++
>  4 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index c455442eaf..4ff36cc83f 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -376,3 +376,14 @@ verbose command lines.  However, the recommended way to select vector
>  lengths is to explicitly enable each desired length.  Therefore only
>  example's (1), (4), and (6) exhibit recommended uses of the properties.
>  
> +SVE User-mode Default Vector Length Property
> +--------------------------------------------
> +
> +For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
> +defined to mirror the Linux kernel parameter file
> +`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
> +is in units of bytes and must be between 16 and 8192.  

Hmm. If a user inputs anything greater than 256, then won't it get
silently reduced to 256?

> +If not specified, the default vector length is 64.
> +
> +If the default length is larger than the maximum vector length enabled
> +with `sve<N>` properties, the actual vector length will be reduced.

Here it's pointed out that the default may be reduced, but it implies that
that only happens if an sve<N> property is also given. Won't users wonder
why they're only getting vectors that are 256 bytes large even when they
ask for more?

Thanks,
drew

> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index be9a4dceae..9f0a5f84d5 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1006,6 +1006,11 @@ struct ARMCPU {
>      /* Used to set the maximum vector length the cpu will support.  */
>      uint32_t sve_max_vq;
>  
> +#ifdef CONFIG_USER_ONLY
> +    /* Used to set the default vector length at process start. */
> +    uint32_t sve_default_vq;
> +#endif
> +
>      /*
>       * 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
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 752b15bb79..2866dd7658 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -201,7 +201,8 @@ static void arm_cpu_reset(DeviceState *dev)
>          env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 16, 2, 3);
>          /* with reasonable vector length */
>          if (cpu_isar_feature(aa64_sve, cpu)) {
> -            env->vfp.zcr_el[1] = MIN(cpu->sve_max_vq - 1, 3);
> +            env->vfp.zcr_el[1] =
> +                aarch64_sve_zcr_get_valid_len(cpu, cpu->sve_default_vq - 1);
>          }
>          /*
>           * Enable TBI0 but not TBI1.
> @@ -1051,7 +1052,16 @@ static void arm_cpu_initfn(Object *obj)
>      QLIST_INIT(&cpu->pre_el_change_hooks);
>      QLIST_INIT(&cpu->el_change_hooks);
>  
> -#ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_USER_ONLY
> +# ifdef TARGET_AARCH64
> +    /*
> +     * The linux kernel defaults to 512-bit vectors, when sve is supported.
> +     * See documentation for /proc/sys/abi/sve_default_vector_length, and
> +     * our corresponding sve-default-vector-length cpu property.
> +     */
> +    cpu->sve_default_vq = 4;
> +# endif
> +#else
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
>          /* VIRQ and VFIQ are unused with KVM but we add them to maintain
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index c7a1626bec..c690318a9b 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -559,6 +559,59 @@ static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
>      cpu->isar.id_aa64pfr0 = t;
>  }
>  
> +#ifdef CONFIG_USER_ONLY
> +/* Mirror linux /proc/sys/abi/sve_default_vector_length. */
> +static void cpu_arm_set_sve_default_vec_len(Object *obj, Visitor *v,
> +                                            const char *name, void *opaque,
> +                                            Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    int32_t default_len, default_vq, remainder;
> +
> +    if (!visit_type_int32(v, name, &default_len, errp)) {
> +        return;
> +    }
> +
> +    /* Undocumented, but the kernel allows -1 to indicate "maximum". */
> +    if (default_len == -1) {
> +        cpu->sve_default_vq = ARM_MAX_VQ;
> +        return;
> +    }
> +
> +    default_vq = default_len / 16;
> +    remainder = default_len % 16;
> +
> +    /*
> +     * Note that the 512 max comes from include/uapi/asm/sve_context.h
> +     * and is the maximum architectural width of ZCR_ELx.LEN.
> +     */
> +    if (remainder || default_vq < 1 || default_vq > 512) {
> +        error_setg(errp, "cannot set sve-default-vector-length");
> +        if (remainder) {
> +            error_append_hint(errp, "Vector length not a multiple of 16\n");
> +        } else if (default_vq < 1) {
> +            error_append_hint(errp, "Vector length smaller than 16\n");
> +        } else {
> +            error_append_hint(errp, "Vector length larger than %d\n",
> +                              512 * 16);
> +        }
> +        return;
> +    }
> +
> +    cpu->sve_default_vq = default_vq;
> +}
> +
> +static void cpu_arm_get_sve_default_vec_len(Object *obj, Visitor *v,
> +                                            const char *name, void *opaque,
> +                                            Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    int32_t value = cpu->sve_default_vq * 16;
> +
> +    visit_type_int32(v, name, &value, errp);
> +}
> +#endif
> +
>  void aarch64_add_sve_properties(Object *obj)
>  {
>      uint32_t vq;
> @@ -571,6 +624,13 @@ void aarch64_add_sve_properties(Object *obj)
>          object_property_add(obj, name, "bool", cpu_arm_get_sve_vq,
>                              cpu_arm_set_sve_vq, NULL, NULL);
>      }
> +
> +#ifdef CONFIG_USER_ONLY
> +    /* Mirror linux /proc/sys/abi/sve_default_vector_length. */
> +    object_property_add(obj, "sve-default-vector-length", "int32",
> +                        cpu_arm_get_sve_default_vec_len,
> +                        cpu_arm_set_sve_default_vec_len, NULL, NULL);
> +#endif
>  }
>  
>  void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
> -- 
> 2.25.1
> 
> 

Re: [PATCH v2 3/3] target/arm: Add sve-default-vector-length cpu property

[Richard Henderson]

On 7/26/21 4:59 AM, Andrew Jones wrote:
>> +SVE User-mode Default Vector Length Property
>> +--------------------------------------------
>> +
>> +For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
>> +defined to mirror the Linux kernel parameter file
>> +`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
>> +is in units of bytes and must be between 16 and 8192.
> 
> Hmm. If a user inputs anything greater than 256, then won't it get
> silently reduced to 256?

Yes.

>> +If not specified, the default vector length is 64.
>> +
>> +If the default length is larger than the maximum vector length enabled
>> +with `sve<N>` properties, the actual vector length will be reduced.
> 
> Here it's pointed out that the default may be reduced, but it implies that
> that only happens if an sve<N> property is also given. Won't users wonder
> why they're only getting vectors that are 256 bytes large even when they
> ask for more?

I guess adding that 256 is the maximum length supported by qemu should be sufficient?


r~

Re: [PATCH v2 3/3] target/arm: Add sve-default-vector-length cpu property

[Andrew Jones]

On Mon, Jul 26, 2021 at 08:33:52AM -1000, Richard Henderson wrote:
> On 7/26/21 4:59 AM, Andrew Jones wrote:
> > > +SVE User-mode Default Vector Length Property
> > > +--------------------------------------------
> > > +
> > > +For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
> > > +defined to mirror the Linux kernel parameter file
> > > +`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
> > > +is in units of bytes and must be between 16 and 8192.
> > 
> > Hmm. If a user inputs anything greater than 256, then won't it get
> > silently reduced to 256?
> 
> Yes.
> 
> > > +If not specified, the default vector length is 64.
> > > +
> > > +If the default length is larger than the maximum vector length enabled
> > > +with `sve<N>` properties, the actual vector length will be reduced.
> > 
> > Here it's pointed out that the default may be reduced, but it implies that
> > that only happens if an sve<N> property is also given. Won't users wonder
> > why they're only getting vectors that are 256 bytes large even when they
> > ask for more?
> 
> I guess adding that 256 is the maximum length supported by qemu should be sufficient?
>

Works for me.

Thanks,
drew 

Re: [PATCH v2 3/3] target/arm: Add sve-default-vector-length cpu property

[Peter Maydell]

On Mon, 26 Jul 2021 at 19:34, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/26/21 4:59 AM, Andrew Jones wrote:
> >> +SVE User-mode Default Vector Length Property
> >> +--------------------------------------------
> >> +
> >> +For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
> >> +defined to mirror the Linux kernel parameter file
> >> +`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
> >> +is in units of bytes and must be between 16 and 8192.
> >
> > Hmm. If a user inputs anything greater than 256, then won't it get
> > silently reduced to 256?
>
> Yes.
>
> >> +If not specified, the default vector length is 64.
> >> +
> >> +If the default length is larger than the maximum vector length enabled
> >> +with `sve<N>` properties, the actual vector length will be reduced.
> >
> > Here it's pointed out that the default may be reduced, but it implies that
> > that only happens if an sve<N> property is also given. Won't users wonder
> > why they're only getting vectors that are 256 bytes large even when they
> > ask for more?
>
> I guess adding that 256 is the maximum length supported by qemu should be sufficient?

Could you either provide a fixup diff for me to fold into this patch
or else repost a new version? (I don't mind which; if it's just a
docs tweak fixup diff is probably simplest.)

thanks
-- PMM

Re: [PATCH v2 0/3] target/arm: Add sve-default-vector-length cpu property

[Peter Maydell]

On Fri, 23 Jul 2021 at 21:34, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This is intended to resolve #482.
>
> Changes for v2:
>   * Split out length bounding fix to new patch.
>   * Use byte units for sve-default-vector-length.
>   * Support undocumented -1 "maximum".
>   * Add documentation.

I'm going to apply this to target-arm.next with the following
docs tweak squashed into patch 3:

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 4ff36cc83f0..7b97df442aa 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -379,11 +379,14 @@ example's (1), (4), and (6) exhibit recommended
uses of the properties.
 SVE User-mode Default Vector Length Property
 --------------------------------------------

-For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
+For qemu-aarch64, the cpu property ``sve-default-vector-length=N`` is
 defined to mirror the Linux kernel parameter file
-`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
-is in units of bytes and must be between 16 and 8192.
+``/proc/sys/abi/sve_default_vector_length``.  The default length, ``N``,
+is in units of bytes and must be between 16 and 8192.
 If not specified, the default vector length is 64.

 If the default length is larger than the maximum vector length enabled
-with `sve<N>` properties, the actual vector length will be reduced.
+with ``sve<N>`` properties, the actual vector length will be reduced.
+
+If this property is set to ``-1`` then the default vector length
+is set to the maximum possible length.

thanks
-- PMM

Re: [PATCH v2 0/3] target/arm: Add sve-default-vector-length cpu property

[Andrew Jones]

On Mon, Jul 26, 2021 at 01:42:45PM +0100, Peter Maydell wrote:
> On Fri, 23 Jul 2021 at 21:34, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > This is intended to resolve #482.
> >
> > Changes for v2:
> >   * Split out length bounding fix to new patch.
> >   * Use byte units for sve-default-vector-length.
> >   * Support undocumented -1 "maximum".
> >   * Add documentation.
> 
> I'm going to apply this to target-arm.next with the following
> docs tweak squashed into patch 3:
> 
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index 4ff36cc83f0..7b97df442aa 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -379,11 +379,14 @@ example's (1), (4), and (6) exhibit recommended
> uses of the properties.
>  SVE User-mode Default Vector Length Property
>  --------------------------------------------
> 
> -For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
> +For qemu-aarch64, the cpu property ``sve-default-vector-length=N`` is
>  defined to mirror the Linux kernel parameter file
> -`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
> -is in units of bytes and must be between 16 and 8192.
> +``/proc/sys/abi/sve_default_vector_length``.  The default length, ``N``,
> +is in units of bytes and must be between 16 and 8192.
>  If not specified, the default vector length is 64.
> 
>  If the default length is larger than the maximum vector length enabled
> -with `sve<N>` properties, the actual vector length will be reduced.
> +with ``sve<N>`` properties, the actual vector length will be reduced.
> +
> +If this property is set to ``-1`` then the default vector length
> +is set to the maximum possible length.

This file is full of single backtick usage. Isn't it better to stay
consistent? Or do we need a patch that converts all the rest now?

Thanks,
drew

> 
> thanks
> -- PMM
> 

Re: [PATCH v2 0/3] target/arm: Add sve-default-vector-length cpu property

[Peter Maydell]

On Mon, 26 Jul 2021 at 16:01, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Jul 26, 2021 at 01:42:45PM +0100, Peter Maydell wrote:
> > On Fri, 23 Jul 2021 at 21:34, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> > >
> > > This is intended to resolve #482.
> > >
> > > Changes for v2:
> > >   * Split out length bounding fix to new patch.
> > >   * Use byte units for sve-default-vector-length.
> > >   * Support undocumented -1 "maximum".
> > >   * Add documentation.
> >
> > I'm going to apply this to target-arm.next with the following
> > docs tweak squashed into patch 3:
> >
> > diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> > index 4ff36cc83f0..7b97df442aa 100644
> > --- a/docs/system/arm/cpu-features.rst
> > +++ b/docs/system/arm/cpu-features.rst
> > @@ -379,11 +379,14 @@ example's (1), (4), and (6) exhibit recommended
> > uses of the properties.
> >  SVE User-mode Default Vector Length Property
> >  --------------------------------------------
> >
> > -For qemu-aarch64, the cpu property `sve-default-vector-length=N` is
> > +For qemu-aarch64, the cpu property ``sve-default-vector-length=N`` is
> >  defined to mirror the Linux kernel parameter file
> > -`/proc/sys/abi/sve_default_vector_length`.  The default length, `N`,
> > -is in units of bytes and must be between 16 and 8192.
> > +``/proc/sys/abi/sve_default_vector_length``.  The default length, ``N``,
> > +is in units of bytes and must be between 16 and 8192.
> >  If not specified, the default vector length is 64.
> >
> >  If the default length is larger than the maximum vector length enabled
> > -with `sve<N>` properties, the actual vector length will be reduced.
> > +with ``sve<N>`` properties, the actual vector length will be reduced.
> > +
> > +If this property is set to ``-1`` then the default vector length
> > +is set to the maximum possible length.
>
> This file is full of single backtick usage. Isn't it better to stay
> consistent? Or do we need a patch that converts all the rest now?

I just sent one of those:
https://patchew.org/QEMU/20210726142338.31872-1-peter.maydell@linaro.org/

-- PMM