All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
@ 2021-08-23 16:06 Andrew Jones
  2021-08-23 16:06 ` [PATCH v2 1/4] " Andrew Jones
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Andrew Jones @ 2021-08-23 16:06 UTC (permalink / raw)
  To: qemu-devel, qemu-arm
  Cc: peter.maydell, richard.henderson, ishii.shuuichir, philmd

v2:
 - Completed testing
 - Removed extra space in an error message
 - Added Phil's r-b's

While reviewing the new A64FX CPU type it became clear that CPU
types should be able to specify which SVE vector lengths are
supported. This series adds a new bitmap member to ARMCPU and
modifies arm_cpu_sve_finalize() to validate inputs against it.
So far we only need to set the bitmap for the 'max' CPU type
though and, since it supports all vector lengths, we just fill
the whole thing.

This series was inspired by Richard Henderson's suggestion to
replace arm_cpu_sve_finalize's kvm_supported bitmap with something
that could be shared with TCG.

Thanks,
drew


Andrew Jones (4):
  target/arm/cpu: Introduce sve_vq_supported bitmap
  target/arm/kvm64: Ensure sve vls map is completely clear
  target/arm/cpu64: Replace kvm_supported with sve_vq_supported
  target/arm/cpu64: Validate sve vector lengths are supported

 target/arm/cpu.h   |   4 ++
 target/arm/cpu64.c | 118 +++++++++++++++++++++------------------------
 target/arm/kvm64.c |   2 +-
 3 files changed, 61 insertions(+), 63 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 1/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
@ 2021-08-23 16:06 ` Andrew Jones
  2021-08-23 17:43   ` Richard Henderson
  2021-08-23 16:06 ` [PATCH v2 2/4] target/arm/kvm64: Ensure sve vls map is completely clear Andrew Jones
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Andrew Jones @ 2021-08-23 16:06 UTC (permalink / raw)
  To: qemu-devel, qemu-arm
  Cc: peter.maydell, richard.henderson, ishii.shuuichir, philmd

Allow CPUs that support SVE to specify which SVE vector lengths they
support by setting them in this bitmap. Currently only the 'max' and
'host' CPU types supports SVE and 'host' requires KVM which obtains
its supported bitmap from the host. So, we only need to initialize the
bitmap for 'max' with TCG. And, since 'max' should support all SVE
vector lengths we simply fill the bitmap. Future CPU types may have
less trivial maps though.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/arm/cpu.h   | 4 ++++
 target/arm/cpu64.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9f0a5f84d503..cc645b57421f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1019,9 +1019,13 @@ struct ARMCPU {
      * While processing properties during initialization, corresponding
      * sve_vq_init bits are set for bits in sve_vq_map that have been
      * set by properties.
+     *
+     * Bits set in sve_vq_supported represent valid vector lengths for
+     * the CPU type.
      */
     DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ);
     DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ);
+    DECLARE_BITMAP(sve_vq_supported, ARM_MAX_VQ);
 
     /* Generic timer counter frequency, in Hz */
     uint64_t gt_cntfrq_hz;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index c690318a9b63..eb9318c83b74 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -840,6 +840,8 @@ static void aarch64_max_initfn(Object *obj)
         /* Default to PAUTH on, with the architected algorithm. */
         qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
         qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+
+        bitmap_fill(cpu->sve_vq_supported, ARM_MAX_VQ);
     }
 
     aarch64_add_sve_properties(obj);
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 2/4] target/arm/kvm64: Ensure sve vls map is completely clear
  2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
  2021-08-23 16:06 ` [PATCH v2 1/4] " Andrew Jones
@ 2021-08-23 16:06 ` Andrew Jones
  2021-08-23 17:45   ` Richard Henderson
  2021-08-23 16:06 ` [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported Andrew Jones
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Andrew Jones @ 2021-08-23 16:06 UTC (permalink / raw)
  To: qemu-devel, qemu-arm
  Cc: peter.maydell, richard.henderson, ishii.shuuichir, philmd

bitmap_clear() only clears the given range. While the given
range should be sufficient in this case we might as well be
100% sure all bits are zeroed by using bitmap_zero().

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/arm/kvm64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 59982d470d37..e790d6c9a573 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -740,7 +740,7 @@ void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
     uint32_t vq = 0;
     int i, j;
 
-    bitmap_clear(map, 0, ARM_MAX_VQ);
+    bitmap_zero(map, ARM_MAX_VQ);
 
     /*
      * KVM ensures all host CPUs support the same set of vector lengths.
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported
  2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
  2021-08-23 16:06 ` [PATCH v2 1/4] " Andrew Jones
  2021-08-23 16:06 ` [PATCH v2 2/4] target/arm/kvm64: Ensure sve vls map is completely clear Andrew Jones
@ 2021-08-23 16:06 ` Andrew Jones
  2021-08-23 17:53   ` Richard Henderson
  2021-08-23 16:06 ` [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported Andrew Jones
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Andrew Jones @ 2021-08-23 16:06 UTC (permalink / raw)
  To: qemu-devel, qemu-arm
  Cc: peter.maydell, richard.henderson, ishii.shuuichir, philmd

Now that we have an ARMCPU member sve_vq_supported we no longer
need the local kvm_supported bitmap for KVM's supported vector
lengths.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/arm/cpu64.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index eb9318c83b74..557fd4757740 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -265,14 +265,17 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
      * any of the above.  Finally, if SVE is not disabled, then at least one
      * vector length must be enabled.
      */
-    DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
     DECLARE_BITMAP(tmp, ARM_MAX_VQ);
     uint32_t vq, max_vq = 0;
 
-    /* Collect the set of vector lengths supported by KVM. */
-    bitmap_zero(kvm_supported, ARM_MAX_VQ);
+    /*
+     * CPU models specify a set of supported vector lengths which are
+     * enabled by default.  Attempting to enable any vector length not set
+     * in the supported bitmap results in an error.  When KVM is enabled we
+     * fetch the supported bitmap from the host.
+     */
     if (kvm_enabled() && kvm_arm_sve_supported()) {
-        kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
+        kvm_arm_sve_get_vls(CPU(cpu), cpu->sve_vq_supported);
     } else if (kvm_enabled()) {
         assert(!cpu_isar_feature(aa64_sve, cpu));
     }
@@ -299,7 +302,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
              * For KVM we have to automatically enable all supported unitialized
              * lengths, even when the smaller lengths are not all powers-of-two.
              */
-            bitmap_andnot(tmp, kvm_supported, cpu->sve_vq_init, max_vq);
+            bitmap_andnot(tmp, cpu->sve_vq_supported, cpu->sve_vq_init, max_vq);
             bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, tmp, max_vq);
         } else {
             /* Propagate enabled bits down through required powers-of-two. */
@@ -322,12 +325,12 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
             /* Disabling a supported length disables all larger lengths. */
             for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
                 if (test_bit(vq - 1, cpu->sve_vq_init) &&
-                    test_bit(vq - 1, kvm_supported)) {
+                    test_bit(vq - 1, cpu->sve_vq_supported)) {
                     break;
                 }
             }
             max_vq = vq <= ARM_MAX_VQ ? vq - 1 : ARM_MAX_VQ;
-            bitmap_andnot(cpu->sve_vq_map, kvm_supported,
+            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);
@@ -392,7 +395,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
 
     if (kvm_enabled()) {
         /* Ensure the set of lengths matches what KVM supports. */
-        bitmap_xor(tmp, cpu->sve_vq_map, kvm_supported, max_vq);
+        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)) {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported
  2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
                   ` (2 preceding siblings ...)
  2021-08-23 16:06 ` [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported Andrew Jones
@ 2021-08-23 16:06 ` Andrew Jones
  2021-08-23 18:04   ` Richard Henderson
  2021-08-26 13:22 ` [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Peter Maydell
  2021-08-27  8:30 ` ishii.shuuichir
  5 siblings, 1 reply; 18+ messages in thread
From: Andrew Jones @ 2021-08-23 16:06 UTC (permalink / raw)
  To: qemu-devel, qemu-arm
  Cc: peter.maydell, richard.henderson, ishii.shuuichir, philmd

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..2f0cbddab568 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",
+                                          vq * 128, max_vq * 128);
+                        return;
+                    }
+                }
             }
         }
     }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-23 16:06 ` [PATCH v2 1/4] " Andrew Jones
@ 2021-08-23 17:43   ` Richard Henderson
  0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-23 17:43 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, philmd, ishii.shuuichir

On 8/23/21 9:06 AM, Andrew Jones wrote:
> Allow CPUs that support SVE to specify which SVE vector lengths they
> support by setting them in this bitmap. Currently only the 'max' and
> 'host' CPU types supports SVE and 'host' requires KVM which obtains
> its supported bitmap from the host. So, we only need to initialize the
> bitmap for 'max' with TCG. And, since 'max' should support all SVE
> vector lengths we simply fill the bitmap. Future CPU types may have
> less trivial maps though.
> 
> Signed-off-by: Andrew Jones<drjones@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   target/arm/cpu.h   | 4 ++++
>   target/arm/cpu64.c | 2 ++
>   2 files changed, 6 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] target/arm/kvm64: Ensure sve vls map is completely clear
  2021-08-23 16:06 ` [PATCH v2 2/4] target/arm/kvm64: Ensure sve vls map is completely clear Andrew Jones
@ 2021-08-23 17:45   ` Richard Henderson
  0 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2021-08-23 17:45 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, philmd, ishii.shuuichir

On 8/23/21 9:06 AM, Andrew Jones wrote:
> bitmap_clear() only clears the given range. While the given
> range should be sufficient in this case we might as well be
> 100% sure all bits are zeroed by using bitmap_zero().
> 
> Signed-off-by: Andrew Jones<drjones@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   target/arm/kvm64.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported
  2021-08-23 16:06 ` [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported Andrew Jones
@ 2021-08-23 17:53   ` Richard Henderson
  2021-08-24  6:28     ` Andrew Jones
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2021-08-23 17:53 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, philmd, ishii.shuuichir

On 8/23/21 9:06 AM, Andrew Jones wrote:
> Now that we have an ARMCPU member sve_vq_supported we no longer
> need the local kvm_supported bitmap for KVM's supported vector
> lengths.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   target/arm/cpu64.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index eb9318c83b74..557fd4757740 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -265,14 +265,17 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>        * any of the above.  Finally, if SVE is not disabled, then at least one
>        * vector length must be enabled.
>        */
> -    DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
>       DECLARE_BITMAP(tmp, ARM_MAX_VQ);
>       uint32_t vq, max_vq = 0;
>   
> -    /* Collect the set of vector lengths supported by KVM. */
> -    bitmap_zero(kvm_supported, ARM_MAX_VQ);
> +    /*
> +     * CPU models specify a set of supported vector lengths which are
> +     * enabled by default.  Attempting to enable any vector length not set
> +     * in the supported bitmap results in an error.  When KVM is enabled we
> +     * fetch the supported bitmap from the host.
> +     */
>       if (kvm_enabled() && kvm_arm_sve_supported()) {
> -        kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
> +        kvm_arm_sve_get_vls(CPU(cpu), cpu->sve_vq_supported);
>       } else if (kvm_enabled()) {
>           assert(!cpu_isar_feature(aa64_sve, cpu));
>       }

I think this whole stanza should now be moved into kvm_arm_get_host_cpu_features, where we 
detect sve and fetch ID_AA64ZFR0_EL1.

As a separate patch, since this one is simply the variable rename.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported
  2021-08-23 16:06 ` [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported Andrew Jones
@ 2021-08-23 18:04   ` Richard Henderson
  2021-08-24  6:28     ` Andrew Jones
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2021-08-23 18:04 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell, philmd, ishii.shuuichir

On 8/23/21 9:06 AM, 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(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> +        } else {
> +            if (kvm_enabled()) {

Nit: better as

     } else if (...) {

if I'm reading all of the diff context correctly.


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported
  2021-08-23 18:04   ` Richard Henderson
@ 2021-08-24  6:28     ` Andrew Jones
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Jones @ 2021-08-24  6:28 UTC (permalink / raw)
  To: Richard Henderson
  Cc: peter.maydell, qemu-arm, philmd, qemu-devel, ishii.shuuichir

On Mon, Aug 23, 2021 at 11:04:49AM -0700, Richard Henderson wrote:
> On 8/23/21 9:06 AM, 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(-)
> 
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> > +        } else {
> > +            if (kvm_enabled()) {
> 
> Nit: better as
> 
>     } else if (...) {
> 
> if I'm reading all of the diff context correctly.
>

Yeah, the diff is definitely not easy to read, or even the code
after its applied for that matter... We can't use 'else if' here
because the 'else { if' is part of a pattern like below

  if (...) {
     if (...) {

     } else {

     }
  } else {
     if (kvm_enabled()) {

     } else {

     }
  }

Thanks,
drew



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported
  2021-08-23 17:53   ` Richard Henderson
@ 2021-08-24  6:28     ` Andrew Jones
  2021-08-24  6:47       ` Andrew Jones
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Jones @ 2021-08-24  6:28 UTC (permalink / raw)
  To: Richard Henderson
  Cc: peter.maydell, qemu-arm, philmd, qemu-devel, ishii.shuuichir

On Mon, Aug 23, 2021 at 10:53:48AM -0700, Richard Henderson wrote:
> On 8/23/21 9:06 AM, Andrew Jones wrote:
> > Now that we have an ARMCPU member sve_vq_supported we no longer
> > need the local kvm_supported bitmap for KVM's supported vector
> > lengths.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >   target/arm/cpu64.c | 19 +++++++++++--------
> >   1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index eb9318c83b74..557fd4757740 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -265,14 +265,17 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
> >        * any of the above.  Finally, if SVE is not disabled, then at least one
> >        * vector length must be enabled.
> >        */
> > -    DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
> >       DECLARE_BITMAP(tmp, ARM_MAX_VQ);
> >       uint32_t vq, max_vq = 0;
> > -    /* Collect the set of vector lengths supported by KVM. */
> > -    bitmap_zero(kvm_supported, ARM_MAX_VQ);
> > +    /*
> > +     * CPU models specify a set of supported vector lengths which are
> > +     * enabled by default.  Attempting to enable any vector length not set
> > +     * in the supported bitmap results in an error.  When KVM is enabled we
> > +     * fetch the supported bitmap from the host.
> > +     */
> >       if (kvm_enabled() && kvm_arm_sve_supported()) {
> > -        kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
> > +        kvm_arm_sve_get_vls(CPU(cpu), cpu->sve_vq_supported);
> >       } else if (kvm_enabled()) {
> >           assert(!cpu_isar_feature(aa64_sve, cpu));
> >       }
> 
> I think this whole stanza should now be moved into
> kvm_arm_get_host_cpu_features, where we detect sve and fetch
> ID_AA64ZFR0_EL1.
> 
> As a separate patch, since this one is simply the variable rename.

Good idea. I'll do that for v3.

> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>

Thanks,
drew 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported
  2021-08-24  6:28     ` Andrew Jones
@ 2021-08-24  6:47       ` Andrew Jones
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Jones @ 2021-08-24  6:47 UTC (permalink / raw)
  To: Richard Henderson
  Cc: peter.maydell, qemu-arm, philmd, qemu-devel, ishii.shuuichir

On Tue, Aug 24, 2021 at 08:28:55AM +0200, Andrew Jones wrote:
> On Mon, Aug 23, 2021 at 10:53:48AM -0700, Richard Henderson wrote:
> > On 8/23/21 9:06 AM, Andrew Jones wrote:
> > > Now that we have an ARMCPU member sve_vq_supported we no longer
> > > need the local kvm_supported bitmap for KVM's supported vector
> > > lengths.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > ---
> > >   target/arm/cpu64.c | 19 +++++++++++--------
> > >   1 file changed, 11 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > > index eb9318c83b74..557fd4757740 100644
> > > --- a/target/arm/cpu64.c
> > > +++ b/target/arm/cpu64.c
> > > @@ -265,14 +265,17 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
> > >        * any of the above.  Finally, if SVE is not disabled, then at least one
> > >        * vector length must be enabled.
> > >        */
> > > -    DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
> > >       DECLARE_BITMAP(tmp, ARM_MAX_VQ);
> > >       uint32_t vq, max_vq = 0;
> > > -    /* Collect the set of vector lengths supported by KVM. */
> > > -    bitmap_zero(kvm_supported, ARM_MAX_VQ);
> > > +    /*
> > > +     * CPU models specify a set of supported vector lengths which are
> > > +     * enabled by default.  Attempting to enable any vector length not set
> > > +     * in the supported bitmap results in an error.  When KVM is enabled we
> > > +     * fetch the supported bitmap from the host.
> > > +     */
> > >       if (kvm_enabled() && kvm_arm_sve_supported()) {
> > > -        kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
> > > +        kvm_arm_sve_get_vls(CPU(cpu), cpu->sve_vq_supported);
> > >       } else if (kvm_enabled()) {
> > >           assert(!cpu_isar_feature(aa64_sve, cpu));
> > >       }
> > 
> > I think this whole stanza should now be moved into
> > kvm_arm_get_host_cpu_features, where we detect sve and fetch
> > ID_AA64ZFR0_EL1.
> > 
> > As a separate patch, since this one is simply the variable rename.
> 
> Good idea. I'll do that for v3.

Actually, I'll post an independent series for this idea rather than
a v3 with another patch. With enough changes we can avoid several
scratch vcpus, but that's getting too far outside the scope of this
series.

Thanks,
drew



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
                   ` (3 preceding siblings ...)
  2021-08-23 16:06 ` [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported Andrew Jones
@ 2021-08-26 13:22 ` Peter Maydell
  2021-08-27  8:30 ` ishii.shuuichir
  5 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2021-08-26 13:22 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Philippe Mathieu-Daudé,
	qemu-arm, Richard Henderson, QEMU Developers, Shuuichirou Ishii

On Mon, 23 Aug 2021 at 17:06, Andrew Jones <drjones@redhat.com> wrote:
>
> v2:
>  - Completed testing
>  - Removed extra space in an error message
>  - Added Phil's r-b's
>
> While reviewing the new A64FX CPU type it became clear that CPU
> types should be able to specify which SVE vector lengths are
> supported. This series adds a new bitmap member to ARMCPU and
> modifies arm_cpu_sve_finalize() to validate inputs against it.
> So far we only need to set the bitmap for the 'max' CPU type
> though and, since it supports all vector lengths, we just fill
> the whole thing.



Applied to target-arm.next, thanks.

-- PMM


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
                   ` (4 preceding siblings ...)
  2021-08-26 13:22 ` [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Peter Maydell
@ 2021-08-27  8:30 ` ishii.shuuichir
  2021-08-27  8:57   ` Peter Maydell
  2021-08-27 10:03   ` Andrew Jones
  5 siblings, 2 replies; 18+ messages in thread
From: ishii.shuuichir @ 2021-08-27  8:30 UTC (permalink / raw)
  To: 'Andrew Jones'
  Cc: peter.maydell, richard.henderson, qemu-devel, qemu-arm,
	ishii.shuuichir, philmd


Thank you, Andrew, for creating the patches.
And thank you all for your comments.

I have applied the suggested v2 patch series by andrew locally, 
and reviewed the next version of the a64fx patch series as follows. 
I would appreciate if you could comment on whether there are
any problems with the fixes before I post the next version of the patch.
(The a64fx patch series to be posted later will be split as previously posted.)

------------------------------------------------------------------------------------------------
diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 59acf0eeaf..850787495b 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -55,6 +55,7 @@ Supported guest CPU types:
 - ``cortex-a53`` (64-bit)
 - ``cortex-a57`` (64-bit)
 - ``cortex-a72`` (64-bit)
+- ``a64fx`` (64-bit)
 - ``host`` (with KVM only)
 - ``max`` (same as ``host`` for KVM; best possible emulation with TCG)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0b..10286d3fd6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -200,6 +200,7 @@ static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("cortex-a53"),
     ARM_CPU_TYPE_NAME("cortex-a57"),
     ARM_CPU_TYPE_NAME("cortex-a72"),
+    ARM_CPU_TYPE_NAME("a64fx"),
     ARM_CPU_TYPE_NAME("host"),
     ARM_CPU_TYPE_NAME("max"),
 };
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2866dd7658..2d9f779cb6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1350,10 +1350,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
     Error *local_err = NULL;

     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-        arm_cpu_sve_finalize(cpu, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
-            return;
+        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
+            arm_cpu_sve_finalize(cpu, &local_err);
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+                return;
+            }
         }

         /*
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index cc645b5742..bf8d9ddaa1 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2149,6 +2149,7 @@ enum arm_features {
     ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
     ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+    ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
 };

 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 2f0cbddab5..18e813264a 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -841,10 +841,80 @@ static void aarch64_max_initfn(Object *obj)
                         cpu_max_set_sve_max_vq, NULL, NULL);
 }

+static void a64fx_cpu_set_sve(Object *obj)
+{
+    int i;
+    Error *errp = NULL;
+    ARMCPU *cpu = ARM_CPU(obj);
+    /* Suppport of A64FX's vector length are 128,256 and 512bit only */
+    const char *vl[] = {"sve128", "sve256", "sve512"};
+
+    object_property_add_bool(obj, "sve", cpu_arm_get_sve, cpu_arm_set_sve);
+    object_property_set_bool(obj, "sve", true, &errp);
+
+    for(i = 0; i <sizeof(vl)/sizeof(vl[0]); i++){
+        object_property_add(obj, vl[i], "bool", cpu_arm_get_sve_vq,
+                            cpu_arm_set_sve_vq, NULL, NULL);
+        object_property_set_bool(obj, vl[i], true, &errp);
+    }
+
+    bitmap_zero(cpu->sve_vq_supported, ARM_MAX_VQ);
+    set_bit(0, cpu->sve_vq_supported); /* 128bit */
+    set_bit(1, cpu->sve_vq_supported); /* 256bit */
+    set_bit(3, cpu->sve_vq_supported); /* 512bit */
+
+    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;
+}
+
+static void aarch64_a64fx_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    cpu->dtb_compatible = "arm,a64fx";
+    set_feature(&cpu->env, ARM_FEATURE_A64FX);
+    set_feature(&cpu->env, ARM_FEATURE_V8);
+    set_feature(&cpu->env, ARM_FEATURE_NEON);
+    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
+    set_feature(&cpu->env, ARM_FEATURE_EL2);
+    set_feature(&cpu->env, ARM_FEATURE_EL3);
+    set_feature(&cpu->env, ARM_FEATURE_PMU);
+    cpu->midr = 0x461f0010;
+    cpu->revidr = 0x00000000;
+    cpu->ctr = 0x86668006;
+    cpu->reset_sctlr = 0x30000180;
+    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions */
+    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
+    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
+    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
+    cpu->id_aa64afr0 = 0x0000000000000000;
+    cpu->id_aa64afr1 = 0x0000000000000000;
+    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
+    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
+    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
+    cpu->isar.id_aa64isar0 = 0x0000000010211120;
+    cpu->isar.id_aa64isar1 = 0x0000000000010001;
+    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
+    cpu->clidr = 0x0000000080000023;
+    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
+    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
+    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
+    cpu->dcz_blocksize = 6; /* 256 bytes */
+    cpu->gic_num_lrs = 4;
+    cpu->gic_vpribits = 5;
+    cpu->gic_vprebits = 5;
+
+    /* Set SVE properties */
+    a64fx_cpu_set_sve(obj);
+
+    /* TODO:  Add A64FX specific HPC extension registers */
+}
+
 static const ARMCPUInfo aarch64_cpus[] = {
     { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
     { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
     { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
+    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
     { .name = "max",                .initfn = aarch64_max_initfn },
 };

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 8252b85bb8..bf30bee462 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -472,6 +472,11 @@ static void test_query_cpu_model_expansion(const void *data)
         assert_has_feature_enabled(qts, "max", "sve128");
         assert_has_feature_enabled(qts, "cortex-a57", "pmu");
         assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
+        assert_has_feature_enabled(qts, "a64fx", "pmu");
+        assert_has_feature_enabled(qts, "a64fx", "aarch64");
+        assert_has_feature_enabled(qts, "a64fx", "sve");
+        assert_has_feature_enabled(qts, "a64fx", "sve128");
+        assert_has_feature_enabled(qts, "a64fx", "sve256");
+        assert_has_feature_enabled(qts, "a64fx", "sve512");

         sve_tests_default(qts, "max");
         pauth_tests_default(qts, "max");
------------------------------------------------------------------------------------------------

By the way, There is one thing that bothers me about the above modification.
The A64FX supports only 128-bit, 256-bit, and 512-bit SVE vector lengths, but the
The following process in the arm_cpu_sve_finalize() function sets all bits of cpu->sve_vq_map to 1.

/* Set all bits not explicitly set within sve-max-vq. */
bitmap_complement(tmp, cpu->sve_vq_init, max_vq);
bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, tmp, max_vq);

As a result, enter a routine where the following process will be executed.

error_setg(errp, "cannot set sve-max-vq=%d", cpu->sve_max_vq);

Therefore, We have applied the following fix, is this ok?
------------------------------------------------------------------------------------------------
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1350,10 +1350,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
     Error *local_err = NULL;

     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-        arm_cpu_sve_finalize(cpu, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
-            return;
+        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
+            arm_cpu_sve_finalize(cpu, &local_err);
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+                return;
+            }
         }
------------------------------------------------------------------------------------------------

Please your comments if we have misunderstood.
Best regards.

> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Tuesday, August 24, 2021 1:07 AM
> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> richard.henderson@linaro.org; peter.maydell@linaro.org; philmd@redhat.com
> Subject: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
> 
> v2:
>  - Completed testing
>  - Removed extra space in an error message
>  - Added Phil's r-b's
> 
> While reviewing the new A64FX CPU type it became clear that CPU types should
> be able to specify which SVE vector lengths are supported. This series adds a new
> bitmap member to ARMCPU and modifies arm_cpu_sve_finalize() to validate
> inputs against it.
> So far we only need to set the bitmap for the 'max' CPU type though and, since it
> supports all vector lengths, we just fill the whole thing.
> 
> This series was inspired by Richard Henderson's suggestion to replace
> arm_cpu_sve_finalize's kvm_supported bitmap with something that could be
> shared with TCG.
> 
> Thanks,
> drew
> 
> 
> Andrew Jones (4):
>   target/arm/cpu: Introduce sve_vq_supported bitmap
>   target/arm/kvm64: Ensure sve vls map is completely clear
>   target/arm/cpu64: Replace kvm_supported with sve_vq_supported
>   target/arm/cpu64: Validate sve vector lengths are supported
> 
>  target/arm/cpu.h   |   4 ++
>  target/arm/cpu64.c | 118
> +++++++++++++++++++++------------------------
>  target/arm/kvm64.c |   2 +-
>  3 files changed, 61 insertions(+), 63 deletions(-)
> 
> --
> 2.31.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-27  8:30 ` ishii.shuuichir
@ 2021-08-27  8:57   ` Peter Maydell
  2021-08-30  0:02     ` ishii.shuuichir
  2021-08-27 10:03   ` Andrew Jones
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2021-08-27  8:57 UTC (permalink / raw)
  To: ishii.shuuichir
  Cc: philmd, Andrew Jones, qemu-arm, richard.henderson, qemu-devel

On Fri, 27 Aug 2021 at 09:30, ishii.shuuichir@fujitsu.com
<ishii.shuuichir@fujitsu.com> wrote:
>
>
> Thank you, Andrew, for creating the patches.
> And thank you all for your comments.
>
> I have applied the suggested v2 patch series by andrew locally,

FYI, Andrew's patches are now upstream so you'll be able to base your
next revision of your patches directly on upstream master when you're
ready to send it out.

-- PMM


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-27  8:30 ` ishii.shuuichir
  2021-08-27  8:57   ` Peter Maydell
@ 2021-08-27 10:03   ` Andrew Jones
  2021-08-30  4:49     ` ishii.shuuichir
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Jones @ 2021-08-27 10:03 UTC (permalink / raw)
  To: ishii.shuuichir
  Cc: peter.maydell, qemu-arm, richard.henderson, qemu-devel, philmd

On Fri, Aug 27, 2021 at 08:30:07AM +0000, ishii.shuuichir@fujitsu.com wrote:
> 
> Thank you, Andrew, for creating the patches.
> And thank you all for your comments.
> 
> I have applied the suggested v2 patch series by andrew locally, 
> and reviewed the next version of the a64fx patch series as follows. 
> I would appreciate if you could comment on whether there are
> any problems with the fixes before I post the next version of the patch.
> (The a64fx patch series to be posted later will be split as previously posted.)
> 
> ------------------------------------------------------------------------------------------------
> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> index 59acf0eeaf..850787495b 100644
> --- a/docs/system/arm/virt.rst
> +++ b/docs/system/arm/virt.rst
> @@ -55,6 +55,7 @@ Supported guest CPU types:
>  - ``cortex-a53`` (64-bit)
>  - ``cortex-a57`` (64-bit)
>  - ``cortex-a72`` (64-bit)
> +- ``a64fx`` (64-bit)
>  - ``host`` (with KVM only)
>  - ``max`` (same as ``host`` for KVM; best possible emulation with TCG)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 81eda46b0b..10286d3fd6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -200,6 +200,7 @@ static const char *valid_cpus[] = {
>      ARM_CPU_TYPE_NAME("cortex-a53"),
>      ARM_CPU_TYPE_NAME("cortex-a57"),
>      ARM_CPU_TYPE_NAME("cortex-a72"),
> +    ARM_CPU_TYPE_NAME("a64fx"),
>      ARM_CPU_TYPE_NAME("host"),
>      ARM_CPU_TYPE_NAME("max"),
>  };
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2866dd7658..2d9f779cb6 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1350,10 +1350,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>      Error *local_err = NULL;
> 
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> -        arm_cpu_sve_finalize(cpu, &local_err);
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> -            return;
> +        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> +            arm_cpu_sve_finalize(cpu, &local_err);
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }

We don't want to do this anymore. Now we want to call
arm_cpu_sve_finalize() for all cpu models.

>          }
> 
>          /*
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index cc645b5742..bf8d9ddaa1 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2149,6 +2149,7 @@ enum arm_features {
>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>      ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
> +    ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
>  };
> 
>  static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 2f0cbddab5..18e813264a 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -841,10 +841,80 @@ static void aarch64_max_initfn(Object *obj)
>                          cpu_max_set_sve_max_vq, NULL, NULL);
>  }
> 
> +static void a64fx_cpu_set_sve(Object *obj)
> +{
> +    int i;
> +    Error *errp = NULL;
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    /* Suppport of A64FX's vector length are 128,256 and 512bit only */
> +    const char *vl[] = {"sve128", "sve256", "sve512"};
> +
> +    object_property_add_bool(obj, "sve", cpu_arm_get_sve, cpu_arm_set_sve);
> +    object_property_set_bool(obj, "sve", true, &errp);
> +
> +    for(i = 0; i <sizeof(vl)/sizeof(vl[0]); i++){
> +        object_property_add(obj, vl[i], "bool", cpu_arm_get_sve_vq,
> +                            cpu_arm_set_sve_vq, NULL, NULL);
> +        object_property_set_bool(obj, vl[i], true, &errp);
> +    }

We don't want to do any of the above anymore either. We can add all the
properties with aarch64_add_sve_properties() now because only the
supported vector lengths may be enabled.

> +
> +    bitmap_zero(cpu->sve_vq_supported, ARM_MAX_VQ);
> +    set_bit(0, cpu->sve_vq_supported); /* 128bit */
> +    set_bit(1, cpu->sve_vq_supported); /* 256bit */
> +    set_bit(3, cpu->sve_vq_supported); /* 512bit */

This is correct, but since it's only a few lines it can be put directly
into aarch64_a64fx_initfn().

> +
> +    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) + 1;

This isn't necessary. arm_cpu_sve_finalize() will manage it.

> +}
> +
> +static void aarch64_a64fx_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    cpu->dtb_compatible = "arm,a64fx";
> +    set_feature(&cpu->env, ARM_FEATURE_A64FX);
> +    set_feature(&cpu->env, ARM_FEATURE_V8);
> +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +    set_feature(&cpu->env, ARM_FEATURE_EL2);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
> +    set_feature(&cpu->env, ARM_FEATURE_PMU);
> +    cpu->midr = 0x461f0010;
> +    cpu->revidr = 0x00000000;
> +    cpu->ctr = 0x86668006;
> +    cpu->reset_sctlr = 0x30000180;
> +    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions */
> +    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
> +    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
> +    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
> +    cpu->id_aa64afr0 = 0x0000000000000000;
> +    cpu->id_aa64afr1 = 0x0000000000000000;
> +    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
> +    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
> +    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
> +    cpu->isar.id_aa64isar0 = 0x0000000010211120;
> +    cpu->isar.id_aa64isar1 = 0x0000000000010001;
> +    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
> +    cpu->clidr = 0x0000000080000023;
> +    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
> +    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
> +    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
> +    cpu->dcz_blocksize = 6; /* 256 bytes */
> +    cpu->gic_num_lrs = 4;
> +    cpu->gic_vpribits = 5;
> +    cpu->gic_vprebits = 5;
> +
> +    /* Set SVE properties */
> +    a64fx_cpu_set_sve(obj);
> +
> +    /* TODO:  Add A64FX specific HPC extension registers */
> +}
> +
>  static const ARMCPUInfo aarch64_cpus[] = {
>      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
>      { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
> +    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
>      { .name = "max",                .initfn = aarch64_max_initfn },
>  };
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 8252b85bb8..bf30bee462 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -472,6 +472,11 @@ static void test_query_cpu_model_expansion(const void *data)
>          assert_has_feature_enabled(qts, "max", "sve128");
>          assert_has_feature_enabled(qts, "cortex-a57", "pmu");
>          assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
> +        assert_has_feature_enabled(qts, "a64fx", "pmu");
> +        assert_has_feature_enabled(qts, "a64fx", "aarch64");
> +        assert_has_feature_enabled(qts, "a64fx", "sve");
> +        assert_has_feature_enabled(qts, "a64fx", "sve128");
> +        assert_has_feature_enabled(qts, "a64fx", "sve256");
> +        assert_has_feature_enabled(qts, "a64fx", "sve512");

You should also add tests that make sure no other vectors may be enabled.
Also, you can use assert_sve_vls() to check more than one vector length
is enabled at once. So, something like

/*
 * For A64FX SVE should be enabled by default with the vectors
 * sve128, sve256, and sve512
 */
 assert_has_feature_enabled(qts, "a64fx", "sve");
 assert_sve_vls(qts, "a64fx", 0xb, NULL);

/*
 * A64FX does not support any other vector lengths besides those
 * that are enabled by default.
 */
 assert_error(qts, "a64fx", "cannot enable sve384", "{ 'sve384': true }");
 assert_error(qts, "a64fx", "cannot enable sve640", "{ 'sve640': true }");


You could also test that completely disabling sve works and that disabling
sve256 (which also disables sve512) and/or only test disabling sve512
works, but that would only be testing the same code covered by tests with
the 'max' cpu type, since that code is shared.


> 
>          sve_tests_default(qts, "max");
>          pauth_tests_default(qts, "max");
> ------------------------------------------------------------------------------------------------
> 
> By the way, There is one thing that bothers me about the above modification.
> The A64FX supports only 128-bit, 256-bit, and 512-bit SVE vector lengths, but the
> The following process in the arm_cpu_sve_finalize() function sets all bits of cpu->sve_vq_map to 1.
> 
> /* Set all bits not explicitly set within sve-max-vq. */
> bitmap_complement(tmp, cpu->sve_vq_init, max_vq);
> bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, tmp, max_vq);
> 
> As a result, enter a routine where the following process will be executed.
> 
> error_setg(errp, "cannot set sve-max-vq=%d", cpu->sve_max_vq);

This is the correct behavior and the rest of the error message explains
why

 error_append_hint(errp, "This CPU does not support "
                   "the vector length %d-bits.\n", vq * 128);

In A64FX's case that will point out the vector length 384-bits, which
indeed A64FX does not support.

As I've stated a few different times, the sve-max-vq property is of
marginal use, as it only works for CPU models that support all vector
lengths, including non-power-of-2 lengths. This is because the property
says to enable all vector lengths smaller and including sve-max-vq and to
disable all the rest, but if the CPU doesn't support non-power-of-2 vector
lengths, then sve-max-vq will not work for anything other than max-vq=1
and max-vq=2. For this reason, I didn't even bring this property over to
the 'host' cpu type. I think I once suggested that you don't bring it over
to A64FX either and I'll suggest that again.

> 
> Therefore, We have applied the following fix, is this ok?

No :-)

Thanks,
drew

> ------------------------------------------------------------------------------------------------
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1350,10 +1350,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>      Error *local_err = NULL;
> 
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> -        arm_cpu_sve_finalize(cpu, &local_err);
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> -            return;
> +        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> +            arm_cpu_sve_finalize(cpu, &local_err);
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
>          }
> ------------------------------------------------------------------------------------------------
> 
> Please your comments if we have misunderstood.
> Best regards.
> 
> > -----Original Message-----
> > From: Andrew Jones <drjones@redhat.com>
> > Sent: Tuesday, August 24, 2021 1:07 AM
> > To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> > richard.henderson@linaro.org; peter.maydell@linaro.org; philmd@redhat.com
> > Subject: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
> > 
> > v2:
> >  - Completed testing
> >  - Removed extra space in an error message
> >  - Added Phil's r-b's
> > 
> > While reviewing the new A64FX CPU type it became clear that CPU types should
> > be able to specify which SVE vector lengths are supported. This series adds a new
> > bitmap member to ARMCPU and modifies arm_cpu_sve_finalize() to validate
> > inputs against it.
> > So far we only need to set the bitmap for the 'max' CPU type though and, since it
> > supports all vector lengths, we just fill the whole thing.
> > 
> > This series was inspired by Richard Henderson's suggestion to replace
> > arm_cpu_sve_finalize's kvm_supported bitmap with something that could be
> > shared with TCG.
> > 
> > Thanks,
> > drew
> > 
> > 
> > Andrew Jones (4):
> >   target/arm/cpu: Introduce sve_vq_supported bitmap
> >   target/arm/kvm64: Ensure sve vls map is completely clear
> >   target/arm/cpu64: Replace kvm_supported with sve_vq_supported
> >   target/arm/cpu64: Validate sve vector lengths are supported
> > 
> >  target/arm/cpu.h   |   4 ++
> >  target/arm/cpu64.c | 118
> > +++++++++++++++++++++------------------------
> >  target/arm/kvm64.c |   2 +-
> >  3 files changed, 61 insertions(+), 63 deletions(-)
> > 
> > --
> > 2.31.1
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-27  8:57   ` Peter Maydell
@ 2021-08-30  0:02     ` ishii.shuuichir
  0 siblings, 0 replies; 18+ messages in thread
From: ishii.shuuichir @ 2021-08-30  0:02 UTC (permalink / raw)
  To: 'Peter Maydell'
  Cc: Andrew Jones, richard.henderson, qemu-devel, qemu-arm,
	ishii.shuuichir, philmd

> FYI, Andrew's patches are now upstream so you'll be able to base your next
> revision of your patches directly on upstream master when you're ready to send it
> out.

Thanks for the comment.
The reason I applied it locally was because I wanted to check the fixes
before the Andrew’s patches was merged into upstream.

When it is merged upstream, I will pull it and create my patches.

Best regards.

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Friday, August 27, 2021 5:58 PM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>
> Cc: Andrew Jones <drjones@redhat.com>; richard.henderson@linaro.org;
> philmd@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Subject: Re: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
> 
> On Fri, 27 Aug 2021 at 09:30, ishii.shuuichir@fujitsu.com
> <ishii.shuuichir@fujitsu.com> wrote:
> >
> >
> > Thank you, Andrew, for creating the patches.
> > And thank you all for your comments.
> >
> > I have applied the suggested v2 patch series by andrew locally,
> 
> FYI, Andrew's patches are now upstream so you'll be able to base your next
> revision of your patches directly on upstream master when you're ready to send it
> out.
> 
> -- PMM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
  2021-08-27 10:03   ` Andrew Jones
@ 2021-08-30  4:49     ` ishii.shuuichir
  0 siblings, 0 replies; 18+ messages in thread
From: ishii.shuuichir @ 2021-08-30  4:49 UTC (permalink / raw)
  To: 'Andrew Jones'
  Cc: peter.maydell, richard.henderson, qemu-devel, qemu-arm,
	ishii.shuuichir, philmd


Thanks for the comments.
We're sorry for the time it took you due to our lack of understanding.

> As I've stated a few different times, the sve-max-vq property is of marginal use, as
> it only works for CPU models that support all vector lengths, including
> non-power-of-2 lengths. This is because the property says to enable all vector
> lengths smaller and including sve-max-vq and to disable all the rest, but if the
> CPU doesn't support non-power-of-2 vector lengths, then sve-max-vq will not
> work for anything other than max-vq=1 and max-vq=2. For this reason, I didn't
> even bring this property over to the 'host' cpu type. I think I once suggested that
> you don't bring it over to A64FX either and I'll suggest that again.

The setting process of cpu->sve_max_vq was left untouched in the a64fx_cpu_set_sve function
that we originally tried to implement, and our understanding of it was lacking.
Andrew is right about the specification of the sve-max-vq property.

As you suggested, we will not implement the sve-max-vq setting process in a64fx.

> > Therefore, We have applied the following fix, is this ok?
> 
> No :-)

We'd like to repost the patch to reflect the comments noted.

Best regards.

> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Friday, August 27, 2021 7:04 PM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>
> Cc: richard.henderson@linaro.org; peter.maydell@linaro.org;
> philmd@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Subject: Re: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap
> 
> On Fri, Aug 27, 2021 at 08:30:07AM +0000, ishii.shuuichir@fujitsu.com wrote:
> >
> > Thank you, Andrew, for creating the patches.
> > And thank you all for your comments.
> >
> > I have applied the suggested v2 patch series by andrew locally, and
> > reviewed the next version of the a64fx patch series as follows.
> > I would appreciate if you could comment on whether there are any
> > problems with the fixes before I post the next version of the patch.
> > (The a64fx patch series to be posted later will be split as previously
> > posted.)
> >
> > ----------------------------------------------------------------------
> > -------------------------- diff --git a/docs/system/arm/virt.rst
> > b/docs/system/arm/virt.rst index 59acf0eeaf..850787495b 100644
> > --- a/docs/system/arm/virt.rst
> > +++ b/docs/system/arm/virt.rst
> > @@ -55,6 +55,7 @@ Supported guest CPU types:
> >  - ``cortex-a53`` (64-bit)
> >  - ``cortex-a57`` (64-bit)
> >  - ``cortex-a72`` (64-bit)
> > +- ``a64fx`` (64-bit)
> >  - ``host`` (with KVM only)
> >  - ``max`` (same as ``host`` for KVM; best possible emulation with
> > TCG)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index
> > 81eda46b0b..10286d3fd6 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -200,6 +200,7 @@ static const char *valid_cpus[] = {
> >      ARM_CPU_TYPE_NAME("cortex-a53"),
> >      ARM_CPU_TYPE_NAME("cortex-a57"),
> >      ARM_CPU_TYPE_NAME("cortex-a72"),
> > +    ARM_CPU_TYPE_NAME("a64fx"),
> >      ARM_CPU_TYPE_NAME("host"),
> >      ARM_CPU_TYPE_NAME("max"),
> >  };
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c index
> > 2866dd7658..2d9f779cb6 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1350,10 +1350,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu,
> Error **errp)
> >      Error *local_err = NULL;
> >
> >      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> > -        arm_cpu_sve_finalize(cpu, &local_err);
> > -        if (local_err != NULL) {
> > -            error_propagate(errp, local_err);
> > -            return;
> > +        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> > +            arm_cpu_sve_finalize(cpu, &local_err);
> > +            if (local_err != NULL) {
> > +                error_propagate(errp, local_err);
> > +                return;
> > +            }
> 
> We don't want to do this anymore. Now we want to call
> arm_cpu_sve_finalize() for all cpu models.
> 
> >          }
> >
> >          /*
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h index
> > cc645b5742..bf8d9ddaa1 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -2149,6 +2149,7 @@ enum arm_features {
> >      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
> >      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> >      ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
> > +    ARM_FEATURE_A64FX, /* Fujitsu A64FX processor */
> >  };
> >
> >  static inline int arm_feature(CPUARMState *env, int feature) diff
> > --git a/target/arm/cpu64.c b/target/arm/cpu64.c index
> > 2f0cbddab5..18e813264a 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -841,10 +841,80 @@ static void aarch64_max_initfn(Object *obj)
> >                          cpu_max_set_sve_max_vq, NULL, NULL);  }
> >
> > +static void a64fx_cpu_set_sve(Object *obj) {
> > +    int i;
> > +    Error *errp = NULL;
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    /* Suppport of A64FX's vector length are 128,256 and 512bit only */
> > +    const char *vl[] = {"sve128", "sve256", "sve512"};
> > +
> > +    object_property_add_bool(obj, "sve", cpu_arm_get_sve,
> cpu_arm_set_sve);
> > +    object_property_set_bool(obj, "sve", true, &errp);
> > +
> > +    for(i = 0; i <sizeof(vl)/sizeof(vl[0]); i++){
> > +        object_property_add(obj, vl[i], "bool", cpu_arm_get_sve_vq,
> > +                            cpu_arm_set_sve_vq, NULL, NULL);
> > +        object_property_set_bool(obj, vl[i], true, &errp);
> > +    }
> 
> We don't want to do any of the above anymore either. We can add all the properties
> with aarch64_add_sve_properties() now because only the supported vector
> lengths may be enabled.
> 
> > +
> > +    bitmap_zero(cpu->sve_vq_supported, ARM_MAX_VQ);
> > +    set_bit(0, cpu->sve_vq_supported); /* 128bit */
> > +    set_bit(1, cpu->sve_vq_supported); /* 256bit */
> > +    set_bit(3, cpu->sve_vq_supported); /* 512bit */
> 
> This is correct, but since it's only a few lines it can be put directly into
> aarch64_a64fx_initfn().
> 
> > +
> > +    cpu->sve_max_vq = find_last_bit(cpu->sve_vq_map, ARM_MAX_VQ) +
> 1;
> 
> This isn't necessary. arm_cpu_sve_finalize() will manage it.
> 
> > +}
> > +
> > +static void aarch64_a64fx_initfn(Object *obj) {
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    cpu->dtb_compatible = "arm,a64fx";
> > +    set_feature(&cpu->env, ARM_FEATURE_A64FX);
> > +    set_feature(&cpu->env, ARM_FEATURE_V8);
> > +    set_feature(&cpu->env, ARM_FEATURE_NEON);
> > +    set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> > +    set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > +    set_feature(&cpu->env, ARM_FEATURE_EL2);
> > +    set_feature(&cpu->env, ARM_FEATURE_EL3);
> > +    set_feature(&cpu->env, ARM_FEATURE_PMU);
> > +    cpu->midr = 0x461f0010;
> > +    cpu->revidr = 0x00000000;
> > +    cpu->ctr = 0x86668006;
> > +    cpu->reset_sctlr = 0x30000180;
> > +    cpu->isar.id_aa64pfr0 =   0x0000000101111111; /* No RAS Extensions
> */
> > +    cpu->isar.id_aa64pfr1 = 0x0000000000000000;
> > +    cpu->isar.id_aa64dfr0 = 0x0000000010305408;
> > +    cpu->isar.id_aa64dfr1 = 0x0000000000000000;
> > +    cpu->id_aa64afr0 = 0x0000000000000000;
> > +    cpu->id_aa64afr1 = 0x0000000000000000;
> > +    cpu->isar.id_aa64mmfr0 = 0x0000000000001122;
> > +    cpu->isar.id_aa64mmfr1 = 0x0000000011212100;
> > +    cpu->isar.id_aa64mmfr2 = 0x0000000000001011;
> > +    cpu->isar.id_aa64isar0 = 0x0000000010211120;
> > +    cpu->isar.id_aa64isar1 = 0x0000000000010001;
> > +    cpu->isar.id_aa64zfr0 = 0x0000000000000000;
> > +    cpu->clidr = 0x0000000080000023;
> > +    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
> > +    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
> > +    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
> > +    cpu->dcz_blocksize = 6; /* 256 bytes */
> > +    cpu->gic_num_lrs = 4;
> > +    cpu->gic_vpribits = 5;
> > +    cpu->gic_vprebits = 5;
> > +
> > +    /* Set SVE properties */
> > +    a64fx_cpu_set_sve(obj);
> > +
> > +    /* TODO:  Add A64FX specific HPC extension registers */ }
> > +
> >  static const ARMCPUInfo aarch64_cpus[] = {
> >      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
> >      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> >      { .name = "cortex-a72",         .initfn = aarch64_a72_initfn },
> > +    { .name = "a64fx",              .initfn = aarch64_a64fx_initfn },
> >      { .name = "max",                .initfn = aarch64_max_initfn },
> >  };
> >
> > diff --git a/tests/qtest/arm-cpu-features.c
> > b/tests/qtest/arm-cpu-features.c index 8252b85bb8..bf30bee462 100644
> > --- a/tests/qtest/arm-cpu-features.c
> > +++ b/tests/qtest/arm-cpu-features.c
> > @@ -472,6 +472,11 @@ static void test_query_cpu_model_expansion(const
> void *data)
> >          assert_has_feature_enabled(qts, "max", "sve128");
> >          assert_has_feature_enabled(qts, "cortex-a57", "pmu");
> >          assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
> > +        assert_has_feature_enabled(qts, "a64fx", "pmu");
> > +        assert_has_feature_enabled(qts, "a64fx", "aarch64");
> > +        assert_has_feature_enabled(qts, "a64fx", "sve");
> > +        assert_has_feature_enabled(qts, "a64fx", "sve128");
> > +        assert_has_feature_enabled(qts, "a64fx", "sve256");
> > +        assert_has_feature_enabled(qts, "a64fx", "sve512");
> 
> You should also add tests that make sure no other vectors may be enabled.
> Also, you can use assert_sve_vls() to check more than one vector length is
> enabled at once. So, something like
> 
> /*
>  * For A64FX SVE should be enabled by default with the vectors
>  * sve128, sve256, and sve512
>  */
>  assert_has_feature_enabled(qts, "a64fx", "sve");  assert_sve_vls(qts, "a64fx",
> 0xb, NULL);
> 
> /*
>  * A64FX does not support any other vector lengths besides those
>  * that are enabled by default.
>  */
>  assert_error(qts, "a64fx", "cannot enable sve384", "{ 'sve384': true }");
> assert_error(qts, "a64fx", "cannot enable sve640", "{ 'sve640': true }");
> 
> 
> You could also test that completely disabling sve works and that disabling
> sve256 (which also disables sve512) and/or only test disabling sve512 works, but
> that would only be testing the same code covered by tests with the 'max' cpu type,
> since that code is shared.
> 
> 
> >
> >          sve_tests_default(qts, "max");
> >          pauth_tests_default(qts, "max");
> > ----------------------------------------------------------------------
> > --------------------------
> >
> > By the way, There is one thing that bothers me about the above modification.
> > The A64FX supports only 128-bit, 256-bit, and 512-bit SVE vector
> > lengths, but the The following process in the arm_cpu_sve_finalize() function
> sets all bits of cpu->sve_vq_map to 1.
> >
> > /* Set all bits not explicitly set within sve-max-vq. */
> > bitmap_complement(tmp, cpu->sve_vq_init, max_vq);
> > bitmap_or(cpu->sve_vq_map, cpu->sve_vq_map, tmp, max_vq);
> >
> > As a result, enter a routine where the following process will be executed.
> >
> > error_setg(errp, "cannot set sve-max-vq=%d", cpu->sve_max_vq);
> 
> This is the correct behavior and the rest of the error message explains why
> 
>  error_append_hint(errp, "This CPU does not support "
>                    "the vector length %d-bits.\n", vq * 128);
> 
> In A64FX's case that will point out the vector length 384-bits, which indeed A64FX
> does not support.
> 
> As I've stated a few different times, the sve-max-vq property is of marginal use, as
> it only works for CPU models that support all vector lengths, including
> non-power-of-2 lengths. This is because the property says to enable all vector
> lengths smaller and including sve-max-vq and to disable all the rest, but if the
> CPU doesn't support non-power-of-2 vector lengths, then sve-max-vq will not
> work for anything other than max-vq=1 and max-vq=2. For this reason, I didn't
> even bring this property over to the 'host' cpu type. I think I once suggested that
> you don't bring it over to A64FX either and I'll suggest that again.
> 
> >
> > Therefore, We have applied the following fix, is this ok?
> 
> No :-)
> 
> Thanks,
> drew
> 
> > ----------------------------------------------------------------------
> > --------------------------
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1350,10 +1350,12 @@ void arm_cpu_finalize_features(ARMCPU *cpu,
> Error **errp)
> >      Error *local_err = NULL;
> >
> >      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> > -        arm_cpu_sve_finalize(cpu, &local_err);
> > -        if (local_err != NULL) {
> > -            error_propagate(errp, local_err);
> > -            return;
> > +        if (!arm_feature(&cpu->env, ARM_FEATURE_A64FX)) {
> > +            arm_cpu_sve_finalize(cpu, &local_err);
> > +            if (local_err != NULL) {
> > +                error_propagate(errp, local_err);
> > +                return;
> > +            }
> >          }
> > ----------------------------------------------------------------------
> > --------------------------
> >
> > Please your comments if we have misunderstood.
> > Best regards.
> >
> > > -----Original Message-----
> > > From: Andrew Jones <drjones@redhat.com>
> > > Sent: Tuesday, August 24, 2021 1:07 AM
> > > To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > > Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> > > richard.henderson@linaro.org; peter.maydell@linaro.org;
> > > philmd@redhat.com
> > > Subject: [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported
> > > bitmap
> > >
> > > v2:
> > >  - Completed testing
> > >  - Removed extra space in an error message
> > >  - Added Phil's r-b's
> > >
> > > While reviewing the new A64FX CPU type it became clear that CPU
> > > types should be able to specify which SVE vector lengths are
> > > supported. This series adds a new bitmap member to ARMCPU and
> > > modifies arm_cpu_sve_finalize() to validate inputs against it.
> > > So far we only need to set the bitmap for the 'max' CPU type though
> > > and, since it supports all vector lengths, we just fill the whole thing.
> > >
> > > This series was inspired by Richard Henderson's suggestion to
> > > replace arm_cpu_sve_finalize's kvm_supported bitmap with something
> > > that could be shared with TCG.
> > >
> > > Thanks,
> > > drew
> > >
> > >
> > > Andrew Jones (4):
> > >   target/arm/cpu: Introduce sve_vq_supported bitmap
> > >   target/arm/kvm64: Ensure sve vls map is completely clear
> > >   target/arm/cpu64: Replace kvm_supported with sve_vq_supported
> > >   target/arm/cpu64: Validate sve vector lengths are supported
> > >
> > >  target/arm/cpu.h   |   4 ++
> > >  target/arm/cpu64.c | 118
> > > +++++++++++++++++++++------------------------
> > >  target/arm/kvm64.c |   2 +-
> > >  3 files changed, 61 insertions(+), 63 deletions(-)
> > >
> > > --
> > > 2.31.1
> >


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-08-30  4:52 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23 16:06 [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Andrew Jones
2021-08-23 16:06 ` [PATCH v2 1/4] " Andrew Jones
2021-08-23 17:43   ` Richard Henderson
2021-08-23 16:06 ` [PATCH v2 2/4] target/arm/kvm64: Ensure sve vls map is completely clear Andrew Jones
2021-08-23 17:45   ` Richard Henderson
2021-08-23 16:06 ` [PATCH v2 3/4] target/arm/cpu64: Replace kvm_supported with sve_vq_supported Andrew Jones
2021-08-23 17:53   ` Richard Henderson
2021-08-24  6:28     ` Andrew Jones
2021-08-24  6:47       ` Andrew Jones
2021-08-23 16:06 ` [PATCH v2 4/4] target/arm/cpu64: Validate sve vector lengths are supported Andrew Jones
2021-08-23 18:04   ` Richard Henderson
2021-08-24  6:28     ` Andrew Jones
2021-08-26 13:22 ` [PATCH v2 0/4] target/arm/cpu: Introduce sve_vq_supported bitmap Peter Maydell
2021-08-27  8:30 ` ishii.shuuichir
2021-08-27  8:57   ` Peter Maydell
2021-08-30  0:02     ` ishii.shuuichir
2021-08-27 10:03   ` Andrew Jones
2021-08-30  4:49     ` ishii.shuuichir

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.