[Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

With the recent KVM guest SVE support pull request [1] KVM will be
ready for guests with SVE. This series provides the QEMU bits for
that enablement. The series starts with the bits needed for the KVM
SVE ioctls. Then it enables the arm 'max'cpu type, which with TCG
already supports SVE, to also support SVE when using KVM. Next
a new QMP query is added that allows users to ask which vector
lengths are supported by the host, allowing them to select a valid
set of vectors for the guest. In order to select those vectors a
new property 'sve-vls-map' is added to the 'max' cpu type, and then
also to the 'host' cpu type. The table below shows the resulting user
interfaces.

   CPU type | accel | sve-max-vq | sve-vls-map
   -------------------------------------------
 1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
 2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
 3)    host | kvm   |  N/A       |  $VLS_MAP

Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
all supported when $VLS_MAP is zero, or when the vqs are selected
in $VLS_MAP.

(2) is the same as (1) except KVM has the final say on what
vqs are valid.

(3) doesn't accept sve-max-vq because a guest that uses this
property without sve-vls-map cannot be safely migrated.

There is never any need to provide both properties, but if both
are provided then they are checked for consistency.

The QMP query returns a list of valid vq lists. For example, if
a guest can use vqs 1, 2, 3, and 4, then the following list will
be returned

 [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]

Another example might be 1, 2, 4, as the architecture states 3
is optional. In that case the list would be

 [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]

This may look redundant, but it's necessary to provide a future-
proof query, because while KVM currently requires vector sets to
be strict truncations of the full valid vector set, that may change
at some point. Additionally, TCG doesn't have this restriction so
more vector sets can be returned that aren't so easily derived from
the full set already. (Note, this series doesn't do that yet. See
the TODO below.)

And now for what might be a bit more controversial; how we input
the valid vector set with sve-vls-map. Well, sve-vls-map is a
64-bit bitmap, which is admittedly not user friendly and also not
the same size as KVM's vls bitmap (which is 8 64-bit words). Here's
the justification:

 1) We want to use the QEMU command line in order for the information
    to be migrated without needing to add more VM state.
 2) It's not easy/pretty to input arrays on the QEMU command line.
 3) We already need to use the QMP query to get a valid set, which
    isn't user friendly either, meaning we're already in libvirt
    territory.
 4) A 64-bit map (supporting up to 8192-bit vectors) is likely big
    enough for quite some time (currently KVM and TCG only support
    2048-bit vectors).
 5) If user friendliness is needed more than migratability then
    the 'max' cpu type can be used with the sve-max-vq property.
 6) It's possible to probe the full valid vector set from the
    command line by using something like sve-vls-map=0xffff and
    then, when it fails, the error message will state the correct
    map, e.g. 0xb.

TODO:
 1) More testing. Initial testing looks good, and I'm doing more
    now, but wanted to get the series out for review in parallel.
 2) Extension to target/arm/arch_dump.c for SVE state. I'll try
    to get to this early next week.
 3) Modify the QMP query to output all valid vector sets for
    TCG, rather than just the ones derived by truncation as
    is required by KVM.

[1] https://www.spinics.net/lists/kvm-arm/msg35844.html

Thanks!

Andrew Jones (13):
  target/arm/kvm64: fix error returns
  update-linux-headers: Add sve_context.h to asm-arm64
  HACK: linux header update
  target/arm/kvm: Move the get/put of fpsimd registers out
  target/arm/kvm: Add kvm_arch_get/put_sve
  target/arm/kvm: max cpu: Enable SVE when available
  target/arm/kvm: max cpu: Allow sve max vector length setting
  target/arm/monitor: Add query-sve-vector-lengths
  target/arm/kvm: Export kvm_arm_get_sve_vls
  target/arm/monitor: kvm: only return valid sve vector sets
  target/arm/cpu64: max cpu: Introduce sve-vls-map
  target/arm/kvm: max cpu: Add support for sve-vls-map
  target/arm/kvm: host cpu: Add support for sve-vls-map

 linux-headers/asm-arm64/kvm.h         |  41 +++
 linux-headers/asm-arm64/sve_context.h |  53 ++++
 linux-headers/linux/kvm.h             |   5 +
 qapi/target.json                      |  34 +++
 scripts/update-linux-headers.sh       |   3 +
 target/arm/cpu.c                      |   5 +
 target/arm/cpu.h                      |   9 +
 target/arm/cpu64.c                    |  81 ++++-
 target/arm/helper.c                   |  10 +-
 target/arm/kvm.c                      |   5 +
 target/arm/kvm64.c                    | 418 ++++++++++++++++++++++----
 target/arm/kvm_arm.h                  |  32 ++
 target/arm/monitor.c                  | 106 +++++++
 tests/qmp-cmd-test.c                  |   1 +
 14 files changed, 732 insertions(+), 71 deletions(-)
 create mode 100644 linux-headers/asm-arm64/sve_context.h

-- 
2.20.1

[Qemu-devel] [PATCH 01/13] target/arm/kvm64: fix error returns

[Andrew Jones]

A couple return -EINVAL's forget their '-'s.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/kvm64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e3ba1492482f..ba232b27a6d3 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -841,7 +841,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     write_cpustate_to_list(cpu, true);
 
     if (!write_list_to_kvmstate(cpu, level)) {
-        return EINVAL;
+        return -EINVAL;
     }
 
     kvm_arm_sync_mpstate_to_kvm(cpu);
@@ -982,7 +982,7 @@ int kvm_arch_get_registers(CPUState *cs)
     }
 
     if (!write_kvmstate_to_list(cpu)) {
-        return EINVAL;
+        return -EINVAL;
     }
     /* Note that it's OK to have registers which aren't in CPUState,
      * so we can ignore a failure return here.
-- 
2.20.1

Re: [Qemu-devel] [PATCH 01/13] target/arm/kvm64: fix error returns

[Auger Eric]

Hi,

On 5/12/19 10:36 AM, Andrew Jones wrote:
> A couple return -EINVAL's forget their '-'s.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/kvm64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index e3ba1492482f..ba232b27a6d3 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -841,7 +841,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      write_cpustate_to_list(cpu, true);
>  
>      if (!write_list_to_kvmstate(cpu, level)) {
> -        return EINVAL;
> +        return -EINVAL;
>      }
>  
>      kvm_arm_sync_mpstate_to_kvm(cpu);
> @@ -982,7 +982,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      }
>  
>      if (!write_kvmstate_to_list(cpu)) {
> -        return EINVAL;
> +        return -EINVAL;
>      }
>      /* Note that it's OK to have registers which aren't in CPUState,
>       * so we can ignore a failure return here.
> 
note, if I am not wrong, in existing call sites the returned value is
never tested actually.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

[Qemu-devel] [PATCH 02/13] update-linux-headers: Add sve_context.h to asm-arm64

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 scripts/update-linux-headers.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index c3819d2b983d..e1fce54f8aa3 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -99,6 +99,9 @@ for arch in $ARCHLIST; do
         cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch"
     done
 
+    if [ $arch = arm64 ]; then
+        cp "$tmpdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/"
+    fi
     if [ $arch = mips ]; then
         cp "$tmpdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/"
         cp "$tmpdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/"
-- 
2.20.1

Re: [Qemu-devel] [PATCH 02/13] update-linux-headers: Add sve_context.h to asm-arm64

[Auger Eric]

Hi,

On 5/12/19 10:36 AM, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  scripts/update-linux-headers.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index c3819d2b983d..e1fce54f8aa3 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -99,6 +99,9 @@ for arch in $ARCHLIST; do
>          cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch"
>      done
>  
> +    if [ $arch = arm64 ]; then
> +        cp "$tmpdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/"
> +    fi
>      if [ $arch = mips ]; then
>          cp "$tmpdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/"
>          cp "$tmpdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/"
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [Qemu-devel] [PATCH 02/13] update-linux-headers: Add sve_context.h to asm-arm64

[Andrew Jones]

On Wed, Jun 05, 2019 at 09:21:18AM +0200, Auger Eric wrote:
> Hi,
> 
> On 5/12/19 10:36 AM, Andrew Jones wrote:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  scripts/update-linux-headers.sh | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> > index c3819d2b983d..e1fce54f8aa3 100755
> > --- a/scripts/update-linux-headers.sh
> > +++ b/scripts/update-linux-headers.sh
> > @@ -99,6 +99,9 @@ for arch in $ARCHLIST; do
> >          cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch"
> >      done
> >  
> > +    if [ $arch = arm64 ]; then
> > +        cp "$tmpdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/"
> > +    fi
> >      if [ $arch = mips ]; then
> >          cp "$tmpdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/"
> >          cp "$tmpdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/"
> > 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>

Thanks, but I noticed we don't need this anymore. Not since b1b9e0dc78a8e
applied an identical patch. I've dropped this from the v2 I'm working on
now.

Thanks,
drew

[Qemu-devel] [PATCH 03/13] HACK: linux header update

[Andrew Jones]

---
 linux-headers/asm-arm64/kvm.h         | 41 +++++++++++++++++++++
 linux-headers/asm-arm64/sve_context.h | 53 +++++++++++++++++++++++++++
 linux-headers/linux/kvm.h             |  5 +++
 3 files changed, 99 insertions(+)
 create mode 100644 linux-headers/asm-arm64/sve_context.h

diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index e6a98c14c848..3e73a4c67b67 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -35,6 +35,7 @@
 #include <linux/psci.h>
 #include <linux/types.h>
 #include <asm/ptrace.h>
+#include <asm/sve_context.h>
 
 #define __KVM_HAVE_GUEST_DEBUG
 #define __KVM_HAVE_IRQ_LINE
@@ -102,6 +103,7 @@ struct kvm_regs {
 #define KVM_ARM_VCPU_EL1_32BIT		1 /* CPU running a 32bit VM */
 #define KVM_ARM_VCPU_PSCI_0_2		2 /* CPU uses PSCI v0.2 */
 #define KVM_ARM_VCPU_PMU_V3		3 /* Support guest PMUv3 */
+#define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
 
 struct kvm_vcpu_init {
 	__u32 target;
@@ -226,6 +228,45 @@ struct kvm_vcpu_events {
 					 KVM_REG_ARM_FW | ((r) & 0xffff))
 #define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
 
+/* SVE registers */
+#define KVM_REG_ARM64_SVE		(0x15 << KVM_REG_ARM_COPROC_SHIFT)
+
+/* Z- and P-regs occupy blocks at the following offsets within this range: */
+#define KVM_REG_ARM64_SVE_ZREG_BASE	0
+#define KVM_REG_ARM64_SVE_PREG_BASE	0x400
+#define KVM_REG_ARM64_SVE_FFR_BASE	0x600
+
+#define KVM_ARM64_SVE_NUM_ZREGS		__SVE_NUM_ZREGS
+#define KVM_ARM64_SVE_NUM_PREGS		__SVE_NUM_PREGS
+
+#define KVM_ARM64_SVE_MAX_SLICES	32
+
+#define KVM_REG_ARM64_SVE_ZREG(n, i)					\
+	(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | KVM_REG_ARM64_SVE_ZREG_BASE | \
+	 KVM_REG_SIZE_U2048 |						\
+	 (((n) & (KVM_ARM64_SVE_NUM_ZREGS - 1)) << 5) |			\
+	 ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1)))
+
+#define KVM_REG_ARM64_SVE_PREG(n, i)					\
+	(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | KVM_REG_ARM64_SVE_PREG_BASE | \
+	 KVM_REG_SIZE_U256 |						\
+	 (((n) & (KVM_ARM64_SVE_NUM_PREGS - 1)) << 5) |			\
+	 ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1)))
+
+#define KVM_REG_ARM64_SVE_FFR(i)					\
+	(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | KVM_REG_ARM64_SVE_FFR_BASE | \
+	 KVM_REG_SIZE_U256 |						\
+	 ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1)))
+
+#define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
+#define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
+
+/* Vector lengths pseudo-register: */
+#define KVM_REG_ARM64_SVE_VLS		(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
+					 KVM_REG_SIZE_U512 | 0xffff)
+#define KVM_ARM64_SVE_VLS_WORDS	\
+	((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1)
+
 /* Device Control API: ARM VGIC */
 #define KVM_DEV_ARM_VGIC_GRP_ADDR	0
 #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS	1
diff --git a/linux-headers/asm-arm64/sve_context.h b/linux-headers/asm-arm64/sve_context.h
new file mode 100644
index 000000000000..1d0e3e1d0950
--- /dev/null
+++ b/linux-headers/asm-arm64/sve_context.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* Copyright (C) 2017-2018 ARM Limited */
+
+/*
+ * For use by other UAPI headers only.
+ * Do not make direct use of header or its definitions.
+ */
+
+#ifndef __ASM_SVE_CONTEXT_H
+#define __ASM_SVE_CONTEXT_H
+
+#include <linux/types.h>
+
+#define __SVE_VQ_BYTES		16	/* number of bytes per quadword */
+
+#define __SVE_VQ_MIN		1
+#define __SVE_VQ_MAX		512
+
+#define __SVE_VL_MIN		(__SVE_VQ_MIN * __SVE_VQ_BYTES)
+#define __SVE_VL_MAX		(__SVE_VQ_MAX * __SVE_VQ_BYTES)
+
+#define __SVE_NUM_ZREGS		32
+#define __SVE_NUM_PREGS		16
+
+#define __sve_vl_valid(vl)			\
+	((vl) % __SVE_VQ_BYTES == 0 &&		\
+	 (vl) >= __SVE_VL_MIN &&		\
+	 (vl) <= __SVE_VL_MAX)
+
+#define __sve_vq_from_vl(vl)	((vl) / __SVE_VQ_BYTES)
+#define __sve_vl_from_vq(vq)	((vq) * __SVE_VQ_BYTES)
+
+#define __SVE_ZREG_SIZE(vq)	((__u32)(vq) * __SVE_VQ_BYTES)
+#define __SVE_PREG_SIZE(vq)	((__u32)(vq) * (__SVE_VQ_BYTES / 8))
+#define __SVE_FFR_SIZE(vq)	__SVE_PREG_SIZE(vq)
+
+#define __SVE_ZREGS_OFFSET	0
+#define __SVE_ZREG_OFFSET(vq, n) \
+	(__SVE_ZREGS_OFFSET + __SVE_ZREG_SIZE(vq) * (n))
+#define __SVE_ZREGS_SIZE(vq) \
+	(__SVE_ZREG_OFFSET(vq, __SVE_NUM_ZREGS) - __SVE_ZREGS_OFFSET)
+
+#define __SVE_PREGS_OFFSET(vq) \
+	(__SVE_ZREGS_OFFSET + __SVE_ZREGS_SIZE(vq))
+#define __SVE_PREG_OFFSET(vq, n) \
+	(__SVE_PREGS_OFFSET(vq) + __SVE_PREG_SIZE(vq) * (n))
+#define __SVE_PREGS_SIZE(vq) \
+	(__SVE_PREG_OFFSET(vq, __SVE_NUM_PREGS) - __SVE_PREGS_OFFSET(vq))
+
+#define __SVE_FFR_OFFSET(vq) \
+	(__SVE_PREGS_OFFSET(vq) + __SVE_PREGS_SIZE(vq))
+
+#endif /* ! _UAPI__ASM_SVE_CONTEXT_H */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index b53ee5974802..1a639ae0ceed 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -988,6 +988,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_ARM_VM_IPA_SIZE 165
 #define KVM_CAP_MANUAL_DIRTY_LOG_PROTECT 166
 #define KVM_CAP_HYPERV_CPUID 167
+#define KVM_CAP_ARM_SVE 168
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1145,6 +1146,7 @@ struct kvm_dirty_tlb {
 #define KVM_REG_SIZE_U256	0x0050000000000000ULL
 #define KVM_REG_SIZE_U512	0x0060000000000000ULL
 #define KVM_REG_SIZE_U1024	0x0070000000000000ULL
+#define KVM_REG_SIZE_U2048	0x0080000000000000ULL
 
 struct kvm_reg_list {
 	__u64 n; /* number of regs */
@@ -1440,6 +1442,9 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_HYPERV_CPUID */
 #define KVM_GET_SUPPORTED_HV_CPUID _IOWR(KVMIO, 0xc1, struct kvm_cpuid2)
 
+/* Available with KVM_CAP_ARM_SVE */
+#define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */
-- 
2.20.1

[Qemu-devel] [PATCH 04/13] target/arm/kvm: Move the get/put of fpsimd registers out

[Andrew Jones]

Move the getting/putting of the fpsimd registers out of
kvm_arch_get/put_registers() into their own helper functions
to prepare for alternatively getting/putting SVE registers.

No functional change.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/kvm64.c | 148 +++++++++++++++++++++++++++------------------
 1 file changed, 88 insertions(+), 60 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index ba232b27a6d3..61947f3716e1 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -706,13 +706,53 @@ int kvm_arm_cpreg_level(uint64_t regidx)
 #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
                  KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
 
+static int kvm_arch_put_fpsimd(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    uint32_t fpr;
+    int i, ret;
+
+    for (i = 0; i < 32; i++) {
+        uint64_t *q = aa64_vfp_qreg(env, i);
+#ifdef HOST_WORDS_BIGENDIAN
+        uint64_t fp_val[2] = { q[1], q[0] };
+        reg.addr = (uintptr_t)fp_val;
+#else
+        reg.addr = (uintptr_t)q;
+#endif
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    fpr = vfp_get_fpsr(env);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    fpr = vfp_get_fpcr(env);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    return 0;
+}
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     struct kvm_one_reg reg;
-    uint32_t fpr;
     uint64_t val;
-    int i;
-    int ret;
+    int i, ret;
     unsigned int el;
 
     ARMCPU *cpu = ARM_CPU(cs);
@@ -802,33 +842,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
-    /* Advanced SIMD and FP registers. */
-    for (i = 0; i < 32; i++) {
-        uint64_t *q = aa64_vfp_qreg(env, i);
-#ifdef HOST_WORDS_BIGENDIAN
-        uint64_t fp_val[2] = { q[1], q[0] };
-        reg.addr = (uintptr_t)fp_val;
-#else
-        reg.addr = (uintptr_t)q;
-#endif
-        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
-        if (ret) {
-            return ret;
-        }
-    }
-
-    reg.addr = (uintptr_t)(&fpr);
-    fpr = vfp_get_fpsr(env);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
-    if (ret) {
-        return ret;
-    }
-
-    fpr = vfp_get_fpcr(env);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_arch_put_fpsimd(cs);
     if (ret) {
         return ret;
     }
@@ -849,14 +863,54 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     return ret;
 }
 
+static int kvm_arch_get_fpsimd(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    uint32_t fpr;
+    int i, ret;
+
+    for (i = 0; i < 32; i++) {
+        uint64_t *q = aa64_vfp_qreg(env, i);
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)q;
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        } else {
+#ifdef HOST_WORDS_BIGENDIAN
+            uint64_t t;
+            t = q[0], q[0] = q[1], q[1] = t;
+#endif
+        }
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+    vfp_set_fpsr(env, fpr);
+
+    reg.addr = (uintptr_t)(&fpr);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+    vfp_set_fpcr(env, fpr);
+
+    return 0;
+}
+
 int kvm_arch_get_registers(CPUState *cs)
 {
     struct kvm_one_reg reg;
     uint64_t val;
-    uint32_t fpr;
     unsigned int el;
-    int i;
-    int ret;
+    int i, ret;
 
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -945,36 +999,10 @@ int kvm_arch_get_registers(CPUState *cs)
         env->spsr = env->banked_spsr[i];
     }
 
-    /* Advanced SIMD and FP registers */
-    for (i = 0; i < 32; i++) {
-        uint64_t *q = aa64_vfp_qreg(env, i);
-        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-        reg.addr = (uintptr_t)q;
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
-        if (ret) {
-            return ret;
-        } else {
-#ifdef HOST_WORDS_BIGENDIAN
-            uint64_t t;
-            t = q[0], q[0] = q[1], q[1] = t;
-#endif
-        }
-    }
-
-    reg.addr = (uintptr_t)(&fpr);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
-    if (ret) {
-        return ret;
-    }
-    vfp_set_fpsr(env, fpr);
-
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_arch_get_fpsimd(cs);
     if (ret) {
         return ret;
     }
-    vfp_set_fpcr(env, fpr);
 
     ret = kvm_get_vcpu_events(cpu);
     if (ret) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 04/13] target/arm/kvm: Move the get/put of fpsimd registers out

[Auger Eric]

Hi Drew,

On 5/12/19 10:36 AM, Andrew Jones wrote:
> Move the getting/putting of the fpsimd registers out of
> kvm_arch_get/put_registers() into their own helper functions
> to prepare for alternatively getting/putting SVE registers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/kvm64.c | 148 +++++++++++++++++++++++++++------------------
>  1 file changed, 88 insertions(+), 60 deletions(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index ba232b27a6d3..61947f3716e1 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -706,13 +706,53 @@ int kvm_arm_cpreg_level(uint64_t regidx)
>  #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
>                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>  
> +static int kvm_arch_put_fpsimd(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    uint32_t fpr;
> +    int i, ret;
> +
> +    for (i = 0; i < 32; i++) {
> +        uint64_t *q = aa64_vfp_qreg(env, i);
> +#ifdef HOST_WORDS_BIGENDIAN
> +        uint64_t fp_val[2] = { q[1], q[0] };
> +        reg.addr = (uintptr_t)fp_val;
> +#else
> +        reg.addr = (uintptr_t)q;
> +#endif
> +        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    reg.addr = (uintptr_t)(&fpr);
> +    fpr = vfp_get_fpsr(env);
> +    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    reg.addr = (uintptr_t)(&fpr);
I don't think you need this assignment
> +    fpr = vfp_get_fpcr(env);
> +    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>  int kvm_arch_put_registers(CPUState *cs, int level)
>  {
>      struct kvm_one_reg reg;
> -    uint32_t fpr;
>      uint64_t val;
> -    int i;
> -    int ret;
> +    int i, ret;
>      unsigned int el;
>  
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -802,33 +842,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          }
>      }
>  
> -    /* Advanced SIMD and FP registers. */
> -    for (i = 0; i < 32; i++) {
> -        uint64_t *q = aa64_vfp_qreg(env, i);
> -#ifdef HOST_WORDS_BIGENDIAN
> -        uint64_t fp_val[2] = { q[1], q[0] };
> -        reg.addr = (uintptr_t)fp_val;
> -#else
> -        reg.addr = (uintptr_t)q;
> -#endif
> -        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> -        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> -        if (ret) {
> -            return ret;
> -        }
> -    }
> -
> -    reg.addr = (uintptr_t)(&fpr);
> -    fpr = vfp_get_fpsr(env);
> -    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> -    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    fpr = vfp_get_fpcr(env);
> -    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
> -    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    ret = kvm_arch_put_fpsimd(cs);
>      if (ret) {
>          return ret;
>      }
> @@ -849,14 +863,54 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      return ret;
>  }
>  
> +static int kvm_arch_get_fpsimd(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    uint32_t fpr;
> +    int i, ret;
> +
> +    for (i = 0; i < 32; i++) {
> +        uint64_t *q = aa64_vfp_qreg(env, i);
> +        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> +        reg.addr = (uintptr_t)q;
> +        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +        if (ret) {
> +            return ret;
> +        } else {
> +#ifdef HOST_WORDS_BIGENDIAN
> +            uint64_t t;
> +            t = q[0], q[0] = q[1], q[1] = t;
> +#endif
> +        }
> +    }
> +
> +    reg.addr = (uintptr_t)(&fpr);
> +    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +    vfp_set_fpsr(env, fpr);
> +
> +    reg.addr = (uintptr_t)(&fpr);
same here
> +    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +    vfp_set_fpcr(env, fpr);
> +
> +    return 0;
> +}
> +
>  int kvm_arch_get_registers(CPUState *cs)
>  {
>      struct kvm_one_reg reg;
>      uint64_t val;
> -    uint32_t fpr;
>      unsigned int el;
> -    int i;
> -    int ret;
> +    int i, ret;
>  
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -945,36 +999,10 @@ int kvm_arch_get_registers(CPUState *cs)
>          env->spsr = env->banked_spsr[i];
>      }
>  
> -    /* Advanced SIMD and FP registers */
> -    for (i = 0; i < 32; i++) {
> -        uint64_t *q = aa64_vfp_qreg(env, i);
> -        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> -        reg.addr = (uintptr_t)q;
> -        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> -        if (ret) {
> -            return ret;
> -        } else {
> -#ifdef HOST_WORDS_BIGENDIAN
> -            uint64_t t;
> -            t = q[0], q[0] = q[1], q[1] = t;
> -#endif
> -        }
> -    }
> -
> -    reg.addr = (uintptr_t)(&fpr);
> -    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> -    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> -    if (ret) {
> -        return ret;
> -    }
> -    vfp_set_fpsr(env, fpr);
> -
> -    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
> -    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    ret = kvm_arch_get_fpsimd(cs);
>      if (ret) {
>          return ret;
>      }
> -    vfp_set_fpcr(env, fpr);
>  
>      ret = kvm_get_vcpu_events(cpu);
>      if (ret) {
> 

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [Qemu-devel] [PATCH 04/13] target/arm/kvm: Move the get/put of fpsimd registers out

[Andrew Jones]

On Wed, Jun 05, 2019 at 09:15:49AM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 5/12/19 10:36 AM, Andrew Jones wrote:
> > Move the getting/putting of the fpsimd registers out of
> > kvm_arch_get/put_registers() into their own helper functions
> > to prepare for alternatively getting/putting SVE registers.
> > 
> > No functional change.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/kvm64.c | 148 +++++++++++++++++++++++++++------------------
> >  1 file changed, 88 insertions(+), 60 deletions(-)
> > 
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index ba232b27a6d3..61947f3716e1 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -706,13 +706,53 @@ int kvm_arm_cpreg_level(uint64_t regidx)
> >  #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
> >                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
> >  
> > +static int kvm_arch_put_fpsimd(CPUState *cs)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    uint32_t fpr;
> > +    int i, ret;
> > +
> > +    for (i = 0; i < 32; i++) {
> > +        uint64_t *q = aa64_vfp_qreg(env, i);
> > +#ifdef HOST_WORDS_BIGENDIAN
> > +        uint64_t fp_val[2] = { q[1], q[0] };
> > +        reg.addr = (uintptr_t)fp_val;
> > +#else
> > +        reg.addr = (uintptr_t)q;
> > +#endif
> > +        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> > +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +        if (ret) {
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    reg.addr = (uintptr_t)(&fpr);
> > +    fpr = vfp_get_fpsr(env);
> > +    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
> > +    reg.addr = (uintptr_t)(&fpr);
> I don't think you need this assignment

You're right, and they weren't in the original code, but I added them
in order to be sure that the register get/set blocks were complete
units. It's one thing to factor stuff like that out of a loop, but
here we had two almost independent blocks so it looked a bit odd.

[...]

> 
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>

Thanks,
drew 

[Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Andrew Jones]

These are the SVE equivalents to kvm_arch_get/put_fpsimd.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 4 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 61947f3716e1..86362f4cd7d0 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -658,11 +658,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
 bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
 {
     /* Return true if the regidx is a register we should synchronize
-     * via the cpreg_tuples array (ie is not a core reg we sync by
-     * hand in kvm_arch_get/put_registers())
+     * via the cpreg_tuples array (ie is not a core or sve reg that
+     * we sync by hand in kvm_arch_get/put_registers())
      */
     switch (regidx & KVM_REG_ARM_COPROC_MASK) {
     case KVM_REG_ARM_CORE:
+    case KVM_REG_ARM64_SVE:
         return false;
     default:
         return true;
@@ -748,6 +749,61 @@ static int kvm_arch_put_fpsimd(CPUState *cs)
     return 0;
 }
 
+static int kvm_arch_put_sve(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int n, ret;
+
+    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
+        uint64_t *q = aa64_vfp_qreg(env, n);
+#ifdef HOST_WORDS_BIGENDIAN
+        uint64_t d[ARM_MAX_VQ * 2];
+        int i;
+        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
+            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
+        }
+        reg.addr = (uintptr_t)d;
+#else
+        reg.addr = (uintptr_t)q;
+#endif
+        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    for (n = 0; n < KVM_ARM64_SVE_NUM_PREGS; n++) {
+        uint64_t *p = &env->vfp.pregs[n].p[0];
+#ifdef HOST_WORDS_BIGENDIAN
+        uint64_t d[ARM_MAX_VQ * 2];
+        int i;
+        for (i = 0; i < cpu->sve_max_vq * 2 / 8; i++) {
+            d[i] = p[cpu->sve_max_vq * 2 / 8 - 1 - i];
+        }
+        reg.addr = (uintptr_t)d;
+#else
+        reg.addr = (uintptr_t)p;
+#endif
+        reg.id = KVM_REG_ARM64_SVE_PREG(n, 0);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    reg.addr = (uintptr_t)&env->vfp.pregs[FFR_PRED_NUM].p[0];
+    reg.id = KVM_REG_ARM64_SVE_FFR(0);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    return 0;
+}
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     struct kvm_one_reg reg;
@@ -842,7 +898,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
-    ret = kvm_arch_put_fpsimd(cs);
+    if (!cpu_isar_feature(aa64_sve, cpu)) {
+        ret = kvm_arch_put_fpsimd(cs);
+    } else {
+        ret = kvm_arch_put_sve(cs);
+    }
     if (ret) {
         return ret;
     }
@@ -905,6 +965,61 @@ static int kvm_arch_get_fpsimd(CPUState *cs)
     return 0;
 }
 
+static int kvm_arch_get_sve(CPUState *cs)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int n, ret;
+
+    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
+        uint64_t *q = aa64_vfp_qreg(env, n);
+        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);
+        reg.addr = (uintptr_t)q;
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        } else {
+#ifdef HOST_WORDS_BIGENDIAN
+            int i = 0, j = cpu->sve_max_vq * 2 - 1;
+            while (i < j) {
+                uint64_t t;
+                t = q[i], q[i] = q[j], q[j] = t;
+                ++i, --j;
+            }
+#endif
+        }
+    }
+
+    for (n = 0; n < KVM_ARM64_SVE_NUM_PREGS; n++) {
+        uint64_t *p = &env->vfp.pregs[n].p[0];
+        reg.id = KVM_REG_ARM64_SVE_PREG(n, 0);
+        reg.addr = (uintptr_t)p;
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        } else {
+#ifdef HOST_WORDS_BIGENDIAN
+            int i = 0, j = cpu->sve_max_vq * 2 / 8 - 1;
+            while (i < j) {
+                uint64_t t;
+                t = q[i], q[i] = q[j], q[j] = t;
+                ++i, --j;
+            }
+#endif
+        }
+    }
+
+    reg.addr = (uintptr_t)&env->vfp.pregs[FFR_PRED_NUM].p[0];
+    reg.id = KVM_REG_ARM64_SVE_FFR(0);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    return 0;
+}
+
 int kvm_arch_get_registers(CPUState *cs)
 {
     struct kvm_one_reg reg;
@@ -999,7 +1114,11 @@ int kvm_arch_get_registers(CPUState *cs)
         env->spsr = env->banked_spsr[i];
     }
 
-    ret = kvm_arch_get_fpsimd(cs);
+    if (!cpu_isar_feature(aa64_sve, cpu)) {
+        ret = kvm_arch_get_fpsimd(cs);
+    } else {
+        ret = kvm_arch_get_sve(cs);
+    }
     if (ret) {
         return ret;
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Dave Martin]

On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 123 insertions(+), 4 deletions(-)

[...]

> +static int kvm_arch_put_sve(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int n, ret;
> +
> +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> +        uint64_t *q = aa64_vfp_qreg(env, n);
> +#ifdef HOST_WORDS_BIGENDIAN
> +        uint64_t d[ARM_MAX_VQ * 2];
> +        int i;
> +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> +        }

Out of interest, why do all this swabbing?  It seems expensive.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Andrew Jones]

On Mon, May 13, 2019 at 01:31:11PM +0100, Dave Martin wrote:
> On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> > These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 123 insertions(+), 4 deletions(-)
> 
> [...]
> 
> > +static int kvm_arch_put_sve(CPUState *cs)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    int n, ret;
> > +
> > +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> > +        uint64_t *q = aa64_vfp_qreg(env, n);
> > +#ifdef HOST_WORDS_BIGENDIAN
> > +        uint64_t d[ARM_MAX_VQ * 2];
> > +        int i;
> > +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> > +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> > +        }
> 
> Out of interest, why do all this swabbing?  It seems expensive.
>

QEMU keeps its 128-bit and larger words in the same order (least
significant word first) for both host endian types. We need to
do word swapping every time we set/get them to/from KVM.

Thanks,
drew

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Dave Martin]

On Mon, May 13, 2019 at 02:55:01PM +0100, Andrew Jones wrote:
> On Mon, May 13, 2019 at 01:31:11PM +0100, Dave Martin wrote:
> > On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> > > These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 123 insertions(+), 4 deletions(-)
> > 
> > [...]
> > 
> > > +static int kvm_arch_put_sve(CPUState *cs)
> > > +{
> > > +    ARMCPU *cpu = ARM_CPU(cs);
> > > +    CPUARMState *env = &cpu->env;
> > > +    struct kvm_one_reg reg;
> > > +    int n, ret;
> > > +
> > > +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> > > +        uint64_t *q = aa64_vfp_qreg(env, n);
> > > +#ifdef HOST_WORDS_BIGENDIAN
> > > +        uint64_t d[ARM_MAX_VQ * 2];
> > > +        int i;
> > > +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> > > +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> > > +        }
> > 
> > Out of interest, why do all this swabbing?  It seems expensive.
> >
> 
> QEMU keeps its 128-bit and larger words in the same order (least
> significant word first) for both host endian types. We need to
> do word swapping every time we set/get them to/from KVM.

I'm not sure whether this is appropriate here, though it depends on
what QEMU does with the data.

Something non-obvious to be aware of:

As exposed through the signal frame and the KVM ABI, the memory
representation of an SVE reg is invariant with respect to the
endianness.

IIUC, the byte order seen for a V-reg in KVM_REG_ARM_CORE and for
the equivalent Z-reg in KVM_REG_ARM64_SVE would be the opposite of each
other on BE, but the same on LE.

This is a feature of the archtiecture: a V-reg can be stored as a single
value, but Z-regs are in general too big to be treated as a single
value: they are always treated as a sequence of elements, and the
largest element size supported is 64 bits, not 128.  IIUC, there is no
direct native way to store with 128-bit swabbing: some explicit data
processing operation would also be needed to swap adjacent 64-bit
elements in the vector around the store/load.

This is not specified in the ABI documentation -- I should address that.

If this is infeasible for KVM to work with, we could perhaps change it,
but I'm not too keen on that at this stage.  KVM_REG_ARM64_SVE_VLS
has a similar behaviour: it's a vector of 64-bit possibly-swabbed words,
not a single possibly-swabbed 512-bit word.


Looking at the kernel, I may have screwed up in places where the
two representations interact, like fpsimd_to_sve().  I should take a
look at that...  This doesn't affect the KVM ABI though.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Peter Maydell]

On Mon, 13 May 2019 at 16:31, Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Mon, May 13, 2019 at 02:55:01PM +0100, Andrew Jones wrote:
> > QEMU keeps its 128-bit and larger words in the same order (least
> > significant word first) for both host endian types. We need to
> > do word swapping every time we set/get them to/from KVM.
>
> I'm not sure whether this is appropriate here, though it depends on
> what QEMU does with the data.

The layout is optimised for TCG emulation to be able
to work with it, I think (rth would have the definite
reason, though).

> Something non-obvious to be aware of:
>
> As exposed through the signal frame and the KVM ABI, the memory
> representation of an SVE reg is invariant with respect to the
> endianness.

Yes; we handle this conversion as we write out the signal frame:
https://github.com/qemu/qemu/blob/master/linux-user/aarch64/signal.c#L184

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Dave Martin]

On Mon, May 13, 2019 at 04:40:52PM +0100, Peter Maydell wrote:
> On Mon, 13 May 2019 at 16:31, Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Mon, May 13, 2019 at 02:55:01PM +0100, Andrew Jones wrote:
> > > QEMU keeps its 128-bit and larger words in the same order (least
> > > significant word first) for both host endian types. We need to
> > > do word swapping every time we set/get them to/from KVM.
> >
> > I'm not sure whether this is appropriate here, though it depends on
> > what QEMU does with the data.
> 
> The layout is optimised for TCG emulation to be able
> to work with it, I think (rth would have the definite
> reason, though).

So long as we are agreed about the ABI, this is none of my concern :)

> > Something non-obvious to be aware of:
> >
> > As exposed through the signal frame and the KVM ABI, the memory
> > representation of an SVE reg is invariant with respect to the
> > endianness.
> 
> Yes; we handle this conversion as we write out the signal frame:
> https://github.com/qemu/qemu/blob/master/linux-user/aarch64/signal.c#L184

Right.  I hadn't focused consciously on this, since the architecture
does the work for us in the kernel (mostly).

I will check the documentation to make sure the behaviour is clearly
described, anyhow.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Richard Henderson]

On 5/13/19 5:31 AM, Dave Martin wrote:
> On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
>> These are the SVE equivalents to kvm_arch_get/put_fpsimd.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> ---
>>  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 123 insertions(+), 4 deletions(-)
> 
> [...]
> 
>> +static int kvm_arch_put_sve(CPUState *cs)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_one_reg reg;
>> +    int n, ret;
>> +
>> +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
>> +        uint64_t *q = aa64_vfp_qreg(env, n);
>> +#ifdef HOST_WORDS_BIGENDIAN
>> +        uint64_t d[ARM_MAX_VQ * 2];
>> +        int i;
>> +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
>> +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
>> +        }
> 
> Out of interest, why do all this swabbing?  It seems expensive.

Indeed, to me this seems to be the wrong kind of swabbing here.  Exactly what
format is KVM expecting?  Surely it should be the one used by the unpredicated
LDR/STR instructions.  Anything else would seem to be working against the
architecture.

If so, the format is, architecturally, a stream of bytes in index order, which
corresponds to a little-endian stream of words.  So the loop I'd expect to see
here is

    for (i = 0, n = cpu->sve_max_vq; i < n; ++i) {
        d[i] = bswap64(q[i]);
    }


r~

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Andrew Jones]

On Mon, May 13, 2019 at 09:40:29AM -0700, Richard Henderson wrote:
> On 5/13/19 5:31 AM, Dave Martin wrote:
> > On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> >> These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> >>
> >> Signed-off-by: Andrew Jones <drjones@redhat.com>
> >> ---
> >>  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 123 insertions(+), 4 deletions(-)
> > 
> > [...]
> > 
> >> +static int kvm_arch_put_sve(CPUState *cs)
> >> +{
> >> +    ARMCPU *cpu = ARM_CPU(cs);
> >> +    CPUARMState *env = &cpu->env;
> >> +    struct kvm_one_reg reg;
> >> +    int n, ret;
> >> +
> >> +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> >> +        uint64_t *q = aa64_vfp_qreg(env, n);
> >> +#ifdef HOST_WORDS_BIGENDIAN
> >> +        uint64_t d[ARM_MAX_VQ * 2];
> >> +        int i;
> >> +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> >> +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> >> +        }
> > 
> > Out of interest, why do all this swabbing?  It seems expensive.
> 
> Indeed, to me this seems to be the wrong kind of swabbing here.  Exactly what
> format is KVM expecting?  Surely it should be the one used by the unpredicated
> LDR/STR instructions.  Anything else would seem to be working against the
> architecture.
> 
> If so, the format is, architecturally, a stream of bytes in index order, which
> corresponds to a little-endian stream of words.  So the loop I'd expect to see
> here is
> 
>     for (i = 0, n = cpu->sve_max_vq; i < n; ++i) {
>         d[i] = bswap64(q[i]);
>     }

That's the opposite of what we do for fpsimd registers though. I'm
fine with doing whatever KVM/TCG needs, but so far I was just following
the same pattern we already have.

Thanks,
drew

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Richard Henderson]

On 5/13/19 11:14 AM, Andrew Jones wrote:
>> Indeed, to me this seems to be the wrong kind of swabbing here.  Exactly what
>> format is KVM expecting?  Surely it should be the one used by the unpredicated
>> LDR/STR instructions.  Anything else would seem to be working against the
>> architecture.
>>
>> If so, the format is, architecturally, a stream of bytes in index order, which
>> corresponds to a little-endian stream of words.  So the loop I'd expect to see
>> here is
>>
>>     for (i = 0, n = cpu->sve_max_vq; i < n; ++i) {
>>         d[i] = bswap64(q[i]);
>>     }
> 
> That's the opposite of what we do for fpsimd registers though. I'm
> fine with doing whatever KVM/TCG needs, but so far I was just following
> the same pattern we already have.

The behaviour of the hardware is different for LDR of fpsimd registers.

FP&SIMD LDR operates on datasize (8, 16, 32, 64, 128 bits).
SVE LDR always operates on bytes.


r~

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Dave Martin]

On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 123 insertions(+), 4 deletions(-)

[...]

> +static int kvm_arch_put_sve(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int n, ret;
> +
> +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> +        uint64_t *q = aa64_vfp_qreg(env, n);
> +#ifdef HOST_WORDS_BIGENDIAN
> +        uint64_t d[ARM_MAX_VQ * 2];
> +        int i;
> +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> +        }
> +        reg.addr = (uintptr_t)d;
> +#else
> +        reg.addr = (uintptr_t)q;
> +#endif
> +        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);

Will this silently go wrong if more than one slice is required (i.e.,
the register size grows beyond 8192 bits?)

[...]

Cheers
---Dave

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Andrew Jones]

On Mon, May 13, 2019 at 01:43:56PM +0100, Dave Martin wrote:
> On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> > These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 123 insertions(+), 4 deletions(-)
> 
> [...]
> 
> > +static int kvm_arch_put_sve(CPUState *cs)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    int n, ret;
> > +
> > +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> > +        uint64_t *q = aa64_vfp_qreg(env, n);
> > +#ifdef HOST_WORDS_BIGENDIAN
> > +        uint64_t d[ARM_MAX_VQ * 2];
> > +        int i;
> > +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> > +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> > +        }
> > +        reg.addr = (uintptr_t)d;
> > +#else
> > +        reg.addr = (uintptr_t)q;
> > +#endif
> > +        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);
> 
> Will this silently go wrong if more than one slice is required (i.e.,
> the register size grows beyond 8192 bits?)

Yeah, I could probably implement the slice loop now and add a
function that returns 1 (for now) like your vcpu_sve_slices()
function in KVM. I'll do that for v2.

Thanks,
drew



> 
> [...]
> 
> Cheers
> ---Dave
> 

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Dave Martin]

On Mon, May 13, 2019 at 03:07:26PM +0100, Andrew Jones wrote:
> On Mon, May 13, 2019 at 01:43:56PM +0100, Dave Martin wrote:
> > On Sun, May 12, 2019 at 09:36:16AM +0100, Andrew Jones wrote:
> > > These are the SVE equivalents to kvm_arch_get/put_fpsimd.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  target/arm/kvm64.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 123 insertions(+), 4 deletions(-)
> > 
> > [...]
> > 
> > > +static int kvm_arch_put_sve(CPUState *cs)
> > > +{
> > > +    ARMCPU *cpu = ARM_CPU(cs);
> > > +    CPUARMState *env = &cpu->env;
> > > +    struct kvm_one_reg reg;
> > > +    int n, ret;
> > > +
> > > +    for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; n++) {
> > > +        uint64_t *q = aa64_vfp_qreg(env, n);
> > > +#ifdef HOST_WORDS_BIGENDIAN
> > > +        uint64_t d[ARM_MAX_VQ * 2];
> > > +        int i;
> > > +        for (i = 0; i < cpu->sve_max_vq * 2; i++) {
> > > +            d[i] = q[cpu->sve_max_vq * 2 - 1 - i];
> > > +        }
> > > +        reg.addr = (uintptr_t)d;
> > > +#else
> > > +        reg.addr = (uintptr_t)q;
> > > +#endif
> > > +        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);
> > 
> > Will this silently go wrong if more than one slice is required (i.e.,
> > the register size grows beyond 8192 bits?)
> 
> Yeah, I could probably implement the slice loop now and add a
> function that returns 1 (for now) like your vcpu_sve_slices()
> function in KVM. I'll do that for v2.

Or just add a sanity check that the vector length is <= 2048 bits.

Support for larger vectors is untestable for now, since the kernel
doesn't support that and would never expose it.


On that point, could TCG easily be made to expose a larger vector length
to the kernel?  I'd be interested to see what happened.

The kernel should warn and hide the larger vector lengths from KVM,
but I've not been able to test that.

It's only worth trying this out if the hacks to QEMU to enable testing
this were pretty trivial, though.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Richard Henderson]

On 5/13/19 7:39 AM, Dave Martin wrote:
> On that point, could TCG easily be made to expose a larger vector length
> to the kernel?  I'd be interested to see what happened.

It would be easy enough to extend the maximum vector length within TCG.

Increase ARM_MAX_VQ.  Alter the couple of places where we manipulate ZCR.LEN to
extend the current 4-bit mask.

How large do you need the max to be, for testing?


r~

Re: [Qemu-devel] [PATCH 05/13] target/arm/kvm: Add kvm_arch_get/put_sve

[Dave Martin]

On Mon, May 13, 2019 at 05:58:59PM +0100, Richard Henderson wrote:
> On 5/13/19 7:39 AM, Dave Martin wrote:
> > On that point, could TCG easily be made to expose a larger vector length
> > to the kernel?  I'd be interested to see what happened.
> 
> It would be easy enough to extend the maximum vector length within TCG.
> 
> Increase ARM_MAX_VQ.  Alter the couple of places where we manipulate ZCR.LEN to
> extend the current 4-bit mask.
> 
> How large do you need the max to be, for testing?

Anything upwards of 256 bytes is interesting.

The architecture reserves space for it to grow up to 9 bits, though it's
unlikely it would ever get that large in reality.

So if you wanted to go crazy, you might be able to go up to 8192 bytes.

This is just for fun, since it goes outside the architecture and Linux
officially doesn't support it today in any case.  So definitely not a
priority!

Cheers
---Dave

[Qemu-devel] [PATCH 06/13] target/arm/kvm: max cpu: Enable SVE when available

[Andrew Jones]

Enable SVE in the KVM guest when the 'max' cpu type is configured
and KVM supports it. KVM SVE requires use of the new finalize
vcpu ioctl, so we add that now too.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu64.c   |  1 +
 target/arm/kvm.c     |  5 +++++
 target/arm/kvm64.c   | 16 +++++++++++++++-
 target/arm/kvm_arm.h | 12 ++++++++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 228906f26786..6c19ef6837d5 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -292,6 +292,7 @@ static void aarch64_max_initfn(Object *obj)
 
     if (kvm_enabled()) {
         kvm_arm_set_cpu_features_from_host(cpu);
+        cpu->sve_max_vq = ARM_MAX_VQ;
     } else {
         uint64_t t;
         uint32_t u;
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 599563461264..c51db4229d0f 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -50,6 +50,11 @@ int kvm_arm_vcpu_init(CPUState *cs)
     return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
 }
 
+int kvm_arm_vcpu_finalize(CPUState *cs, int feature)
+{
+    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_FINALIZE, &feature);
+}
+
 void kvm_arm_init_serror_injection(CPUState *cs)
 {
     cap_has_inject_serror_esr = kvm_check_extension(cs->kvm_state,
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 86362f4cd7d0..c2d92df75353 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -622,13 +622,20 @@ int kvm_arch_init_vcpu(CPUState *cs)
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
     }
     if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
-            cpu->has_pmu = false;
+        cpu->has_pmu = false;
     }
     if (cpu->has_pmu) {
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
     } else {
         unset_feature(&env->features, ARM_FEATURE_PMU);
     }
+    if (cpu->sve_max_vq) {
+        if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
+            cpu->sve_max_vq = 0;
+        } else {
+            cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
+        }
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
@@ -636,6 +643,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return ret;
     }
 
+    if (cpu->sve_max_vq) {
+        ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
+        if (ret) {
+            return ret;
+        }
+    }
+
     /*
      * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
      * Currently KVM has its own idea about MPIDR assignment, so we
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 2a07333c615f..c488ec3ab410 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -27,6 +27,18 @@
  */
 int kvm_arm_vcpu_init(CPUState *cs);
 
+/**
+ * kvm_arm_vcpu_finalize
+ * @cs: CPUState
+ * @feature: int
+ *
+ * Finalizes the configuration of the specified VCPU feature
+ * by invoking the KVM_ARM_VCPU_FINALIZE ioctl.
+ *
+ * Returns: 0 if success else < 0 error code
+ */
+int kvm_arm_vcpu_finalize(CPUState *cs, int feature);
+
 /**
  * kvm_arm_register_device:
  * @mr: memory region for this device
-- 
2.20.1

Re: [Qemu-devel] [PATCH 06/13] target/arm/kvm: max cpu: Enable SVE when available

[Auger Eric]

Hi Drew,

On 5/12/19 10:36 AM, Andrew Jones wrote:
> Enable SVE in the KVM guest when the 'max' cpu type is configured
> and KVM supports it. KVM SVE requires use of the new finalize
> vcpu ioctl, so we add that now too.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu64.c   |  1 +
>  target/arm/kvm.c     |  5 +++++
>  target/arm/kvm64.c   | 16 +++++++++++++++-
>  target/arm/kvm_arm.h | 12 ++++++++++++
>  4 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 228906f26786..6c19ef6837d5 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -292,6 +292,7 @@ static void aarch64_max_initfn(Object *obj)
>  
>      if (kvm_enabled()) {
>          kvm_arm_set_cpu_features_from_host(cpu);
> +        cpu->sve_max_vq = ARM_MAX_VQ;
same line in the !kvm_enabled path. Maybe you can set the sve_max_vq
field in a subsequent patch and just introduce the finalize and
capability checking in that patch?
>      } else {
>          uint64_t t;
>          uint32_t u;
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 599563461264..c51db4229d0f 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -50,6 +50,11 @@ int kvm_arm_vcpu_init(CPUState *cs)
>      return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
>  }
>  
> +int kvm_arm_vcpu_finalize(CPUState *cs, int feature)
> +{
> +    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_FINALIZE, &feature);
> +}
> +
>  void kvm_arm_init_serror_injection(CPUState *cs)
>  {
>      cap_has_inject_serror_esr = kvm_check_extension(cs->kvm_state,
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 86362f4cd7d0..c2d92df75353 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -622,13 +622,20 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
>      }
>      if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
> -            cpu->has_pmu = false;
> +        cpu->has_pmu = false;
nit: maybe document this unrelated indent fix in the commit msg?
>      }
>      if (cpu->has_pmu) {
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
>      } else {
>          unset_feature(&env->features, ARM_FEATURE_PMU);
>      }
> +    if (cpu->sve_max_vq) {
> +        if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
> +            cpu->sve_max_vq = 0;
> +        } else {
> +            cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> +        }
> +    }
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
> @@ -636,6 +643,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return ret;
>      }
>  
> +    if (cpu->sve_max_vq) {
> +        ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
>      /*
>       * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
>       * Currently KVM has its own idea about MPIDR assignment, so we
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 2a07333c615f..c488ec3ab410 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -27,6 +27,18 @@
>   */
>  int kvm_arm_vcpu_init(CPUState *cs);
>  
> +/**
> + * kvm_arm_vcpu_finalize
> + * @cs: CPUState
> + * @feature: int
feature bitmap or bit?
> + *
> + * Finalizes the configuration of the specified VCPU feature
> + * by invoking the KVM_ARM_VCPU_FINALIZE ioctl.
> + *
> + * Returns: 0 if success else < 0 error code
> + */
> +int kvm_arm_vcpu_finalize(CPUState *cs, int feature);
> +
>  /**
>   * kvm_arm_register_device:
>   * @mr: memory region for this device
> 

Re: [Qemu-devel] [PATCH 06/13] target/arm/kvm: max cpu: Enable SVE when available

[Andrew Jones]

On Wed, Jun 05, 2019 at 11:09:56AM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 5/12/19 10:36 AM, Andrew Jones wrote:
> > Enable SVE in the KVM guest when the 'max' cpu type is configured
> > and KVM supports it. KVM SVE requires use of the new finalize
> > vcpu ioctl, so we add that now too.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu64.c   |  1 +
> >  target/arm/kvm.c     |  5 +++++
> >  target/arm/kvm64.c   | 16 +++++++++++++++-
> >  target/arm/kvm_arm.h | 12 ++++++++++++
> >  4 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 228906f26786..6c19ef6837d5 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -292,6 +292,7 @@ static void aarch64_max_initfn(Object *obj)
> >  
> >      if (kvm_enabled()) {
> >          kvm_arm_set_cpu_features_from_host(cpu);
> > +        cpu->sve_max_vq = ARM_MAX_VQ;
> same line in the !kvm_enabled path. Maybe you can set the sve_max_vq
> field in a subsequent patch and just introduce the finalize and
> capability checking in that patch?

This gets changed in a subsequent patch, so factoring now would
be wasted code motion. I'm not sure the finalize function is worth
its own patch, so I'm inclined to leave this as is.

> >      } else {
> >          uint64_t t;
> >          uint32_t u;
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 599563461264..c51db4229d0f 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -50,6 +50,11 @@ int kvm_arm_vcpu_init(CPUState *cs)
> >      return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
> >  }
> >  
> > +int kvm_arm_vcpu_finalize(CPUState *cs, int feature)
> > +{
> > +    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_FINALIZE, &feature);
> > +}
> > +
> >  void kvm_arm_init_serror_injection(CPUState *cs)
> >  {
> >      cap_has_inject_serror_esr = kvm_check_extension(cs->kvm_state,
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 86362f4cd7d0..c2d92df75353 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -622,13 +622,20 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
> >      }
> >      if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
> > -            cpu->has_pmu = false;
> > +        cpu->has_pmu = false;
> nit: maybe document this unrelated indent fix in the commit msg?

It's pretty obvious without extra commentary, IMHO.

> >      }
> >      if (cpu->has_pmu) {
> >          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> >      } else {
> >          unset_feature(&env->features, ARM_FEATURE_PMU);
> >      }
> > +    if (cpu->sve_max_vq) {
> > +        if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
> > +            cpu->sve_max_vq = 0;
> > +        } else {
> > +            cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > +        }
> > +    }
> >  
> >      /* Do KVM_ARM_VCPU_INIT ioctl */
> >      ret = kvm_arm_vcpu_init(cs);
> > @@ -636,6 +643,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          return ret;
> >      }
> >  
> > +    if (cpu->sve_max_vq) {
> > +        ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
> > +        if (ret) {
> > +            return ret;
> > +        }
> > +    }
> > +
> >      /*
> >       * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
> >       * Currently KVM has its own idea about MPIDR assignment, so we
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index 2a07333c615f..c488ec3ab410 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -27,6 +27,18 @@
> >   */
> >  int kvm_arm_vcpu_init(CPUState *cs);
> >  
> > +/**
> > + * kvm_arm_vcpu_finalize
> > + * @cs: CPUState
> > + * @feature: int
> feature bitmap or bit?

Neither. I can improve this by stating these integers must be one
of the set defined in the "KVM_ARM_VCPU_FINALIZE" section of
kernel doc Documentation/virtual/kvm/api.txt though.

> > + *
> > + * Finalizes the configuration of the specified VCPU feature
> > + * by invoking the KVM_ARM_VCPU_FINALIZE ioctl.
> > + *
> > + * Returns: 0 if success else < 0 error code
> > + */
> > +int kvm_arm_vcpu_finalize(CPUState *cs, int feature);
> > +
> >  /**
> >   * kvm_arm_register_device:
> >   * @mr: memory region for this device
> > 
> 

Thanks,
drew

[Qemu-devel] [PATCH 07/13] target/arm/kvm: max cpu: Allow sve max vector length setting

[Andrew Jones]

Allow the cpu type 'max' sve-max-vq property to work with kvm
too. If the property is not specified then the maximum kvm
supports is used. If it is specified we check that kvm supports
that exact length or error out if it doesn't.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu.h   |  4 +++
 target/arm/cpu64.c |  7 ++--
 target/arm/kvm64.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 86 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 733b840a7127..8292d547e8f9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3122,6 +3122,10 @@ static inline uint64_t arm_sctlr(CPUARMState *env, int el)
     }
 }
 
+static inline int arm_cpu_fls64(uint64_t v)
+{
+    return !v ? 0 : 64 - clz64(v);
+}
 
 /* Return true if the processor is in big-endian mode. */
 static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 6c19ef6837d5..3756e7e2a3e5 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -292,7 +292,7 @@ static void aarch64_max_initfn(Object *obj)
 
     if (kvm_enabled()) {
         kvm_arm_set_cpu_features_from_host(cpu);
-        cpu->sve_max_vq = ARM_MAX_VQ;
+        cpu->sve_max_vq = -1; /* set in kvm_arch_init_vcpu() */
     } else {
         uint64_t t;
         uint32_t u;
@@ -374,9 +374,10 @@ static void aarch64_max_initfn(Object *obj)
 #endif
 
         cpu->sve_max_vq = ARM_MAX_VQ;
-        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
-                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
     }
+
+    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
+                        cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
 }
 
 struct ARMCPUInfo {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index c2d92df75353..0c666e405357 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -446,6 +446,59 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
     }
 }
 
+static int kvm_arm_get_sve_vls(CPUState *cs, uint64_t sve_vls[])
+{
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_ARM64_SVE_VLS,
+        .addr = (uint64_t)&sve_vls[0],
+    };
+    int i, ret;
+
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    ret = 0;
+    for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
+        if (sve_vls[i]) {
+            ret = arm_cpu_fls64(sve_vls[i]) + i * 64;
+            break;
+        }
+    }
+
+    return ret;
+}
+
+static int kvm_arm_set_sve_vls(CPUState *cs, uint64_t sve_vls[], int max_vq)
+{
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_ARM64_SVE_VLS,
+        .addr = (uint64_t)&sve_vls[0],
+    };
+    int i;
+
+    for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
+        if (sve_vls[i]) {
+            int vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
+            while (vq > max_vq) {
+                sve_vls[i] &= ~BIT_MASK(vq - 1);
+                vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
+            }
+            if (vq < max_vq) {
+                error_report("sve-max-vq=%d is not a valid length", max_vq);
+                error_printf("next lowest is %d\n", vq);
+                return -EINVAL;
+            }
+            if (vq == max_vq) {
+                break;
+            }
+        }
+    }
+
+    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+}
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
@@ -605,7 +658,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
     if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
         !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
-        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
+        error_report("KVM is not supported for this guest CPU type");
         return -EINVAL;
     }
 
@@ -631,7 +684,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
     if (cpu->sve_max_vq) {
         if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
-            cpu->sve_max_vq = 0;
+            if (cpu->sve_max_vq == -1) {
+                cpu->sve_max_vq = 0;
+            } else {
+                error_report("This KVM host does not support SVE");
+                return -EINVAL;
+            }
         } else {
             cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
         }
@@ -644,6 +702,24 @@ int kvm_arch_init_vcpu(CPUState *cs)
     }
 
     if (cpu->sve_max_vq) {
+        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
+        ret = kvm_arm_get_sve_vls(cs, sve_vls);
+        if (ret < 0) {
+            return ret;
+        }
+        if (cpu->sve_max_vq == -1) {
+            cpu->sve_max_vq = ret;
+        } else if (cpu->sve_max_vq > ret) {
+            error_report("This KVM host does not support SVE vectors "
+                         "of length %d quadwords (%d bytes)",
+                         cpu->sve_max_vq, cpu->sve_max_vq * 16);
+            return -EINVAL;
+        } else {
+            ret = kvm_arm_set_sve_vls(cs, sve_vls, cpu->sve_max_vq);
+            if (ret < 0) {
+                return ret;
+            }
+        }
         ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
         if (ret) {
             return ret;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 07/13] target/arm/kvm: max cpu: Allow sve max vector length setting

[Richard Henderson]

On 5/12/19 1:36 AM, Andrew Jones wrote:
> @@ -292,7 +292,7 @@ static void aarch64_max_initfn(Object *obj)
>  
>      if (kvm_enabled()) {
>          kvm_arm_set_cpu_features_from_host(cpu);
> -        cpu->sve_max_vq = ARM_MAX_VQ;
> +        cpu->sve_max_vq = -1; /* set in kvm_arch_init_vcpu() */

Perhaps a better comment is "unspecified, to be finalized in ..."

> +    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> +                        cpu_max_set_sve_vq, NULL, NULL, &error_fatal);

as one can set it here, via the command-line.

> @@ -644,6 +702,24 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>  
>      if (cpu->sve_max_vq) {
> +        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
> +        ret = kvm_arm_get_sve_vls(cs, sve_vls);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        if (cpu->sve_max_vq == -1) {
> +            cpu->sve_max_vq = ret;

You still cannot set a value larger than ARM_MAX_VQ, as that is the fixed
amount of space allocated within the cpu structure.


r~

Re: [Qemu-devel] [PATCH 07/13] target/arm/kvm: max cpu: Allow sve max vector length setting

[Andrew Jones]

On Mon, May 13, 2019 at 10:19:25AM -0700, Richard Henderson wrote:
> On 5/12/19 1:36 AM, Andrew Jones wrote:
> > @@ -292,7 +292,7 @@ static void aarch64_max_initfn(Object *obj)
> >  
> >      if (kvm_enabled()) {
> >          kvm_arm_set_cpu_features_from_host(cpu);
> > -        cpu->sve_max_vq = ARM_MAX_VQ;
> > +        cpu->sve_max_vq = -1; /* set in kvm_arch_init_vcpu() */
> 
> Perhaps a better comment is "unspecified, to be finalized in ..."

OK

> 
> > +    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> > +                        cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> 
> as one can set it here, via the command-line.
> 
> > @@ -644,6 +702,24 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      }
> >  
> >      if (cpu->sve_max_vq) {
> > +        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
> > +        ret = kvm_arm_get_sve_vls(cs, sve_vls);
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> > +        if (cpu->sve_max_vq == -1) {
> > +            cpu->sve_max_vq = ret;
> 
> You still cannot set a value larger than ARM_MAX_VQ, as that is the fixed
> amount of space allocated within the cpu structure.
>

Good point. I should check against ARM_MAX_VQ here.

Thanks,
drew 

Re: [Qemu-devel] [PATCH 07/13] target/arm/kvm: max cpu: Allow sve max vector length setting

[Auger Eric]

Hi Drew,
On 5/12/19 10:36 AM, Andrew Jones wrote:
> Allow the cpu type 'max' sve-max-vq property to work with kvm
> too. If the property is not specified then the maximum kvm
> supports is used. If it is specified we check that kvm supports
> that exact length or error out if it doesn't.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.h   |  4 +++
>  target/arm/cpu64.c |  7 ++--
>  target/arm/kvm64.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 86 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 733b840a7127..8292d547e8f9 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3122,6 +3122,10 @@ static inline uint64_t arm_sctlr(CPUARMState *env, int el)
>      }
>  }
>  
> +static inline int arm_cpu_fls64(uint64_t v)
> +{
> +    return !v ? 0 : 64 - clz64(v);
> +}
>  
>  /* Return true if the processor is in big-endian mode. */
>  static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 6c19ef6837d5..3756e7e2a3e5 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -292,7 +292,7 @@ static void aarch64_max_initfn(Object *obj)
>  
>      if (kvm_enabled()) {
>          kvm_arm_set_cpu_features_from_host(cpu);
> -        cpu->sve_max_vq = ARM_MAX_VQ;
> +        cpu->sve_max_vq = -1; /* set in kvm_arch_init_vcpu() */
>      } else {
>          uint64_t t;
>          uint32_t u;
> @@ -374,9 +374,10 @@ static void aarch64_max_initfn(Object *obj)
>  #endif
>  
>          cpu->sve_max_vq = ARM_MAX_VQ;
> -        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> -                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
>      }
> +
> +    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> +                        cpu_max_set_sve_vq, NULL, NULL, &error_fatal);>  }
>  
>  struct ARMCPUInfo {
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index c2d92df75353..0c666e405357 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -446,6 +446,59 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
>      }
>  }
>  
> +static int kvm_arm_get_sve_vls(CPUState *cs, uint64_t sve_vls[])
> +{
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_ARM64_SVE_VLS,
> +        .addr = (uint64_t)&sve_vls[0],
> +    };
> +    int i, ret;
> +
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    ret = 0;
is it mandated?
> +    for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> +        if (sve_vls[i]) {
> +            ret = arm_cpu_fls64(sve_vls[i]) + i * 64;
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static int kvm_arm_set_sve_vls(CPUState *cs, uint64_t sve_vls[], int max_vq)
> +{
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_ARM64_SVE_VLS,
> +        .addr = (uint64_t)&sve_vls[0],
> +    };
> +    int i;
> +
> +    for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> +        if (sve_vls[i]) {
> +            int vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
nit: add a line
> +            while (vq > max_vq) {
> +                sve_vls[i] &= ~BIT_MASK(vq - 1);
Isn't BIT_MASK for 32b. MAKE_64BIT_MASK?
> +                vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
> +            }
> +            if (vq < max_vq) {
I don't really get this check: having vq less than max_vq does not seems
weird. Do you absolutely want vq=max_vq?
> +                error_report("sve-max-vq=%d is not a valid length", max_vq);
> +                error_printf("next lowest is %d\n", vq);
why mixing error_report/printf?
> +                return -EINVAL;
> +            }
> +            if (vq == max_vq) {
> +                break;
> +            }
> +        }
> +    }
> +
> +    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +}
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> @@ -605,7 +658,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>      if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
>          !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
> -        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
> +        error_report("KVM is not supported for this guest CPU type");
>          return -EINVAL;
>      }
>  
> @@ -631,7 +684,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>      if (cpu->sve_max_vq) {
>          if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
> -            cpu->sve_max_vq = 0;
> +            if (cpu->sve_max_vq == -1) {> +                cpu->sve_max_vq = 0;
> +            } else {
> +                error_report("This KVM host does not support SVE");
> +                return -EINVAL;
> +            }
>          } else {
>              cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
>          }
> @@ -644,6 +702,24 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>  
>      if (cpu->sve_max_vq) {
> +        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
line
> +        ret = kvm_arm_get_sve_vls(cs, sve_vls);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        if (cpu->sve_max_vq == -1) {> +            cpu->sve_max_vq = ret;
> +        } else if (cpu->sve_max_vq > ret) {
> +            error_report("This KVM host does not support SVE vectors "
I would rephrase the error mesg with something like:
This KVM host supports SVE vectors of max VQ=%d whereas requested VQ is %d
> +                         "of length %d quadwords (%d bytes)",
> +                         cpu->sve_max_vq, cpu->sve_max_vq * 16);
> +            return -EINVAL;
> +        } else {
> +            ret = kvm_arm_set_sve_vls(cs, sve_vls, cpu->sve_max_vq);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
>          ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
>          if (ret) {
>              return ret;
> 
Thanks

Eric

Re: [Qemu-devel] [PATCH 07/13] target/arm/kvm: max cpu: Allow sve max vector length setting

[Andrew Jones]

On Thu, Jun 06, 2019 at 10:30:00AM +0200, Auger Eric wrote:
> Hi Drew,
> On 5/12/19 10:36 AM, Andrew Jones wrote:
> > Allow the cpu type 'max' sve-max-vq property to work with kvm
> > too. If the property is not specified then the maximum kvm
> > supports is used. If it is specified we check that kvm supports
> > that exact length or error out if it doesn't.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu.h   |  4 +++
> >  target/arm/cpu64.c |  7 ++--
> >  target/arm/kvm64.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 86 insertions(+), 5 deletions(-)
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 733b840a7127..8292d547e8f9 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -3122,6 +3122,10 @@ static inline uint64_t arm_sctlr(CPUARMState *env, int el)
> >      }
> >  }
> >  
> > +static inline int arm_cpu_fls64(uint64_t v)
> > +{
> > +    return !v ? 0 : 64 - clz64(v);
> > +}
> >  
> >  /* Return true if the processor is in big-endian mode. */
> >  static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 6c19ef6837d5..3756e7e2a3e5 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -292,7 +292,7 @@ static void aarch64_max_initfn(Object *obj)
> >  
> >      if (kvm_enabled()) {
> >          kvm_arm_set_cpu_features_from_host(cpu);
> > -        cpu->sve_max_vq = ARM_MAX_VQ;
> > +        cpu->sve_max_vq = -1; /* set in kvm_arch_init_vcpu() */
> >      } else {
> >          uint64_t t;
> >          uint32_t u;
> > @@ -374,9 +374,10 @@ static void aarch64_max_initfn(Object *obj)
> >  #endif
> >  
> >          cpu->sve_max_vq = ARM_MAX_VQ;
> > -        object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> > -                            cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
> >      }
> > +
> > +    object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
> > +                        cpu_max_set_sve_vq, NULL, NULL, &error_fatal);>  }
> >  
> >  struct ARMCPUInfo {
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index c2d92df75353..0c666e405357 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -446,6 +446,59 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> >      }
> >  }
> >  
> > +static int kvm_arm_get_sve_vls(CPUState *cs, uint64_t sve_vls[])
> > +{
> > +    struct kvm_one_reg reg = {
> > +        .id = KVM_REG_ARM64_SVE_VLS,
> > +        .addr = (uint64_t)&sve_vls[0],
> > +    };
> > +    int i, ret;
> > +
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
> > +    ret = 0;
> is it mandated?

Yes, this can be removed, as we know ret=0 here.

> > +    for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> > +        if (sve_vls[i]) {
> > +            ret = arm_cpu_fls64(sve_vls[i]) + i * 64;
> > +            break;
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int kvm_arm_set_sve_vls(CPUState *cs, uint64_t sve_vls[], int max_vq)
> > +{
> > +    struct kvm_one_reg reg = {
> > +        .id = KVM_REG_ARM64_SVE_VLS,
> > +        .addr = (uint64_t)&sve_vls[0],
> > +    };
> > +    int i;
> > +
> > +    for (i = KVM_ARM64_SVE_VLS_WORDS - 1; i >= 0; --i) {
> > +        if (sve_vls[i]) {
> > +            int vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
> nit: add a line
> > +            while (vq > max_vq) {
> > +                sve_vls[i] &= ~BIT_MASK(vq - 1);
> Isn't BIT_MASK for 32b. MAKE_64BIT_MASK?

We should just need a 'UL', not a 'ULL'. I think I'll change all
my BIT_MASK() usage to just BIT() for v2 though, even though
there doesn't appear to be a difference for the cases I've used
it.

> > +                vq = arm_cpu_fls64(sve_vls[i]) + i * 64;
> > +            }
> > +            if (vq < max_vq) {
> I don't really get this check: having vq less than max_vq does not seems
> weird. Do you absolutely want vq=max_vq?

In this context 'vq' is the max vq KVM supports and max_vq is the maximum
the user requested. So if the user wants a maximum greater than what is
possible, then that's an error.

> > +                error_report("sve-max-vq=%d is not a valid length", max_vq);
> > +                error_printf("next lowest is %d\n", vq);
> why mixing error_report/printf?

This is how we're supposed to do two line error messages, afaik.

> > +                return -EINVAL;
> > +            }
> > +            if (vq == max_vq) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +}
> > +
> >  static inline void set_feature(uint64_t *features, int feature)
> >  {
> >      *features |= 1ULL << feature;
> > @@ -605,7 +658,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >  
> >      if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
> >          !object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU)) {
> > -        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
> > +        error_report("KVM is not supported for this guest CPU type");
> >          return -EINVAL;
> >      }
> >  
> > @@ -631,7 +684,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      }
> >      if (cpu->sve_max_vq) {
> >          if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
> > -            cpu->sve_max_vq = 0;
> > +            if (cpu->sve_max_vq == -1) {> +                cpu->sve_max_vq = 0;
> > +            } else {
> > +                error_report("This KVM host does not support SVE");
> > +                return -EINVAL;
> > +            }
> >          } else {
> >              cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> >          }
> > @@ -644,6 +702,24 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      }
> >  
> >      if (cpu->sve_max_vq) {
> > +        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
> line
> > +        ret = kvm_arm_get_sve_vls(cs, sve_vls);
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> > +        if (cpu->sve_max_vq == -1) {> +            cpu->sve_max_vq = ret;
> > +        } else if (cpu->sve_max_vq > ret) {
> > +            error_report("This KVM host does not support SVE vectors "
> I would rephrase the error mesg with something like:
> This KVM host supports SVE vectors of max VQ=%d whereas requested VQ is %d

Yup, giving a better hint as to what the host does support is a good idea.

> > +                         "of length %d quadwords (%d bytes)",
> > +                         cpu->sve_max_vq, cpu->sve_max_vq * 16);
> > +            return -EINVAL;
> > +        } else {
> > +            ret = kvm_arm_set_sve_vls(cs, sve_vls, cpu->sve_max_vq);
> > +            if (ret < 0) {
> > +                return ret;
> > +            }
> > +        }
> >          ret = kvm_arm_vcpu_finalize(cs, KVM_ARM_VCPU_SVE);
> >          if (ret) {
> >              return ret;
> > 
> Thanks
> 
> Eric

Thanks,
drew

[Qemu-devel] [PATCH 08/13] target/arm/monitor: Add query-sve-vector-lengths

[Andrew Jones]

Provide a QMP interface to query the supported SVE vector lengths.
A migratable guest will need to explicitly specify a valid set of
lengths on the command line and that set can be obtained from the
list returned with this QMP command.

This patch only introduces the QMP command with the TCG implementation.
The result may not yet be correct for KVM. Following patches ensure
the KVM result is correct.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 qapi/target.json     | 34 ++++++++++++++++++++++++
 target/arm/monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qmp-cmd-test.c |  1 +
 3 files changed, 97 insertions(+)

diff --git a/qapi/target.json b/qapi/target.json
index 1d4d54b6002e..ca1e85254780 100644
--- a/qapi/target.json
+++ b/qapi/target.json
@@ -397,6 +397,40 @@
 { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
   'if': 'defined(TARGET_ARM)' }
 
+##
+# @SVEVectorLengths:
+#
+# The struct contains a list of integers where each integer is a valid
+# SVE vector length for a KVM guest on this host. The vector lengths
+# are in quadword (128-bit) units, e.g. '4' means 512 bits (64 bytes).
+#
+# @vls:      list of vector lengths in quadwords.
+#
+# Since: 4.1
+##
+{ 'struct': 'SVEVectorLengths',
+  'data': { 'vls': ['int'] },
+  'if': 'defined(TARGET_ARM)' }
+
+##
+# @query-sve-vector-lengths:
+#
+# This command is ARM-only. It will return a list of SVEVectorLengths
+# objects. The list describes all valid SVE vector length sets.
+#
+# Returns: a list of SVEVectorLengths objects
+#
+# Since: 4.1
+#
+# -> { "execute": "query-sve-vector-lengths" }
+# <- { "return": [ { "vls": [ 1 ] },
+#                  { "vls": [ 1, 2 ] },
+#                  { "vls": [ 1, 2, 4 ] } ] }
+#
+##
+{ 'command': 'query-sve-vector-lengths', 'returns': ['SVEVectorLengths'],
+  'if': 'defined(TARGET_ARM)' }
+
 ##
 # @CpuModelExpansionInfo:
 #
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 41b32b94b258..8b2afa255c92 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -24,6 +24,7 @@
 #include "hw/boards.h"
 #include "kvm_arm.h"
 #include "qapi/qapi-commands-target.h"
+#include "monitor/hmp-target.h"
 
 static GICCapability *gic_cap_new(int version)
 {
@@ -82,3 +83,64 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
 
     return head;
 }
+
+static SVEVectorLengths *qmp_sve_vls_get(void)
+{
+    CPUArchState *env = mon_get_cpu_env();
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    SVEVectorLengths *vls = g_new(SVEVectorLengths, 1);
+    intList **v = &vls->vls;
+    int i;
+
+    if (cpu->sve_max_vq == 0) {
+        *v = g_new0(intList, 1); /* one vl of 0 means none supported */
+        return vls;
+    }
+
+    for (i = 1; i <= cpu->sve_max_vq; ++i) {
+        *v = g_new0(intList, 1);
+        (*v)->value = i;
+        v = &(*v)->next;
+    }
+
+    return vls;
+}
+
+static SVEVectorLengths *qmp_sve_vls_dup_and_truncate(SVEVectorLengths *vls)
+{
+    SVEVectorLengths *trunc_vls;
+    intList **v, *p = vls->vls;
+
+    if (!p->next) {
+        return NULL;
+    }
+
+    trunc_vls = g_new(SVEVectorLengths, 1);
+    v = &trunc_vls->vls;
+
+    for (; p->next; p = p->next) {
+        *v = g_new0(intList, 1);
+        (*v)->value = p->value;
+        v = &(*v)->next;
+    }
+
+    return trunc_vls;
+}
+
+SVEVectorLengthsList *qmp_query_sve_vector_lengths(Error **errp)
+{
+    SVEVectorLengthsList *vls_list = g_new0(SVEVectorLengthsList, 1);
+    SVEVectorLengths *vls = qmp_sve_vls_get();
+
+    while (vls) {
+        vls_list->value = vls;
+        vls = qmp_sve_vls_dup_and_truncate(vls);
+        if (vls) {
+            SVEVectorLengthsList *next = vls_list;
+            vls_list = g_new0(SVEVectorLengthsList, 1);
+            vls_list->next = next;
+        }
+    }
+
+    return vls_list;
+}
diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
index 9f5228cd9951..3d714dbc6a4a 100644
--- a/tests/qmp-cmd-test.c
+++ b/tests/qmp-cmd-test.c
@@ -90,6 +90,7 @@ static bool query_is_blacklisted(const char *cmd)
         /* Success depends on target arch: */
         "query-cpu-definitions",  /* arm, i386, ppc, s390x */
         "query-gic-capabilities", /* arm */
+        "query-sve-vector-lengths", /* arm */
         /* Success depends on target-specific build configuration: */
         "query-pci",              /* CONFIG_PCI */
         /* Success depends on launching SEV guest */
-- 
2.20.1

Re: [Qemu-devel] [PATCH 08/13] target/arm/monitor: Add query-sve-vector-lengths

[Markus Armbruster]

Andrew Jones <drjones@redhat.com> writes:

> Provide a QMP interface to query the supported SVE vector lengths.
> A migratable guest will need to explicitly specify a valid set of
> lengths on the command line and that set can be obtained from the
> list returned with this QMP command.
>
> This patch only introduces the QMP command with the TCG implementation.
> The result may not yet be correct for KVM. Following patches ensure
> the KVM result is correct.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  qapi/target.json     | 34 ++++++++++++++++++++++++
>  target/arm/monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qmp-cmd-test.c |  1 +
>  3 files changed, 97 insertions(+)
>
> diff --git a/qapi/target.json b/qapi/target.json
> index 1d4d54b6002e..ca1e85254780 100644
> --- a/qapi/target.json
> +++ b/qapi/target.json
> @@ -397,6 +397,40 @@
>  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>    'if': 'defined(TARGET_ARM)' }
>  
> +##
> +# @SVEVectorLengths:
> +#
> +# The struct contains a list of integers where each integer is a valid

Suggest to s/The struct contains/Contains/.

> +# SVE vector length for a KVM guest on this host. The vector lengths
> +# are in quadword (128-bit) units, e.g. '4' means 512 bits (64 bytes).

Any particular reason for counting quad-words instead of bytes, or
perhaps bits?

> +#
> +# @vls:      list of vector lengths in quadwords.
> +#
> +# Since: 4.1
> +##
> +{ 'struct': 'SVEVectorLengths',
> +  'data': { 'vls': ['int'] },
> +  'if': 'defined(TARGET_ARM)' }
> +
> +##
> +# @query-sve-vector-lengths:
> +#
> +# This command is ARM-only. It will return a list of SVEVectorLengths

No other target-specific command documents its target-specificness like
this.  Suggest

   # Query valid SVE vector length sets.

> +# objects. The list describes all valid SVE vector length sets.
> +#
> +# Returns: a list of SVEVectorLengths objects
> +#
> +# Since: 4.1
> +#
> +# -> { "execute": "query-sve-vector-lengths" }
> +# <- { "return": [ { "vls": [ 1 ] },
> +#                  { "vls": [ 1, 2 ] },
> +#                  { "vls": [ 1, 2, 4 ] } ] }
> +#
> +##
> +{ 'command': 'query-sve-vector-lengths', 'returns': ['SVEVectorLengths'],
> +  'if': 'defined(TARGET_ARM)' }
> +
>  ##
>  # @CpuModelExpansionInfo:
>  #
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 41b32b94b258..8b2afa255c92 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -24,6 +24,7 @@
>  #include "hw/boards.h"
>  #include "kvm_arm.h"
>  #include "qapi/qapi-commands-target.h"
> +#include "monitor/hmp-target.h"

Uh, hmp-target.h when the patch is supposedly about QMP only...

>  
>  static GICCapability *gic_cap_new(int version)
>  {
> @@ -82,3 +83,64 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
>  
>      return head;
>  }
> +
> +static SVEVectorLengths *qmp_sve_vls_get(void)
> +{
> +    CPUArchState *env = mon_get_cpu_env();

Aha, you need it for mon_get_cpu_env().

mon_get_cpu_env() returns the current monitor's current CPU.  This is an
HMP thing, QMP commands should never access it.

Looks like you use it to find one of the CPUs, so you can access its
->sve_max_vq.

"One of the CPUs" smells odd: what if they aren't all the same?  Perhaps
that can't happen.  I don't know, you tell me :)

If any CPU will do, what about simply using first_cpu?

> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    SVEVectorLengths *vls = g_new(SVEVectorLengths, 1);
> +    intList **v = &vls->vls;
> +    int i;
> +
> +    if (cpu->sve_max_vq == 0) {
> +        *v = g_new0(intList, 1); /* one vl of 0 means none supported */
> +        return vls;
> +    }
> +
> +    for (i = 1; i <= cpu->sve_max_vq; ++i) {
> +        *v = g_new0(intList, 1);
> +        (*v)->value = i;
> +        v = &(*v)->next;
> +    }

What this loop does is not immediately obvious.  I think you could use a
function comment.

> +
> +    return vls;
> +}
> +
> +static SVEVectorLengths *qmp_sve_vls_dup_and_truncate(SVEVectorLengths *vls)
> +{
> +    SVEVectorLengths *trunc_vls;
> +    intList **v, *p = vls->vls;
> +
> +    if (!p->next) {
> +        return NULL;
> +    }
> +
> +    trunc_vls = g_new(SVEVectorLengths, 1);
> +    v = &trunc_vls->vls;
> +
> +    for (; p->next; p = p->next) {
> +        *v = g_new0(intList, 1);
> +        (*v)->value = p->value;
> +        v = &(*v)->next;
> +    }
> +
> +    return trunc_vls;
> +}

More so.

> +
> +SVEVectorLengthsList *qmp_query_sve_vector_lengths(Error **errp)
> +{
> +    SVEVectorLengthsList *vls_list = g_new0(SVEVectorLengthsList, 1);
> +    SVEVectorLengths *vls = qmp_sve_vls_get();
> +
> +    while (vls) {
> +        vls_list->value = vls;
> +        vls = qmp_sve_vls_dup_and_truncate(vls);
> +        if (vls) {
> +            SVEVectorLengthsList *next = vls_list;
> +            vls_list = g_new0(SVEVectorLengthsList, 1);
> +            vls_list->next = next;
> +        }
> +    }
> +
> +    return vls_list;
> +}
> diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> index 9f5228cd9951..3d714dbc6a4a 100644
> --- a/tests/qmp-cmd-test.c
> +++ b/tests/qmp-cmd-test.c
> @@ -90,6 +90,7 @@ static bool query_is_blacklisted(const char *cmd)
>          /* Success depends on target arch: */
>          "query-cpu-definitions",  /* arm, i386, ppc, s390x */
>          "query-gic-capabilities", /* arm */
> +        "query-sve-vector-lengths", /* arm */
>          /* Success depends on target-specific build configuration: */
>          "query-pci",              /* CONFIG_PCI */
>          /* Success depends on launching SEV guest */

Re: [Qemu-devel] [PATCH 08/13] target/arm/monitor: Add query-sve-vector-lengths

[Andrew Jones]

On Mon, May 13, 2019 at 06:12:38PM +0200, Markus Armbruster wrote:
> Andrew Jones <drjones@redhat.com> writes:
> 
> > Provide a QMP interface to query the supported SVE vector lengths.
> > A migratable guest will need to explicitly specify a valid set of
> > lengths on the command line and that set can be obtained from the
> > list returned with this QMP command.
> >
> > This patch only introduces the QMP command with the TCG implementation.
> > The result may not yet be correct for KVM. Following patches ensure
> > the KVM result is correct.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  qapi/target.json     | 34 ++++++++++++++++++++++++
> >  target/arm/monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qmp-cmd-test.c |  1 +
> >  3 files changed, 97 insertions(+)
> >
> > diff --git a/qapi/target.json b/qapi/target.json
> > index 1d4d54b6002e..ca1e85254780 100644
> > --- a/qapi/target.json
> > +++ b/qapi/target.json
> > @@ -397,6 +397,40 @@
> >  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
> >    'if': 'defined(TARGET_ARM)' }
> >  
> > +##
> > +# @SVEVectorLengths:
> > +#
> > +# The struct contains a list of integers where each integer is a valid
> 
> Suggest to s/The struct contains/Contains/.

OK

> 
> > +# SVE vector length for a KVM guest on this host. The vector lengths
> > +# are in quadword (128-bit) units, e.g. '4' means 512 bits (64 bytes).
> 
> Any particular reason for counting quad-words instead of bytes, or
> perhaps bits?

It can be considered just bits here, but when set in sve-vls-map those
bits are chosen to mean quadwords as that allows us to get up to 8192-bit
vectors with a single 64-bit word. Maybe I should write more of that here
to clarify.

> 
> > +#
> > +# @vls:      list of vector lengths in quadwords.
> > +#
> > +# Since: 4.1
> > +##
> > +{ 'struct': 'SVEVectorLengths',
> > +  'data': { 'vls': ['int'] },
> > +  'if': 'defined(TARGET_ARM)' }
> > +
> > +##
> > +# @query-sve-vector-lengths:
> > +#
> > +# This command is ARM-only. It will return a list of SVEVectorLengths
> 
> No other target-specific command documents its target-specificness like
> this.  Suggest

Well, it's pretty similar to query-gic-capabilities, which is what I used
as a template, but I'm happy to change it to whatever you suggest :)

> 
>    # Query valid SVE vector length sets.
> 
> > +# objects. The list describes all valid SVE vector length sets.
> > +#
> > +# Returns: a list of SVEVectorLengths objects
> > +#
> > +# Since: 4.1
> > +#
> > +# -> { "execute": "query-sve-vector-lengths" }
> > +# <- { "return": [ { "vls": [ 1 ] },
> > +#                  { "vls": [ 1, 2 ] },
> > +#                  { "vls": [ 1, 2, 4 ] } ] }
> > +#
> > +##
> > +{ 'command': 'query-sve-vector-lengths', 'returns': ['SVEVectorLengths'],
> > +  'if': 'defined(TARGET_ARM)' }
> > +

Yup, will do

> >  ##
> >  # @CpuModelExpansionInfo:
> >  #
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index 41b32b94b258..8b2afa255c92 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/boards.h"
> >  #include "kvm_arm.h"
> >  #include "qapi/qapi-commands-target.h"
> > +#include "monitor/hmp-target.h"
> 
> Uh, hmp-target.h when the patch is supposedly about QMP only...
> 
> >  
> >  static GICCapability *gic_cap_new(int version)
> >  {
> > @@ -82,3 +83,64 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> >  
> >      return head;
> >  }
> > +
> > +static SVEVectorLengths *qmp_sve_vls_get(void)
> > +{
> > +    CPUArchState *env = mon_get_cpu_env();
> 
> Aha, you need it for mon_get_cpu_env().
> 
> mon_get_cpu_env() returns the current monitor's current CPU.  This is an
> HMP thing, QMP commands should never access it.
> 
> Looks like you use it to find one of the CPUs, so you can access its
> ->sve_max_vq.
> 
> "One of the CPUs" smells odd: what if they aren't all the same?  Perhaps
> that can't happen.  I don't know, you tell me :)
> 
> If any CPU will do, what about simply using first_cpu?

first_cpu will work. We currently only allow the same vector length set
for all cpus. I'll change it and drop the HMP things.

> 
> > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > +    SVEVectorLengths *vls = g_new(SVEVectorLengths, 1);
> > +    intList **v = &vls->vls;
> > +    int i;
> > +
> > +    if (cpu->sve_max_vq == 0) {
> > +        *v = g_new0(intList, 1); /* one vl of 0 means none supported */
> > +        return vls;
> > +    }
> > +
> > +    for (i = 1; i <= cpu->sve_max_vq; ++i) {
> > +        *v = g_new0(intList, 1);
> > +        (*v)->value = i;
> > +        v = &(*v)->next;
> > +    }
> 
> What this loop does is not immediately obvious.  I think you could use a
> function comment.

OK

> 
> > +
> > +    return vls;
> > +}
> > +
> > +static SVEVectorLengths *qmp_sve_vls_dup_and_truncate(SVEVectorLengths *vls)
> > +{
> > +    SVEVectorLengths *trunc_vls;
> > +    intList **v, *p = vls->vls;
> > +
> > +    if (!p->next) {
> > +        return NULL;
> > +    }
> > +
> > +    trunc_vls = g_new(SVEVectorLengths, 1);
> > +    v = &trunc_vls->vls;
> > +
> > +    for (; p->next; p = p->next) {
> > +        *v = g_new0(intList, 1);
> > +        (*v)->value = p->value;
> > +        v = &(*v)->next;
> > +    }
> > +
> > +    return trunc_vls;
> > +}
> 
> More so.

More OK :)

> 
> > +
> > +SVEVectorLengthsList *qmp_query_sve_vector_lengths(Error **errp)
> > +{
> > +    SVEVectorLengthsList *vls_list = g_new0(SVEVectorLengthsList, 1);
> > +    SVEVectorLengths *vls = qmp_sve_vls_get();
> > +
> > +    while (vls) {
> > +        vls_list->value = vls;
> > +        vls = qmp_sve_vls_dup_and_truncate(vls);
> > +        if (vls) {
> > +            SVEVectorLengthsList *next = vls_list;
> > +            vls_list = g_new0(SVEVectorLengthsList, 1);
> > +            vls_list->next = next;
> > +        }
> > +    }
> > +
> > +    return vls_list;
> > +}
> > diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> > index 9f5228cd9951..3d714dbc6a4a 100644
> > --- a/tests/qmp-cmd-test.c
> > +++ b/tests/qmp-cmd-test.c
> > @@ -90,6 +90,7 @@ static bool query_is_blacklisted(const char *cmd)
> >          /* Success depends on target arch: */
> >          "query-cpu-definitions",  /* arm, i386, ppc, s390x */
> >          "query-gic-capabilities", /* arm */
> > +        "query-sve-vector-lengths", /* arm */
> >          /* Success depends on target-specific build configuration: */
> >          "query-pci",              /* CONFIG_PCI */
> >          /* Success depends on launching SEV guest */

Thanks,
drew

Re: [Qemu-devel] [PATCH 08/13] target/arm/monitor: Add query-sve-vector-lengths

[Markus Armbruster]

Andrew Jones <drjones@redhat.com> writes:

> On Mon, May 13, 2019 at 06:12:38PM +0200, Markus Armbruster wrote:
>> Andrew Jones <drjones@redhat.com> writes:
>> 
>> > Provide a QMP interface to query the supported SVE vector lengths.
>> > A migratable guest will need to explicitly specify a valid set of
>> > lengths on the command line and that set can be obtained from the
>> > list returned with this QMP command.
>> >
>> > This patch only introduces the QMP command with the TCG implementation.
>> > The result may not yet be correct for KVM. Following patches ensure
>> > the KVM result is correct.
>> >
>> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > ---
>> >  qapi/target.json     | 34 ++++++++++++++++++++++++
>> >  target/arm/monitor.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
>> >  tests/qmp-cmd-test.c |  1 +
>> >  3 files changed, 97 insertions(+)
>> >
>> > diff --git a/qapi/target.json b/qapi/target.json
>> > index 1d4d54b6002e..ca1e85254780 100644
>> > --- a/qapi/target.json
>> > +++ b/qapi/target.json
>> > @@ -397,6 +397,40 @@
>> >  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>> >    'if': 'defined(TARGET_ARM)' }
>> >  
>> > +##
>> > +# @SVEVectorLengths:
>> > +#
>> > +# The struct contains a list of integers where each integer is a valid
>> 
>> Suggest to s/The struct contains/Contains/.
>
> OK
>
>> 
>> > +# SVE vector length for a KVM guest on this host. The vector lengths
>> > +# are in quadword (128-bit) units, e.g. '4' means 512 bits (64 bytes).
>> 
>> Any particular reason for counting quad-words instead of bytes, or
>> perhaps bits?
>
> It can be considered just bits here, but when set in sve-vls-map those
> bits are chosen to mean quadwords as that allows us to get up to 8192-bit
> vectors with a single 64-bit word. Maybe I should write more of that here
> to clarify.

Please do.

In general, QMP prefers the plain units over arbitrary multiples,
e.g. Byte, not Mebibyte.  Sadly, many violations of this rule have crept
in.

More complete documentation should help us determine the unit we want
here.

>> > +#
>> > +# @vls:      list of vector lengths in quadwords.
>> > +#
>> > +# Since: 4.1
>> > +##
>> > +{ 'struct': 'SVEVectorLengths',
>> > +  'data': { 'vls': ['int'] },
>> > +  'if': 'defined(TARGET_ARM)' }
>> > +
>> > +##
>> > +# @query-sve-vector-lengths:
>> > +#
>> > +# This command is ARM-only. It will return a list of SVEVectorLengths
>> 
>> No other target-specific command documents its target-specificness like
>> this.  Suggest
>
> Well, it's pretty similar to query-gic-capabilities, which is what I used
> as a template, but I'm happy to change it to whatever you suggest :)

Here's the documentation we generate for query-gic-capabilities:

 -- Command: query-gic-capabilities

     This command is ARM-only.  It will return a list of GICCapability
     objects that describe its capability bits.

     Returns: a list of GICCapability objects.

     Since: 2.6

     Example:
          -> { "execute": "query-gic-capabilities" }
          <- { "return": [{ "version": 2, "emulated": true, "kernel": false },
                          { "version": 3, "emulated": false, "kernel": true } ] }

     If: 'defined(TARGET_ARM)'

The "If:" line is generated from the schema's condition.  It's not as
readable as I'd like it to be, but it's there, and it always matches the
code.

"This command is ARM-only" is redundant.  Escaped review.  A patch to
drop it would be welcome.

>>    # Query valid SVE vector length sets.
>> 
>> > +# objects. The list describes all valid SVE vector length sets.
>> > +#
>> > +# Returns: a list of SVEVectorLengths objects
>> > +#
>> > +# Since: 4.1
>> > +#
>> > +# -> { "execute": "query-sve-vector-lengths" }
>> > +# <- { "return": [ { "vls": [ 1 ] },
>> > +#                  { "vls": [ 1, 2 ] },
>> > +#                  { "vls": [ 1, 2, 4 ] } ] }
>> > +#
>> > +##
>> > +{ 'command': 'query-sve-vector-lengths', 'returns': ['SVEVectorLengths'],
>> > +  'if': 'defined(TARGET_ARM)' }
>> > +
>
> Yup, will do

Took me a few seconds to connect this to my "# Query valid SVE vector
length sets." line %-}

>> >  ##
>> >  # @CpuModelExpansionInfo:
>> >  #
[...]

[Qemu-devel] [PATCH 09/13] target/arm/kvm: Export kvm_arm_get_sve_vls

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/kvm64.c   |  7 +++++--
 target/arm/kvm_arm.h | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 0c666e405357..11c6334a7c08 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -446,7 +446,8 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
     }
 }
 
-static int kvm_arm_get_sve_vls(CPUState *cs, uint64_t sve_vls[])
+int kvm_arm_get_sve_vls(CPUState *cs,
+                        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS])
 {
     struct kvm_one_reg reg = {
         .id = KVM_REG_ARM64_SVE_VLS,
@@ -470,7 +471,9 @@ static int kvm_arm_get_sve_vls(CPUState *cs, uint64_t sve_vls[])
     return ret;
 }
 
-static int kvm_arm_set_sve_vls(CPUState *cs, uint64_t sve_vls[], int max_vq)
+static int kvm_arm_set_sve_vls(CPUState *cs,
+                               uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS],
+                               int max_vq)
 {
     struct kvm_one_reg reg = {
         .id = KVM_REG_ARM64_SVE_VLS,
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index c488ec3ab410..748ed8d54985 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -248,6 +248,26 @@ int kvm_arm_vgic_probe(void);
 void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
 void kvm_arm_pmu_init(CPUState *cs);
 
+/**
+ * kvm_arm_get_sve_vls
+ * @cs: CPUState
+ * @sve_vls: valid vector length bitmap
+ *
+ * Get the valid vector length bitmap. If a bit 'bit' is set
+ * then the host supports a vector length of (bit * 16) bytes.
+ *
+ * For example, if
+ *
+ *   sve_vls[0] = 0xb and
+ *   sve_vls[1 ... KVM_ARM64_SVE_VLS_WORDS-1] = 0,
+ *
+ * then the host supports 16, 32, and 64 byte vector lengths.
+ *
+ * Returns: the highest set bit if successful else < 0 error code
+ */
+int kvm_arm_get_sve_vls(CPUState *cs,
+                        uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS]);
+
 #else
 
 static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
-- 
2.20.1

[Qemu-devel] [PATCH 10/13] target/arm/monitor: kvm: only return valid sve vector sets

[Andrew Jones]

While the TCG SVE implementation can implement all vector lengths
which are a quadword multiple, up to some maximum length, KVM can
only provide what the host supports, and not all multiples are
required to be supported by the architecture. With this patch
we extend the QMP query to ask KVM for the valid vectors when KVM
is enabled.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/monitor.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 8b2afa255c92..3e13dd7c7b7a 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -84,6 +84,41 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
     return head;
 }
 
+#ifdef CONFIG_KVM
+static SVEVectorLengths *qmp_kvm_sve_vls_get(void)
+{
+    CPUArchState *env = mon_get_cpu_env();
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
+    SVEVectorLengths *vls = g_new(SVEVectorLengths, 1);
+    intList **v = &vls->vls;
+    int ret, i;
+
+    ret = kvm_arm_get_sve_vls(CPU(cpu), sve_vls);
+    if (ret <= 0) {
+        *v = g_new0(intList, 1); /* one vl of 0 means none supported */
+        return vls;
+    }
+
+    for (i = KVM_ARM64_SVE_VQ_MIN; i <= ret; ++i) {
+        int bitval = (sve_vls[(i - KVM_ARM64_SVE_VQ_MIN) / 64] >>
+                      ((i - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1;
+        if (bitval) {
+            *v = g_new0(intList, 1);
+            (*v)->value = i;
+            v = &(*v)->next;
+        }
+    }
+
+    return vls;
+}
+#else
+static SVEVectorLengths *qmp_kvm_sve_vls_get(void)
+{
+    return NULL;
+}
+#endif
+
 static SVEVectorLengths *qmp_sve_vls_get(void)
 {
     CPUArchState *env = mon_get_cpu_env();
@@ -130,7 +165,13 @@ static SVEVectorLengths *qmp_sve_vls_dup_and_truncate(SVEVectorLengths *vls)
 SVEVectorLengthsList *qmp_query_sve_vector_lengths(Error **errp)
 {
     SVEVectorLengthsList *vls_list = g_new0(SVEVectorLengthsList, 1);
-    SVEVectorLengths *vls = qmp_sve_vls_get();
+    SVEVectorLengths *vls;
+
+    if (kvm_enabled()) {
+        vls = qmp_kvm_sve_vls_get();
+    } else {
+        vls = qmp_sve_vls_get();
+    }
 
     while (vls) {
         vls_list->value = vls;
-- 
2.20.1

[Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Andrew Jones]

Introduce another cpu property to control SVE vector lengths,
sve-vls-map, which allows the user to explicitly select the
set of vector lengths the guest can use. The map must conform
to QEMU's limits and architectural constraints, checked when
the property is set. Inconsistencies with sve-max-vq are also
checked. The bit number of a set bit in the map represents the
allowed vector length in number of quadwords.

Note, as the map is implemented with a single 64-bit word we
currently only support up to 8192-bit vectors. As QEMU and
KVM only support up to 2048-bit vectors then this sufficient
now, and probably for some time. Extending the bitmap beyond
a single word will likely require changing the property to
a string and adding yet another parser to QEMU.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu.c     |  4 +++
 target/arm/cpu.h     |  3 ++
 target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
 target/arm/helper.c  | 10 ++++++-
 target/arm/monitor.c |  9 ++++--
 5 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a181fa8dc1a7..ea0e24bba8b6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1188,6 +1188,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         set_feature(env, ARM_FEATURE_VBAR);
     }
 
+    if (!kvm_enabled() && !cpu->sve_vls_map) {
+        cpu->sve_vls_map = BIT_MASK(cpu->sve_max_vq) - 1;
+    }
+
     register_cp_regs_for_features(cpu);
     arm_cpu_register_gdb_regs_for_features(cpu);
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8292d547e8f9..f0d0ce759ba8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -920,6 +920,9 @@ struct ARMCPU {
 
     /* Used to set the maximum vector length the cpu will support.  */
     uint32_t sve_max_vq;
+
+    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
+    uint64_t sve_vls_map;
 };
 
 static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3756e7e2a3e5..9ac702d54136 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -273,14 +273,73 @@ static void cpu_max_set_sve_vq(Object *obj, Visitor *v, const char *name,
 
     visit_type_uint32(v, name, &cpu->sve_max_vq, &err);
 
-    if (!err && (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ)) {
-        error_setg(&err, "unsupported SVE vector length");
-        error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
-                          ARM_MAX_VQ);
+    if (!err) {
+        if (cpu->sve_max_vq == 0 || cpu->sve_max_vq > ARM_MAX_VQ) {
+            error_setg(&err, "unsupported SVE vector length");
+            error_append_hint(&err, "Valid sve-max-vq in range [1-%d]\n",
+                              ARM_MAX_VQ);
+        } else if (cpu->sve_vls_map &&
+                   cpu->sve_max_vq != arm_cpu_fls64(cpu->sve_vls_map)) {
+            error_setg(&err, "sve-vls-map and sve-max-vq are inconsistent");
+        }
     }
     error_propagate(errp, err);
 }
 
+static void cpu_get_sve_vls_map(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    visit_type_uint64(v, name, &cpu->sve_vls_map, errp);
+}
+
+static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    Error *err = NULL;
+    uint64_t mask = ~(BIT_MASK(ARM_MAX_VQ - 1) - 1);
+    int i;
+
+    visit_type_uint64(v, name, &cpu->sve_vls_map, errp);
+
+    if (!err) {
+        if (cpu->sve_vls_map == 0) {
+            error_setg(&err, "SVE vector length map cannot be zero");
+        } else if (cpu->sve_vls_map & mask) {
+            error_setg(&err, "SVE vector length map has unsupported lengths");
+            error_append_hint(&err, "Valid vector lengths in range [1-%d]\n",
+                              ARM_MAX_VQ);
+        } else if (cpu->sve_max_vq != ARM_MAX_VQ &&
+                   cpu->sve_max_vq != arm_cpu_fls64(cpu->sve_vls_map)) {
+            /*
+             * If the user provides both sve-max-vq and sve-vls-map, with
+             * sve-max-vq first, then, unless ARM_MAX_VQ is selected for
+             * sve-max-vq, we can catch inconsistencies. If ARM_MAX_VQ was
+             * selected then sve-max-vq will get silently overwritten with
+             * the max sve-vls-map provides. This is the best we can do
+             * without initially setting sve-max-vq to -1 like we do for KVM.
+             */
+            error_setg(&err, "sve-vls-map and sve-max-vq are inconsistent");
+        } else {
+            cpu->sve_max_vq = arm_cpu_fls64(cpu->sve_vls_map);
+            mask = 0;
+            for (i = 1; i < cpu->sve_max_vq; ++i) {
+                if (is_power_of_2(i)) {
+                    mask |= BIT_MASK(i - 1);
+                }
+            }
+            if ((cpu->sve_vls_map & mask) != mask) {
+                error_setg(&err, "SVE vector length map is missing lengths");
+                error_append_hint(&err, "All power-of-2 vector lengths up to "
+                                  "the max supported length are required.\n");
+            }
+        }
+    }
+
+    error_propagate(errp, err);
+}
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -374,6 +433,9 @@ static void aarch64_max_initfn(Object *obj)
 #endif
 
         cpu->sve_max_vq = ARM_MAX_VQ;
+
+        object_property_add(obj, "sve-vls-map", "uint64", cpu_get_sve_vls_map,
+                            cpu_set_sve_vls_map, NULL, NULL, &error_fatal);
     }
 
     object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1e6eb0d0f360..bedec1ea0b27 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
 static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                       uint64_t value)
 {
+    ARMCPU *cpu = arm_env_get_cpu(env);
     int cur_el = arm_current_el(env);
     int old_len = sve_zcr_len_for_el(env, cur_el);
     int new_len;
 
     /* Bits other than [3:0] are RAZ/WI.  */
-    raw_write(env, ri, value & 0xf);
+    value &= 0xf;
+
+    if (value && !(BIT_MASK(value) & cpu->sve_vls_map)) {
+        uint64_t map = cpu->sve_vls_map & (BIT_MASK(value) - 1);
+        value = arm_cpu_fls64(map) - 1;
+    }
+
+    raw_write(env, ri, value);
 
     /*
      * Because we arrived here, we know both FP and SVE are enabled;
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 3e13dd7c7b7a..192012659e36 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -133,9 +133,12 @@ static SVEVectorLengths *qmp_sve_vls_get(void)
     }
 
     for (i = 1; i <= cpu->sve_max_vq; ++i) {
-        *v = g_new0(intList, 1);
-        (*v)->value = i;
-        v = &(*v)->next;
+        int bitval = (cpu->sve_vls_map >> (i - 1)) & 1;
+        if (bitval) {
+            *v = g_new0(intList, 1);
+            (*v)->value = i;
+            v = &(*v)->next;
+        }
     }
 
     return vls;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Dave Martin]

On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> Introduce another cpu property to control SVE vector lengths,
> sve-vls-map, which allows the user to explicitly select the
> set of vector lengths the guest can use. The map must conform
> to QEMU's limits and architectural constraints, checked when
> the property is set. Inconsistencies with sve-max-vq are also
> checked. The bit number of a set bit in the map represents the
> allowed vector length in number of quadwords.
> 
> Note, as the map is implemented with a single 64-bit word we
> currently only support up to 8192-bit vectors. As QEMU and
> KVM only support up to 2048-bit vectors then this sufficient
> now, and probably for some time. Extending the bitmap beyond
> a single word will likely require changing the property to
> a string and adding yet another parser to QEMU.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.c     |  4 +++
>  target/arm/cpu.h     |  3 ++
>  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
>  target/arm/helper.c  | 10 ++++++-
>  target/arm/monitor.c |  9 ++++--
>  5 files changed, 88 insertions(+), 8 deletions(-)
> 

[...]

> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8292d547e8f9..f0d0ce759ba8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -920,6 +920,9 @@ struct ARMCPU {
>  
>      /* Used to set the maximum vector length the cpu will support.  */
>      uint32_t sve_max_vq;
> +
> +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> +    uint64_t sve_vls_map;

Just to be clear, the representation here is different from the
representation in KVM_REG_ARM64_SVE_VLS?

In the latter, bit n represents vector length ((n + 1) * 16) bytes.

(QEMU is free to choose its own internal representation, naturally.)

[...]

> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c

[...]

> +static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
> +                                void *opaque, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    Error *err = NULL;
> +    uint64_t mask = ~(BIT_MASK(ARM_MAX_VQ - 1) - 1);
> +    int i;
> +
> +    visit_type_uint64(v, name, &cpu->sve_vls_map, errp);
> +
> +    if (!err) {
> +        if (cpu->sve_vls_map == 0) {
> +            error_setg(&err, "SVE vector length map cannot be zero");

Maybe say "empty" here, since the map represents a set?

(But it this is just for debug rather than reporting errors to the user,
it probably doesn't matter much.)

[...]

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 1e6eb0d0f360..bedec1ea0b27 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
>  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                        uint64_t value)
>  {
> +    ARMCPU *cpu = arm_env_get_cpu(env);
>      int cur_el = arm_current_el(env);
>      int old_len = sve_zcr_len_for_el(env, cur_el);
>      int new_len;
>  
>      /* Bits other than [3:0] are RAZ/WI.  */
> -    raw_write(env, ri, value & 0xf);
> +    value &= 0xf;

You might want to sanity-check that the max vq you configured for the
vcpu is <= 16 here.

> +
> +    if (value && !(BIT_MASK(value) & cpu->sve_vls_map)) {
> +        uint64_t map = cpu->sve_vls_map & (BIT_MASK(value) - 1);
> +        value = arm_cpu_fls64(map) - 1;
> +    }
> +
> +    raw_write(env, ri, value);

[...]

Cheers
---Dave

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Andrew Jones]

On Mon, May 13, 2019 at 12:26:36PM +0100, Dave Martin wrote:
> On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> > Introduce another cpu property to control SVE vector lengths,
> > sve-vls-map, which allows the user to explicitly select the
> > set of vector lengths the guest can use. The map must conform
> > to QEMU's limits and architectural constraints, checked when
> > the property is set. Inconsistencies with sve-max-vq are also
> > checked. The bit number of a set bit in the map represents the
> > allowed vector length in number of quadwords.
> > 
> > Note, as the map is implemented with a single 64-bit word we
> > currently only support up to 8192-bit vectors. As QEMU and
> > KVM only support up to 2048-bit vectors then this sufficient
> > now, and probably for some time. Extending the bitmap beyond
> > a single word will likely require changing the property to
> > a string and adding yet another parser to QEMU.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu.c     |  4 +++
> >  target/arm/cpu.h     |  3 ++
> >  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
> >  target/arm/helper.c  | 10 ++++++-
> >  target/arm/monitor.c |  9 ++++--
> >  5 files changed, 88 insertions(+), 8 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 8292d547e8f9..f0d0ce759ba8 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -920,6 +920,9 @@ struct ARMCPU {
> >  
> >      /* Used to set the maximum vector length the cpu will support.  */
> >      uint32_t sve_max_vq;
> > +
> > +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> > +    uint64_t sve_vls_map;
> 
> Just to be clear, the representation here is different from the
> representation in KVM_REG_ARM64_SVE_VLS?
> 
> In the latter, bit n represents vector length ((n + 1) * 16) bytes.

KVM also uses bitnum * 16. bitnum is the the bit number, not the shift.
So you can't have bitnum=0. 'n' must be a shift, as it would need to
allow zero in order to represent KVM_ARM64_SVE_VQ_MIN.

> 
> (QEMU is free to choose its own internal representation, naturally.)
> 
> [...]
> 
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> 
> [...]
> 
> > +static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    Error *err = NULL;
> > +    uint64_t mask = ~(BIT_MASK(ARM_MAX_VQ - 1) - 1);
> > +    int i;
> > +
> > +    visit_type_uint64(v, name, &cpu->sve_vls_map, errp);
> > +
> > +    if (!err) {
> > +        if (cpu->sve_vls_map == 0) {
> > +            error_setg(&err, "SVE vector length map cannot be zero");
> 
> Maybe say "empty" here, since the map represents a set?

Empty does sound better when considering the map represents a set, but
the user will be inputting a number for the map, so zero will be what
they attempted to input.

> 
> (But it this is just for debug rather than reporting errors to the user,
> it probably doesn't matter much.)

I'm also not too worried about which term we use, so I'm happy to change
it if you'd like.

> 
> [...]
> 
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 1e6eb0d0f360..bedec1ea0b27 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> >  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >                        uint64_t value)
> >  {
> > +    ARMCPU *cpu = arm_env_get_cpu(env);
> >      int cur_el = arm_current_el(env);
> >      int old_len = sve_zcr_len_for_el(env, cur_el);
> >      int new_len;
> >  
> >      /* Bits other than [3:0] are RAZ/WI.  */
> > -    raw_write(env, ri, value & 0xf);
> > +    value &= 0xf;
> 
> You might want to sanity-check that the max vq you configured for the
> vcpu is <= 16 here.

It's already checked by the cpu property set function at input time.

> 
> > +
> > +    if (value && !(BIT_MASK(value) & cpu->sve_vls_map)) {
> > +        uint64_t map = cpu->sve_vls_map & (BIT_MASK(value) - 1);
> > +        value = arm_cpu_fls64(map) - 1;
> > +    }
> > +
> > +    raw_write(env, ri, value);
> 
> [...]
> 
> Cheers
> ---Dave
> 

Thanks,
drew

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Dave Martin]

On Mon, May 13, 2019 at 01:30:23PM +0100, Andrew Jones wrote:
> On Mon, May 13, 2019 at 12:26:36PM +0100, Dave Martin wrote:
> > On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> > > Introduce another cpu property to control SVE vector lengths,
> > > sve-vls-map, which allows the user to explicitly select the
> > > set of vector lengths the guest can use. The map must conform
> > > to QEMU's limits and architectural constraints, checked when
> > > the property is set. Inconsistencies with sve-max-vq are also
> > > checked. The bit number of a set bit in the map represents the
> > > allowed vector length in number of quadwords.
> > > 
> > > Note, as the map is implemented with a single 64-bit word we
> > > currently only support up to 8192-bit vectors. As QEMU and
> > > KVM only support up to 2048-bit vectors then this sufficient
> > > now, and probably for some time. Extending the bitmap beyond
> > > a single word will likely require changing the property to
> > > a string and adding yet another parser to QEMU.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  target/arm/cpu.c     |  4 +++
> > >  target/arm/cpu.h     |  3 ++
> > >  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
> > >  target/arm/helper.c  | 10 ++++++-
> > >  target/arm/monitor.c |  9 ++++--
> > >  5 files changed, 88 insertions(+), 8 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > index 8292d547e8f9..f0d0ce759ba8 100644
> > > --- a/target/arm/cpu.h
> > > +++ b/target/arm/cpu.h
> > > @@ -920,6 +920,9 @@ struct ARMCPU {
> > >  
> > >      /* Used to set the maximum vector length the cpu will support.  */
> > >      uint32_t sve_max_vq;
> > > +
> > > +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> > > +    uint64_t sve_vls_map;
> > 
> > Just to be clear, the representation here is different from the
> > representation in KVM_REG_ARM64_SVE_VLS?
> > 
> > In the latter, bit n represents vector length ((n + 1) * 16) bytes.
> 
> KVM also uses bitnum * 16. bitnum is the the bit number, not the shift.

Can you point to the relevant kernel code?  This isn't what I thought I
wrote...

> So you can't have bitnum=0. 'n' must be a shift, as it would need to
> allow zero in order to represent KVM_ARM64_SVE_VQ_MIN.

[...]

> > > +static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
> > > +                                void *opaque, Error **errp)
> > > +{
> > > +    ARMCPU *cpu = ARM_CPU(obj);
> > > +    Error *err = NULL;
> > > +    uint64_t mask = ~(BIT_MASK(ARM_MAX_VQ - 1) - 1);
> > > +    int i;
> > > +
> > > +    visit_type_uint64(v, name, &cpu->sve_vls_map, errp);
> > > +
> > > +    if (!err) {
> > > +        if (cpu->sve_vls_map == 0) {
> > > +            error_setg(&err, "SVE vector length map cannot be zero");
> > 
> > Maybe say "empty" here, since the map represents a set?
> 
> Empty does sound better when considering the map represents a set, but
> the user will be inputting a number for the map, so zero will be what
> they attempted to input.

If the user is inputting the encoded number directly, I guess it makes
sense to describe the empty set as "zero".

> > (But it this is just for debug rather than reporting errors to the user,
> > it probably doesn't matter much.)
> 
> I'm also not too worried about which term we use, so I'm happy to change
> it if you'd like.

I guess I don't have a strong opinion.

[...]

> > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > index 1e6eb0d0f360..bedec1ea0b27 100644
> > > --- a/target/arm/helper.c
> > > +++ b/target/arm/helper.c
> > > @@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> > >  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > >                        uint64_t value)
> > >  {
> > > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > >      int cur_el = arm_current_el(env);
> > >      int old_len = sve_zcr_len_for_el(env, cur_el);
> > >      int new_len;
> > >  
> > >      /* Bits other than [3:0] are RAZ/WI.  */
> > > -    raw_write(env, ri, value & 0xf);
> > > +    value &= 0xf;
> > 
> > You might want to sanity-check that the max vq you configured for the
> > vcpu is <= 16 here.
> 
> It's already checked by the cpu property set function at input time.

The check is in one place, but the assumptions based on it are
potentially all over the place -- it may be tricky to track them
all down if you ever add support for >8192-bit vectors.

(Not a big deal from my point of view, but I'd migration to larger
vectors to be as smooth as reasonably possible if we ever find we have
to do it.)
 
Cheers
---Dave

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Andrew Jones]

On Mon, May 13, 2019 at 01:41:26PM +0100, Dave Martin wrote:
> On Mon, May 13, 2019 at 01:30:23PM +0100, Andrew Jones wrote:
> > On Mon, May 13, 2019 at 12:26:36PM +0100, Dave Martin wrote:
> > > On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> > > > Introduce another cpu property to control SVE vector lengths,
> > > > sve-vls-map, which allows the user to explicitly select the
> > > > set of vector lengths the guest can use. The map must conform
> > > > to QEMU's limits and architectural constraints, checked when
> > > > the property is set. Inconsistencies with sve-max-vq are also
> > > > checked. The bit number of a set bit in the map represents the
> > > > allowed vector length in number of quadwords.
> > > > 
> > > > Note, as the map is implemented with a single 64-bit word we
> > > > currently only support up to 8192-bit vectors. As QEMU and
> > > > KVM only support up to 2048-bit vectors then this sufficient
> > > > now, and probably for some time. Extending the bitmap beyond
> > > > a single word will likely require changing the property to
> > > > a string and adding yet another parser to QEMU.
> > > > 
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  target/arm/cpu.c     |  4 +++
> > > >  target/arm/cpu.h     |  3 ++
> > > >  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
> > > >  target/arm/helper.c  | 10 ++++++-
> > > >  target/arm/monitor.c |  9 ++++--
> > > >  5 files changed, 88 insertions(+), 8 deletions(-)
> > > > 
> > > 
> > > [...]
> > > 
> > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > index 8292d547e8f9..f0d0ce759ba8 100644
> > > > --- a/target/arm/cpu.h
> > > > +++ b/target/arm/cpu.h
> > > > @@ -920,6 +920,9 @@ struct ARMCPU {
> > > >  
> > > >      /* Used to set the maximum vector length the cpu will support.  */
> > > >      uint32_t sve_max_vq;
> > > > +
> > > > +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> > > > +    uint64_t sve_vls_map;
> > > 
> > > Just to be clear, the representation here is different from the
> > > representation in KVM_REG_ARM64_SVE_VLS?
> > > 
> > > In the latter, bit n represents vector length ((n + 1) * 16) bytes.
> > 
> > KVM also uses bitnum * 16. bitnum is the the bit number, not the shift.
> 
> Can you point to the relevant kernel code?  This isn't what I thought I
> wrote...

The Documentation/virtual/kvm/api.txt documentation has

 > if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
 >    ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >>
 >                ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1))
 >        /* Vector length vq * 16 bytes supported */
 > else
 >        /* Vector length vq * 16 bytes not supported */

Taking vq=1, we check (vector_lengths[0] >> 0) to see if we have the
length (1 * 16) bytes. Since bitnum 1 is (1 << 0) that means the shift
is zero and (bitnum * 16) is the same as (vq * 16). With ((n + 1) * 16),
n would have to be zero, which is not a valid bitnum, but is a valid
bit shift.

> 
> > So you can't have bitnum=0. 'n' must be a shift, as it would need to
> > allow zero in order to represent KVM_ARM64_SVE_VQ_MIN.
> 
> [...]
> 
> > > > +static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
> > > > +                                void *opaque, Error **errp)
> > > > +{
> > > > +    ARMCPU *cpu = ARM_CPU(obj);
> > > > +    Error *err = NULL;
> > > > +    uint64_t mask = ~(BIT_MASK(ARM_MAX_VQ - 1) - 1);
> > > > +    int i;
> > > > +
> > > > +    visit_type_uint64(v, name, &cpu->sve_vls_map, errp);
> > > > +
> > > > +    if (!err) {
> > > > +        if (cpu->sve_vls_map == 0) {
> > > > +            error_setg(&err, "SVE vector length map cannot be zero");
> > > 
> > > Maybe say "empty" here, since the map represents a set?
> > 
> > Empty does sound better when considering the map represents a set, but
> > the user will be inputting a number for the map, so zero will be what
> > they attempted to input.
> 
> If the user is inputting the encoded number directly, I guess it makes
> sense to describe the empty set as "zero".
> 
> > > (But it this is just for debug rather than reporting errors to the user,
> > > it probably doesn't matter much.)
> > 
> > I'm also not too worried about which term we use, so I'm happy to change
> > it if you'd like.
> 
> I guess I don't have a strong opinion.
> 
> [...]
> 
> > > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > > index 1e6eb0d0f360..bedec1ea0b27 100644
> > > > --- a/target/arm/helper.c
> > > > +++ b/target/arm/helper.c
> > > > @@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> > > >  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > > >                        uint64_t value)
> > > >  {
> > > > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > > >      int cur_el = arm_current_el(env);
> > > >      int old_len = sve_zcr_len_for_el(env, cur_el);
> > > >      int new_len;
> > > >  
> > > >      /* Bits other than [3:0] are RAZ/WI.  */
> > > > -    raw_write(env, ri, value & 0xf);
> > > > +    value &= 0xf;
> > > 
> > > You might want to sanity-check that the max vq you configured for the
> > > vcpu is <= 16 here.
> > 
> > It's already checked by the cpu property set function at input time.
> 
> The check is in one place, but the assumptions based on it are
> potentially all over the place -- it may be tricky to track them
> all down if you ever add support for >8192-bit vectors.
> 
> (Not a big deal from my point of view, but I'd migration to larger
> vectors to be as smooth as reasonably possible if we ever find we have
> to do it.)
>

Adding asserts might be a good idea to catch bugs when expanding the
supported vector lengths. I can certainly add one here. Maybe Richard
has some ideas as well for this.

Thanks,
drew

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Dave Martin]

On Mon, May 13, 2019 at 01:57:40PM +0100, Andrew Jones wrote:
> On Mon, May 13, 2019 at 01:41:26PM +0100, Dave Martin wrote:
> > On Mon, May 13, 2019 at 01:30:23PM +0100, Andrew Jones wrote:
> > > On Mon, May 13, 2019 at 12:26:36PM +0100, Dave Martin wrote:
> > > > On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> > > > > Introduce another cpu property to control SVE vector lengths,
> > > > > sve-vls-map, which allows the user to explicitly select the
> > > > > set of vector lengths the guest can use. The map must conform
> > > > > to QEMU's limits and architectural constraints, checked when
> > > > > the property is set. Inconsistencies with sve-max-vq are also
> > > > > checked. The bit number of a set bit in the map represents the
> > > > > allowed vector length in number of quadwords.
> > > > > 
> > > > > Note, as the map is implemented with a single 64-bit word we
> > > > > currently only support up to 8192-bit vectors. As QEMU and
> > > > > KVM only support up to 2048-bit vectors then this sufficient
> > > > > now, and probably for some time. Extending the bitmap beyond
> > > > > a single word will likely require changing the property to
> > > > > a string and adding yet another parser to QEMU.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  target/arm/cpu.c     |  4 +++
> > > > >  target/arm/cpu.h     |  3 ++
> > > > >  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
> > > > >  target/arm/helper.c  | 10 ++++++-
> > > > >  target/arm/monitor.c |  9 ++++--
> > > > >  5 files changed, 88 insertions(+), 8 deletions(-)
> > > > > 
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > > index 8292d547e8f9..f0d0ce759ba8 100644
> > > > > --- a/target/arm/cpu.h
> > > > > +++ b/target/arm/cpu.h
> > > > > @@ -920,6 +920,9 @@ struct ARMCPU {
> > > > >  
> > > > >      /* Used to set the maximum vector length the cpu will support.  */
> > > > >      uint32_t sve_max_vq;
> > > > > +
> > > > > +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> > > > > +    uint64_t sve_vls_map;
> > > > 
> > > > Just to be clear, the representation here is different from the
> > > > representation in KVM_REG_ARM64_SVE_VLS?
> > > > 
> > > > In the latter, bit n represents vector length ((n + 1) * 16) bytes.
> > > 
> > > KVM also uses bitnum * 16. bitnum is the the bit number, not the shift.
> > 
> > Can you point to the relevant kernel code?  This isn't what I thought I
> > wrote...
> 
> The Documentation/virtual/kvm/api.txt documentation has
> 
>  > if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
>  >    ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >>
>  >                ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1))
>  >        /* Vector length vq * 16 bytes supported */
>  > else
>  >        /* Vector length vq * 16 bytes not supported */
> 
> Taking vq=1, we check (vector_lengths[0] >> 0) to see if we have the
> length (1 * 16) bytes. Since bitnum 1 is (1 << 0) that means the shift
> is zero and (bitnum * 16) is the same as (vq * 16). With ((n + 1) * 16),
> n would have to be zero, which is not a valid bitnum, but is a valid
> bit shift.

OK, so it sounds like the interpretation of the KVM ABI is what I intended.

What is "bitnum" though?  Most of the time, people seem to number bits
starting from 0 -- though of course that's just a convention.

> > > So you can't have bitnum=0. 'n' must be a shift, as it would need to
> > > allow zero in order to represent KVM_ARM64_SVE_VQ_MIN.

[...]

> > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > > > index 1e6eb0d0f360..bedec1ea0b27 100644
> > > > > --- a/target/arm/helper.c
> > > > > +++ b/target/arm/helper.c
> > > > > @@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> > > > >  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > > > >                        uint64_t value)
> > > > >  {
> > > > > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > > > >      int cur_el = arm_current_el(env);
> > > > >      int old_len = sve_zcr_len_for_el(env, cur_el);
> > > > >      int new_len;
> > > > >  
> > > > >      /* Bits other than [3:0] are RAZ/WI.  */
> > > > > -    raw_write(env, ri, value & 0xf);
> > > > > +    value &= 0xf;
> > > > 
> > > > You might want to sanity-check that the max vq you configured for the
> > > > vcpu is <= 16 here.
> > > 
> > > It's already checked by the cpu property set function at input time.
> > 
> > The check is in one place, but the assumptions based on it are
> > potentially all over the place -- it may be tricky to track them
> > all down if you ever add support for >8192-bit vectors.
> > 
> > (Not a big deal from my point of view, but I'd migration to larger
> > vectors to be as smooth as reasonably possible if we ever find we have
> > to do it.)
> >
> 
> Adding asserts might be a good idea to catch bugs when expanding the
> supported vector lengths. I can certainly add one here. Maybe Richard
> has some ideas as well for this.

Sure, just wanted to make the point, since this might be an issue in the
future.

The compromise in the kernel implementation was similar: in many places
the code is agnostic to larger vectors, but in a few (such as the ioctl
reg accessors) this seemed too cumbersome to justify for now, so I went
with a simpler implementation with some added sanity-checks.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Andrew Jones]

On Mon, May 13, 2019 at 02:12:48PM +0100, Dave Martin wrote:
> On Mon, May 13, 2019 at 01:57:40PM +0100, Andrew Jones wrote:
> > On Mon, May 13, 2019 at 01:41:26PM +0100, Dave Martin wrote:
> > > On Mon, May 13, 2019 at 01:30:23PM +0100, Andrew Jones wrote:
> > > > On Mon, May 13, 2019 at 12:26:36PM +0100, Dave Martin wrote:
> > > > > On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> > > > > > Introduce another cpu property to control SVE vector lengths,
> > > > > > sve-vls-map, which allows the user to explicitly select the
> > > > > > set of vector lengths the guest can use. The map must conform
> > > > > > to QEMU's limits and architectural constraints, checked when
> > > > > > the property is set. Inconsistencies with sve-max-vq are also
> > > > > > checked. The bit number of a set bit in the map represents the
> > > > > > allowed vector length in number of quadwords.
> > > > > > 
> > > > > > Note, as the map is implemented with a single 64-bit word we
> > > > > > currently only support up to 8192-bit vectors. As QEMU and
> > > > > > KVM only support up to 2048-bit vectors then this sufficient
> > > > > > now, and probably for some time. Extending the bitmap beyond
> > > > > > a single word will likely require changing the property to
> > > > > > a string and adding yet another parser to QEMU.
> > > > > > 
> > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > > ---
> > > > > >  target/arm/cpu.c     |  4 +++
> > > > > >  target/arm/cpu.h     |  3 ++
> > > > > >  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
> > > > > >  target/arm/helper.c  | 10 ++++++-
> > > > > >  target/arm/monitor.c |  9 ++++--
> > > > > >  5 files changed, 88 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > > > index 8292d547e8f9..f0d0ce759ba8 100644
> > > > > > --- a/target/arm/cpu.h
> > > > > > +++ b/target/arm/cpu.h
> > > > > > @@ -920,6 +920,9 @@ struct ARMCPU {
> > > > > >  
> > > > > >      /* Used to set the maximum vector length the cpu will support.  */
> > > > > >      uint32_t sve_max_vq;
> > > > > > +
> > > > > > +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> > > > > > +    uint64_t sve_vls_map;
> > > > > 
> > > > > Just to be clear, the representation here is different from the
> > > > > representation in KVM_REG_ARM64_SVE_VLS?
> > > > > 
> > > > > In the latter, bit n represents vector length ((n + 1) * 16) bytes.
> > > > 
> > > > KVM also uses bitnum * 16. bitnum is the the bit number, not the shift.
> > > 
> > > Can you point to the relevant kernel code?  This isn't what I thought I
> > > wrote...
> > 
> > The Documentation/virtual/kvm/api.txt documentation has
> > 
> >  > if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
> >  >    ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >>
> >  >                ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1))
> >  >        /* Vector length vq * 16 bytes supported */
> >  > else
> >  >        /* Vector length vq * 16 bytes not supported */
> > 
> > Taking vq=1, we check (vector_lengths[0] >> 0) to see if we have the
> > length (1 * 16) bytes. Since bitnum 1 is (1 << 0) that means the shift
> > is zero and (bitnum * 16) is the same as (vq * 16). With ((n + 1) * 16),
> > n would have to be zero, which is not a valid bitnum, but is a valid
> > bit shift.
> 
> OK, so it sounds like the interpretation of the KVM ABI is what I intended.
> 
> What is "bitnum" though?  Most of the time, people seem to number bits
> starting from 0 -- though of course that's just a convention.

Eh, I see the problem. I used the word 'number', when I meant 'position'.
Indeed the bit number of bit position 1 is zero. Sorry about the confusion.
For v2 I can try to ensure I don't use the wrong terminology.

> 
> > > > So you can't have bitnum=0. 'n' must be a shift, as it would need to
> > > > allow zero in order to represent KVM_ARM64_SVE_VQ_MIN.
> 
> [...]
> 
> > > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > > > > > index 1e6eb0d0f360..bedec1ea0b27 100644
> > > > > > --- a/target/arm/helper.c
> > > > > > +++ b/target/arm/helper.c
> > > > > > @@ -5254,12 +5254,20 @@ uint32_t sve_zcr_len_for_el(CPUARMState *env, int el)
> > > > > >  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > > > > >                        uint64_t value)
> > > > > >  {
> > > > > > +    ARMCPU *cpu = arm_env_get_cpu(env);
> > > > > >      int cur_el = arm_current_el(env);
> > > > > >      int old_len = sve_zcr_len_for_el(env, cur_el);
> > > > > >      int new_len;
> > > > > >  
> > > > > >      /* Bits other than [3:0] are RAZ/WI.  */
> > > > > > -    raw_write(env, ri, value & 0xf);
> > > > > > +    value &= 0xf;
> > > > > 
> > > > > You might want to sanity-check that the max vq you configured for the
> > > > > vcpu is <= 16 here.
> > > > 
> > > > It's already checked by the cpu property set function at input time.
> > > 
> > > The check is in one place, but the assumptions based on it are
> > > potentially all over the place -- it may be tricky to track them
> > > all down if you ever add support for >8192-bit vectors.
> > > 
> > > (Not a big deal from my point of view, but I'd migration to larger
> > > vectors to be as smooth as reasonably possible if we ever find we have
> > > to do it.)
> > >
> > 
> > Adding asserts might be a good idea to catch bugs when expanding the
> > supported vector lengths. I can certainly add one here. Maybe Richard
> > has some ideas as well for this.
> 
> Sure, just wanted to make the point, since this might be an issue in the
> future.
> 
> The compromise in the kernel implementation was similar: in many places
> the code is agnostic to larger vectors, but in a few (such as the ioctl
> reg accessors) this seemed too cumbersome to justify for now, so I went
> with a simpler implementation with some added sanity-checks.
>

Thanks,
drew

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Dave Martin]

On Mon, May 13, 2019 at 02:45:12PM +0100, Andrew Jones wrote:
> On Mon, May 13, 2019 at 02:12:48PM +0100, Dave Martin wrote:
> > On Mon, May 13, 2019 at 01:57:40PM +0100, Andrew Jones wrote:
> > > On Mon, May 13, 2019 at 01:41:26PM +0100, Dave Martin wrote:
> > > > On Mon, May 13, 2019 at 01:30:23PM +0100, Andrew Jones wrote:
> > > > > On Mon, May 13, 2019 at 12:26:36PM +0100, Dave Martin wrote:
> > > > > > On Sun, May 12, 2019 at 09:36:22AM +0100, Andrew Jones wrote:
> > > > > > > Introduce another cpu property to control SVE vector lengths,
> > > > > > > sve-vls-map, which allows the user to explicitly select the
> > > > > > > set of vector lengths the guest can use. The map must conform
> > > > > > > to QEMU's limits and architectural constraints, checked when
> > > > > > > the property is set. Inconsistencies with sve-max-vq are also
> > > > > > > checked. The bit number of a set bit in the map represents the
> > > > > > > allowed vector length in number of quadwords.
> > > > > > > 
> > > > > > > Note, as the map is implemented with a single 64-bit word we
> > > > > > > currently only support up to 8192-bit vectors. As QEMU and
> > > > > > > KVM only support up to 2048-bit vectors then this sufficient
> > > > > > > now, and probably for some time. Extending the bitmap beyond
> > > > > > > a single word will likely require changing the property to
> > > > > > > a string and adding yet another parser to QEMU.
> > > > > > > 
> > > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > > > ---
> > > > > > >  target/arm/cpu.c     |  4 +++
> > > > > > >  target/arm/cpu.h     |  3 ++
> > > > > > >  target/arm/cpu64.c   | 70 +++++++++++++++++++++++++++++++++++++++++---
> > > > > > >  target/arm/helper.c  | 10 ++++++-
> > > > > > >  target/arm/monitor.c |  9 ++++--
> > > > > > >  5 files changed, 88 insertions(+), 8 deletions(-)
> > > > > > > 
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > > > > index 8292d547e8f9..f0d0ce759ba8 100644
> > > > > > > --- a/target/arm/cpu.h
> > > > > > > +++ b/target/arm/cpu.h
> > > > > > > @@ -920,6 +920,9 @@ struct ARMCPU {
> > > > > > >  
> > > > > > >      /* Used to set the maximum vector length the cpu will support.  */
> > > > > > >      uint32_t sve_max_vq;
> > > > > > > +
> > > > > > > +    /* Each bit represents a supported vector length of (bitnum * 16) bytes */
> > > > > > > +    uint64_t sve_vls_map;
> > > > > > 
> > > > > > Just to be clear, the representation here is different from the
> > > > > > representation in KVM_REG_ARM64_SVE_VLS?
> > > > > > 
> > > > > > In the latter, bit n represents vector length ((n + 1) * 16) bytes.
> > > > > 
> > > > > KVM also uses bitnum * 16. bitnum is the the bit number, not the shift.
> > > > 
> > > > Can you point to the relevant kernel code?  This isn't what I thought I
> > > > wrote...
> > > 
> > > The Documentation/virtual/kvm/api.txt documentation has
> > > 
> > >  > if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
> > >  >    ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >>
> > >  >                ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1))
> > >  >        /* Vector length vq * 16 bytes supported */
> > >  > else
> > >  >        /* Vector length vq * 16 bytes not supported */
> > > 
> > > Taking vq=1, we check (vector_lengths[0] >> 0) to see if we have the
> > > length (1 * 16) bytes. Since bitnum 1 is (1 << 0) that means the shift
> > > is zero and (bitnum * 16) is the same as (vq * 16). With ((n + 1) * 16),
> > > n would have to be zero, which is not a valid bitnum, but is a valid
> > > bit shift.
> > 
> > OK, so it sounds like the interpretation of the KVM ABI is what I intended.
> > 
> > What is "bitnum" though?  Most of the time, people seem to number bits
> > starting from 0 -- though of course that's just a convention.
> 
> Eh, I see the problem. I used the word 'number', when I meant 'position'.
> Indeed the bit number of bit position 1 is zero. Sorry about the confusion.
> For v2 I can try to ensure I don't use the wrong terminology.

I'm not sure there's an actual problem here.  I found the terminology
unfamiliar, that's all.

My main concern was that people might be confused about the kernel ABI
here, but it sounds like your interpretation is as I intended.

But by all means clarify the comments if you think there's a risk of
QEMU folks being confused.

[...]

Cheers
---Dave

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Markus Armbruster]

Andrew Jones <drjones@redhat.com> writes:

> Introduce another cpu property to control SVE vector lengths,
> sve-vls-map, which allows the user to explicitly select the
> set of vector lengths the guest can use. The map must conform
> to QEMU's limits and architectural constraints, checked when
> the property is set. Inconsistencies with sve-max-vq are also
> checked. The bit number of a set bit in the map represents the
> allowed vector length in number of quadwords.
>
> Note, as the map is implemented with a single 64-bit word we
> currently only support up to 8192-bit vectors. As QEMU and
> KVM only support up to 2048-bit vectors then this sufficient
> now, and probably for some time. Extending the bitmap beyond
> a single word will likely require changing the property to
> a string and adding yet another parser to QEMU.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Please add an example of how the new property should be used on the
command line to your commit message.  I'd ask for a documentation update
as well if we had any to update.

Re: [Qemu-devel] [PATCH 11/13] target/arm/cpu64: max cpu: Introduce sve-vls-map

[Andrew Jones]

On Mon, May 13, 2019 at 05:25:23PM +0200, Markus Armbruster wrote:
> Andrew Jones <drjones@redhat.com> writes:
> 
> > Introduce another cpu property to control SVE vector lengths,
> > sve-vls-map, which allows the user to explicitly select the
> > set of vector lengths the guest can use. The map must conform
> > to QEMU's limits and architectural constraints, checked when
> > the property is set. Inconsistencies with sve-max-vq are also
> > checked. The bit number of a set bit in the map represents the
> > allowed vector length in number of quadwords.
> >
> > Note, as the map is implemented with a single 64-bit word we
> > currently only support up to 8192-bit vectors. As QEMU and
> > KVM only support up to 2048-bit vectors then this sufficient
> > now, and probably for some time. Extending the bitmap beyond
> > a single word will likely require changing the property to
> > a string and adding yet another parser to QEMU.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> Please add an example of how the new property should be used on the
> command line to your commit message.  I'd ask for a documentation update
> as well if we had any to update.

Will do.

Thanks,
drew

[Qemu-devel] [PATCH 12/13] target/arm/kvm: max cpu: Add support for sve-vls-map

[Andrew Jones]

The max cpu type can have its SVE vector lengths explicitly set
with the sve-vls-map property. This patch allows that property
to work when KVM is in use. The map must conform to additional
constraints for KVM which are checked at vcpu init.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu64.c |  7 +++---
 target/arm/kvm64.c | 56 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 9ac702d54136..94f3dd5b51e5 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -310,7 +310,7 @@ static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
             error_setg(&err, "SVE vector length map has unsupported lengths");
             error_append_hint(&err, "Valid vector lengths in range [1-%d]\n",
                               ARM_MAX_VQ);
-        } else if (cpu->sve_max_vq != ARM_MAX_VQ &&
+        } else if (cpu->sve_max_vq != ARM_MAX_VQ && cpu->sve_max_vq != -1 &&
                    cpu->sve_max_vq != arm_cpu_fls64(cpu->sve_vls_map)) {
             /*
              * If the user provides both sve-max-vq and sve-vls-map, with
@@ -433,13 +433,12 @@ static void aarch64_max_initfn(Object *obj)
 #endif
 
         cpu->sve_max_vq = ARM_MAX_VQ;
-
-        object_property_add(obj, "sve-vls-map", "uint64", cpu_get_sve_vls_map,
-                            cpu_set_sve_vls_map, NULL, NULL, &error_fatal);
     }
 
     object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
                         cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
+    object_property_add(obj, "sve-vls-map", "uint64", cpu_get_sve_vls_map,
+                        cpu_set_sve_vls_map, NULL, NULL, &error_fatal);
 }
 
 struct ARMCPUInfo {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 11c6334a7c08..5506f019c190 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -685,9 +685,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
     } else {
         unset_feature(&env->features, ARM_FEATURE_PMU);
     }
-    if (cpu->sve_max_vq) {
+    if (cpu->sve_max_vq || cpu->sve_vls_map) {
         if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SVE)) {
-            if (cpu->sve_max_vq == -1) {
+            if (cpu->sve_max_vq == -1 && !cpu->sve_vls_map) {
                 cpu->sve_max_vq = 0;
             } else {
                 error_report("This KVM host does not support SVE");
@@ -704,12 +704,62 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return ret;
     }
 
-    if (cpu->sve_max_vq) {
+    if (cpu->sve_max_vq || cpu->sve_vls_map) {
         uint64_t sve_vls[KVM_ARM64_SVE_VLS_WORDS];
         ret = kvm_arm_get_sve_vls(cs, sve_vls);
         if (ret < 0) {
             return ret;
         }
+        if (cpu->sve_vls_map) {
+            uint64_t ovls;
+            int i;
+
+            /*
+             * We currently only support a single VLS word, as that should
+             * be sufficient for some time (vq=64 means a 8192-bit vector
+             * and KVM currently only supports up to 2048-bit vectors).
+             * The choice to only support a single word for now is due to
+             * the need to input it on the command line. It's much simpler
+             * to input a word as a cpu property than an array of words.
+             * So for now just warn if we detect our assumption was wrong.
+             */
+            for (i = 1; i < KVM_ARM64_SVE_VLS_WORDS; ++i) {
+                if (sve_vls[i]) {
+                    warn_report("KVM supports vector lengths larger than "
+                                "sve-vls-map can select");
+                    sve_vls[i] = 0;
+                }
+            }
+
+            ovls = sve_vls[0];
+            sve_vls[0] = cpu->sve_vls_map;
+
+            if (cpu->sve_vls_map & ~ovls) {
+                error_report("sve-vls-map=0x%lx is not valid on this host "
+                             "which supports 0x%lx", cpu->sve_vls_map, ovls);
+                return -EINVAL;
+            }
+
+            i = arm_cpu_fls64(cpu->sve_vls_map);
+            if (cpu->sve_max_vq && cpu->sve_max_vq != -1 &&
+                cpu->sve_max_vq != i) {
+                error_report("sve-vls-map and sve-max-vq are inconsistent");
+                return -EINVAL;
+            }
+            cpu->sve_max_vq = i;
+
+            /*
+             * sve-vls-map must have all the same vector lengths up to its
+             * max vq that the host supports.
+             */
+            if (cpu->sve_vls_map != (ovls & (BIT_MASK(cpu->sve_max_vq) - 1))) {
+                error_report("sve-vls-map=0x%lx is not valid on this host "
+                             "which supports 0x%lx", cpu->sve_vls_map, ovls);
+                error_printf("All host vector lengths up to %d must also "
+                             "be selected.\n", cpu->sve_max_vq);
+                return -EINVAL;
+            }
+        }
         if (cpu->sve_max_vq == -1) {
             cpu->sve_max_vq = ret;
         } else if (cpu->sve_max_vq > ret) {
-- 
2.20.1

[Qemu-devel] [PATCH 13/13] target/arm/kvm: host cpu: Add support for sve-vls-map

[Andrew Jones]

Allow the host cpu type to enable SVE in guests with the sve-vls-map
cpu property.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu.c   |  1 +
 target/arm/cpu.h   |  2 ++
 target/arm/cpu64.c | 12 +++++++++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ea0e24bba8b6..a5c01ff42c78 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2222,6 +2222,7 @@ static void arm_host_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
 
     kvm_arm_set_cpu_features_from_host(cpu);
+    aarch64_add_sve_vls_map_property(obj);
     arm_cpu_post_init(obj);
 }
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f0d0ce759ba8..13731ccb39f3 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -976,11 +976,13 @@ int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 void aarch64_sve_change_el(CPUARMState *env, int old_el,
                            int new_el, bool el0_a64);
+void aarch64_add_sve_vls_map_property(Object *obj);
 #else
 static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
 static inline void aarch64_sve_change_el(CPUARMState *env, int o,
                                          int n, bool a)
 { }
+void aarch64_add_sve_vls_map_property(Object *obj) { }
 #endif
 
 target_ulong do_arm_semihosting(CPUARMState *env);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 94f3dd5b51e5..3b0b900a4d97 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -310,7 +310,8 @@ static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
             error_setg(&err, "SVE vector length map has unsupported lengths");
             error_append_hint(&err, "Valid vector lengths in range [1-%d]\n",
                               ARM_MAX_VQ);
-        } else if (cpu->sve_max_vq != ARM_MAX_VQ && cpu->sve_max_vq != -1 &&
+        } else if (cpu->sve_max_vq && cpu->sve_max_vq != ARM_MAX_VQ &&
+                   cpu->sve_max_vq != -1 &&
                    cpu->sve_max_vq != arm_cpu_fls64(cpu->sve_vls_map)) {
             /*
              * If the user provides both sve-max-vq and sve-vls-map, with
@@ -340,6 +341,12 @@ static void cpu_set_sve_vls_map(Object *obj, Visitor *v, const char *name,
     error_propagate(errp, err);
 }
 
+void aarch64_add_sve_vls_map_property(Object *obj)
+{
+    object_property_add(obj, "sve-vls-map", "uint64", cpu_get_sve_vls_map,
+                        cpu_set_sve_vls_map, NULL, NULL, &error_fatal);
+}
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -437,8 +444,7 @@ static void aarch64_max_initfn(Object *obj)
 
     object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_vq,
                         cpu_max_set_sve_vq, NULL, NULL, &error_fatal);
-    object_property_add(obj, "sve-vls-map", "uint64", cpu_get_sve_vls_map,
-                        cpu_set_sve_vls_map, NULL, NULL, &error_fatal);
+    aarch64_add_sve_vls_map_property(obj);
 }
 
 struct ARMCPUInfo {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 13/13] target/arm/kvm: host cpu: Add support for sve-vls-map

[Markus Armbruster]

Andrew Jones <drjones@redhat.com> writes:

> Allow the host cpu type to enable SVE in guests with the sve-vls-map
> cpu property.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.c   |  1 +
>  target/arm/cpu.h   |  2 ++
>  target/arm/cpu64.c | 12 +++++++++---
>  3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index ea0e24bba8b6..a5c01ff42c78 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2222,6 +2222,7 @@ static void arm_host_initfn(Object *obj)
>      ARMCPU *cpu = ARM_CPU(obj);
>  
>      kvm_arm_set_cpu_features_from_host(cpu);
> +    aarch64_add_sve_vls_map_property(obj);
>      arm_cpu_post_init(obj);
>  }
>  
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f0d0ce759ba8..13731ccb39f3 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -976,11 +976,13 @@ int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
>  void aarch64_sve_change_el(CPUARMState *env, int old_el,
>                             int new_el, bool el0_a64);
> +void aarch64_add_sve_vls_map_property(Object *obj);
>  #else
>  static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
>  static inline void aarch64_sve_change_el(CPUARMState *env, int o,
>                                           int n, bool a)
>  { }
> +void aarch64_add_sve_vls_map_property(Object *obj) { }

qemu/target/arm/cpu.h:985:6: error: no previous prototype for ‘aarch64_add_sve_vls_map_property’ [-Werror=missing-prototypes]
 void aarch64_add_sve_vls_map_property(Object *obj) { }
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Did you forget static inline?

>  #endif
>  
>  target_ulong do_arm_semihosting(CPUARMState *env);
[...]

Re: [Qemu-devel] [PATCH 13/13] target/arm/kvm: host cpu: Add support for sve-vls-map

[Andrew Jones]

On Mon, May 13, 2019 at 05:37:09PM +0200, Markus Armbruster wrote:
> Andrew Jones <drjones@redhat.com> writes:
> 
> > Allow the host cpu type to enable SVE in guests with the sve-vls-map
> > cpu property.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu.c   |  1 +
> >  target/arm/cpu.h   |  2 ++
> >  target/arm/cpu64.c | 12 +++++++++---
> >  3 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index ea0e24bba8b6..a5c01ff42c78 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -2222,6 +2222,7 @@ static void arm_host_initfn(Object *obj)
> >      ARMCPU *cpu = ARM_CPU(obj);
> >  
> >      kvm_arm_set_cpu_features_from_host(cpu);
> > +    aarch64_add_sve_vls_map_property(obj);
> >      arm_cpu_post_init(obj);
> >  }
> >  
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index f0d0ce759ba8..13731ccb39f3 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -976,11 +976,13 @@ int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> >  void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
> >  void aarch64_sve_change_el(CPUARMState *env, int old_el,
> >                             int new_el, bool el0_a64);
> > +void aarch64_add_sve_vls_map_property(Object *obj);
> >  #else
> >  static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
> >  static inline void aarch64_sve_change_el(CPUARMState *env, int o,
> >                                           int n, bool a)
> >  { }
> > +void aarch64_add_sve_vls_map_property(Object *obj) { }
> 
> qemu/target/arm/cpu.h:985:6: error: no previous prototype for ‘aarch64_add_sve_vls_map_property’ [-Werror=missing-prototypes]
>  void aarch64_add_sve_vls_map_property(Object *obj) { }
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Did you forget static inline?

I sure did. Thanks for finding this!

drew

> 
> >  #endif
> >  
> >  target_ulong do_arm_semihosting(CPUARMState *env);
> [...]

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrea Bolognani]

On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:
[...]
>    CPU type | accel | sve-max-vq | sve-vls-map
>    -------------------------------------------
>  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
>  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
>  3)    host | kvm   |  N/A       |  $VLS_MAP
> 
> Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> all supported when $VLS_MAP is zero, or when the vqs are selected
> in $VLS_MAP.

I'm a bit confused by the nomenclature here. VL clearly stands for
Vector Length, but what does VQ stand for? You seem to be using the
two terms pretty much interchangeably throughout the cover letter.

[...]
> There is never any need to provide both properties, but if both
> are provided then they are checked for consistency.

I would personally just error out when both are provided.

> The QMP query returns a list of valid vq lists. For example, if
> a guest can use vqs 1, 2, 3, and 4, then the following list will
> be returned
> 
>  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> 
> Another example might be 1, 2, 4, as the architecture states 3
> is optional. In that case the list would be
> 
>  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]

I think the proposed QMP command is problematic, because it reports
the valid vector lengths for either KVM or TCG based on which
accelerator is currently enabled: it should report all information
at all times instead, similarly to how query-gic-capabilities does
it.

[...]
> And now for what might be a bit more controversial; how we input
> the valid vector set with sve-vls-map. Well, sve-vls-map is a
> 64-bit bitmap, which is admittedly not user friendly and also not
> the same size as KVM's vls bitmap (which is 8 64-bit words). Here's
> the justification:
> 
>  1) We want to use the QEMU command line in order for the information
>     to be migrated without needing to add more VM state.
>  2) It's not easy/pretty to input arrays on the QEMU command line.
>  3) We already need to use the QMP query to get a valid set, which
>     isn't user friendly either, meaning we're already in libvirt
>     territory.
>  4) A 64-bit map (supporting up to 8192-bit vectors) is likely big
>     enough for quite some time (currently KVM and TCG only support
>     2048-bit vectors).
>  5) If user friendliness is needed more than migratability then
>     the 'max' cpu type can be used with the sve-max-vq property.
>  6) It's possible to probe the full valid vector set from the
>     command line by using something like sve-vls-map=0xffff and
>     then, when it fails, the error message will state the correct
>     map, e.g. 0xb.

I don't have a problem with having to use a bitmap internally,
though libvirt will clearly want to expose a more approachable
interface to users.

However, QMP reporting the information in the current format means
we'd have to build an additional parser on top of the bitmap handling
and conversion routines we'll clearly need to make this work; plus it
just feels weird that the information reported by QMP can't be used
on the command line without going through some tranformation first.

Wouldn't it make more sense to both accept and report bitmaps?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Dave Martin]

On Mon, May 13, 2019 at 10:32:46AM +0100, Andrea Bolognani wrote:
> On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:
> [...]
> >    CPU type | accel | sve-max-vq | sve-vls-map
> >    -------------------------------------------
> >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> >  3)    host | kvm   |  N/A       |  $VLS_MAP
> > 
> > Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> > all supported when $VLS_MAP is zero, or when the vqs are selected
> > in $VLS_MAP.
> 
> I'm a bit confused by the nomenclature here. VL clearly stands for
> Vector Length, but what does VQ stand for? You seem to be using the
> two terms pretty much interchangeably throughout the cover letter.

From the Linux end, "vector length" or VL refers to the size of a vector
register, either in no particular unit or in bytes.

"VQ" refers specifically to the vector length in 128-bit quadwords.

In some situations, neither terminology is obviously better than the
other, such as in the way KVM_REG_ARM64_SVE_VLS is encoded.

> [...]
> > There is never any need to provide both properties, but if both
> > are provided then they are checked for consistency.
> 
> I would personally just error out when both are provided.
> 
> > The QMP query returns a list of valid vq lists. For example, if
> > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > be returned
> > 
> >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > 
> > Another example might be 1, 2, 4, as the architecture states 3
> > is optional. In that case the list would be
> > 
> >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> 
> I think the proposed QMP command is problematic, because it reports
> the valid vector lengths for either KVM or TCG based on which
> accelerator is currently enabled: it should report all information
> at all times instead, similarly to how query-gic-capabilities does
> it.

I wonder if this is premature flexibility?

The size of these lists is going to get cumbersome if the architecture
is ever extended.  Even today, we might need over 100 items in this
(nested) list.  If this is to be presented to the user this will be
far from friendly, it could get much worse if the architecutre changes
in future to allow larger vectors or more flexible virtualisation.

Could we just have a list of supported vector lengths and a possibly
empty list of additional capabilities that describe what kinds of
flexibility are allowed?

So, for example, we might support vector lengths of 1, 2, 4 and 8
quadwords, with the the ability to clamp the max vector length the
guest sees: the kernel ABI guarantees that you can do this, even
if you can't disable/enable each individual vector length independently.

So, [ 1, 2, 4, 8 ] seems sufficient to describe this in a forwards
compatible way.

Some day, we might report { "independent", [ 1, 2, 4, 8, 16, 32, ... ] }

I'm guessing about the data representation here.

[...]

Cheers
---Dave

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Mon, May 13, 2019 at 12:15:56PM +0100, Dave Martin wrote:
> On Mon, May 13, 2019 at 10:32:46AM +0100, Andrea Bolognani wrote:
> > On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:
> > [...]
> > >    CPU type | accel | sve-max-vq | sve-vls-map
> > >    -------------------------------------------
> > >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> > >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> > >  3)    host | kvm   |  N/A       |  $VLS_MAP
> > > 
> > > Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> > > all supported when $VLS_MAP is zero, or when the vqs are selected
> > > in $VLS_MAP.
> > 
> > I'm a bit confused by the nomenclature here. VL clearly stands for
> > Vector Length, but what does VQ stand for? You seem to be using the
> > two terms pretty much interchangeably throughout the cover letter.
> 
> From the Linux end, "vector length" or VL refers to the size of a vector
> register, either in no particular unit or in bytes.
> 
> "VQ" refers specifically to the vector length in 128-bit quadwords.
> 
> In some situations, neither terminology is obviously better than the
> other, such as in the way KVM_REG_ARM64_SVE_VLS is encoded.
> 
> > [...]
> > > There is never any need to provide both properties, but if both
> > > are provided then they are checked for consistency.
> > 
> > I would personally just error out when both are provided.
> > 
> > > The QMP query returns a list of valid vq lists. For example, if
> > > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > > be returned
> > > 
> > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > > 
> > > Another example might be 1, 2, 4, as the architecture states 3
> > > is optional. In that case the list would be
> > > 
> > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> > 
> > I think the proposed QMP command is problematic, because it reports
> > the valid vector lengths for either KVM or TCG based on which
> > accelerator is currently enabled: it should report all information
> > at all times instead, similarly to how query-gic-capabilities does
> > it.
> 
> I wonder if this is premature flexibility?
> 
> The size of these lists is going to get cumbersome if the architecture
> is ever extended.  Even today, we might need over 100 items in this
> (nested) list.  If this is to be presented to the user this will be
> far from friendly, it could get much worse if the architecutre changes
> in future to allow larger vectors or more flexible virtualisation.
> 
> Could we just have a list of supported vector lengths and a possibly
> empty list of additional capabilities that describe what kinds of
> flexibility are allowed?
> 
> So, for example, we might support vector lengths of 1, 2, 4 and 8
> quadwords, with the the ability to clamp the max vector length the
> guest sees: the kernel ABI guarantees that you can do this, even
> if you can't disable/enable each individual vector length independently.
> 
> So, [ 1, 2, 4, 8 ] seems sufficient to describe this in a forwards
> compatible way.
> 
> Some day, we might report { "independent", [ 1, 2, 4, 8, 16, 32, ... ] }
> 
> I'm guessing about the data representation here.
> 

I think that could work, and something along those lines even crossed my
mind. Let's see what libvirt folk say. I'm not overly concerned about
user friendless here though, as users aren't running QMP commands and
parsing json by hand too much.

Thanks,
drew

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Dave Martin]

On Mon, May 13, 2019 at 01:38:57PM +0100, Andrew Jones wrote:
> On Mon, May 13, 2019 at 12:15:56PM +0100, Dave Martin wrote:
> > On Mon, May 13, 2019 at 10:32:46AM +0100, Andrea Bolognani wrote:
> > > On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:

[...]

> > > > The QMP query returns a list of valid vq lists. For example, if
> > > > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > > > be returned
> > > > 
> > > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > > > 
> > > > Another example might be 1, 2, 4, as the architecture states 3
> > > > is optional. In that case the list would be
> > > > 
> > > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> > > 
> > > I think the proposed QMP command is problematic, because it reports
> > > the valid vector lengths for either KVM or TCG based on which
> > > accelerator is currently enabled: it should report all information
> > > at all times instead, similarly to how query-gic-capabilities does
> > > it.
> > 
> > I wonder if this is premature flexibility?
> > 
> > The size of these lists is going to get cumbersome if the architecture
> > is ever extended.  Even today, we might need over 100 items in this
> > (nested) list.  If this is to be presented to the user this will be
> > far from friendly, it could get much worse if the architecutre changes
> > in future to allow larger vectors or more flexible virtualisation.
> > 
> > Could we just have a list of supported vector lengths and a possibly
> > empty list of additional capabilities that describe what kinds of
> > flexibility are allowed?
> > 
> > So, for example, we might support vector lengths of 1, 2, 4 and 8
> > quadwords, with the the ability to clamp the max vector length the
> > guest sees: the kernel ABI guarantees that you can do this, even
> > if you can't disable/enable each individual vector length independently.
> > 
> > So, [ 1, 2, 4, 8 ] seems sufficient to describe this in a forwards
> > compatible way.
> > 
> > Some day, we might report { "independent", [ 1, 2, 4, 8, 16, 32, ... ] }
> > 
> > I'm guessing about the data representation here.
> > 
> 
> I think that could work, and something along those lines even crossed my
> mind. Let's see what libvirt folk say. I'm not overly concerned about
> user friendless here though, as users aren't running QMP commands and
> parsing json by hand too much.

One concern I have is that if the architecture ever supports masking out
vector lengths individually, the size of this description becomes
something like O(2^n) in the maximum vector length, instead of O(n^2).

That could be very painful to deal with...

Cheers
---Dave

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Mon, May 13, 2019 at 11:32:46AM +0200, Andrea Bolognani wrote:
> On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:
> [...]
> >    CPU type | accel | sve-max-vq | sve-vls-map
> >    -------------------------------------------
> >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> >  3)    host | kvm   |  N/A       |  $VLS_MAP
> > 
> > Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> > all supported when $VLS_MAP is zero, or when the vqs are selected
> > in $VLS_MAP.
> 
> I'm a bit confused by the nomenclature here. VL clearly stands for
> Vector Length, but what does VQ stand for? You seem to be using the
> two terms pretty much interchangeably throughout the cover letter.

As Dave pointed out, they're both lengths, but VQ specifically points
out that the unit is 'Q'uadwords. We could use VQS instead of VLS,
"Vector Lengths" sounds better.

> 
> [...]
> > There is never any need to provide both properties, but if both
> > are provided then they are checked for consistency.
> 
> I would personally just error out when both are provided.

I'm fine with that if nobody else objects.

> 
> > The QMP query returns a list of valid vq lists. For example, if
> > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > be returned
> > 
> >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > 
> > Another example might be 1, 2, 4, as the architecture states 3
> > is optional. In that case the list would be
> > 
> >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> 
> I think the proposed QMP command is problematic, because it reports
> the valid vector lengths for either KVM or TCG based on which
> accelerator is currently enabled: it should report all information
> at all times instead, similarly to how query-gic-capabilities does
> it.

OK, and then with a flag stating which is when then. Dave points out
we may want to reduce the list to a single set and then add flags
to indicate what can be done with it in order to derive other sets.
What do you think about that?

> 
> [...]
> > And now for what might be a bit more controversial; how we input
> > the valid vector set with sve-vls-map. Well, sve-vls-map is a
> > 64-bit bitmap, which is admittedly not user friendly and also not
> > the same size as KVM's vls bitmap (which is 8 64-bit words). Here's
> > the justification:
> > 
> >  1) We want to use the QEMU command line in order for the information
> >     to be migrated without needing to add more VM state.
> >  2) It's not easy/pretty to input arrays on the QEMU command line.
> >  3) We already need to use the QMP query to get a valid set, which
> >     isn't user friendly either, meaning we're already in libvirt
> >     territory.
> >  4) A 64-bit map (supporting up to 8192-bit vectors) is likely big
> >     enough for quite some time (currently KVM and TCG only support
> >     2048-bit vectors).
> >  5) If user friendliness is needed more than migratability then
> >     the 'max' cpu type can be used with the sve-max-vq property.
> >  6) It's possible to probe the full valid vector set from the
> >     command line by using something like sve-vls-map=0xffff and
> >     then, when it fails, the error message will state the correct
> >     map, e.g. 0xb.
> 
> I don't have a problem with having to use a bitmap internally,
> though libvirt will clearly want to expose a more approachable
> interface to users.
> 
> However, QMP reporting the information in the current format means
> we'd have to build an additional parser on top of the bitmap handling
> and conversion routines we'll clearly need to make this work; plus it
> just feels weird that the information reported by QMP can't be used
> on the command line without going through some tranformation first.
> 
> Wouldn't it make more sense to both accept and report bitmaps?

If we eventually need more than one word for the bitmap then it'll
require parsing and bitmap composition code in libvirt anyway. I
was thinking by pointing out each bit separately that we could
boundlessly grow the list without having to change anything in
libvirt later.

Thanks,
drew

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrea Bolognani]

On Mon, 2019-05-13 at 14:36 +0200, Andrew Jones wrote:
> On Mon, May 13, 2019 at 11:32:46AM +0200, Andrea Bolognani wrote:
> > On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:
> > [...]
> > >    CPU type | accel | sve-max-vq | sve-vls-map
> > >    -------------------------------------------
> > >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> > >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> > >  3)    host | kvm   |  N/A       |  $VLS_MAP
> > > 
> > > Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> > > all supported when $VLS_MAP is zero, or when the vqs are selected
> > > in $VLS_MAP.
> > 
> > I'm a bit confused by the nomenclature here. VL clearly stands for
> > Vector Length, but what does VQ stand for? You seem to be using the
> > two terms pretty much interchangeably throughout the cover letter.
> 
> As Dave pointed out, they're both lengths, but VQ specifically points
> out that the unit is 'Q'uadwords. We could use VQS instead of VLS,
> "Vector Lengths" sounds better.

Alright, it makes sense - once you've managed to figure out what
exactly a "quadword" is, at least :)

Since we expect management applications to use QMP to discover what
vector lengths are supported and then provide an explicit map, I
think it's fair to say that the ability to specify a single maximum
vector length is going to be exclusively used as a convenience for
command line users.

In that light, I think it would be reasonable for the usage to look
along the lines of

  -cpu host,sve-vl-map=0xd # machine-friendly variant
  -cpu max,sve-vl-max=512  # user-friendly variant

with documentation clearly pointing out that the two options expect
completely different formats - but that was the case even before,
so we would have had to document that anyway.

> > > The QMP query returns a list of valid vq lists. For example, if
> > > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > > be returned
> > > 
> > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > > 
> > > Another example might be 1, 2, 4, as the architecture states 3
> > > is optional. In that case the list would be
> > > 
> > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> > 
> > I think the proposed QMP command is problematic, because it reports
> > the valid vector lengths for either KVM or TCG based on which
> > accelerator is currently enabled: it should report all information
> > at all times instead, similarly to how query-gic-capabilities does
> > it.
> 
> OK, and then with a flag stating which is when then.

Correct.

> Dave points out
> we may want to reduce the list to a single set and then add flags
> to indicate what can be done with it in order to derive other sets.
> What do you think about that?

So right now all that can be done is truncating the list by removing
an arbitrary number of elements from the end, right? Eg. if you have
[ 1, 2, 4 ] you can use either that or [ 1, 2 ] or [ 1 ]. But in the
future you might also be able to mask single elements in the middle
of the list, thus enabling things like [ 1, 4 ].

That doesn't sound very complicated to support in libvirt, though I
have to say that I'm not a big fan of this proposal because as far as
I can see it basically means implementing the very same logic twice,
once in QEMU and then once more in libvirt.

> > [...]
> > > And now for what might be a bit more controversial; how we input
> > > the valid vector set with sve-vls-map. Well, sve-vls-map is a
> > > 64-bit bitmap, which is admittedly not user friendly and also not
> > > the same size as KVM's vls bitmap (which is 8 64-bit words). Here's
> > > the justification:
> > > 
> > >  1) We want to use the QEMU command line in order for the information
> > >     to be migrated without needing to add more VM state.
> > >  2) It's not easy/pretty to input arrays on the QEMU command line.
> > >  3) We already need to use the QMP query to get a valid set, which
> > >     isn't user friendly either, meaning we're already in libvirt
> > >     territory.
> > >  4) A 64-bit map (supporting up to 8192-bit vectors) is likely big
> > >     enough for quite some time (currently KVM and TCG only support
> > >     2048-bit vectors).
> > >  5) If user friendliness is needed more than migratability then
> > >     the 'max' cpu type can be used with the sve-max-vq property.
> > >  6) It's possible to probe the full valid vector set from the
> > >     command line by using something like sve-vls-map=0xffff and
> > >     then, when it fails, the error message will state the correct
> > >     map, e.g. 0xb.
> > 
> > I don't have a problem with having to use a bitmap internally,
> > though libvirt will clearly want to expose a more approachable
> > interface to users.
> > 
> > However, QMP reporting the information in the current format means
> > we'd have to build an additional parser on top of the bitmap handling
> > and conversion routines we'll clearly need to make this work; plus it
> > just feels weird that the information reported by QMP can't be used
> > on the command line without going through some tranformation first.
> > 
> > Wouldn't it make more sense to both accept and report bitmaps?
> 
> If we eventually need more than one word for the bitmap then it'll
> require parsing and bitmap composition code in libvirt anyway. I
> was thinking by pointing out each bit separately that we could
> boundlessly grow the list without having to change anything in
> libvirt later.

If the size of the bitmap on the KVM side is 512 bits, why don't we
just make it that size on the QEMU side too from the start?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Tue, May 14, 2019 at 02:29:51PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-05-13 at 14:36 +0200, Andrew Jones wrote:
> > On Mon, May 13, 2019 at 11:32:46AM +0200, Andrea Bolognani wrote:
> > > On Sun, 2019-05-12 at 10:36 +0200, Andrew Jones wrote:
> > > [...]
> > > >    CPU type | accel | sve-max-vq | sve-vls-map
> > > >    -------------------------------------------
> > > >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> > > >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> > > >  3)    host | kvm   |  N/A       |  $VLS_MAP
> > > > 
> > > > Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> > > > all supported when $VLS_MAP is zero, or when the vqs are selected
> > > > in $VLS_MAP.
> > > 
> > > I'm a bit confused by the nomenclature here. VL clearly stands for
> > > Vector Length, but what does VQ stand for? You seem to be using the
> > > two terms pretty much interchangeably throughout the cover letter.
> > 
> > As Dave pointed out, they're both lengths, but VQ specifically points
> > out that the unit is 'Q'uadwords. We could use VQS instead of VLS,
> > "Vector Lengths" sounds better.
> 
> Alright, it makes sense - once you've managed to figure out what
> exactly a "quadword" is, at least :)
> 
> Since we expect management applications to use QMP to discover what
> vector lengths are supported and then provide an explicit map, I
> think it's fair to say that the ability to specify a single maximum
> vector length is going to be exclusively used as a convenience for
> command line users.
> 
> In that light, I think it would be reasonable for the usage to look
> along the lines of
> 
>   -cpu host,sve-vl-map=0xd # machine-friendly variant
>   -cpu max,sve-vl-max=512  # user-friendly variant

We already have sve-max-vq, so I'm not sure we want to rename it.

> 
> with documentation clearly pointing out that the two options expect
> completely different formats - but that was the case even before,
> so we would have had to document that anyway.
> 
> > > > The QMP query returns a list of valid vq lists. For example, if
> > > > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > > > be returned
> > > > 
> > > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > > > 
> > > > Another example might be 1, 2, 4, as the architecture states 3
> > > > is optional. In that case the list would be
> > > > 
> > > >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> > > 
> > > I think the proposed QMP command is problematic, because it reports
> > > the valid vector lengths for either KVM or TCG based on which
> > > accelerator is currently enabled: it should report all information
> > > at all times instead, similarly to how query-gic-capabilities does
> > > it.
> > 
> > OK, and then with a flag stating which is when then.
> 
> Correct.
> 
> > Dave points out
> > we may want to reduce the list to a single set and then add flags
> > to indicate what can be done with it in order to derive other sets.
> > What do you think about that?
> 
> So right now all that can be done is truncating the list by removing
> an arbitrary number of elements from the end, right? Eg. if you have
> [ 1, 2, 4 ] you can use either that or [ 1, 2 ] or [ 1 ]. But in the
> future you might also be able to mask single elements in the middle
> of the list, thus enabling things like [ 1, 4 ].
> 
> That doesn't sound very complicated to support in libvirt, though I
> have to say that I'm not a big fan of this proposal because as far as
> I can see it basically means implementing the very same logic twice,
> once in QEMU and then once more in libvirt.

So if big tables of bits aren't a problem for QMP queries, then I'll
just leave the design as it is.

> 
> > > [...]
> > > > And now for what might be a bit more controversial; how we input
> > > > the valid vector set with sve-vls-map. Well, sve-vls-map is a
> > > > 64-bit bitmap, which is admittedly not user friendly and also not
> > > > the same size as KVM's vls bitmap (which is 8 64-bit words). Here's
> > > > the justification:
> > > > 
> > > >  1) We want to use the QEMU command line in order for the information
> > > >     to be migrated without needing to add more VM state.
> > > >  2) It's not easy/pretty to input arrays on the QEMU command line.
> > > >  3) We already need to use the QMP query to get a valid set, which
> > > >     isn't user friendly either, meaning we're already in libvirt
> > > >     territory.
> > > >  4) A 64-bit map (supporting up to 8192-bit vectors) is likely big
> > > >     enough for quite some time (currently KVM and TCG only support
> > > >     2048-bit vectors).
> > > >  5) If user friendliness is needed more than migratability then
> > > >     the 'max' cpu type can be used with the sve-max-vq property.
> > > >  6) It's possible to probe the full valid vector set from the
> > > >     command line by using something like sve-vls-map=0xffff and
> > > >     then, when it fails, the error message will state the correct
> > > >     map, e.g. 0xb.
> > > 
> > > I don't have a problem with having to use a bitmap internally,
> > > though libvirt will clearly want to expose a more approachable
> > > interface to users.
> > > 
> > > However, QMP reporting the information in the current format means
> > > we'd have to build an additional parser on top of the bitmap handling
> > > and conversion routines we'll clearly need to make this work; plus it
> > > just feels weird that the information reported by QMP can't be used
> > > on the command line without going through some tranformation first.
> > > 
> > > Wouldn't it make more sense to both accept and report bitmaps?
> > 
> > If we eventually need more than one word for the bitmap then it'll
> > require parsing and bitmap composition code in libvirt anyway. I
> > was thinking by pointing out each bit separately that we could
> > boundlessly grow the list without having to change anything in
> > libvirt later.
> 
> If the size of the bitmap on the KVM side is 512 bits, why don't we
> just make it that size on the QEMU side too from the start?

I'd still only want to input 64-bits on the command line, otherwise
we get into inputting arrays, which isn't easy. KVM's interface is
meant for expansion, but it won't be using most of those bits for
quite some time either.

Thanks,
drew

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrea Bolognani]

On Tue, 2019-05-14 at 14:53 +0200, Andrew Jones wrote:
> On Tue, May 14, 2019 at 02:29:51PM +0200, Andrea Bolognani wrote:
> > Since we expect management applications to use QMP to discover what
> > vector lengths are supported and then provide an explicit map, I
> > think it's fair to say that the ability to specify a single maximum
> > vector length is going to be exclusively used as a convenience for
> > command line users.
> > 
> > In that light, I think it would be reasonable for the usage to look
> > along the lines of
> > 
> >   -cpu host,sve-vl-map=0xd # machine-friendly variant
> >   -cpu max,sve-vl-max=512  # user-friendly variant
> 
> We already have sve-max-vq, so I'm not sure we want to rename it.

Oh, I didn't realize that was the case. And of course it already
takes a number of quadwords as argument, I suppose? That's pretty
unfortunate :(

Perhaps we could consider deprecating it in favor of a user-friendly
variant that's actually suitable for regular humans, like the one I
suggest above?

[...]
> > > Dave points out
> > > we may want to reduce the list to a single set and then add flags
> > > to indicate what can be done with it in order to derive other sets.
> > > What do you think about that?
> > 
> > So right now all that can be done is truncating the list by removing
> > an arbitrary number of elements from the end, right? Eg. if you have
> > [ 1, 2, 4 ] you can use either that or [ 1, 2 ] or [ 1 ]. But in the
> > future you might also be able to mask single elements in the middle
> > of the list, thus enabling things like [ 1, 4 ].
> > 
> > That doesn't sound very complicated to support in libvirt, though I
> > have to say that I'm not a big fan of this proposal because as far as
> > I can see it basically means implementing the very same logic twice,
> > once in QEMU and then once more in libvirt.
> 
> So if big tables of bits aren't a problem for QMP queries, then I'll
> just leave the design as it is.

I thought about it a bit more and perhaps the simplified design is
better after all.

Whatever the interface looks like on the QEMU side, we're going to
want to offer libvirt users two options for configuring vector
lengths: listing all desired vector lengths explicitly (basically
sev-vl-map but more verbose and readable) and providing just the
biggest desired vector length (like in sev-max-vq).

In the latter case, we'll want to expand the user-provided value
into an explicit list anyway in order to guarantee guest ABI
stability, and doing so when a single bitmap has been obtained via
QMP seems like it would be more manageable.

Sorry for the flip-flop, but after all isn't this exactly what
upstream design discussion is supposed to be all about? :)

[...]
> > If the size of the bitmap on the KVM side is 512 bits, why don't we
> > just make it that size on the QEMU side too from the start?
> 
> I'd still only want to input 64-bits on the command line, otherwise
> we get into inputting arrays, which isn't easy. KVM's interface is
> meant for expansion, but it won't be using most of those bits for
> quite some time either.

I'm probably missing something entirely obvious, but couldn't you
just have a single, possibly fairly massive (up to 128 hex digits if
I'm not mistaken) value on the command line and just work with that
one, no arrays necessary?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Richard Henderson]

On 5/14/19 9:03 AM, Andrea Bolognani wrote:
> On Tue, 2019-05-14 at 14:53 +0200, Andrew Jones wrote:
>> We already have sve-max-vq, so I'm not sure we want to rename it.
> 
> Oh, I didn't realize that was the case. And of course it already
> takes a number of quadwords as argument, I suppose? That's pretty
> unfortunate :(
> 
> Perhaps we could consider deprecating it in favor of a user-friendly
> variant that's actually suitable for regular humans, like the one I
> suggest above?

Why is =4 less user-friendly than =512?

I don't actually see "total bits in vector" as more user-friendly than "number
of quadwords" when it comes to non-powers-of-2 like =7 vs =896 or =13 vs =1664.


r~

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrea Bolognani]

On Tue, 2019-05-14 at 13:14 -0700, Richard Henderson wrote:
> On 5/14/19 9:03 AM, Andrea Bolognani wrote:
> > On Tue, 2019-05-14 at 14:53 +0200, Andrew Jones wrote:
> > > We already have sve-max-vq, so I'm not sure we want to rename it.
> > 
> > Oh, I didn't realize that was the case. And of course it already
> > takes a number of quadwords as argument, I suppose? That's pretty
> > unfortunate :(
> > 
> > Perhaps we could consider deprecating it in favor of a user-friendly
> > variant that's actually suitable for regular humans, like the one I
> > suggest above?
> 
> Why is =4 less user-friendly than =512?
> 
> I don't actually see "total bits in vector" as more user-friendly than "number
> of quadwords" when it comes to non-powers-of-2 like =7 vs =896 or =13 vs =1664.

I would wager most people are intimately familiar with bits, bytes
and multiples due to having to work with them daily. Quadwords, not
so much.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Dave Martin]

On Wed, May 15, 2019 at 09:03:58AM +0100, Andrea Bolognani wrote:
> On Tue, 2019-05-14 at 13:14 -0700, Richard Henderson wrote:
> > On 5/14/19 9:03 AM, Andrea Bolognani wrote:
> > > On Tue, 2019-05-14 at 14:53 +0200, Andrew Jones wrote:
> > > > We already have sve-max-vq, so I'm not sure we want to rename it.
> > > 
> > > Oh, I didn't realize that was the case. And of course it already
> > > takes a number of quadwords as argument, I suppose? That's pretty
> > > unfortunate :(
> > > 
> > > Perhaps we could consider deprecating it in favor of a user-friendly
> > > variant that's actually suitable for regular humans, like the one I
> > > suggest above?
> > 
> > Why is =4 less user-friendly than =512?
> > 
> > I don't actually see "total bits in vector" as more user-friendly than "number
> > of quadwords" when it comes to non-powers-of-2 like =7 vs =896 or =13 vs =1664.
> 
> I would wager most people are intimately familiar with bits, bytes
> and multiples due to having to work with them daily. Quadwords, not
> so much.

Generally I tend to agree.  For kvmtool I leaned torward quadwords
purely because

	16,32,48,64,80,96,112,128,144,160,176,192,208

is a big pain to type compared with

	1,2,3,4,5,6,7,8,9,10,11,12,13

Even though I prefer to specify vector lengths in bytes everywhere else
in the Linux user API (precisely to avoid the confusion you object to).

This isn't finalised yet for kvmtool -- I need to rework the patches
and may not include it at all initially: kvmtool doesn't support
migration, which is the main usecase for being able to specify an exact
set of vector lengths AFAICT.

Since this is otherwise only useful for migration, experimentation or
machine-driven configuration, a bitmask

	0x1fff

as some have suggested may well be a pragmatic alternative for kvmtool.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrea Bolognani]

On Wed, 2019-05-15 at 12:14 +0100, Dave Martin wrote:
> On Wed, May 15, 2019 at 09:03:58AM +0100, Andrea Bolognani wrote:
> > On Tue, 2019-05-14 at 13:14 -0700, Richard Henderson wrote:
> > > Why is =4 less user-friendly than =512?
> > > 
> > > I don't actually see "total bits in vector" as more user-friendly than "number
> > > of quadwords" when it comes to non-powers-of-2 like =7 vs =896 or =13 vs =1664.
> > 
> > I would wager most people are intimately familiar with bits, bytes
> > and multiples due to having to work with them daily. Quadwords, not
> > so much.
> 
> Generally I tend to agree.  For kvmtool I leaned torward quadwords
> purely because
> 
> 	16,32,48,64,80,96,112,128,144,160,176,192,208
> 
> is a big pain to type compared with
> 
> 	1,2,3,4,5,6,7,8,9,10,11,12,13
> 
> Even though I prefer to specify vector lengths in bytes everywhere else
> in the Linux user API (precisely to avoid the confusion you object to).
> 
> This isn't finalised yet for kvmtool -- I need to rework the patches
> and may not include it at all initially: kvmtool doesn't support
> migration, which is the main usecase for being able to specify an exact
> set of vector lengths AFAICT.
> 
> Since this is otherwise only useful for migration, experimentation or
> machine-driven configuration, a bitmask
> 
> 	0x1fff
> 
> as some have suggested may well be a pragmatic alternative for kvmtool.

Just to be clear, I have suggested using bits (or bytes or megabytes
depending on the exact value) only for the command-line-user-oriented
sve-vl-max option, which would take a single value.

For interoperation with the management layer, on the other hand,
using a bitmap is perfectly fine, and whether the values encoded
within are expressed in quadwords or whatever other format is largely
irrelevant, so long as it it's properly documented of course.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Dave Martin]

On Wed, May 15, 2019 at 12:28:20PM +0100, Andrea Bolognani wrote:
> On Wed, 2019-05-15 at 12:14 +0100, Dave Martin wrote:
> > On Wed, May 15, 2019 at 09:03:58AM +0100, Andrea Bolognani wrote:
> > > On Tue, 2019-05-14 at 13:14 -0700, Richard Henderson wrote:
> > > > Why is =4 less user-friendly than =512?
> > > > 
> > > > I don't actually see "total bits in vector" as more user-friendly than "number
> > > > of quadwords" when it comes to non-powers-of-2 like =7 vs =896 or =13 vs =1664.
> > > 
> > > I would wager most people are intimately familiar with bits, bytes
> > > and multiples due to having to work with them daily. Quadwords, not
> > > so much.
> > 
> > Generally I tend to agree.  For kvmtool I leaned torward quadwords
> > purely because
> > 
> > 	16,32,48,64,80,96,112,128,144,160,176,192,208
> > 
> > is a big pain to type compared with
> > 
> > 	1,2,3,4,5,6,7,8,9,10,11,12,13
> > 
> > Even though I prefer to specify vector lengths in bytes everywhere else
> > in the Linux user API (precisely to avoid the confusion you object to).
> > 
> > This isn't finalised yet for kvmtool -- I need to rework the patches
> > and may not include it at all initially: kvmtool doesn't support
> > migration, which is the main usecase for being able to specify an exact
> > set of vector lengths AFAICT.
> > 
> > Since this is otherwise only useful for migration, experimentation or
> > machine-driven configuration, a bitmask
> > 
> > 	0x1fff
> > 
> > as some have suggested may well be a pragmatic alternative for kvmtool.
> 
> Just to be clear, I have suggested using bits (or bytes or megabytes
> depending on the exact value) only for the command-line-user-oriented
> sve-vl-max option, which would take a single value.
> 
> For interoperation with the management layer, on the other hand,
> using a bitmap is perfectly fine, and whether the values encoded
> within are expressed in quadwords or whatever other format is largely
> irrelevant, so long as it it's properly documented of course.

Seems fair.

Cheers
---Dave

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Tue, May 14, 2019 at 06:03:09PM +0200, Andrea Bolognani wrote:
> I thought about it a bit more and perhaps the simplified design is
> better after all.
> 
> Whatever the interface looks like on the QEMU side, we're going to
> want to offer libvirt users two options for configuring vector
> lengths: listing all desired vector lengths explicitly (basically
> sev-vl-map but more verbose and readable) and providing just the
> biggest desired vector length (like in sev-max-vq).
> 
> In the latter case, we'll want to expand the user-provided value
> into an explicit list anyway in order to guarantee guest ABI
> stability, and doing so when a single bitmap has been obtained via
> QMP seems like it would be more manageable.
> 
> Sorry for the flip-flop, but after all isn't this exactly what
> upstream design discussion is supposed to be all about? :)

Yup, no problem. I'm actually liking the idea of the single map plus
flags. We won't need two implementations (QEMU and libvirt), we'll
only need one (libvirt). The QEMU QMP side will only need to state
what should be implemented using the flags. Also, as we already
agreed, we need TCG and KVM flags anyway, so we're already in flag
land.

> 
> [...]
> > > If the size of the bitmap on the KVM side is 512 bits, why don't we
> > > just make it that size on the QEMU side too from the start?
> > 
> > I'd still only want to input 64-bits on the command line, otherwise
> > we get into inputting arrays, which isn't easy. KVM's interface is
> > meant for expansion, but it won't be using most of those bits for
> > quite some time either.
> 
> I'm probably missing something entirely obvious, but couldn't you
> just have a single, possibly fairly massive (up to 128 hex digits if
> I'm not mistaken) value on the command line and just work with that
> one, no arrays necessary?
>

We could, and I like the idea. It just hadn't crossed my mind due to
implementation tunnel vision.

Thanks,
drew

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Peter Maydell]

On Sun, 12 May 2019 at 09:36, Andrew Jones <drjones@redhat.com> wrote:
>
> With the recent KVM guest SVE support pull request [1] KVM will be
> ready for guests with SVE. This series provides the QEMU bits for
> that enablement. The series starts with the bits needed for the KVM
> SVE ioctls. Then it enables the arm 'max'cpu type, which with TCG
> already supports SVE, to also support SVE when using KVM. Next
> a new QMP query is added that allows users to ask which vector
> lengths are supported by the host, allowing them to select a valid
> set of vectors for the guest. In order to select those vectors a
> new property 'sve-vls-map' is added to the 'max' cpu type, and then
> also to the 'host' cpu type. The table below shows the resulting user
> interfaces.
>
>    CPU type | accel | sve-max-vq | sve-vls-map
>    -------------------------------------------
>  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
>  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
>  3)    host | kvm   |  N/A       |  $VLS_MAP
>
> Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> all supported when $VLS_MAP is zero, or when the vqs are selected
> in $VLS_MAP.
>
> (2) is the same as (1) except KVM has the final say on what
> vqs are valid.
>
> (3) doesn't accept sve-max-vq because a guest that uses this
> property without sve-vls-map cannot be safely migrated.

Is this "migrated between two hosts with the same host CPU
type but with different kernel versions which expose different
subsets of the host's permitted vector lengths" ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Mon, May 13, 2019 at 10:52:06AM +0100, Peter Maydell wrote:
> On Sun, 12 May 2019 at 09:36, Andrew Jones <drjones@redhat.com> wrote:
> >
> > With the recent KVM guest SVE support pull request [1] KVM will be
> > ready for guests with SVE. This series provides the QEMU bits for
> > that enablement. The series starts with the bits needed for the KVM
> > SVE ioctls. Then it enables the arm 'max'cpu type, which with TCG
> > already supports SVE, to also support SVE when using KVM. Next
> > a new QMP query is added that allows users to ask which vector
> > lengths are supported by the host, allowing them to select a valid
> > set of vectors for the guest. In order to select those vectors a
> > new property 'sve-vls-map' is added to the 'max' cpu type, and then
> > also to the 'host' cpu type. The table below shows the resulting user
> > interfaces.
> >
> >    CPU type | accel | sve-max-vq | sve-vls-map
> >    -------------------------------------------
> >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> >  3)    host | kvm   |  N/A       |  $VLS_MAP
> >
> > Where for (1) $MAX_VQ sets the maximum vq and smaller vqs are
> > all supported when $VLS_MAP is zero, or when the vqs are selected
> > in $VLS_MAP.
> >
> > (2) is the same as (1) except KVM has the final say on what
> > vqs are valid.
> >
> > (3) doesn't accept sve-max-vq because a guest that uses this
> > property without sve-vls-map cannot be safely migrated.
> 
> Is this "migrated between two hosts with the same host CPU
> type but with different kernel versions which expose different
> subsets of the host's permitted vector lengths" ?
>

That's one example. Also, I see attempting to make/use only migrate-safe
cpu properties for our primary KVM cpu type ('host') as a step in the
right direction to eventually making a more migratable KVM cpu type. But
I do see that (3) is a bit of an oxymoron considering the 'host' cpu
type isn't really migrate-safe unless the hosts are identical (which
should technically require the same host kernel too) anyway.

Thanks,
drew

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Richard Henderson]

On 5/12/19 1:36 AM, Andrew Jones wrote:
>    CPU type | accel | sve-max-vq | sve-vls-map
>    -------------------------------------------
>  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
>  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
>  3)    host | kvm   |  N/A       |  $VLS_MAP

This doesn't seem right.  Why is -cpu host not whatever the host supports?  It
certainly has been so far.  I really don't see how -cpu max makes any sense for
kvm.


> The QMP query returns a list of valid vq lists. For example, if
> a guest can use vqs 1, 2, 3, and 4, then the following list will
> be returned
> 
>  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> 
> Another example might be 1, 2, 4, as the architecture states 3
> is optional. In that case the list would be
> 
>  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> 
> This may look redundant, but it's necessary to provide a future-
> proof query, because while KVM currently requires vector sets to
> be strict truncations of the full valid vector set, that may change
> at some point.

How and why would that make sense?

Real hardware is going to support one set of vector lengths.  Whether VQ=3 is
valid or not is not going to depend on the maximum VQ, surely.

I'll also note that if we want to support the theoretical
beyond-current-architecture maximum VQ=512, such that migration works
seemlessly with current hardware, then we're going to have to change the
migration format.

So far I'm supporting only the current architecture maximum VQ=16.  Which
seemed plenty, given that the first round of hardware only supports VQ=4.



r~

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Andrew Jones]

On Mon, May 13, 2019 at 11:46:29AM -0700, Richard Henderson wrote:
> On 5/12/19 1:36 AM, Andrew Jones wrote:
> >    CPU type | accel | sve-max-vq | sve-vls-map
> >    -------------------------------------------
> >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> >  3)    host | kvm   |  N/A       |  $VLS_MAP
> 
> This doesn't seem right.  Why is -cpu host not whatever the host supports?  It
> certainly has been so far. 

-cpu host can support whatever the host (hardware + KVM ) supports, but if
a user doesn't want to expose all of it to the guest, then the user doesn't
have to. For example, the host cpu may have a PMU, but the guest doesn't
necessarily get one (-cpu host,pmu=off).

> I really don't see how -cpu max makes any sense for
> kvm.
>

It's already supported for kvm. This series just extends that support
to match tcg's sve support. The reason it's supported is that you can
then use '-cpu max' along with '-machine accel=kvm:tcg' in a command
line and it'll just work.
 
> 
> > The QMP query returns a list of valid vq lists. For example, if
> > a guest can use vqs 1, 2, 3, and 4, then the following list will
> > be returned
> > 
> >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 3 ], [ 1, 2, 3, 4 ] ]
> > 
> > Another example might be 1, 2, 4, as the architecture states 3
> > is optional. In that case the list would be
> > 
> >  [ [ 1 ], [ 1, 2 ], [ 1, 2, 4 ] ]
> > 
> > This may look redundant, but it's necessary to provide a future-
> > proof query, because while KVM currently requires vector sets to
> > be strict truncations of the full valid vector set, that may change
> > at some point.
> 
> How and why would that make sense?
> 
> Real hardware is going to support one set of vector lengths.

The guest's view of the hardware will be a single set. That set can be
different for different guests though, within the constraints of the
architecture and KVM or TCG implementations.

> Whether VQ=3 is
> valid or not is not going to depend on the maximum VQ, surely.

Exactly. That's why we need a way to explicitly state what is supported.
We can't assume anything from the max VQ alone. Additionally, while
strict truncation is required now for KVM, things may be more flexible
later. TCG is already more flexible. For TCG, all sets that at least
include all the power-of-2 lengths up to the maximum VQ are valid, as
the architecture states. Plus, all the sets that can be derived by adding
one ore more optional lengths to those sets are also valid. Listing each
of them allows management software to know what's going to work and what's
not without having to know all the rules itself.

> 
> I'll also note that if we want to support the theoretical
> beyond-current-architecture maximum VQ=512, such that migration works
> seemlessly with current hardware, then we're going to have to change the
> migration format.
> 
> So far I'm supporting only the current architecture maximum VQ=16.  Which
> seemed plenty, given that the first round of hardware only supports VQ=4.
> 

I agree. The changes won't be small to QEMU, but hopefully we can design
a QMP query that won't need to change, saving libvirt some pain.

Thanks,
drew

Re: [Qemu-devel] [PATCH 00/13] target/arm/kvm: enable SVE in guests

[Peter Maydell]

On Mon, 13 May 2019 at 19:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/12/19 1:36 AM, Andrew Jones wrote:
> >    CPU type | accel | sve-max-vq | sve-vls-map
> >    -------------------------------------------
> >  1)     max | tcg   |  $MAX_VQ   |  $VLS_MAP
> >  2)     max | kvm   |  $MAX_VQ   |  $VLS_MAP
> >  3)    host | kvm   |  N/A       |  $VLS_MAP
>
> This doesn't seem right.  Why is -cpu host not whatever the host supports?  It
> certainly has been so far.  I really don't see how -cpu max makes any sense for
> kvm.

The point of '-cpu max' is that it works and gives you the
best thing QEMU can support regardless of what accelerator
is in use. This means that you don't need to do tedious
workarounds like "if KVM then -cpu host else -cpu somethingelse".

thanks
-- PMM