[PATCH 00/14] KVM: arm64: SVE cleanups

[PATCH 00/14] KVM: arm64: SVE cleanups

[Dave Martin]

This series contains some cleanups applicable to the SVE KVM support
patches merged into kvmarm/next.  These arose from Andrew Jones'
review.

Apart from some minor changes to error codes and checking, these are
mostly cosmetic / sytlistic changes only.

Although straightforward, these changes have not been tested yet:
I'm posting them now so that reviewers can make a start on them.

The patches are based on
git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
5d8d4af24460 ("arm64: KVM: Fix system register enumeration")

This series in git:
git://linux-arm.org/linux-dm.git sve-kvm-fixes/v1/head
http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/sve-kvm-fixes/v1/head

Dave Martin (14):
  arm64/sve: Clarify vq map semantics
  KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up
    SVE
  KVM: arm: Make vcpu finalization stubs into inline functions
  KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to
    WARNs
  KVM: arm64/sve: Clean up UAPI register ID definitions
  KVM: arm64/sve: Miscellaneous tidyups in guest.c
  KVM: arm64/sve: Make register ioctl access errors more consistent
  KVM: arm64/sve: WARN when avoiding divide-by-zero in
    sve_reg_to_region()
  KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing
  KVM: arm64/sve: Explain validity checks in set_sve_vls()
  KVM: arm/arm64: Clean up vcpu finalization function parameter naming
  KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE
  KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation
  KVM: arm64: Clarify access behaviour for out-of-range SVE register
    slice IDs

 Documentation/virtual/kvm/api.txt | 24 ++++++-----
 arch/arm/include/asm/kvm_host.h   | 13 ++++--
 arch/arm64/include/asm/fpsimd.h   |  4 --
 arch/arm64/include/asm/kvm_host.h |  4 +-
 arch/arm64/include/uapi/asm/kvm.h | 32 ++++++++++----
 arch/arm64/kernel/fpsimd.c        |  7 ++-
 arch/arm64/kvm/guest.c            | 91 +++++++++++++++++++++++----------------
 arch/arm64/kvm/reset.c            |  6 +--
 arch/arm64/kvm/sys_regs.c         |  4 +-
 virt/kvm/arm/arm.c                |  2 +-
 10 files changed, 116 insertions(+), 71 deletions(-)

-- 
2.1.4

[PATCH 00/14] KVM: arm64: SVE cleanups

[Dave Martin]

This series contains some cleanups applicable to the SVE KVM support
patches merged into kvmarm/next.  These arose from Andrew Jones'
review.

Apart from some minor changes to error codes and checking, these are
mostly cosmetic / sytlistic changes only.

Although straightforward, these changes have not been tested yet:
I'm posting them now so that reviewers can make a start on them.

The patches are based on
git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
5d8d4af24460 ("arm64: KVM: Fix system register enumeration")

This series in git:
git://linux-arm.org/linux-dm.git sve-kvm-fixes/v1/head
http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/sve-kvm-fixes/v1/head

Dave Martin (14):
  arm64/sve: Clarify vq map semantics
  KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up
    SVE
  KVM: arm: Make vcpu finalization stubs into inline functions
  KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to
    WARNs
  KVM: arm64/sve: Clean up UAPI register ID definitions
  KVM: arm64/sve: Miscellaneous tidyups in guest.c
  KVM: arm64/sve: Make register ioctl access errors more consistent
  KVM: arm64/sve: WARN when avoiding divide-by-zero in
    sve_reg_to_region()
  KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing
  KVM: arm64/sve: Explain validity checks in set_sve_vls()
  KVM: arm/arm64: Clean up vcpu finalization function parameter naming
  KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE
  KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation
  KVM: arm64: Clarify access behaviour for out-of-range SVE register
    slice IDs

 Documentation/virtual/kvm/api.txt | 24 ++++++-----
 arch/arm/include/asm/kvm_host.h   | 13 ++++--
 arch/arm64/include/asm/fpsimd.h   |  4 --
 arch/arm64/include/asm/kvm_host.h |  4 +-
 arch/arm64/include/uapi/asm/kvm.h | 32 ++++++++++----
 arch/arm64/kernel/fpsimd.c        |  7 ++-
 arch/arm64/kvm/guest.c            | 91 +++++++++++++++++++++++----------------
 arch/arm64/kvm/reset.c            |  6 +--
 arch/arm64/kvm/sys_regs.c         |  4 +-
 virt/kvm/arm/arm.c                |  2 +-
 10 files changed, 116 insertions(+), 71 deletions(-)

-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 01/14] arm64/sve: Clarify vq map semantics

[Dave Martin]

Currently the meanings of sve_vq_map and the ancillary helpers
__bit_to_vq() and __vq_to_bit() are not clearly explained.

This patch makes the explanatory comment clearer, and removes the
duplicate comment from fpsimd.h.

The WARN_ON() currently present in __bit_to_vq() confuses the
intended use of this helper.  Since these are low-level helpers not
intended for general-purpose use anyway, it is better not to make
guesses about how these functions will be used: rather, this patch
removes the WARN_ON() and relies on callers to use the helpers
sensibly.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/fpsimd.h | 4 ----
 arch/arm64/kernel/fpsimd.c      | 7 ++++++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index ad6d2e4..df62bbd 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -92,7 +92,6 @@ extern u64 read_zcr_features(void);
 
 extern int __ro_after_init sve_max_vl;
 extern int __ro_after_init sve_max_virtualisable_vl;
-/* Set of available vector lengths, as vq_to_bit(vq): */
 extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
 
 /*
@@ -107,9 +106,6 @@ static inline unsigned int __vq_to_bit(unsigned int vq)
 
 static inline unsigned int __bit_to_vq(unsigned int bit)
 {
-	if (WARN_ON(bit >= SVE_VQ_MAX))
-		bit = SVE_VQ_MAX - 1;
-
 	return SVE_VQ_MAX - bit;
 }
 
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 577296b..56afa40 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -135,10 +135,15 @@ static int sve_default_vl = -1;
 /* Maximum supported vector length across all CPUs (initially poisoned) */
 int __ro_after_init sve_max_vl = SVE_VL_MIN;
 int __ro_after_init sve_max_virtualisable_vl = SVE_VL_MIN;
-/* Set of available vector lengths, as vq_to_bit(vq): */
+
+/*
+ * Set of available vector lengths,
+ * where length vq encoded as bit __vq_to_bit(vq):
+ */
 __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
 /* Set of vector lengths present on at least one cpu: */
 static __ro_after_init DECLARE_BITMAP(sve_vq_partial_map, SVE_VQ_MAX);
+
 static void __percpu *efi_sve_state;
 
 #else /* ! CONFIG_ARM64_SVE */
-- 
2.1.4

[PATCH 01/14] arm64/sve: Clarify vq map semantics

[Dave Martin]

Currently the meanings of sve_vq_map and the ancillary helpers
__bit_to_vq() and __vq_to_bit() are not clearly explained.

This patch makes the explanatory comment clearer, and removes the
duplicate comment from fpsimd.h.

The WARN_ON() currently present in __bit_to_vq() confuses the
intended use of this helper.  Since these are low-level helpers not
intended for general-purpose use anyway, it is better not to make
guesses about how these functions will be used: rather, this patch
removes the WARN_ON() and relies on callers to use the helpers
sensibly.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/fpsimd.h | 4 ----
 arch/arm64/kernel/fpsimd.c      | 7 ++++++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index ad6d2e4..df62bbd 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -92,7 +92,6 @@ extern u64 read_zcr_features(void);
 
 extern int __ro_after_init sve_max_vl;
 extern int __ro_after_init sve_max_virtualisable_vl;
-/* Set of available vector lengths, as vq_to_bit(vq): */
 extern __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
 
 /*
@@ -107,9 +106,6 @@ static inline unsigned int __vq_to_bit(unsigned int vq)
 
 static inline unsigned int __bit_to_vq(unsigned int bit)
 {
-	if (WARN_ON(bit >= SVE_VQ_MAX))
-		bit = SVE_VQ_MAX - 1;
-
 	return SVE_VQ_MAX - bit;
 }
 
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 577296b..56afa40 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -135,10 +135,15 @@ static int sve_default_vl = -1;
 /* Maximum supported vector length across all CPUs (initially poisoned) */
 int __ro_after_init sve_max_vl = SVE_VL_MIN;
 int __ro_after_init sve_max_virtualisable_vl = SVE_VL_MIN;
-/* Set of available vector lengths, as vq_to_bit(vq): */
+
+/*
+ * Set of available vector lengths,
+ * where length vq encoded as bit __vq_to_bit(vq):
+ */
 __ro_after_init DECLARE_BITMAP(sve_vq_map, SVE_VQ_MAX);
 /* Set of vector lengths present on at least one cpu: */
 static __ro_after_init DECLARE_BITMAP(sve_vq_partial_map, SVE_VQ_MAX);
+
 static void __percpu *efi_sve_state;
 
 #else /* ! CONFIG_ARM64_SVE */
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 02/14] KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up SVE

[Dave Martin]

The introduction of kvm_arm_init_arch_resources() looks like
premature factoring, since nothing else uses this hook yet and it
is not clear what will use it in the future.

For now, let's not pretend that this is a general thing:

This patch simply renames the function to kvm_arm_init_sve(),
retaining the arm stub version under the new name.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 2 +-
 arch/arm64/include/asm/kvm_host.h | 2 +-
 arch/arm64/kvm/reset.c            | 2 +-
 virt/kvm/arm/arm.c                | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index e80cfc1..d956273 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -54,7 +54,7 @@
 
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
-static inline int kvm_arm_init_arch_resources(void) { return 0; }
+static inline int kvm_arm_init_sve(void) { return 0; }
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 9d57cf8..6adf08b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -59,7 +59,7 @@
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
 extern unsigned int kvm_sve_max_vl;
-int kvm_arm_init_arch_resources(void);
+int kvm_arm_init_sve(void);
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index f13378d..8847f38 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -110,7 +110,7 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 
 unsigned int kvm_sve_max_vl;
 
-int kvm_arm_init_arch_resources(void)
+int kvm_arm_init_sve(void)
 {
 	if (system_supports_sve()) {
 		kvm_sve_max_vl = sve_max_virtualisable_vl;
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9edbf0f..7039c99c 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -1682,7 +1682,7 @@ int kvm_arch_init(void *opaque)
 	if (err)
 		return err;
 
-	err = kvm_arm_init_arch_resources();
+	err = kvm_arm_init_sve();
 	if (err)
 		return err;
 
-- 
2.1.4

[PATCH 02/14] KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up SVE

[Dave Martin]

The introduction of kvm_arm_init_arch_resources() looks like
premature factoring, since nothing else uses this hook yet and it
is not clear what will use it in the future.

For now, let's not pretend that this is a general thing:

This patch simply renames the function to kvm_arm_init_sve(),
retaining the arm stub version under the new name.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 2 +-
 arch/arm64/include/asm/kvm_host.h | 2 +-
 arch/arm64/kvm/reset.c            | 2 +-
 virt/kvm/arm/arm.c                | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index e80cfc1..d956273 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -54,7 +54,7 @@
 
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
-static inline int kvm_arm_init_arch_resources(void) { return 0; }
+static inline int kvm_arm_init_sve(void) { return 0; }
 
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 9d57cf8..6adf08b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -59,7 +59,7 @@
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
 extern unsigned int kvm_sve_max_vl;
-int kvm_arm_init_arch_resources(void);
+int kvm_arm_init_sve(void);
 
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index f13378d..8847f38 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -110,7 +110,7 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 
 unsigned int kvm_sve_max_vl;
 
-int kvm_arm_init_arch_resources(void)
+int kvm_arm_init_sve(void)
 {
 	if (system_supports_sve()) {
 		kvm_sve_max_vl = sve_max_virtualisable_vl;
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9edbf0f..7039c99c 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -1682,7 +1682,7 @@ int kvm_arch_init(void *opaque)
 	if (err)
 		return err;
 
-	err = kvm_arm_init_arch_resources();
+	err = kvm_arm_init_sve();
 	if (err)
 		return err;
 
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 03/14] KVM: arm: Make vcpu finalization stubs into inline functions

[Dave Martin]

The vcpu finalization stubs kvm_arm_vcpu_finalize() and
kvm_arm_vcpu_is_finalized() are currently #defines for ARM, which
limits the type-checking that the compiler can do at runtime.

The only reason for them to be #defines was to avoid reliance on
the definition of struct kvm_vcpu, which is not available here due
to circular #include problems.  However, because these are stubs
containing no code, they don't need the definition of struct
kvm_vcpu after all; only a declaration is needed (which is
available already).

So in the interests of cleanliness, this patch converts them to
inline functions.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d956273..7feddac 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -412,7 +412,14 @@ static inline int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type)
 	return 0;
 }
 
-#define kvm_arm_vcpu_finalize(vcpu, what) (-EINVAL)
-#define kvm_arm_vcpu_is_finalized(vcpu) true
+static inline int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
+{
+	return -EINVAL;
+}
+
+static inline bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
+{
+	return true;
+}
 
 #endif /* __ARM_KVM_HOST_H__ */
-- 
2.1.4

[PATCH 03/14] KVM: arm: Make vcpu finalization stubs into inline functions

[Dave Martin]

The vcpu finalization stubs kvm_arm_vcpu_finalize() and
kvm_arm_vcpu_is_finalized() are currently #defines for ARM, which
limits the type-checking that the compiler can do at runtime.

The only reason for them to be #defines was to avoid reliance on
the definition of struct kvm_vcpu, which is not available here due
to circular #include problems.  However, because these are stubs
containing no code, they don't need the definition of struct
kvm_vcpu after all; only a declaration is needed (which is
available already).

So in the interests of cleanliness, this patch converts them to
inline functions.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d956273..7feddac 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -412,7 +412,14 @@ static inline int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type)
 	return 0;
 }
 
-#define kvm_arm_vcpu_finalize(vcpu, what) (-EINVAL)
-#define kvm_arm_vcpu_is_finalized(vcpu) true
+static inline int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
+{
+	return -EINVAL;
+}
+
+static inline bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu)
+{
+	return true;
+}
 
 #endif /* __ARM_KVM_HOST_H__ */
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 04/14] KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to WARNs

[Dave Martin]

Because of the logic in kvm_arm_sys_reg_{get,set}_reg() and
sve_id_visibility(), we should never call
{get,set}_id_aa64zfr0_el1() for a vcpu where !vcpu_has_sve(vcpu).

To avoid the code giving the impression that it is valid for these
functions to be called in this situation, and to help the compiler
make the right optimisation decisions, this patch adds WARN_ON()
for these cases.

Given the way the logic is spread out, this seems preferable to
dropping the checks altogether.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 09e9b06..7046c76 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1144,7 +1144,7 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val;
 
-	if (!vcpu_has_sve(vcpu))
+	if (WARN_ON(!vcpu_has_sve(vcpu)))
 		return -ENOENT;
 
 	val = guest_id_aa64zfr0_el1(vcpu);
@@ -1159,7 +1159,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 	int err;
 	u64 val;
 
-	if (!vcpu_has_sve(vcpu))
+	if (WARN_ON(!vcpu_has_sve(vcpu)))
 		return -ENOENT;
 
 	err = reg_from_user(&val, uaddr, id);
-- 
2.1.4

[PATCH 04/14] KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to WARNs

[Dave Martin]

Because of the logic in kvm_arm_sys_reg_{get,set}_reg() and
sve_id_visibility(), we should never call
{get,set}_id_aa64zfr0_el1() for a vcpu where !vcpu_has_sve(vcpu).

To avoid the code giving the impression that it is valid for these
functions to be called in this situation, and to help the compiler
make the right optimisation decisions, this patch adds WARN_ON()
for these cases.

Given the way the logic is spread out, this seems preferable to
dropping the checks altogether.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 09e9b06..7046c76 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1144,7 +1144,7 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val;
 
-	if (!vcpu_has_sve(vcpu))
+	if (WARN_ON(!vcpu_has_sve(vcpu)))
 		return -ENOENT;
 
 	val = guest_id_aa64zfr0_el1(vcpu);
@@ -1159,7 +1159,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 	int err;
 	u64 val;
 
-	if (!vcpu_has_sve(vcpu))
+	if (WARN_ON(!vcpu_has_sve(vcpu)))
 		return -ENOENT;
 
 	err = reg_from_user(&val, uaddr, id);
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions

[Dave Martin]

Currently, the SVE register ID macros are not all defined in the
same way, and advertise the fact that FFR maps onto the nonexistent
predicate register P16.  This is really just for kernel
convenience, and may lead userspace into bad habits.

Instead, this patch masks the ID macro arguments so that
architecturally invalid register numbers will not be passed through
any more, and uses a literal KVM_REG_ARM64_SVE_FFR_BASE macro to
define KVM_REG_ARM64_SVE_FFR(), similarly to the way the _ZREG()
and _PREG() macros are defined.

Rather than plugging in magic numbers for the number of Z- and P-
registers and the maximum possible number of register slices, this
patch provides definitions for those too.  Userspace is going to
need them in any case, and it makes sense for them to come from
<uapi/asm/kvm.h>.

sve_reg_to_region() uses convenience constants that are defined in
a different way, and also makes use of the fact that the FFR IDs
are really contiguous with the P15 IDs, so this patch retains the
existing convenience constants in guest.c, supplemented with a
couple of sanity checks to check for consistency with with the UAPI
header.

Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/uapi/asm/kvm.h | 32 +++++++++++++++++++++++---------
 arch/arm64/kvm/guest.c            |  9 +++++++++
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 6963b7e..2a04ef0 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/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
@@ -233,16 +234,29 @@ struct kvm_vcpu_events {
 /* 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_REG_ARM64_SVE_ZREG(n, i)	(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
-					 KVM_REG_ARM64_SVE_ZREG_BASE |	\
-					 KVM_REG_SIZE_U2048 |		\
-					 ((n) << 5) | (i))
-#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) << 5) | (i))
-#define KVM_REG_ARM64_SVE_FFR(i)	KVM_REG_ARM64_SVE_PREG(16, i)
+#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)))
 
 /* Vector lengths pseudo-register: */
 #define KVM_REG_ARM64_SVE_VLS		(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 4f7b26b..2e449e1 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -325,6 +325,15 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 
 	size_t sve_state_size;
 
+	const u64 last_preg_id = KVM_REG_ARM64_SVE_PREG(SVE_NUM_PREGS - 1,
+							SVE_NUM_SLICES - 1);
+
+	/* Verify that the P-regs and FFR really do have contiguous IDs: */
+	BUILD_BUG_ON(KVM_REG_ARM64_SVE_FFR(0) != last_preg_id + 1);
+
+	/* Verify that we match the UAPI header: */
+	BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES);
+
 	/* Only the first slice ever exists, for now: */
 	if ((reg->id & SVE_REG_SLICE_MASK) != 0)
 		return -ENOENT;
-- 
2.1.4

[PATCH 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions

[Dave Martin]

Currently, the SVE register ID macros are not all defined in the
same way, and advertise the fact that FFR maps onto the nonexistent
predicate register P16.  This is really just for kernel
convenience, and may lead userspace into bad habits.

Instead, this patch masks the ID macro arguments so that
architecturally invalid register numbers will not be passed through
any more, and uses a literal KVM_REG_ARM64_SVE_FFR_BASE macro to
define KVM_REG_ARM64_SVE_FFR(), similarly to the way the _ZREG()
and _PREG() macros are defined.

Rather than plugging in magic numbers for the number of Z- and P-
registers and the maximum possible number of register slices, this
patch provides definitions for those too.  Userspace is going to
need them in any case, and it makes sense for them to come from
<uapi/asm/kvm.h>.

sve_reg_to_region() uses convenience constants that are defined in
a different way, and also makes use of the fact that the FFR IDs
are really contiguous with the P15 IDs, so this patch retains the
existing convenience constants in guest.c, supplemented with a
couple of sanity checks to check for consistency with with the UAPI
header.

Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/uapi/asm/kvm.h | 32 +++++++++++++++++++++++---------
 arch/arm64/kvm/guest.c            |  9 +++++++++
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 6963b7e..2a04ef0 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/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
@@ -233,16 +234,29 @@ struct kvm_vcpu_events {
 /* 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_REG_ARM64_SVE_ZREG(n, i)	(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
-					 KVM_REG_ARM64_SVE_ZREG_BASE |	\
-					 KVM_REG_SIZE_U2048 |		\
-					 ((n) << 5) | (i))
-#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) << 5) | (i))
-#define KVM_REG_ARM64_SVE_FFR(i)	KVM_REG_ARM64_SVE_PREG(16, i)
+#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)))
 
 /* Vector lengths pseudo-register: */
 #define KVM_REG_ARM64_SVE_VLS		(KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 4f7b26b..2e449e1 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -325,6 +325,15 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 
 	size_t sve_state_size;
 
+	const u64 last_preg_id = KVM_REG_ARM64_SVE_PREG(SVE_NUM_PREGS - 1,
+							SVE_NUM_SLICES - 1);
+
+	/* Verify that the P-regs and FFR really do have contiguous IDs: */
+	BUILD_BUG_ON(KVM_REG_ARM64_SVE_FFR(0) != last_preg_id + 1);
+
+	/* Verify that we match the UAPI header: */
+	BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES);
+
 	/* Only the first slice ever exists, for now: */
 	if ((reg->id & SVE_REG_SLICE_MASK) != 0)
 		return -ENOENT;
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:09PM +0100, Dave Martin wrote:
> Currently, the SVE register ID macros are not all defined in the
> same way, and advertise the fact that FFR maps onto the nonexistent
> predicate register P16.  This is really just for kernel
> convenience, and may lead userspace into bad habits.
> 
> Instead, this patch masks the ID macro arguments so that
> architecturally invalid register numbers will not be passed through
> any more, and uses a literal KVM_REG_ARM64_SVE_FFR_BASE macro to
> define KVM_REG_ARM64_SVE_FFR(), similarly to the way the _ZREG()
> and _PREG() macros are defined.
> 
> Rather than plugging in magic numbers for the number of Z- and P-
> registers and the maximum possible number of register slices, this
> patch provides definitions for those too.  Userspace is going to
> need them in any case, and it makes sense for them to come from
> <uapi/asm/kvm.h>.
> 
> sve_reg_to_region() uses convenience constants that are defined in
> a different way, and also makes use of the fact that the FFR IDs
> are really contiguous with the P15 IDs, so this patch retains the
> existing convenience constants in guest.c, supplemented with a
> couple of sanity checks to check for consistency with with the UAPI

s/with with/with/

> header.
> 
> Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/include/uapi/asm/kvm.h | 32 +++++++++++++++++++++++---------
>  arch/arm64/kvm/guest.c            |  9 +++++++++
>  2 files changed, 32 insertions(+), 9 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:09PM +0100, Dave Martin wrote:
> Currently, the SVE register ID macros are not all defined in the
> same way, and advertise the fact that FFR maps onto the nonexistent
> predicate register P16.  This is really just for kernel
> convenience, and may lead userspace into bad habits.
> 
> Instead, this patch masks the ID macro arguments so that
> architecturally invalid register numbers will not be passed through
> any more, and uses a literal KVM_REG_ARM64_SVE_FFR_BASE macro to
> define KVM_REG_ARM64_SVE_FFR(), similarly to the way the _ZREG()
> and _PREG() macros are defined.
> 
> Rather than plugging in magic numbers for the number of Z- and P-
> registers and the maximum possible number of register slices, this
> patch provides definitions for those too.  Userspace is going to
> need them in any case, and it makes sense for them to come from
> <uapi/asm/kvm.h>.
> 
> sve_reg_to_region() uses convenience constants that are defined in
> a different way, and also makes use of the fact that the FFR IDs
> are really contiguous with the P15 IDs, so this patch retains the
> existing convenience constants in guest.c, supplemented with a
> couple of sanity checks to check for consistency with with the UAPI

s/with with/with/

> header.
> 
> Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/include/uapi/asm/kvm.h | 32 +++++++++++++++++++++++---------
>  arch/arm64/kvm/guest.c            |  9 +++++++++
>  2 files changed, 32 insertions(+), 9 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions

[Dave Martin]

On Mon, Apr 15, 2019 at 05:17:39PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:09PM +0100, Dave Martin wrote:
> > Currently, the SVE register ID macros are not all defined in the
> > same way, and advertise the fact that FFR maps onto the nonexistent
> > predicate register P16.  This is really just for kernel
> > convenience, and may lead userspace into bad habits.
> > 
> > Instead, this patch masks the ID macro arguments so that
> > architecturally invalid register numbers will not be passed through
> > any more, and uses a literal KVM_REG_ARM64_SVE_FFR_BASE macro to
> > define KVM_REG_ARM64_SVE_FFR(), similarly to the way the _ZREG()
> > and _PREG() macros are defined.
> > 
> > Rather than plugging in magic numbers for the number of Z- and P-
> > registers and the maximum possible number of register slices, this
> > patch provides definitions for those too.  Userspace is going to
> > need them in any case, and it makes sense for them to come from
> > <uapi/asm/kvm.h>.
> > 
> > sve_reg_to_region() uses convenience constants that are defined in
> > a different way, and also makes use of the fact that the FFR IDs
> > are really contiguous with the P15 IDs, so this patch retains the
> > existing convenience constants in guest.c, supplemented with a
> > couple of sanity checks to check for consistency with with the UAPI
> 
> s/with with/with/

Good spot, thanks.

> > header.
> > 
> > Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/include/uapi/asm/kvm.h | 32 +++++++++++++++++++++++---------
> >  arch/arm64/kvm/guest.c            |  9 +++++++++
> >  2 files changed, 32 insertions(+), 9 deletions(-)
> >
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks
---Dave

Re: [PATCH 05/14] KVM: arm64/sve: Clean up UAPI register ID definitions

[Dave Martin]

On Mon, Apr 15, 2019 at 05:17:39PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:09PM +0100, Dave Martin wrote:
> > Currently, the SVE register ID macros are not all defined in the
> > same way, and advertise the fact that FFR maps onto the nonexistent
> > predicate register P16.  This is really just for kernel
> > convenience, and may lead userspace into bad habits.
> > 
> > Instead, this patch masks the ID macro arguments so that
> > architecturally invalid register numbers will not be passed through
> > any more, and uses a literal KVM_REG_ARM64_SVE_FFR_BASE macro to
> > define KVM_REG_ARM64_SVE_FFR(), similarly to the way the _ZREG()
> > and _PREG() macros are defined.
> > 
> > Rather than plugging in magic numbers for the number of Z- and P-
> > registers and the maximum possible number of register slices, this
> > patch provides definitions for those too.  Userspace is going to
> > need them in any case, and it makes sense for them to come from
> > <uapi/asm/kvm.h>.
> > 
> > sve_reg_to_region() uses convenience constants that are defined in
> > a different way, and also makes use of the fact that the FFR IDs
> > are really contiguous with the P15 IDs, so this patch retains the
> > existing convenience constants in guest.c, supplemented with a
> > couple of sanity checks to check for consistency with with the UAPI
> 
> s/with with/with/

Good spot, thanks.

> > header.
> > 
> > Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/include/uapi/asm/kvm.h | 32 +++++++++++++++++++++++---------
> >  arch/arm64/kvm/guest.c            |  9 +++++++++
> >  2 files changed, 32 insertions(+), 9 deletions(-)
> >
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 06/14] KVM: arm64/sve: Miscellaneous tidyups in guest.c

[Dave Martin]

 * Remove a few redundant blank lines that are stylistically
   inconsistent with code already in guest.c and are just taking up
   space.

 * Delete a couple of pointless empty default cases from switch
   statements whose behaviour is otherwise obvious anyway.

 * Fix some typos and consolidate some redundantly duplicated
   comments.

 * Respell the slice index check in sve_reg_to_region() as "> 0"
   to be more consistent with what is logically being checked here
   (i.e., "is the slice index too large"), even though we don't try
   to cope with multiple slices yet.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 2e449e1..f5ff7ae 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -290,9 +290,10 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 #define KVM_SVE_PREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_PREG(0, 0))
 
 /*
- * number of register slices required to cover each whole SVE register on vcpu
- * NOTE: If you are tempted to modify this, you must also to rework
- * sve_reg_to_region() to match:
+ * Number of register slices required to cover each whole SVE register.
+ * NOTE: Only the first slice every exists, for now.
+ * If you are tempted to modify this, you must also rework sve_reg_to_region()
+ * to match:
  */
 #define vcpu_sve_slices(vcpu) 1
 
@@ -334,8 +335,7 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 	/* Verify that we match the UAPI header: */
 	BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES);
 
-	/* Only the first slice ever exists, for now: */
-	if ((reg->id & SVE_REG_SLICE_MASK) != 0)
+	if ((reg->id & SVE_REG_SLICE_MASK) > 0)
 		return -ENOENT;
 
 	vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
@@ -520,7 +520,6 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
 {
-	/* Only the first slice ever exists, for now */
 	const unsigned int slices = vcpu_sve_slices(vcpu);
 
 	if (!vcpu_has_sve(vcpu))
@@ -536,7 +535,6 @@ static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
 static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 				u64 __user *uindices)
 {
-	/* Only the first slice ever exists, for now */
 	const unsigned int slices = vcpu_sve_slices(vcpu);
 	u64 reg;
 	unsigned int i, n;
@@ -555,7 +553,6 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 	reg = KVM_REG_ARM64_SVE_VLS;
 	if (put_user(reg, uindices++))
 		return -EFAULT;
-
 	++num_regs;
 
 	for (i = 0; i < slices; i++) {
@@ -563,7 +560,6 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 			reg = KVM_REG_ARM64_SVE_ZREG(n, i);
 			if (put_user(reg, uindices++))
 				return -EFAULT;
-
 			num_regs++;
 		}
 
@@ -571,14 +567,12 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 			reg = KVM_REG_ARM64_SVE_PREG(n, i);
 			if (put_user(reg, uindices++))
 				return -EFAULT;
-
 			num_regs++;
 		}
 
 		reg = KVM_REG_ARM64_SVE_FFR(i);
 		if (put_user(reg, uindices++))
 			return -EFAULT;
-
 		num_regs++;
 	}
 
@@ -645,7 +639,6 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_CORE:	return get_core_reg(vcpu, reg);
 	case KVM_REG_ARM_FW:	return kvm_arm_get_fw_reg(vcpu, reg);
 	case KVM_REG_ARM64_SVE:	return get_sve_reg(vcpu, reg);
-	default: break; /* fall through */
 	}
 
 	if (is_timer_reg(reg->id))
@@ -664,7 +657,6 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_CORE:	return set_core_reg(vcpu, reg);
 	case KVM_REG_ARM_FW:	return kvm_arm_set_fw_reg(vcpu, reg);
 	case KVM_REG_ARM64_SVE:	return set_sve_reg(vcpu, reg);
-	default: break; /* fall through */
 	}
 
 	if (is_timer_reg(reg->id))
-- 
2.1.4

[PATCH 06/14] KVM: arm64/sve: Miscellaneous tidyups in guest.c

[Dave Martin]

 * Remove a few redundant blank lines that are stylistically
   inconsistent with code already in guest.c and are just taking up
   space.

 * Delete a couple of pointless empty default cases from switch
   statements whose behaviour is otherwise obvious anyway.

 * Fix some typos and consolidate some redundantly duplicated
   comments.

 * Respell the slice index check in sve_reg_to_region() as "> 0"
   to be more consistent with what is logically being checked here
   (i.e., "is the slice index too large"), even though we don't try
   to cope with multiple slices yet.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 2e449e1..f5ff7ae 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -290,9 +290,10 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 #define KVM_SVE_PREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_PREG(0, 0))
 
 /*
- * number of register slices required to cover each whole SVE register on vcpu
- * NOTE: If you are tempted to modify this, you must also to rework
- * sve_reg_to_region() to match:
+ * Number of register slices required to cover each whole SVE register.
+ * NOTE: Only the first slice every exists, for now.
+ * If you are tempted to modify this, you must also rework sve_reg_to_region()
+ * to match:
  */
 #define vcpu_sve_slices(vcpu) 1
 
@@ -334,8 +335,7 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 	/* Verify that we match the UAPI header: */
 	BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES);
 
-	/* Only the first slice ever exists, for now: */
-	if ((reg->id & SVE_REG_SLICE_MASK) != 0)
+	if ((reg->id & SVE_REG_SLICE_MASK) > 0)
 		return -ENOENT;
 
 	vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
@@ -520,7 +520,6 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
 {
-	/* Only the first slice ever exists, for now */
 	const unsigned int slices = vcpu_sve_slices(vcpu);
 
 	if (!vcpu_has_sve(vcpu))
@@ -536,7 +535,6 @@ static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
 static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 				u64 __user *uindices)
 {
-	/* Only the first slice ever exists, for now */
 	const unsigned int slices = vcpu_sve_slices(vcpu);
 	u64 reg;
 	unsigned int i, n;
@@ -555,7 +553,6 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 	reg = KVM_REG_ARM64_SVE_VLS;
 	if (put_user(reg, uindices++))
 		return -EFAULT;
-
 	++num_regs;
 
 	for (i = 0; i < slices; i++) {
@@ -563,7 +560,6 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 			reg = KVM_REG_ARM64_SVE_ZREG(n, i);
 			if (put_user(reg, uindices++))
 				return -EFAULT;
-
 			num_regs++;
 		}
 
@@ -571,14 +567,12 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
 			reg = KVM_REG_ARM64_SVE_PREG(n, i);
 			if (put_user(reg, uindices++))
 				return -EFAULT;
-
 			num_regs++;
 		}
 
 		reg = KVM_REG_ARM64_SVE_FFR(i);
 		if (put_user(reg, uindices++))
 			return -EFAULT;
-
 		num_regs++;
 	}
 
@@ -645,7 +639,6 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_CORE:	return get_core_reg(vcpu, reg);
 	case KVM_REG_ARM_FW:	return kvm_arm_get_fw_reg(vcpu, reg);
 	case KVM_REG_ARM64_SVE:	return get_sve_reg(vcpu, reg);
-	default: break; /* fall through */
 	}
 
 	if (is_timer_reg(reg->id))
@@ -664,7 +657,6 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_CORE:	return set_core_reg(vcpu, reg);
 	case KVM_REG_ARM_FW:	return kvm_arm_set_fw_reg(vcpu, reg);
 	case KVM_REG_ARM64_SVE:	return set_sve_reg(vcpu, reg);
-	default: break; /* fall through */
 	}
 
 	if (is_timer_reg(reg->id))
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 07/14] KVM: arm64/sve: Make register ioctl access errors more consistent

[Dave Martin]

Currently, the way error codes are generated when processing the
SVE register access ioctls in a bit haphazard.

This patch refactors the code so that the behaviour is more
consistent: now, -EINVAL should be returned only for unrecognised
register IDs or when some other runtime error occurs.  -ENOENT is
returned for register IDs that are recognised, but whose
corresponding register (or slice) does not exist for the vcpu.

To this end, in {get,set}_sve_reg() we now delegate the
vcpu_has_sve() check down into {get,set}_sve_vls() and
sve_reg_to_region().  The KVM_REG_ARM64_SVE_VLS special case is
picked off first, then sve_reg_to_region() plays the role of
exhaustively validating or rejecting the register ID and (where
accepted) computing the applicable register region as before.

sve_reg_to_region() is rearranged so that -ENOENT or -EPERM is not
returned prematurely, before checking whether reg->id is in a
recognised range.

-EPERM is now only returned when an attempt is made to access an
actually existing register slice on an unfinalized vcpu.

Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
Fixes: 9033bba4b535 ("KVM: arm64/sve: Add pseudo-register for the guest's vector lengths")
Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 52 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f5ff7ae..e45a042 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -221,6 +221,9 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	unsigned int max_vq, vq;
 	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
 
+	if (!vcpu_has_sve(vcpu))
+		return -ENOENT;
+
 	if (WARN_ON(!sve_vl_valid(vcpu->arch.sve_max_vl)))
 		return -EINVAL;
 
@@ -242,6 +245,9 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	unsigned int max_vq, vq;
 	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
 
+	if (!vcpu_has_sve(vcpu))
+		return -ENOENT;
+
 	if (kvm_arm_vcpu_sve_finalized(vcpu))
 		return -EPERM; /* too late! */
 
@@ -304,7 +310,10 @@ struct sve_state_reg_region {
 	unsigned int upad;	/* extra trailing padding in user memory */
 };
 
-/* Get sanitised bounds for user/kernel SVE register copy */
+/*
+ * Validate SVE register ID and get sanitised bounds for user/kernel SVE
+ * register copy
+ */
 static int sve_reg_to_region(struct sve_state_reg_region *region,
 			     struct kvm_vcpu *vcpu,
 			     const struct kvm_one_reg *reg)
@@ -335,25 +344,30 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 	/* Verify that we match the UAPI header: */
 	BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES);
 
-	if ((reg->id & SVE_REG_SLICE_MASK) > 0)
-		return -ENOENT;
-
-	vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
-
 	reg_num = (reg->id & SVE_REG_ID_MASK) >> SVE_REG_ID_SHIFT;
 
 	if (reg->id >= zreg_id_min && reg->id <= zreg_id_max) {
+		if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0)
+			return -ENOENT;
+
+		vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
+
 		reqoffset = SVE_SIG_ZREG_OFFSET(vq, reg_num) -
 				SVE_SIG_REGS_OFFSET;
 		reqlen = KVM_SVE_ZREG_SIZE;
 		maxlen = SVE_SIG_ZREG_SIZE(vq);
 	} else if (reg->id >= preg_id_min && reg->id <= preg_id_max) {
+		if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0)
+			return -ENOENT;
+
+		vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
+
 		reqoffset = SVE_SIG_PREG_OFFSET(vq, reg_num) -
 				SVE_SIG_REGS_OFFSET;
 		reqlen = KVM_SVE_PREG_SIZE;
 		maxlen = SVE_SIG_PREG_SIZE(vq);
 	} else {
-		return -ENOENT;
+		return -EINVAL;
 	}
 
 	sve_state_size = vcpu_sve_state_size(vcpu);
@@ -369,24 +383,22 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 
 static int get_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+	int ret;
 	struct sve_state_reg_region region;
 	char __user *uptr = (char __user *)reg->addr;
 
-	if (!vcpu_has_sve(vcpu))
-		return -ENOENT;
-
 	/* Handle the KVM_REG_ARM64_SVE_VLS pseudo-reg as a special case: */
 	if (reg->id == KVM_REG_ARM64_SVE_VLS)
 		return get_sve_vls(vcpu, reg);
 
-	/* Otherwise, reg is an architectural SVE register... */
+	/* Try to interpret reg ID as an architectural SVE register... */
+	ret = sve_reg_to_region(&region, vcpu, reg);
+	if (ret)
+		return ret;
 
 	if (!kvm_arm_vcpu_sve_finalized(vcpu))
 		return -EPERM;
 
-	if (sve_reg_to_region(&region, vcpu, reg))
-		return -ENOENT;
-
 	if (copy_to_user(uptr, vcpu->arch.sve_state + region.koffset,
 			 region.klen) ||
 	    clear_user(uptr + region.klen, region.upad))
@@ -397,24 +409,22 @@ static int get_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 static int set_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+	int ret;
 	struct sve_state_reg_region region;
 	const char __user *uptr = (const char __user *)reg->addr;
 
-	if (!vcpu_has_sve(vcpu))
-		return -ENOENT;
-
 	/* Handle the KVM_REG_ARM64_SVE_VLS pseudo-reg as a special case: */
 	if (reg->id == KVM_REG_ARM64_SVE_VLS)
 		return set_sve_vls(vcpu, reg);
 
-	/* Otherwise, reg is an architectural SVE register... */
+	/* Try to interpret reg ID as an architectural SVE register... */
+	ret = sve_reg_to_region(&region, vcpu, reg);
+	if (ret)
+		return ret;
 
 	if (!kvm_arm_vcpu_sve_finalized(vcpu))
 		return -EPERM;
 
-	if (sve_reg_to_region(&region, vcpu, reg))
-		return -ENOENT;
-
 	if (copy_from_user(vcpu->arch.sve_state + region.koffset, uptr,
 			   region.klen))
 		return -EFAULT;
-- 
2.1.4

[PATCH 07/14] KVM: arm64/sve: Make register ioctl access errors more consistent

[Dave Martin]

Currently, the way error codes are generated when processing the
SVE register access ioctls in a bit haphazard.

This patch refactors the code so that the behaviour is more
consistent: now, -EINVAL should be returned only for unrecognised
register IDs or when some other runtime error occurs.  -ENOENT is
returned for register IDs that are recognised, but whose
corresponding register (or slice) does not exist for the vcpu.

To this end, in {get,set}_sve_reg() we now delegate the
vcpu_has_sve() check down into {get,set}_sve_vls() and
sve_reg_to_region().  The KVM_REG_ARM64_SVE_VLS special case is
picked off first, then sve_reg_to_region() plays the role of
exhaustively validating or rejecting the register ID and (where
accepted) computing the applicable register region as before.

sve_reg_to_region() is rearranged so that -ENOENT or -EPERM is not
returned prematurely, before checking whether reg->id is in a
recognised range.

-EPERM is now only returned when an attempt is made to access an
actually existing register slice on an unfinalized vcpu.

Fixes: e1c9c98345b3 ("KVM: arm64/sve: Add SVE support to register access ioctl interface")
Fixes: 9033bba4b535 ("KVM: arm64/sve: Add pseudo-register for the guest's vector lengths")
Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 52 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f5ff7ae..e45a042 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -221,6 +221,9 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	unsigned int max_vq, vq;
 	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
 
+	if (!vcpu_has_sve(vcpu))
+		return -ENOENT;
+
 	if (WARN_ON(!sve_vl_valid(vcpu->arch.sve_max_vl)))
 		return -EINVAL;
 
@@ -242,6 +245,9 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	unsigned int max_vq, vq;
 	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
 
+	if (!vcpu_has_sve(vcpu))
+		return -ENOENT;
+
 	if (kvm_arm_vcpu_sve_finalized(vcpu))
 		return -EPERM; /* too late! */
 
@@ -304,7 +310,10 @@ struct sve_state_reg_region {
 	unsigned int upad;	/* extra trailing padding in user memory */
 };
 
-/* Get sanitised bounds for user/kernel SVE register copy */
+/*
+ * Validate SVE register ID and get sanitised bounds for user/kernel SVE
+ * register copy
+ */
 static int sve_reg_to_region(struct sve_state_reg_region *region,
 			     struct kvm_vcpu *vcpu,
 			     const struct kvm_one_reg *reg)
@@ -335,25 +344,30 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 	/* Verify that we match the UAPI header: */
 	BUILD_BUG_ON(SVE_NUM_SLICES != KVM_ARM64_SVE_MAX_SLICES);
 
-	if ((reg->id & SVE_REG_SLICE_MASK) > 0)
-		return -ENOENT;
-
-	vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
-
 	reg_num = (reg->id & SVE_REG_ID_MASK) >> SVE_REG_ID_SHIFT;
 
 	if (reg->id >= zreg_id_min && reg->id <= zreg_id_max) {
+		if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0)
+			return -ENOENT;
+
+		vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
+
 		reqoffset = SVE_SIG_ZREG_OFFSET(vq, reg_num) -
 				SVE_SIG_REGS_OFFSET;
 		reqlen = KVM_SVE_ZREG_SIZE;
 		maxlen = SVE_SIG_ZREG_SIZE(vq);
 	} else if (reg->id >= preg_id_min && reg->id <= preg_id_max) {
+		if (!vcpu_has_sve(vcpu) || (reg->id & SVE_REG_SLICE_MASK) > 0)
+			return -ENOENT;
+
+		vq = sve_vq_from_vl(vcpu->arch.sve_max_vl);
+
 		reqoffset = SVE_SIG_PREG_OFFSET(vq, reg_num) -
 				SVE_SIG_REGS_OFFSET;
 		reqlen = KVM_SVE_PREG_SIZE;
 		maxlen = SVE_SIG_PREG_SIZE(vq);
 	} else {
-		return -ENOENT;
+		return -EINVAL;
 	}
 
 	sve_state_size = vcpu_sve_state_size(vcpu);
@@ -369,24 +383,22 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 
 static int get_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+	int ret;
 	struct sve_state_reg_region region;
 	char __user *uptr = (char __user *)reg->addr;
 
-	if (!vcpu_has_sve(vcpu))
-		return -ENOENT;
-
 	/* Handle the KVM_REG_ARM64_SVE_VLS pseudo-reg as a special case: */
 	if (reg->id == KVM_REG_ARM64_SVE_VLS)
 		return get_sve_vls(vcpu, reg);
 
-	/* Otherwise, reg is an architectural SVE register... */
+	/* Try to interpret reg ID as an architectural SVE register... */
+	ret = sve_reg_to_region(&region, vcpu, reg);
+	if (ret)
+		return ret;
 
 	if (!kvm_arm_vcpu_sve_finalized(vcpu))
 		return -EPERM;
 
-	if (sve_reg_to_region(&region, vcpu, reg))
-		return -ENOENT;
-
 	if (copy_to_user(uptr, vcpu->arch.sve_state + region.koffset,
 			 region.klen) ||
 	    clear_user(uptr + region.klen, region.upad))
@@ -397,24 +409,22 @@ static int get_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 static int set_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
+	int ret;
 	struct sve_state_reg_region region;
 	const char __user *uptr = (const char __user *)reg->addr;
 
-	if (!vcpu_has_sve(vcpu))
-		return -ENOENT;
-
 	/* Handle the KVM_REG_ARM64_SVE_VLS pseudo-reg as a special case: */
 	if (reg->id == KVM_REG_ARM64_SVE_VLS)
 		return set_sve_vls(vcpu, reg);
 
-	/* Otherwise, reg is an architectural SVE register... */
+	/* Try to interpret reg ID as an architectural SVE register... */
+	ret = sve_reg_to_region(&region, vcpu, reg);
+	if (ret)
+		return ret;
 
 	if (!kvm_arm_vcpu_sve_finalized(vcpu))
 		return -EPERM;
 
-	if (sve_reg_to_region(&region, vcpu, reg))
-		return -ENOENT;
-
 	if (copy_from_user(vcpu->arch.sve_state + region.koffset, uptr,
 			   region.klen))
 		return -EFAULT;
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 08/14] KVM: arm64/sve: WARN when avoiding divide-by-zero in sve_reg_to_region()

[Dave Martin]

sve_reg_to_region() currently passes the result of
vcpu_sve_state_size() to array_index_nospec(), effectively
leading to a divide / modulo operation.

Currently the code bails out and returns -EINVAL if
vcpu_sve_state_size() turns out to be zero, in order to avoid going
ahead and attempting to divide by zero.  This is reasonable, but it
should only happen if the kernel contains some other bug that
allowed this code to be reached without the vcpu having been
properly initialised.

To make it clear that this is a defence against bugs rather than
something that the user should be able to trigger, this patch marks
the check with WARN_ON().

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index e45a042..73044e3 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -371,7 +371,7 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 	}
 
 	sve_state_size = vcpu_sve_state_size(vcpu);
-	if (!sve_state_size)
+	if (WARN_ON(!sve_state_size))
 		return -EINVAL;
 
 	region->koffset = array_index_nospec(reqoffset, sve_state_size);
-- 
2.1.4

[PATCH 08/14] KVM: arm64/sve: WARN when avoiding divide-by-zero in sve_reg_to_region()

[Dave Martin]

sve_reg_to_region() currently passes the result of
vcpu_sve_state_size() to array_index_nospec(), effectively
leading to a divide / modulo operation.

Currently the code bails out and returns -EINVAL if
vcpu_sve_state_size() turns out to be zero, in order to avoid going
ahead and attempting to divide by zero.  This is reasonable, but it
should only happen if the kernel contains some other bug that
allowed this code to be reached without the vcpu having been
properly initialised.

To make it clear that this is a defence against bugs rather than
something that the user should be able to trigger, this patch marks
the check with WARN_ON().

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index e45a042..73044e3 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -371,7 +371,7 @@ static int sve_reg_to_region(struct sve_state_reg_region *region,
 	}
 
 	sve_state_size = vcpu_sve_state_size(vcpu);
-	if (!sve_state_size)
+	if (WARN_ON(!sve_state_size))
 		return -EINVAL;
 
 	region->koffset = array_index_nospec(reqoffset, sve_state_size);
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

A complicated DIV_ROUND_UP() expression is currently written out
explicitly in multiple places in order to specify the size of the
bitmap exchanged with userspace to represent the value of the
KVM_REG_ARM64_SVE_VLS pseudo-register.

To make this more readable, this patch replaces these with a single
define.

Since the number of words in a bitmap is just the index of the last
word used + 1, this patch expresses the bound that way instead.
This should make it clearer what is being expressed.

Since use of DIV_ROUND_UP() was the only reason for including
<linux/kernel.h> in guest.c, this patch removes that #include.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 73044e3..f025a2f 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -23,7 +23,6 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/nospec.h>
-#include <linux/kernel.h>
 #include <linux/kvm_host.h>
 #include <linux/module.h>
 #include <linux/stddef.h>
@@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
 #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
 
+#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
+
 static bool vq_present(
-	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
+	const u64 (*const vqs)[SVE_VLS_WORDS],
 	unsigned int vq)
 {
 	return (*vqs)[vq_word(vq)] & vq_mask(vq);
@@ -219,7 +220,7 @@ static bool vq_present(
 static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
+	u64 vqs[SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;
@@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
+	u64 vqs[SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;
-- 
2.1.4

[PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

A complicated DIV_ROUND_UP() expression is currently written out
explicitly in multiple places in order to specify the size of the
bitmap exchanged with userspace to represent the value of the
KVM_REG_ARM64_SVE_VLS pseudo-register.

To make this more readable, this patch replaces these with a single
define.

Since the number of words in a bitmap is just the index of the last
word used + 1, this patch expresses the bound that way instead.
This should make it clearer what is being expressed.

Since use of DIV_ROUND_UP() was the only reason for including
<linux/kernel.h> in guest.c, this patch removes that #include.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 73044e3..f025a2f 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -23,7 +23,6 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/nospec.h>
-#include <linux/kernel.h>
 #include <linux/kvm_host.h>
 #include <linux/module.h>
 #include <linux/stddef.h>
@@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
 #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
 
+#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
+
 static bool vq_present(
-	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
+	const u64 (*const vqs)[SVE_VLS_WORDS],
 	unsigned int vq)
 {
 	return (*vqs)[vq_word(vq)] & vq_mask(vq);
@@ -219,7 +220,7 @@ static bool vq_present(
 static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
+	u64 vqs[SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;
@@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
+	u64 vqs[SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> A complicated DIV_ROUND_UP() expression is currently written out
> explicitly in multiple places in order to specify the size of the
> bitmap exchanged with userspace to represent the value of the
> KVM_REG_ARM64_SVE_VLS pseudo-register.
> 
> To make this more readable, this patch replaces these with a single
> define.
> 
> Since the number of words in a bitmap is just the index of the last
> word used + 1, this patch expresses the bound that way instead.
> This should make it clearer what is being expressed.
> 
> Since use of DIV_ROUND_UP() was the only reason for including
> <linux/kernel.h> in guest.c, this patch removes that #include.
> 
> No functional change.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kvm/guest.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 73044e3..f025a2f 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -23,7 +23,6 @@
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/nospec.h>
> -#include <linux/kernel.h>
>  #include <linux/kvm_host.h>
>  #include <linux/module.h>
>  #include <linux/stddef.h>
> @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
>  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
>  
> +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> +
>  static bool vq_present(
> -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> +	const u64 (*const vqs)[SVE_VLS_WORDS],
>  	unsigned int vq)
>  {
>  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> @@ -219,7 +220,7 @@ static bool vq_present(
>  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> +	u64 vqs[SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
> @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> +	u64 vqs[SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
> -- 
> 2.1.4
>

This is good, but I wonder if we could define the number of VLS words in
the documentation in terms of SVE_VQ_MAX too. Currently it's just the
hard coded 8 ("__u64 vector_lengths[8]").

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> A complicated DIV_ROUND_UP() expression is currently written out
> explicitly in multiple places in order to specify the size of the
> bitmap exchanged with userspace to represent the value of the
> KVM_REG_ARM64_SVE_VLS pseudo-register.
> 
> To make this more readable, this patch replaces these with a single
> define.
> 
> Since the number of words in a bitmap is just the index of the last
> word used + 1, this patch expresses the bound that way instead.
> This should make it clearer what is being expressed.
> 
> Since use of DIV_ROUND_UP() was the only reason for including
> <linux/kernel.h> in guest.c, this patch removes that #include.
> 
> No functional change.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kvm/guest.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 73044e3..f025a2f 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -23,7 +23,6 @@
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/nospec.h>
> -#include <linux/kernel.h>
>  #include <linux/kvm_host.h>
>  #include <linux/module.h>
>  #include <linux/stddef.h>
> @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
>  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
>  
> +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> +
>  static bool vq_present(
> -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> +	const u64 (*const vqs)[SVE_VLS_WORDS],
>  	unsigned int vq)
>  {
>  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> @@ -219,7 +220,7 @@ static bool vq_present(
>  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> +	u64 vqs[SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
> @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> +	u64 vqs[SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
> -- 
> 2.1.4
>

This is good, but I wonder if we could define the number of VLS words in
the documentation in terms of SVE_VQ_MAX too. Currently it's just the
hard coded 8 ("__u64 vector_lengths[8]").

Reviewed-by: Andrew Jones <drjones@redhat.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > A complicated DIV_ROUND_UP() expression is currently written out
> > explicitly in multiple places in order to specify the size of the
> > bitmap exchanged with userspace to represent the value of the
> > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > 
> > To make this more readable, this patch replaces these with a single
> > define.
> > 
> > Since the number of words in a bitmap is just the index of the last
> > word used + 1, this patch expresses the bound that way instead.
> > This should make it clearer what is being expressed.
> > 
> > Since use of DIV_ROUND_UP() was the only reason for including
> > <linux/kernel.h> in guest.c, this patch removes that #include.
> > 
> > No functional change.
> > 
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kvm/guest.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 73044e3..f025a2f 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -23,7 +23,6 @@
> >  #include <linux/errno.h>
> >  #include <linux/err.h>
> >  #include <linux/nospec.h>
> > -#include <linux/kernel.h>
> >  #include <linux/kvm_host.h>
> >  #include <linux/module.h>
> >  #include <linux/stddef.h>
> > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> >  
> > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > +
> >  static bool vq_present(
> > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> >  	unsigned int vq)
> >  {
> >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > @@ -219,7 +220,7 @@ static bool vq_present(
> >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > +	u64 vqs[SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > +	u64 vqs[SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> > -- 
> > 2.1.4
> >
> 
> This is good, but I wonder if we could define the number of VLS words in
> the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> hard coded 8 ("__u64 vector_lengths[8]").
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
was avoiding it here.

[8] is at least impossible to misinterpret, even if it's not the most
self-explanatory option.

Is that OK?

Cheers
---Dave

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > A complicated DIV_ROUND_UP() expression is currently written out
> > explicitly in multiple places in order to specify the size of the
> > bitmap exchanged with userspace to represent the value of the
> > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > 
> > To make this more readable, this patch replaces these with a single
> > define.
> > 
> > Since the number of words in a bitmap is just the index of the last
> > word used + 1, this patch expresses the bound that way instead.
> > This should make it clearer what is being expressed.
> > 
> > Since use of DIV_ROUND_UP() was the only reason for including
> > <linux/kernel.h> in guest.c, this patch removes that #include.
> > 
> > No functional change.
> > 
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kvm/guest.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 73044e3..f025a2f 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -23,7 +23,6 @@
> >  #include <linux/errno.h>
> >  #include <linux/err.h>
> >  #include <linux/nospec.h>
> > -#include <linux/kernel.h>
> >  #include <linux/kvm_host.h>
> >  #include <linux/module.h>
> >  #include <linux/stddef.h>
> > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> >  
> > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > +
> >  static bool vq_present(
> > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> >  	unsigned int vq)
> >  {
> >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > @@ -219,7 +220,7 @@ static bool vq_present(
> >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > +	u64 vqs[SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > +	u64 vqs[SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> > -- 
> > 2.1.4
> >
> 
> This is good, but I wonder if we could define the number of VLS words in
> the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> hard coded 8 ("__u64 vector_lengths[8]").
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
was avoiding it here.

[8] is at least impossible to misinterpret, even if it's not the most
self-explanatory option.

Is that OK?

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > A complicated DIV_ROUND_UP() expression is currently written out
> > > explicitly in multiple places in order to specify the size of the
> > > bitmap exchanged with userspace to represent the value of the
> > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > 
> > > To make this more readable, this patch replaces these with a single
> > > define.
> > > 
> > > Since the number of words in a bitmap is just the index of the last
> > > word used + 1, this patch expresses the bound that way instead.
> > > This should make it clearer what is being expressed.
> > > 
> > > Since use of DIV_ROUND_UP() was the only reason for including
> > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > 
> > > No functional change.
> > > 
> > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  arch/arm64/kvm/guest.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > index 73044e3..f025a2f 100644
> > > --- a/arch/arm64/kvm/guest.c
> > > +++ b/arch/arm64/kvm/guest.c
> > > @@ -23,7 +23,6 @@
> > >  #include <linux/errno.h>
> > >  #include <linux/err.h>
> > >  #include <linux/nospec.h>
> > > -#include <linux/kernel.h>
> > >  #include <linux/kvm_host.h>
> > >  #include <linux/module.h>
> > >  #include <linux/stddef.h>
> > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > >  
> > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > +
> > >  static bool vq_present(
> > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > >  	unsigned int vq)
> > >  {
> > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > @@ -219,7 +220,7 @@ static bool vq_present(
> > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  {
> > >  	unsigned int max_vq, vq;
> > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > +	u64 vqs[SVE_VLS_WORDS];
> > >  
> > >  	if (!vcpu_has_sve(vcpu))
> > >  		return -ENOENT;
> > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  {
> > >  	unsigned int max_vq, vq;
> > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > +	u64 vqs[SVE_VLS_WORDS];
> > >  
> > >  	if (!vcpu_has_sve(vcpu))
> > >  		return -ENOENT;
> > > -- 
> > > 2.1.4
> > >
> > 
> > This is good, but I wonder if we could define the number of VLS words in
> > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > hard coded 8 ("__u64 vector_lengths[8]").
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> was avoiding it here.

It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
prefer we use it than to encourage KVM userspace to scatter 8's around.

> 
> [8] is at least impossible to misinterpret, even if it's not the most
> self-explanatory option.

It's impossible to misinterpret, but also begs the questions of where
it comes from and if it will always be that way forever and ever.

Thanks,
drew

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > A complicated DIV_ROUND_UP() expression is currently written out
> > > explicitly in multiple places in order to specify the size of the
> > > bitmap exchanged with userspace to represent the value of the
> > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > 
> > > To make this more readable, this patch replaces these with a single
> > > define.
> > > 
> > > Since the number of words in a bitmap is just the index of the last
> > > word used + 1, this patch expresses the bound that way instead.
> > > This should make it clearer what is being expressed.
> > > 
> > > Since use of DIV_ROUND_UP() was the only reason for including
> > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > 
> > > No functional change.
> > > 
> > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  arch/arm64/kvm/guest.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > index 73044e3..f025a2f 100644
> > > --- a/arch/arm64/kvm/guest.c
> > > +++ b/arch/arm64/kvm/guest.c
> > > @@ -23,7 +23,6 @@
> > >  #include <linux/errno.h>
> > >  #include <linux/err.h>
> > >  #include <linux/nospec.h>
> > > -#include <linux/kernel.h>
> > >  #include <linux/kvm_host.h>
> > >  #include <linux/module.h>
> > >  #include <linux/stddef.h>
> > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > >  
> > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > +
> > >  static bool vq_present(
> > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > >  	unsigned int vq)
> > >  {
> > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > @@ -219,7 +220,7 @@ static bool vq_present(
> > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  {
> > >  	unsigned int max_vq, vq;
> > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > +	u64 vqs[SVE_VLS_WORDS];
> > >  
> > >  	if (!vcpu_has_sve(vcpu))
> > >  		return -ENOENT;
> > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > >  {
> > >  	unsigned int max_vq, vq;
> > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > +	u64 vqs[SVE_VLS_WORDS];
> > >  
> > >  	if (!vcpu_has_sve(vcpu))
> > >  		return -ENOENT;
> > > -- 
> > > 2.1.4
> > >
> > 
> > This is good, but I wonder if we could define the number of VLS words in
> > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > hard coded 8 ("__u64 vector_lengths[8]").
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> was avoiding it here.

It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
prefer we use it than to encourage KVM userspace to scatter 8's around.

> 
> [8] is at least impossible to misinterpret, even if it's not the most
> self-explanatory option.

It's impossible to misinterpret, but also begs the questions of where
it comes from and if it will always be that way forever and ever.

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > > A complicated DIV_ROUND_UP() expression is currently written out
> > > > explicitly in multiple places in order to specify the size of the
> > > > bitmap exchanged with userspace to represent the value of the
> > > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > > 
> > > > To make this more readable, this patch replaces these with a single
> > > > define.
> > > > 
> > > > Since the number of words in a bitmap is just the index of the last
> > > > word used + 1, this patch expresses the bound that way instead.
> > > > This should make it clearer what is being expressed.
> > > > 
> > > > Since use of DIV_ROUND_UP() was the only reason for including
> > > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > > 
> > > > No functional change.
> > > > 
> > > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > ---
> > > >  arch/arm64/kvm/guest.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > index 73044e3..f025a2f 100644
> > > > --- a/arch/arm64/kvm/guest.c
> > > > +++ b/arch/arm64/kvm/guest.c
> > > > @@ -23,7 +23,6 @@
> > > >  #include <linux/errno.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/nospec.h>
> > > > -#include <linux/kernel.h>
> > > >  #include <linux/kvm_host.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/stddef.h>
> > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > >  
> > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > > +
> > > >  static bool vq_present(
> > > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > > >  	unsigned int vq)
> > > >  {
> > > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > @@ -219,7 +220,7 @@ static bool vq_present(
> > > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  {
> > > >  	unsigned int max_vq, vq;
> > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > +	u64 vqs[SVE_VLS_WORDS];
> > > >  
> > > >  	if (!vcpu_has_sve(vcpu))
> > > >  		return -ENOENT;
> > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  {
> > > >  	unsigned int max_vq, vq;
> > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > +	u64 vqs[SVE_VLS_WORDS];
> > > >  
> > > >  	if (!vcpu_has_sve(vcpu))
> > > >  		return -ENOENT;
> > > > -- 
> > > > 2.1.4
> > > >
> > > 
> > > This is good, but I wonder if we could define the number of VLS words in
> > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > > hard coded 8 ("__u64 vector_lengths[8]").
> > > 
> > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > 
> > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> > was avoiding it here.
> 
> It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
> prefer we use it than to encourage KVM userspace to scatter 8's around.
> 
> > 
> > [8] is at least impossible to misinterpret, even if it's not the most
> > self-explanatory option.
> 
> It's impossible to misinterpret, but also begs the questions of where
> it comes from and if it will always be that way forever and ever.

It is sized to the maximum that makes sense without a major API
redesign, so the expectation is that it will never change (and userspace
should assume that it does not).

I sympathise though, since this case is less intuitive than most.

We could add some dedicated defines for this:

arch/arm64/include/uapi/asm/kvm.h:

#include <asm/sve_context.h> /* which we already have anyway */

#define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
#define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN

/* 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)


Then document as follows:
Documentation/virtual/kvm/api.txt:

__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];

if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */


Does that work for you?

Doing things this way avoids people having to include <asm/sigcontext.h>
(and the consequent clashes with glibc headers).

Cheers
---Dave

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > > A complicated DIV_ROUND_UP() expression is currently written out
> > > > explicitly in multiple places in order to specify the size of the
> > > > bitmap exchanged with userspace to represent the value of the
> > > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > > 
> > > > To make this more readable, this patch replaces these with a single
> > > > define.
> > > > 
> > > > Since the number of words in a bitmap is just the index of the last
> > > > word used + 1, this patch expresses the bound that way instead.
> > > > This should make it clearer what is being expressed.
> > > > 
> > > > Since use of DIV_ROUND_UP() was the only reason for including
> > > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > > 
> > > > No functional change.
> > > > 
> > > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > ---
> > > >  arch/arm64/kvm/guest.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > index 73044e3..f025a2f 100644
> > > > --- a/arch/arm64/kvm/guest.c
> > > > +++ b/arch/arm64/kvm/guest.c
> > > > @@ -23,7 +23,6 @@
> > > >  #include <linux/errno.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/nospec.h>
> > > > -#include <linux/kernel.h>
> > > >  #include <linux/kvm_host.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/stddef.h>
> > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > >  
> > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > > +
> > > >  static bool vq_present(
> > > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > > >  	unsigned int vq)
> > > >  {
> > > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > @@ -219,7 +220,7 @@ static bool vq_present(
> > > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  {
> > > >  	unsigned int max_vq, vq;
> > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > +	u64 vqs[SVE_VLS_WORDS];
> > > >  
> > > >  	if (!vcpu_has_sve(vcpu))
> > > >  		return -ENOENT;
> > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > >  {
> > > >  	unsigned int max_vq, vq;
> > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > +	u64 vqs[SVE_VLS_WORDS];
> > > >  
> > > >  	if (!vcpu_has_sve(vcpu))
> > > >  		return -ENOENT;
> > > > -- 
> > > > 2.1.4
> > > >
> > > 
> > > This is good, but I wonder if we could define the number of VLS words in
> > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > > hard coded 8 ("__u64 vector_lengths[8]").
> > > 
> > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > 
> > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> > was avoiding it here.
> 
> It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
> prefer we use it than to encourage KVM userspace to scatter 8's around.
> 
> > 
> > [8] is at least impossible to misinterpret, even if it's not the most
> > self-explanatory option.
> 
> It's impossible to misinterpret, but also begs the questions of where
> it comes from and if it will always be that way forever and ever.

It is sized to the maximum that makes sense without a major API
redesign, so the expectation is that it will never change (and userspace
should assume that it does not).

I sympathise though, since this case is less intuitive than most.

We could add some dedicated defines for this:

arch/arm64/include/uapi/asm/kvm.h:

#include <asm/sve_context.h> /* which we already have anyway */

#define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
#define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN

/* 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)


Then document as follows:
Documentation/virtual/kvm/api.txt:

__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];

if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */


Does that work for you?

Doing things this way avoids people having to include <asm/sigcontext.h>
(and the consequent clashes with glibc headers).

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote:
> > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > > > A complicated DIV_ROUND_UP() expression is currently written out
> > > > > explicitly in multiple places in order to specify the size of the
> > > > > bitmap exchanged with userspace to represent the value of the
> > > > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > > > 
> > > > > To make this more readable, this patch replaces these with a single
> > > > > define.
> > > > > 
> > > > > Since the number of words in a bitmap is just the index of the last
> > > > > word used + 1, this patch expresses the bound that way instead.
> > > > > This should make it clearer what is being expressed.
> > > > > 
> > > > > Since use of DIV_ROUND_UP() was the only reason for including
> > > > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > > > 
> > > > > No functional change.
> > > > > 
> > > > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > ---
> > > > >  arch/arm64/kvm/guest.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > > index 73044e3..f025a2f 100644
> > > > > --- a/arch/arm64/kvm/guest.c
> > > > > +++ b/arch/arm64/kvm/guest.c
> > > > > @@ -23,7 +23,6 @@
> > > > >  #include <linux/errno.h>
> > > > >  #include <linux/err.h>
> > > > >  #include <linux/nospec.h>
> > > > > -#include <linux/kernel.h>
> > > > >  #include <linux/kvm_host.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/stddef.h>
> > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > > >  
> > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > > > +
> > > > >  static bool vq_present(
> > > > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > > > >  	unsigned int vq)
> > > > >  {
> > > > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > > @@ -219,7 +220,7 @@ static bool vq_present(
> > > > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  {
> > > > >  	unsigned int max_vq, vq;
> > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > >  
> > > > >  	if (!vcpu_has_sve(vcpu))
> > > > >  		return -ENOENT;
> > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  {
> > > > >  	unsigned int max_vq, vq;
> > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > >  
> > > > >  	if (!vcpu_has_sve(vcpu))
> > > > >  		return -ENOENT;
> > > > > -- 
> > > > > 2.1.4
> > > > >
> > > > 
> > > > This is good, but I wonder if we could define the number of VLS words in
> > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > > > hard coded 8 ("__u64 vector_lengths[8]").
> > > > 
> > > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > > 
> > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> > > was avoiding it here.
> > 
> > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
> > prefer we use it than to encourage KVM userspace to scatter 8's around.
> > 
> > > 
> > > [8] is at least impossible to misinterpret, even if it's not the most
> > > self-explanatory option.
> > 
> > It's impossible to misinterpret, but also begs the questions of where
> > it comes from and if it will always be that way forever and ever.
> 
> It is sized to the maximum that makes sense without a major API
> redesign, so the expectation is that it will never change (and userspace
> should assume that it does not).
> 
> I sympathise though, since this case is less intuitive than most.
> 
> We could add some dedicated defines for this:
> 
> arch/arm64/include/uapi/asm/kvm.h:
> 
> #include <asm/sve_context.h> /* which we already have anyway */
> 
> #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> 
> /* 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)
> 
> 
> Then document as follows:
> Documentation/virtual/kvm/api.txt:
> 
> __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> 
> if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> 
> 
> Does that work for you?
> 
> Doing things this way avoids people having to include <asm/sigcontext.h>
> (and the consequent clashes with glibc headers).
>

I like that.

Thanks,
drew 

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote:
> > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > > > A complicated DIV_ROUND_UP() expression is currently written out
> > > > > explicitly in multiple places in order to specify the size of the
> > > > > bitmap exchanged with userspace to represent the value of the
> > > > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > > > 
> > > > > To make this more readable, this patch replaces these with a single
> > > > > define.
> > > > > 
> > > > > Since the number of words in a bitmap is just the index of the last
> > > > > word used + 1, this patch expresses the bound that way instead.
> > > > > This should make it clearer what is being expressed.
> > > > > 
> > > > > Since use of DIV_ROUND_UP() was the only reason for including
> > > > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > > > 
> > > > > No functional change.
> > > > > 
> > > > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > ---
> > > > >  arch/arm64/kvm/guest.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > > index 73044e3..f025a2f 100644
> > > > > --- a/arch/arm64/kvm/guest.c
> > > > > +++ b/arch/arm64/kvm/guest.c
> > > > > @@ -23,7 +23,6 @@
> > > > >  #include <linux/errno.h>
> > > > >  #include <linux/err.h>
> > > > >  #include <linux/nospec.h>
> > > > > -#include <linux/kernel.h>
> > > > >  #include <linux/kvm_host.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/stddef.h>
> > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > > >  
> > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > > > +
> > > > >  static bool vq_present(
> > > > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > > > >  	unsigned int vq)
> > > > >  {
> > > > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > > @@ -219,7 +220,7 @@ static bool vq_present(
> > > > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  {
> > > > >  	unsigned int max_vq, vq;
> > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > >  
> > > > >  	if (!vcpu_has_sve(vcpu))
> > > > >  		return -ENOENT;
> > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > >  {
> > > > >  	unsigned int max_vq, vq;
> > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > >  
> > > > >  	if (!vcpu_has_sve(vcpu))
> > > > >  		return -ENOENT;
> > > > > -- 
> > > > > 2.1.4
> > > > >
> > > > 
> > > > This is good, but I wonder if we could define the number of VLS words in
> > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > > > hard coded 8 ("__u64 vector_lengths[8]").
> > > > 
> > > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > > 
> > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> > > was avoiding it here.
> > 
> > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
> > prefer we use it than to encourage KVM userspace to scatter 8's around.
> > 
> > > 
> > > [8] is at least impossible to misinterpret, even if it's not the most
> > > self-explanatory option.
> > 
> > It's impossible to misinterpret, but also begs the questions of where
> > it comes from and if it will always be that way forever and ever.
> 
> It is sized to the maximum that makes sense without a major API
> redesign, so the expectation is that it will never change (and userspace
> should assume that it does not).
> 
> I sympathise though, since this case is less intuitive than most.
> 
> We could add some dedicated defines for this:
> 
> arch/arm64/include/uapi/asm/kvm.h:
> 
> #include <asm/sve_context.h> /* which we already have anyway */
> 
> #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> 
> /* 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)
> 
> 
> Then document as follows:
> Documentation/virtual/kvm/api.txt:
> 
> __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> 
> if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> 
> 
> Does that work for you?
> 
> Doing things this way avoids people having to include <asm/sigcontext.h>
> (and the consequent clashes with glibc headers).
>

I like that.

Thanks,
drew 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> > On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote:
> > > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> > > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > > > > A complicated DIV_ROUND_UP() expression is currently written out
> > > > > > explicitly in multiple places in order to specify the size of the
> > > > > > bitmap exchanged with userspace to represent the value of the
> > > > > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > > > > 
> > > > > > To make this more readable, this patch replaces these with a single
> > > > > > define.
> > > > > > 
> > > > > > Since the number of words in a bitmap is just the index of the last
> > > > > > word used + 1, this patch expresses the bound that way instead.
> > > > > > This should make it clearer what is being expressed.
> > > > > > 
> > > > > > Since use of DIV_ROUND_UP() was the only reason for including
> > > > > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > > > > 
> > > > > > No functional change.
> > > > > > 
> > > > > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > > ---
> > > > > >  arch/arm64/kvm/guest.c | 9 +++++----
> > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > > > index 73044e3..f025a2f 100644
> > > > > > --- a/arch/arm64/kvm/guest.c
> > > > > > +++ b/arch/arm64/kvm/guest.c
> > > > > > @@ -23,7 +23,6 @@
> > > > > >  #include <linux/errno.h>
> > > > > >  #include <linux/err.h>
> > > > > >  #include <linux/nospec.h>
> > > > > > -#include <linux/kernel.h>
> > > > > >  #include <linux/kvm_host.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/stddef.h>
> > > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > > > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > > > >  
> > > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > > > > +
> > > > > >  static bool vq_present(
> > > > > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > > > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > > > > >  	unsigned int vq)
> > > > > >  {
> > > > > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > > > @@ -219,7 +220,7 @@ static bool vq_present(
> > > > > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  {
> > > > > >  	unsigned int max_vq, vq;
> > > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > > >  
> > > > > >  	if (!vcpu_has_sve(vcpu))
> > > > > >  		return -ENOENT;
> > > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  {
> > > > > >  	unsigned int max_vq, vq;
> > > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > > >  
> > > > > >  	if (!vcpu_has_sve(vcpu))
> > > > > >  		return -ENOENT;
> > > > > > -- 
> > > > > > 2.1.4
> > > > > >
> > > > > 
> > > > > This is good, but I wonder if we could define the number of VLS words in
> > > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > > > > hard coded 8 ("__u64 vector_lengths[8]").
> > > > > 
> > > > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > > > 
> > > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> > > > was avoiding it here.
> > > 
> > > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
> > > prefer we use it than to encourage KVM userspace to scatter 8's around.
> > > 
> > > > 
> > > > [8] is at least impossible to misinterpret, even if it's not the most
> > > > self-explanatory option.
> > > 
> > > It's impossible to misinterpret, but also begs the questions of where
> > > it comes from and if it will always be that way forever and ever.
> > 
> > It is sized to the maximum that makes sense without a major API
> > redesign, so the expectation is that it will never change (and userspace
> > should assume that it does not).
> > 
> > I sympathise though, since this case is less intuitive than most.
> > 
> > We could add some dedicated defines for this:
> > 
> > arch/arm64/include/uapi/asm/kvm.h:
> > 
> > #include <asm/sve_context.h> /* which we already have anyway */
> > 
> > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > 
> > /* 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)
> > 
> > 
> > Then document as follows:
> > Documentation/virtual/kvm/api.txt:
> > 
> > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > 
> > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > 
> > 
> > Does that work for you?
> > 
> > Doing things this way avoids people having to include <asm/sigcontext.h>
> > (and the consequent clashes with glibc headers).
> >
> 
> I like that.

OK, I'll do that then.

I was worried this was going to end up too cumbersome / too verbose, but
it doesn't feel too bad in practice.

Cheers
---Dave

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> > On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote:
> > > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote:
> > > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote:
> > > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote:
> > > > > > A complicated DIV_ROUND_UP() expression is currently written out
> > > > > > explicitly in multiple places in order to specify the size of the
> > > > > > bitmap exchanged with userspace to represent the value of the
> > > > > > KVM_REG_ARM64_SVE_VLS pseudo-register.
> > > > > > 
> > > > > > To make this more readable, this patch replaces these with a single
> > > > > > define.
> > > > > > 
> > > > > > Since the number of words in a bitmap is just the index of the last
> > > > > > word used + 1, this patch expresses the bound that way instead.
> > > > > > This should make it clearer what is being expressed.
> > > > > > 
> > > > > > Since use of DIV_ROUND_UP() was the only reason for including
> > > > > > <linux/kernel.h> in guest.c, this patch removes that #include.
> > > > > > 
> > > > > > No functional change.
> > > > > > 
> > > > > > Suggested-by: Andrew Jones <drjones@redhat.com>
> > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > > ---
> > > > > >  arch/arm64/kvm/guest.c | 9 +++++----
> > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > > > index 73044e3..f025a2f 100644
> > > > > > --- a/arch/arm64/kvm/guest.c
> > > > > > +++ b/arch/arm64/kvm/guest.c
> > > > > > @@ -23,7 +23,6 @@
> > > > > >  #include <linux/errno.h>
> > > > > >  #include <linux/err.h>
> > > > > >  #include <linux/nospec.h>
> > > > > > -#include <linux/kernel.h>
> > > > > >  #include <linux/kvm_host.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/stddef.h>
> > > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> > > > > >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> > > > > >  
> > > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > > > > > +
> > > > > >  static bool vq_present(
> > > > > > -	const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)],
> > > > > > +	const u64 (*const vqs)[SVE_VLS_WORDS],
> > > > > >  	unsigned int vq)
> > > > > >  {
> > > > > >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > > > > > @@ -219,7 +220,7 @@ static bool vq_present(
> > > > > >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  {
> > > > > >  	unsigned int max_vq, vq;
> > > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > > >  
> > > > > >  	if (!vcpu_has_sve(vcpu))
> > > > > >  		return -ENOENT;
> > > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > > >  {
> > > > > >  	unsigned int max_vq, vq;
> > > > > > -	u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)];
> > > > > > +	u64 vqs[SVE_VLS_WORDS];
> > > > > >  
> > > > > >  	if (!vcpu_has_sve(vcpu))
> > > > > >  		return -ENOENT;
> > > > > > -- 
> > > > > > 2.1.4
> > > > > >
> > > > > 
> > > > > This is good, but I wonder if we could define the number of VLS words in
> > > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the
> > > > > hard coded 8 ("__u64 vector_lengths[8]").
> > > > > 
> > > > > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > > > 
> > > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I
> > > > was avoiding it here.
> > > 
> > > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd
> > > prefer we use it than to encourage KVM userspace to scatter 8's around.
> > > 
> > > > 
> > > > [8] is at least impossible to misinterpret, even if it's not the most
> > > > self-explanatory option.
> > > 
> > > It's impossible to misinterpret, but also begs the questions of where
> > > it comes from and if it will always be that way forever and ever.
> > 
> > It is sized to the maximum that makes sense without a major API
> > redesign, so the expectation is that it will never change (and userspace
> > should assume that it does not).
> > 
> > I sympathise though, since this case is less intuitive than most.
> > 
> > We could add some dedicated defines for this:
> > 
> > arch/arm64/include/uapi/asm/kvm.h:
> > 
> > #include <asm/sve_context.h> /* which we already have anyway */
> > 
> > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > 
> > /* 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)
> > 
> > 
> > Then document as follows:
> > Documentation/virtual/kvm/api.txt:
> > 
> > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > 
> > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > 
> > 
> > Does that work for you?
> > 
> > Doing things this way avoids people having to include <asm/sigcontext.h>
> > (and the consequent clashes with glibc headers).
> >
> 
> I like that.

OK, I'll do that then.

I was worried this was going to end up too cumbersome / too verbose, but
it doesn't feel too bad in practice.

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:

[...]

> > arch/arm64/include/uapi/asm/kvm.h:
> > 
> > #include <asm/sve_context.h> /* which we already have anyway */
> > 
> > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > 
> > /* 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)
> > 
> > 
> > Then document as follows:
> > Documentation/virtual/kvm/api.txt:
> > 
> > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > 
> > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > 
> > 
> > Does that work for you?
> > 
> > Doing things this way avoids people having to include <asm/sigcontext.h>
> > (and the consequent clashes with glibc headers).
> >
> 
> I like that.

OK, are you happy with the following on top of this patch?

Cheers
---Dave

--8<--

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 68509de..03df379 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure.
 KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector
 lengths supported by the vcpu to be discovered and configured by
 userspace.  When transferred to or from user memory via KVM_GET_ONE_REG
-or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and
-encodes the set of vector lengths as follows:
+or KVM_SET_ONE_REG, the value of this register is of type
+__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as
+follows:
 
-__u64 vector_lengths[8];
+__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
 
 if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
-    ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1))
+    ((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 */
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 2a04ef0..edd2db8 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -258,9 +258,14 @@ struct kvm_vcpu_events {
 	 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
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f025a2f..5bb909c 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
 #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
 
-#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
-
 static bool vq_present(
-	const u64 (*const vqs)[SVE_VLS_WORDS],
+	const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS],
 	unsigned int vq)
 {
 	return (*vqs)[vq_word(vq)] & vq_mask(vq);
@@ -220,7 +218,7 @@ static bool vq_present(
 static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[SVE_VLS_WORDS];
+	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;
@@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[SVE_VLS_WORDS];
+	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:

[...]

> > arch/arm64/include/uapi/asm/kvm.h:
> > 
> > #include <asm/sve_context.h> /* which we already have anyway */
> > 
> > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > 
> > /* 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)
> > 
> > 
> > Then document as follows:
> > Documentation/virtual/kvm/api.txt:
> > 
> > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > 
> > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > 
> > 
> > Does that work for you?
> > 
> > Doing things this way avoids people having to include <asm/sigcontext.h>
> > (and the consequent clashes with glibc headers).
> >
> 
> I like that.

OK, are you happy with the following on top of this patch?

Cheers
---Dave

--8<--

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 68509de..03df379 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure.
 KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector
 lengths supported by the vcpu to be discovered and configured by
 userspace.  When transferred to or from user memory via KVM_GET_ONE_REG
-or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and
-encodes the set of vector lengths as follows:
+or KVM_SET_ONE_REG, the value of this register is of type
+__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as
+follows:
 
-__u64 vector_lengths[8];
+__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
 
 if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
-    ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1))
+    ((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 */
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 2a04ef0..edd2db8 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -258,9 +258,14 @@ struct kvm_vcpu_events {
 	 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
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f025a2f..5bb909c 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
 #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
 
-#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
-
 static bool vq_present(
-	const u64 (*const vqs)[SVE_VLS_WORDS],
+	const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS],
 	unsigned int vq)
 {
 	return (*vqs)[vq_word(vq)] & vq_mask(vq);
@@ -220,7 +218,7 @@ static bool vq_present(
 static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[SVE_VLS_WORDS];
+	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;
@@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	unsigned int max_vq, vq;
-	u64 vqs[SVE_VLS_WORDS];
+	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
 
 	if (!vcpu_has_sve(vcpu))
 		return -ENOENT;

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Tue, Apr 16, 2019 at 04:52:28PM +0100, Dave Martin wrote:
> On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> 
> [...]
> 
> > > arch/arm64/include/uapi/asm/kvm.h:
> > > 
> > > #include <asm/sve_context.h> /* which we already have anyway */
> > > 
> > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > > 
> > > /* 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)
> > > 
> > > 
> > > Then document as follows:
> > > Documentation/virtual/kvm/api.txt:
> > > 
> > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > > 
> > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > > 
> > > 
> > > Does that work for you?
> > > 
> > > Doing things this way avoids people having to include <asm/sigcontext.h>
> > > (and the consequent clashes with glibc headers).
> > >
> > 
> > I like that.
> 
> OK, are you happy with the following on top of this patch?
> 
> Cheers
> ---Dave
> 
> --8<--
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 68509de..03df379 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure.
>  KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector
>  lengths supported by the vcpu to be discovered and configured by
>  userspace.  When transferred to or from user memory via KVM_GET_ONE_REG
> -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and
> -encodes the set of vector lengths as follows:
> +or KVM_SET_ONE_REG, the value of this register is of type
> +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as
> +follows:
>  
> -__u64 vector_lengths[8];
> +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
>  
>  if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
> -    ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1))
> +    ((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 */
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 2a04ef0..edd2db8 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -258,9 +258,14 @@ struct kvm_vcpu_events {
>  	 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
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index f025a2f..5bb909c 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
>  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
>  
> -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> -
>  static bool vq_present(
> -	const u64 (*const vqs)[SVE_VLS_WORDS],
> +	const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS],
>  	unsigned int vq)
>  {
>  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> @@ -220,7 +218,7 @@ static bool vq_present(
>  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[SVE_VLS_WORDS];
> +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
> @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[SVE_VLS_WORDS];
> +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
>

Yup

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Andrew Jones]

On Tue, Apr 16, 2019 at 04:52:28PM +0100, Dave Martin wrote:
> On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> 
> [...]
> 
> > > arch/arm64/include/uapi/asm/kvm.h:
> > > 
> > > #include <asm/sve_context.h> /* which we already have anyway */
> > > 
> > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > > 
> > > /* 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)
> > > 
> > > 
> > > Then document as follows:
> > > Documentation/virtual/kvm/api.txt:
> > > 
> > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > > 
> > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > > 
> > > 
> > > Does that work for you?
> > > 
> > > Doing things this way avoids people having to include <asm/sigcontext.h>
> > > (and the consequent clashes with glibc headers).
> > >
> > 
> > I like that.
> 
> OK, are you happy with the following on top of this patch?
> 
> Cheers
> ---Dave
> 
> --8<--
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 68509de..03df379 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure.
>  KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector
>  lengths supported by the vcpu to be discovered and configured by
>  userspace.  When transferred to or from user memory via KVM_GET_ONE_REG
> -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and
> -encodes the set of vector lengths as follows:
> +or KVM_SET_ONE_REG, the value of this register is of type
> +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as
> +follows:
>  
> -__u64 vector_lengths[8];
> +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
>  
>  if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
> -    ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1))
> +    ((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 */
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 2a04ef0..edd2db8 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -258,9 +258,14 @@ struct kvm_vcpu_events {
>  	 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
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index f025a2f..5bb909c 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
>  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
>  
> -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> -
>  static bool vq_present(
> -	const u64 (*const vqs)[SVE_VLS_WORDS],
> +	const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS],
>  	unsigned int vq)
>  {
>  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> @@ -220,7 +218,7 @@ static bool vq_present(
>  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[SVE_VLS_WORDS];
> +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
> @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> -	u64 vqs[SVE_VLS_WORDS];
> +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
>  
>  	if (!vcpu_has_sve(vcpu))
>  		return -ENOENT;
>

Yup

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Wed, Apr 17, 2019 at 06:16:56AM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 04:52:28PM +0100, Dave Martin wrote:
> > On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> > > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> > 
> > [...]
> > 
> > > > arch/arm64/include/uapi/asm/kvm.h:
> > > > 
> > > > #include <asm/sve_context.h> /* which we already have anyway */
> > > > 
> > > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > > > 
> > > > /* 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)
> > > > 
> > > > 
> > > > Then document as follows:
> > > > Documentation/virtual/kvm/api.txt:
> > > > 
> > > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > > > 
> > > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > > > 
> > > > 
> > > > Does that work for you?
> > > > 
> > > > Doing things this way avoids people having to include <asm/sigcontext.h>
> > > > (and the consequent clashes with glibc headers).
> > > >
> > > 
> > > I like that.
> > 
> > OK, are you happy with the following on top of this patch?
> > 
> > Cheers
> > ---Dave
> > 
> > --8<--
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 68509de..03df379 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure.
> >  KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector
> >  lengths supported by the vcpu to be discovered and configured by
> >  userspace.  When transferred to or from user memory via KVM_GET_ONE_REG
> > -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and
> > -encodes the set of vector lengths as follows:
> > +or KVM_SET_ONE_REG, the value of this register is of type
> > +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as
> > +follows:
> >  
> > -__u64 vector_lengths[8];
> > +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> >  
> >  if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
> > -    ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1))
> > +    ((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 */
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index 2a04ef0..edd2db8 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -258,9 +258,14 @@ struct kvm_vcpu_events {
> >  	 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
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index f025a2f..5bb909c 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> >  
> > -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > -
> >  static bool vq_present(
> > -	const u64 (*const vqs)[SVE_VLS_WORDS],
> > +	const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS],
> >  	unsigned int vq)
> >  {
> >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > @@ -220,7 +218,7 @@ static bool vq_present(
> >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[SVE_VLS_WORDS];
> > +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> > @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[SVE_VLS_WORDS];
> > +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> >
> 
> Yup
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

OK, thanks
---Dave

Re: [PATCH 09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

[Dave Martin]

On Wed, Apr 17, 2019 at 06:16:56AM +0200, Andrew Jones wrote:
> On Tue, Apr 16, 2019 at 04:52:28PM +0100, Dave Martin wrote:
> > On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote:
> > > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote:
> > 
> > [...]
> > 
> > > > arch/arm64/include/uapi/asm/kvm.h:
> > > > 
> > > > #include <asm/sve_context.h> /* which we already have anyway */
> > > > 
> > > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
> > > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
> > > > 
> > > > /* 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)
> > > > 
> > > > 
> > > > Then document as follows:
> > > > Documentation/virtual/kvm/api.txt:
> > > > 
> > > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> > > > 
> > > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_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 */
> > > > 
> > > > 
> > > > Does that work for you?
> > > > 
> > > > Doing things this way avoids people having to include <asm/sigcontext.h>
> > > > (and the consequent clashes with glibc headers).
> > > >
> > > 
> > > I like that.
> > 
> > OK, are you happy with the following on top of this patch?
> > 
> > Cheers
> > ---Dave
> > 
> > --8<--
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 68509de..03df379 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure.
> >  KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector
> >  lengths supported by the vcpu to be discovered and configured by
> >  userspace.  When transferred to or from user memory via KVM_GET_ONE_REG
> > -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and
> > -encodes the set of vector lengths as follows:
> > +or KVM_SET_ONE_REG, the value of this register is of type
> > +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as
> > +follows:
> >  
> > -__u64 vector_lengths[8];
> > +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS];
> >  
> >  if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX &&
> > -    ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1))
> > +    ((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 */
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index 2a04ef0..edd2db8 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -258,9 +258,14 @@ struct kvm_vcpu_events {
> >  	 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
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index f025a2f..5bb909c 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
> >  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
> >  
> > -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1)
> > -
> >  static bool vq_present(
> > -	const u64 (*const vqs)[SVE_VLS_WORDS],
> > +	const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS],
> >  	unsigned int vq)
> >  {
> >  	return (*vqs)[vq_word(vq)] & vq_mask(vq);
> > @@ -220,7 +218,7 @@ static bool vq_present(
> >  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[SVE_VLS_WORDS];
> > +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> > @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> >  {
> >  	unsigned int max_vq, vq;
> > -	u64 vqs[SVE_VLS_WORDS];
> > +	u64 vqs[KVM_ARM64_SVE_VLS_WORDS];
> >  
> >  	if (!vcpu_has_sve(vcpu))
> >  		return -ENOENT;
> >
> 
> Yup
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

OK, thanks
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 10/14] KVM: arm64/sve: Explain validity checks in set_sve_vls()

[Dave Martin]

Correct virtualization of SVE relies for correctness on code in
set_sve_vls() that verifies consistency between the set of vector
lengths requested by userspace and the set of vector lengths
available on the host.

However, the purpose of this code is not obvious, and not likely to
be apparent at all to people who do not have detailed knowledge of
the SVE system-level architecture.

This patch adds a suitable comment to explain what these checks are
for.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f025a2f..7f10d6d 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -266,6 +266,13 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if (max_vq > sve_vq_from_vl(kvm_sve_max_vl))
 		return -EINVAL;
 
+	/*
+	 * Vector lengths supported by the host can't currently be
+	 * hidden from the guest individually: instead we can only set a
+	 * maxmium via ZCR_EL2.LEN.  So, make sure the available vector
+	 * lengths match the set requested exactly up to the requested
+	 * maximum:
+	 */
 	for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
 		if (vq_present(&vqs, vq) != sve_vq_available(vq))
 			return -EINVAL;
-- 
2.1.4

[PATCH 10/14] KVM: arm64/sve: Explain validity checks in set_sve_vls()

[Dave Martin]

Correct virtualization of SVE relies for correctness on code in
set_sve_vls() that verifies consistency between the set of vector
lengths requested by userspace and the set of vector lengths
available on the host.

However, the purpose of this code is not obvious, and not likely to
be apparent at all to people who do not have detailed knowledge of
the SVE system-level architecture.

This patch adds a suitable comment to explain what these checks are
for.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/guest.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f025a2f..7f10d6d 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -266,6 +266,13 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if (max_vq > sve_vq_from_vl(kvm_sve_max_vl))
 		return -EINVAL;
 
+	/*
+	 * Vector lengths supported by the host can't currently be
+	 * hidden from the guest individually: instead we can only set a
+	 * maxmium via ZCR_EL2.LEN.  So, make sure the available vector
+	 * lengths match the set requested exactly up to the requested
+	 * maximum:
+	 */
 	for (vq = SVE_VQ_MIN; vq <= max_vq; ++vq)
 		if (vq_present(&vqs, vq) != sve_vq_available(vq))
 			return -EINVAL;
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 11/14] KVM: arm/arm64: Clean up vcpu finalization function parameter naming

[Dave Martin]

Currently, the internal vcpu finalization functions use a different
name ("what") for the feature parameter than the name ("feature")
used in the documentation.

To avoid future confusion, this patch converts everything to use
the name "feature" consistently.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 2 +-
 arch/arm64/include/asm/kvm_host.h | 2 +-
 arch/arm64/kvm/reset.c            | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 7feddac..fe77543 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -412,7 +412,7 @@ static inline int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type)
 	return 0;
 }
 
-static inline int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
+static inline int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
 {
 	return -EINVAL;
 }
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 6adf08b..7a096fd 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -627,7 +627,7 @@ void kvm_arch_free_vm(struct kvm *kvm);
 
 int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type);
 
-int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what);
+int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
 bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 8847f38..3402543 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -186,9 +186,9 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
+int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
 {
-	switch (what) {
+	switch (feature) {
 	case KVM_ARM_VCPU_SVE:
 		if (!vcpu_has_sve(vcpu))
 			return -EINVAL;
-- 
2.1.4

[PATCH 11/14] KVM: arm/arm64: Clean up vcpu finalization function parameter naming

[Dave Martin]

Currently, the internal vcpu finalization functions use a different
name ("what") for the feature parameter than the name ("feature")
used in the documentation.

To avoid future confusion, this patch converts everything to use
the name "feature" consistently.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 2 +-
 arch/arm64/include/asm/kvm_host.h | 2 +-
 arch/arm64/kvm/reset.c            | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 7feddac..fe77543 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -412,7 +412,7 @@ static inline int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type)
 	return 0;
 }
 
-static inline int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
+static inline int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
 {
 	return -EINVAL;
 }
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 6adf08b..7a096fd 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -627,7 +627,7 @@ void kvm_arch_free_vm(struct kvm *kvm);
 
 int kvm_arm_setup_stage2(struct kvm *kvm, unsigned long type);
 
-int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what);
+int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature);
 bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 8847f38..3402543 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -186,9 +186,9 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int what)
+int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
 {
-	switch (what) {
+	switch (feature) {
 	case KVM_ARM_VCPU_SVE:
 		if (!vcpu_has_sve(vcpu))
 			return -EINVAL;
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 12/14] KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE

[Dave Martin]

Userspace is only supposed to use KVM_ARM_VCPU_FINALIZE when there
is some vcpu feature that can actually be finalized.

This means that documenting KVM_ARM_VCPU_FINALIZE as available or
not depending on the capabilities present is not helpful.

This patch amends the documentation to describe availability in
terms of which capability is required for each finalizable feature
instead.

In any case, userspace sees the same error (EINVAL) regardless of
whether the given feature is not present or KVM_ARM_VCPU_FINALIZE
is not implemented at all.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 Documentation/virtual/kvm/api.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 68509de..b115b23 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3997,17 +3997,16 @@ userspace should not expect to get any particular value there.
 
 4.119 KVM_ARM_VCPU_FINALIZE
 
-Capability: KVM_CAP_ARM_SVE
 Architectures: arm, arm64
 Type: vcpu ioctl
 Parameters: int feature (in)
 Returns: 0 on success, -1 on error
 Errors:
   EPERM:     feature not enabled, needs configuration, or already finalized
-  EINVAL:    unknown feature
+  EINVAL:    feature unknown or not present
 
 Recognised values for feature:
-  arm64      KVM_ARM_VCPU_SVE
+  arm64      KVM_ARM_VCPU_SVE (requires KVM_CAP_ARM_SVE)
 
 Finalizes the configuration of the specified vcpu feature.
 
-- 
2.1.4

[PATCH 12/14] KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE

[Dave Martin]

Userspace is only supposed to use KVM_ARM_VCPU_FINALIZE when there
is some vcpu feature that can actually be finalized.

This means that documenting KVM_ARM_VCPU_FINALIZE as available or
not depending on the capabilities present is not helpful.

This patch amends the documentation to describe availability in
terms of which capability is required for each finalizable feature
instead.

In any case, userspace sees the same error (EINVAL) regardless of
whether the given feature is not present or KVM_ARM_VCPU_FINALIZE
is not implemented at all.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 Documentation/virtual/kvm/api.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 68509de..b115b23 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3997,17 +3997,16 @@ userspace should not expect to get any particular value there.
 
 4.119 KVM_ARM_VCPU_FINALIZE
 
-Capability: KVM_CAP_ARM_SVE
 Architectures: arm, arm64
 Type: vcpu ioctl
 Parameters: int feature (in)
 Returns: 0 on success, -1 on error
 Errors:
   EPERM:     feature not enabled, needs configuration, or already finalized
-  EINVAL:    unknown feature
+  EINVAL:    feature unknown or not present
 
 Recognised values for feature:
-  arm64      KVM_ARM_VCPU_SVE
+  arm64      KVM_ARM_VCPU_SVE (requires KVM_CAP_ARM_SVE)
 
 Finalizes the configuration of the specified vcpu feature.
 
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 13/14] KVM: Clarify KVM_{SET, GET}_ONE_REG error code documentation

[Dave Martin]

The current error code documentation for KVM_GET_ONE_REG and
KVM_SET_ONE_REG could be read as implying that all architectures
implement these error codes, or that KVM guararntees which error
code is returned in a particular situation.

Because this is not really the case, this patch waters down the
documentation explicitly to remove such guarantees.

EPERM is marked as arm64-specific, since for now arm64 really is
the only architecture that yields this error code for the
finalization-required case.  Keeping this as a distinct error code
is useful however for debugging due to the statefulness of the API
in this instance.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Fixes: 395f562f2b4c ("KVM: Document errors for KVM_GET_ONE_REG and KVM_SET_ONE_REG")
Fixes: 50036ad06b7f ("KVM: arm64/sve: Document KVM API extensions for SVE")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 Documentation/virtual/kvm/api.txt | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index b115b23..74c51c7 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1873,8 +1873,10 @@ Parameters: struct kvm_one_reg (in)
 Returns: 0 on success, negative value on failure
 Errors:
   ENOENT:   no such register
-  EPERM:    register access forbidden for architecture-dependent reasons
-  EINVAL:   other errors, such as bad size encoding for a known register
+  EINVAL:   invalid register ID, or no such register
+  EPERM:    (arm64) register access not allowed before vcpu finalization
+(These error codes are indicative only: do not rely on a specific error
+code being returned in a specific situation.)
 
 struct kvm_one_reg {
        __u64 id;
@@ -2258,10 +2260,12 @@ Architectures: all
 Type: vcpu ioctl
 Parameters: struct kvm_one_reg (in and out)
 Returns: 0 on success, negative value on failure
-Errors:
+Errors include:
   ENOENT:   no such register
-  EPERM:    register access forbidden for architecture-dependent reasons
-  EINVAL:   other errors, such as bad size encoding for a known register
+  EINVAL:   invalid register ID, or no such register
+  EPERM:    (arm64) register access not allowed before vcpu finalization
+(These error codes are indicative only: do not rely on a specific error
+code being returned in a specific situation.)
 
 This ioctl allows to receive the value of a single register implemented
 in a vcpu. The register to read is indicated by the "id" field of the
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 13/14] KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:17PM +0100, Dave Martin wrote:
> The current error code documentation for KVM_GET_ONE_REG and
> KVM_SET_ONE_REG could be read as implying that all architectures
> implement these error codes, or that KVM guararntees which error

guarantees

> code is returned in a particular situation.
> 
> Because this is not really the case, this patch waters down the
> documentation explicitly to remove such guarantees.
> 
> EPERM is marked as arm64-specific, since for now arm64 really is
> the only architecture that yields this error code for the
> finalization-required case.  Keeping this as a distinct error code
> is useful however for debugging due to the statefulness of the API
> in this instance.
> 
> No functional change.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Fixes: 395f562f2b4c ("KVM: Document errors for KVM_GET_ONE_REG and KVM_SET_ONE_REG")
> Fixes: 50036ad06b7f ("KVM: arm64/sve: Document KVM API extensions for SVE")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  Documentation/virtual/kvm/api.txt | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index b115b23..74c51c7 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1873,8 +1873,10 @@ Parameters: struct kvm_one_reg (in)
>  Returns: 0 on success, negative value on failure
>  Errors:
>    ENOENT:   no such register
> -  EPERM:    register access forbidden for architecture-dependent reasons
> -  EINVAL:   other errors, such as bad size encoding for a known register
> +  EINVAL:   invalid register ID, or no such register
> +  EPERM:    (arm64) register access not allowed before vcpu finalization
> +(These error codes are indicative only: do not rely on a specific error
> +code being returned in a specific situation.)
>  
>  struct kvm_one_reg {
>         __u64 id;
> @@ -2258,10 +2260,12 @@ Architectures: all
>  Type: vcpu ioctl
>  Parameters: struct kvm_one_reg (in and out)
>  Returns: 0 on success, negative value on failure
> -Errors:
> +Errors include:
>    ENOENT:   no such register
> -  EPERM:    register access forbidden for architecture-dependent reasons
> -  EINVAL:   other errors, such as bad size encoding for a known register
> +  EINVAL:   invalid register ID, or no such register
> +  EPERM:    (arm64) register access not allowed before vcpu finalization
> +(These error codes are indicative only: do not rely on a specific error
> +code being returned in a specific situation.)
>  
>  This ioctl allows to receive the value of a single register implemented
>  in a vcpu. The register to read is indicated by the "id" field of the
> -- 
> 2.1.4
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 13/14] KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation

[Andrew Jones]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 3025 bytes --]

On Fri, Apr 12, 2019 at 05:28:17PM +0100, Dave Martin wrote:
> The current error code documentation for KVM_GET_ONE_REG and
> KVM_SET_ONE_REG could be read as implying that all architectures
> implement these error codes, or that KVM guararntees which error

guarantees

> code is returned in a particular situation.
> 
> Because this is not really the case, this patch waters down the
> documentation explicitly to remove such guarantees.
> 
> EPERM is marked as arm64-specific, since for now arm64 really is
> the only architecture that yields this error code for the
> finalization-required case.  Keeping this as a distinct error code
> is useful however for debugging due to the statefulness of the API
> in this instance.
> 
> No functional change.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Fixes: 395f562f2b4c ("KVM: Document errors for KVM_GET_ONE_REG and KVM_SET_ONE_REG")
> Fixes: 50036ad06b7f ("KVM: arm64/sve: Document KVM API extensions for SVE")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  Documentation/virtual/kvm/api.txt | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index b115b23..74c51c7 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1873,8 +1873,10 @@ Parameters: struct kvm_one_reg (in)
>  Returns: 0 on success, negative value on failure
>  Errors:
>    ENOENT:   no such register
> -  EPERM:    register access forbidden for architecture-dependent reasons
> -  EINVAL:   other errors, such as bad size encoding for a known register
> +  EINVAL:   invalid register ID, or no such register
> +  EPERM:    (arm64) register access not allowed before vcpu finalization
> +(These error codes are indicative only: do not rely on a specific error
> +code being returned in a specific situation.)
>  
>  struct kvm_one_reg {
>         __u64 id;
> @@ -2258,10 +2260,12 @@ Architectures: all
>  Type: vcpu ioctl
>  Parameters: struct kvm_one_reg (in and out)
>  Returns: 0 on success, negative value on failure
> -Errors:
> +Errors include:
>    ENOENT:   no such register
> -  EPERM:    register access forbidden for architecture-dependent reasons
> -  EINVAL:   other errors, such as bad size encoding for a known register
> +  EINVAL:   invalid register ID, or no such register
> +  EPERM:    (arm64) register access not allowed before vcpu finalization
> +(These error codes are indicative only: do not rely on a specific error
> +code being returned in a specific situation.)
>  
>  This ioctl allows to receive the value of a single register implemented
>  in a vcpu. The register to read is indicated by the "id" field of the
> -- 
> 2.1.4
>

Reviewed-by: Andrew Jones <drjones@redhat.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 13/14] KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation

[Dave Martin]

On Mon, Apr 15, 2019 at 05:21:32PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:17PM +0100, Dave Martin wrote:
> > The current error code documentation for KVM_GET_ONE_REG and
> > KVM_SET_ONE_REG could be read as implying that all architectures
> > implement these error codes, or that KVM guararntees which error
> 
> guarantees
> 
> > code is returned in a particular situation.
> > 
> > Because this is not really the case, this patch waters down the
> > documentation explicitly to remove such guarantees.
> > 
> > EPERM is marked as arm64-specific, since for now arm64 really is
> > the only architecture that yields this error code for the
> > finalization-required case.  Keeping this as a distinct error code
> > is useful however for debugging due to the statefulness of the API
> > in this instance.
> > 
> > No functional change.
> > 
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Fixes: 395f562f2b4c ("KVM: Document errors for KVM_GET_ONE_REG and KVM_SET_ONE_REG")
> > Fixes: 50036ad06b7f ("KVM: arm64/sve: Document KVM API extensions for SVE")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  Documentation/virtual/kvm/api.txt | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index b115b23..74c51c7 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1873,8 +1873,10 @@ Parameters: struct kvm_one_reg (in)
> >  Returns: 0 on success, negative value on failure
> >  Errors:
> >    ENOENT:   no such register
> > -  EPERM:    register access forbidden for architecture-dependent reasons
> > -  EINVAL:   other errors, such as bad size encoding for a known register
> > +  EINVAL:   invalid register ID, or no such register
> > +  EPERM:    (arm64) register access not allowed before vcpu finalization
> > +(These error codes are indicative only: do not rely on a specific error
> > +code being returned in a specific situation.)
> >  
> >  struct kvm_one_reg {
> >         __u64 id;
> > @@ -2258,10 +2260,12 @@ Architectures: all
> >  Type: vcpu ioctl
> >  Parameters: struct kvm_one_reg (in and out)
> >  Returns: 0 on success, negative value on failure
> > -Errors:
> > +Errors include:
> >    ENOENT:   no such register
> > -  EPERM:    register access forbidden for architecture-dependent reasons
> > -  EINVAL:   other errors, such as bad size encoding for a known register
> > +  EINVAL:   invalid register ID, or no such register
> > +  EPERM:    (arm64) register access not allowed before vcpu finalization
> > +(These error codes are indicative only: do not rely on a specific error
> > +code being returned in a specific situation.)
> >  
> >  This ioctl allows to receive the value of a single register implemented
> >  in a vcpu. The register to read is indicated by the "id" field of the
> > -- 
> > 2.1.4
> >
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks
---Dave

Re: [PATCH 13/14] KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation

[Dave Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 3239 bytes --]

On Mon, Apr 15, 2019 at 05:21:32PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:17PM +0100, Dave Martin wrote:
> > The current error code documentation for KVM_GET_ONE_REG and
> > KVM_SET_ONE_REG could be read as implying that all architectures
> > implement these error codes, or that KVM guararntees which error
> 
> guarantees
> 
> > code is returned in a particular situation.
> > 
> > Because this is not really the case, this patch waters down the
> > documentation explicitly to remove such guarantees.
> > 
> > EPERM is marked as arm64-specific, since for now arm64 really is
> > the only architecture that yields this error code for the
> > finalization-required case.  Keeping this as a distinct error code
> > is useful however for debugging due to the statefulness of the API
> > in this instance.
> > 
> > No functional change.
> > 
> > Suggested-by: Andrew Jones <drjones@redhat.com>
> > Fixes: 395f562f2b4c ("KVM: Document errors for KVM_GET_ONE_REG and KVM_SET_ONE_REG")
> > Fixes: 50036ad06b7f ("KVM: arm64/sve: Document KVM API extensions for SVE")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  Documentation/virtual/kvm/api.txt | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index b115b23..74c51c7 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1873,8 +1873,10 @@ Parameters: struct kvm_one_reg (in)
> >  Returns: 0 on success, negative value on failure
> >  Errors:
> >    ENOENT:   no such register
> > -  EPERM:    register access forbidden for architecture-dependent reasons
> > -  EINVAL:   other errors, such as bad size encoding for a known register
> > +  EINVAL:   invalid register ID, or no such register
> > +  EPERM:    (arm64) register access not allowed before vcpu finalization
> > +(These error codes are indicative only: do not rely on a specific error
> > +code being returned in a specific situation.)
> >  
> >  struct kvm_one_reg {
> >         __u64 id;
> > @@ -2258,10 +2260,12 @@ Architectures: all
> >  Type: vcpu ioctl
> >  Parameters: struct kvm_one_reg (in and out)
> >  Returns: 0 on success, negative value on failure
> > -Errors:
> > +Errors include:
> >    ENOENT:   no such register
> > -  EPERM:    register access forbidden for architecture-dependent reasons
> > -  EINVAL:   other errors, such as bad size encoding for a known register
> > +  EINVAL:   invalid register ID, or no such register
> > +  EPERM:    (arm64) register access not allowed before vcpu finalization
> > +(These error codes are indicative only: do not rely on a specific error
> > +code being returned in a specific situation.)
> >  
> >  This ioctl allows to receive the value of a single register implemented
> >  in a vcpu. The register to read is indicated by the "id" field of the
> > -- 
> > 2.1.4
> >
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 14/14] KVM: arm64: Clarify access behaviour for out-of-range SVE register slice IDs

[Dave Martin]

The existing documentation for which SVE register slice IDs are
considered out-of-range, and what happens when userspace tries to
access them, is cryptic.

This patch rewords the text with the aim of making it a bit easier to
understand.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 Documentation/virtual/kvm/api.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 74c51c7..91cb12a 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2159,8 +2159,9 @@ arm64 SVE registers have the following bit patterns:
   0x6050 0000 0015 060 <slice:5>        FFR bits[256*slice + 255 : 256*slice]
   0x6060 0000 0015 ffff                 KVM_REG_ARM64_SVE_VLS pseudo-register
 
-Access to slices beyond the maximum vector length configured for the
-vcpu (i.e., where 16 * slice >= max_vq (**)) will fail with ENOENT.
+Access to register IDs where 2048 * slice >= 128 * max_vq will fail with
+ENOENT.  max_vq is the vcpu's maximum supported vector length in 128-bit
+quadwords: see (**) below.
 
 These registers are only accessible on vcpus for which SVE is enabled.
 See KVM_ARM_VCPU_INIT for details.
-- 
2.1.4

[PATCH 14/14] KVM: arm64: Clarify access behaviour for out-of-range SVE register slice IDs

[Dave Martin]

The existing documentation for which SVE register slice IDs are
considered out-of-range, and what happens when userspace tries to
access them, is cryptic.

This patch rewords the text with the aim of making it a bit easier to
understand.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 Documentation/virtual/kvm/api.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 74c51c7..91cb12a 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2159,8 +2159,9 @@ arm64 SVE registers have the following bit patterns:
   0x6050 0000 0015 060 <slice:5>        FFR bits[256*slice + 255 : 256*slice]
   0x6060 0000 0015 ffff                 KVM_REG_ARM64_SVE_VLS pseudo-register
 
-Access to slices beyond the maximum vector length configured for the
-vcpu (i.e., where 16 * slice >= max_vq (**)) will fail with ENOENT.
+Access to register IDs where 2048 * slice >= 128 * max_vq will fail with
+ENOENT.  max_vq is the vcpu's maximum supported vector length in 128-bit
+quadwords: see (**) below.
 
 These registers are only accessible on vcpus for which SVE is enabled.
 See KVM_ARM_VCPU_INIT for details.
-- 
2.1.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 00/14] KVM: arm64: SVE cleanups

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:04PM +0100, Dave Martin wrote:
> This series contains some cleanups applicable to the SVE KVM support
> patches merged into kvmarm/next.  These arose from Andrew Jones'
> review.
> 
> Apart from some minor changes to error codes and checking, these are
> mostly cosmetic / sytlistic changes only.
> 
> Although straightforward, these changes have not been tested yet:
> I'm posting them now so that reviewers can make a start on them.
> 
> The patches are based on
> git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
> 5d8d4af24460 ("arm64: KVM: Fix system register enumeration")
> 
> This series in git:
> git://linux-arm.org/linux-dm.git sve-kvm-fixes/v1/head
> http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/sve-kvm-fixes/v1/head
> 
> Dave Martin (14):
>   arm64/sve: Clarify vq map semantics
>   KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up
>     SVE
>   KVM: arm: Make vcpu finalization stubs into inline functions
>   KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to
>     WARNs
>   KVM: arm64/sve: Clean up UAPI register ID definitions
>   KVM: arm64/sve: Miscellaneous tidyups in guest.c
>   KVM: arm64/sve: Make register ioctl access errors more consistent
>   KVM: arm64/sve: WARN when avoiding divide-by-zero in
>     sve_reg_to_region()
>   KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing
>   KVM: arm64/sve: Explain validity checks in set_sve_vls()
>   KVM: arm/arm64: Clean up vcpu finalization function parameter naming
>   KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE
>   KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation
>   KVM: arm64: Clarify access behaviour for out-of-range SVE register
>     slice IDs
> 
>  Documentation/virtual/kvm/api.txt | 24 ++++++-----
>  arch/arm/include/asm/kvm_host.h   | 13 ++++--
>  arch/arm64/include/asm/fpsimd.h   |  4 --
>  arch/arm64/include/asm/kvm_host.h |  4 +-
>  arch/arm64/include/uapi/asm/kvm.h | 32 ++++++++++----
>  arch/arm64/kernel/fpsimd.c        |  7 ++-
>  arch/arm64/kvm/guest.c            | 91 +++++++++++++++++++++++----------------
>  arch/arm64/kvm/reset.c            |  6 +--
>  arch/arm64/kvm/sys_regs.c         |  4 +-
>  virt/kvm/arm/arm.c                |  2 +-
>  10 files changed, 116 insertions(+), 71 deletions(-)
> 
> -- 
> 2.1.4
>

For the series

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 00/14] KVM: arm64: SVE cleanups

[Andrew Jones]

On Fri, Apr 12, 2019 at 05:28:04PM +0100, Dave Martin wrote:
> This series contains some cleanups applicable to the SVE KVM support
> patches merged into kvmarm/next.  These arose from Andrew Jones'
> review.
> 
> Apart from some minor changes to error codes and checking, these are
> mostly cosmetic / sytlistic changes only.
> 
> Although straightforward, these changes have not been tested yet:
> I'm posting them now so that reviewers can make a start on them.
> 
> The patches are based on
> git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
> 5d8d4af24460 ("arm64: KVM: Fix system register enumeration")
> 
> This series in git:
> git://linux-arm.org/linux-dm.git sve-kvm-fixes/v1/head
> http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/sve-kvm-fixes/v1/head
> 
> Dave Martin (14):
>   arm64/sve: Clarify vq map semantics
>   KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up
>     SVE
>   KVM: arm: Make vcpu finalization stubs into inline functions
>   KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to
>     WARNs
>   KVM: arm64/sve: Clean up UAPI register ID definitions
>   KVM: arm64/sve: Miscellaneous tidyups in guest.c
>   KVM: arm64/sve: Make register ioctl access errors more consistent
>   KVM: arm64/sve: WARN when avoiding divide-by-zero in
>     sve_reg_to_region()
>   KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing
>   KVM: arm64/sve: Explain validity checks in set_sve_vls()
>   KVM: arm/arm64: Clean up vcpu finalization function parameter naming
>   KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE
>   KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation
>   KVM: arm64: Clarify access behaviour for out-of-range SVE register
>     slice IDs
> 
>  Documentation/virtual/kvm/api.txt | 24 ++++++-----
>  arch/arm/include/asm/kvm_host.h   | 13 ++++--
>  arch/arm64/include/asm/fpsimd.h   |  4 --
>  arch/arm64/include/asm/kvm_host.h |  4 +-
>  arch/arm64/include/uapi/asm/kvm.h | 32 ++++++++++----
>  arch/arm64/kernel/fpsimd.c        |  7 ++-
>  arch/arm64/kvm/guest.c            | 91 +++++++++++++++++++++++----------------
>  arch/arm64/kvm/reset.c            |  6 +--
>  arch/arm64/kvm/sys_regs.c         |  4 +-
>  virt/kvm/arm/arm.c                |  2 +-
>  10 files changed, 116 insertions(+), 71 deletions(-)
> 
> -- 
> 2.1.4
>

For the series

Reviewed-by: Andrew Jones <drjones@redhat.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 00/14] KVM: arm64: SVE cleanups

[Dave Martin]

On Mon, Apr 15, 2019 at 05:22:13PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:04PM +0100, Dave Martin wrote:
> > This series contains some cleanups applicable to the SVE KVM support
> > patches merged into kvmarm/next.  These arose from Andrew Jones'
> > review.
> > 
> > Apart from some minor changes to error codes and checking, these are
> > mostly cosmetic / sytlistic changes only.
> > 
> > Although straightforward, these changes have not been tested yet:
> > I'm posting them now so that reviewers can make a start on them.
> > 
> > The patches are based on
> > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
> > 5d8d4af24460 ("arm64: KVM: Fix system register enumeration")
> > 
> > This series in git:
> > git://linux-arm.org/linux-dm.git sve-kvm-fixes/v1/head
> > http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/sve-kvm-fixes/v1/head
> > 
> > Dave Martin (14):
> >   arm64/sve: Clarify vq map semantics
> >   KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up
> >     SVE
> >   KVM: arm: Make vcpu finalization stubs into inline functions
> >   KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to
> >     WARNs
> >   KVM: arm64/sve: Clean up UAPI register ID definitions
> >   KVM: arm64/sve: Miscellaneous tidyups in guest.c
> >   KVM: arm64/sve: Make register ioctl access errors more consistent
> >   KVM: arm64/sve: WARN when avoiding divide-by-zero in
> >     sve_reg_to_region()
> >   KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing
> >   KVM: arm64/sve: Explain validity checks in set_sve_vls()
> >   KVM: arm/arm64: Clean up vcpu finalization function parameter naming
> >   KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE
> >   KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation
> >   KVM: arm64: Clarify access behaviour for out-of-range SVE register
> >     slice IDs
> > 
> >  Documentation/virtual/kvm/api.txt | 24 ++++++-----
> >  arch/arm/include/asm/kvm_host.h   | 13 ++++--
> >  arch/arm64/include/asm/fpsimd.h   |  4 --
> >  arch/arm64/include/asm/kvm_host.h |  4 +-
> >  arch/arm64/include/uapi/asm/kvm.h | 32 ++++++++++----
> >  arch/arm64/kernel/fpsimd.c        |  7 ++-
> >  arch/arm64/kvm/guest.c            | 91 +++++++++++++++++++++++----------------
> >  arch/arm64/kvm/reset.c            |  6 +--
> >  arch/arm64/kvm/sys_regs.c         |  4 +-
> >  virt/kvm/arm/arm.c                |  2 +-
> >  10 files changed, 116 insertions(+), 71 deletions(-)
> > 
> > -- 
> > 2.1.4
> >
> 
> For the series
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks for the review
---Dave

Re: [PATCH 00/14] KVM: arm64: SVE cleanups

[Dave Martin]

On Mon, Apr 15, 2019 at 05:22:13PM +0200, Andrew Jones wrote:
> On Fri, Apr 12, 2019 at 05:28:04PM +0100, Dave Martin wrote:
> > This series contains some cleanups applicable to the SVE KVM support
> > patches merged into kvmarm/next.  These arose from Andrew Jones'
> > review.
> > 
> > Apart from some minor changes to error codes and checking, these are
> > mostly cosmetic / sytlistic changes only.
> > 
> > Although straightforward, these changes have not been tested yet:
> > I'm posting them now so that reviewers can make a start on them.
> > 
> > The patches are based on
> > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
> > 5d8d4af24460 ("arm64: KVM: Fix system register enumeration")
> > 
> > This series in git:
> > git://linux-arm.org/linux-dm.git sve-kvm-fixes/v1/head
> > http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/sve-kvm-fixes/v1/head
> > 
> > Dave Martin (14):
> >   arm64/sve: Clarify vq map semantics
> >   KVM: arm/arm64: Demote kvm_arm_init_arch_resources() to just set up
> >     SVE
> >   KVM: arm: Make vcpu finalization stubs into inline functions
> >   KVM: arm64/sve: sys_regs: Demote redundant vcpu_has_sve() checks to
> >     WARNs
> >   KVM: arm64/sve: Clean up UAPI register ID definitions
> >   KVM: arm64/sve: Miscellaneous tidyups in guest.c
> >   KVM: arm64/sve: Make register ioctl access errors more consistent
> >   KVM: arm64/sve: WARN when avoiding divide-by-zero in
> >     sve_reg_to_region()
> >   KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing
> >   KVM: arm64/sve: Explain validity checks in set_sve_vls()
> >   KVM: arm/arm64: Clean up vcpu finalization function parameter naming
> >   KVM: Clarify capability requirements for KVM_ARM_VCPU_FINALIZE
> >   KVM: Clarify KVM_{SET,GET}_ONE_REG error code documentation
> >   KVM: arm64: Clarify access behaviour for out-of-range SVE register
> >     slice IDs
> > 
> >  Documentation/virtual/kvm/api.txt | 24 ++++++-----
> >  arch/arm/include/asm/kvm_host.h   | 13 ++++--
> >  arch/arm64/include/asm/fpsimd.h   |  4 --
> >  arch/arm64/include/asm/kvm_host.h |  4 +-
> >  arch/arm64/include/uapi/asm/kvm.h | 32 ++++++++++----
> >  arch/arm64/kernel/fpsimd.c        |  7 ++-
> >  arch/arm64/kvm/guest.c            | 91 +++++++++++++++++++++++----------------
> >  arch/arm64/kvm/reset.c            |  6 +--
> >  arch/arm64/kvm/sys_regs.c         |  4 +-
> >  virt/kvm/arm/arm.c                |  2 +-
> >  10 files changed, 116 insertions(+), 71 deletions(-)
> > 
> > -- 
> > 2.1.4
> >
> 
> For the series
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks for the review
---Dave

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm