[PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

As observed during review the pKVM support for saving host SVE state is
broken if an asymmetric system has VLs larger than the maximum shared
VL, fix this by discovering then using the maximum VL for allocations
and using RDVL during the save/restore process.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Changes in v4:
- Roll in Catalin's acks.
- Link to v3: https://lore.kernel.org/r/20240607-kvm-arm64-fix-pkvm-sve-vl-v3-0-59e781706d65@kernel.org

Changes in v3:
- Replace %u with %lu in late CPU error message.
Changes in v2:
- Downgrade check for a late CPU increasing maximum VL to a warning only
  but do it unconditionally since pKVM prevents late CPUs anyway.
- Commit log tweaks.
- Link to v1: https://lore.kernel.org/r/20240605-kvm-arm64-fix-pkvm-sve-vl-v1-0-680d6b43b4c1@kernel.org

---
Mark Brown (4):
      arm64/fpsimd: Introduce __bit_to_vl() helper
      arm64/fpsimd: Discover maximum vector length implemented by any CPU
      KVM: arm64: Fix FFR offset calculation for pKVM host state save and restore
      KVM: arm64: Avoid underallocating storage for host SVE state

 arch/arm64/include/asm/fpsimd.h         | 17 +++++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  2 +-
 arch/arm64/include/asm/kvm_hyp.h        |  3 ++-
 arch/arm64/include/asm/kvm_pkvm.h       |  2 +-
 arch/arm64/kernel/fpsimd.c              | 38 +++++++++++++++++++++++++++------
 arch/arm64/kvm/hyp/fpsimd.S             |  5 +++++
 arch/arm64/kvm/hyp/include/hyp/switch.h |  2 +-
 arch/arm64/kvm/hyp/nvhe/hyp-main.c      |  6 +++---
 arch/arm64/kvm/hyp/nvhe/pkvm.c          |  2 +-
 arch/arm64/kvm/reset.c                  |  6 +++---
 10 files changed, 65 insertions(+), 18 deletions(-)
---
base-commit: afb91f5f8ad7af172d993a34fde1947892408f53
change-id: 20240604-kvm-arm64-fix-pkvm-sve-vl-13cd71fd7db0

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH v4 1/4] arm64/fpsimd: Introduce __bit_to_vl() helper

[Mark Brown]

In all cases where we use the existing __bit_to_vq() helper we immediately
convert the result into a VL. Provide and use __bit_to_vl() doing this
directly.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h |  4 ++++
 arch/arm64/kernel/fpsimd.c      | 12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index bc69ac368d73..51c21265b4fa 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -172,6 +172,10 @@ static inline unsigned int __bit_to_vq(unsigned int bit)
 	return SVE_VQ_MAX - bit;
 }
 
+static inline unsigned int __bit_to_vl(unsigned int bit)
+{
+	return sve_vl_from_vq(__bit_to_vq(bit));
+}
 
 struct vl_info {
 	enum vec_type type;
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 82e8a6017382..22542fb81812 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -530,7 +530,7 @@ static unsigned int find_supported_vector_length(enum vec_type type,
 
 	bit = find_next_bit(info->vq_map, SVE_VQ_MAX,
 			    __vq_to_bit(sve_vq_from_vl(vl)));
-	return sve_vl_from_vq(__bit_to_vq(bit));
+	return __bit_to_vl(bit);
 }
 
 #if defined(CONFIG_ARM64_SVE) && defined(CONFIG_SYSCTL)
@@ -1103,7 +1103,7 @@ int vec_verify_vq_map(enum vec_type type)
 	 * Mismatches above sve_max_virtualisable_vl are fine, since
 	 * no guest is allowed to configure ZCR_EL2.LEN to exceed this:
 	 */
-	if (sve_vl_from_vq(__bit_to_vq(b)) <= info->max_virtualisable_vl) {
+	if (__bit_to_vl(b) <= info->max_virtualisable_vl) {
 		pr_warn("%s: cpu%d: Unsupported vector length(s) present\n",
 			info->name, smp_processor_id());
 		return -EINVAL;
@@ -1169,7 +1169,7 @@ void __init sve_setup(void)
 		set_bit(__vq_to_bit(SVE_VQ_MIN), info->vq_map);
 
 	max_bit = find_first_bit(info->vq_map, SVE_VQ_MAX);
-	info->max_vl = sve_vl_from_vq(__bit_to_vq(max_bit));
+	info->max_vl = __bit_to_vl(max_bit);
 
 	/*
 	 * For the default VL, pick the maximum supported value <= 64.
@@ -1188,7 +1188,7 @@ void __init sve_setup(void)
 		/* No virtualisable VLs?  This is architecturally forbidden. */
 		info->max_virtualisable_vl = SVE_VQ_MIN;
 	else /* b + 1 < SVE_VQ_MAX */
-		info->max_virtualisable_vl = sve_vl_from_vq(__bit_to_vq(b + 1));
+		info->max_virtualisable_vl = __bit_to_vl(b + 1);
 
 	if (info->max_virtualisable_vl > info->max_vl)
 		info->max_virtualisable_vl = info->max_vl;
@@ -1305,10 +1305,10 @@ void __init sme_setup(void)
 	WARN_ON(bitmap_empty(info->vq_map, SVE_VQ_MAX));
 
 	min_bit = find_last_bit(info->vq_map, SVE_VQ_MAX);
-	info->min_vl = sve_vl_from_vq(__bit_to_vq(min_bit));
+	info->min_vl = __bit_to_vl(min_bit);
 
 	max_bit = find_first_bit(info->vq_map, SVE_VQ_MAX);
-	info->max_vl = sve_vl_from_vq(__bit_to_vq(max_bit));
+	info->max_vl = __bit_to_vl(max_bit);
 
 	WARN_ON(info->min_vl > info->max_vl);
 

-- 
2.39.2

[PATCH v4 2/4] arm64/fpsimd: Discover maximum vector length implemented by any CPU

[Mark Brown]

When discovering the vector lengths for SVE and SME we do not currently
record the maximum VL supported on any individual CPU.  This is expected
to be the same for all CPUs but the architecture allows asymmetry, if we
do encounter an asymmetric system then some CPUs may support VLs higher
than the maximum Linux will use.  Since the pKVM hypervisor needs to
support saving and restoring anything the host can physically set it
needs to know the maximum value any CPU could have, add support for
enumerating it and validation for late CPUs.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h | 13 +++++++++++++
 arch/arm64/kernel/fpsimd.c      | 26 +++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 51c21265b4fa..cd19713c9deb 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -188,6 +188,9 @@ struct vl_info {
 	int max_vl;
 	int max_virtualisable_vl;
 
+	/* Maximum vector length observed on any CPU */
+	int max_cpu_vl;
+
 	/*
 	 * Set of available vector lengths,
 	 * where length vq encoded as bit __vq_to_bit(vq):
@@ -278,6 +281,11 @@ static inline int vec_max_virtualisable_vl(enum vec_type type)
 	return vl_info[type].max_virtualisable_vl;
 }
 
+static inline int vec_max_cpu_vl(enum vec_type type)
+{
+	return vl_info[type].max_cpu_vl;
+}
+
 static inline int sve_max_vl(void)
 {
 	return vec_max_vl(ARM64_VEC_SVE);
@@ -288,6 +296,11 @@ static inline int sve_max_virtualisable_vl(void)
 	return vec_max_virtualisable_vl(ARM64_VEC_SVE);
 }
 
+static inline int sve_max_cpu_vl(void)
+{
+	return vec_max_cpu_vl(ARM64_VEC_SVE);
+}
+
 /* Ensure vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX before calling this function */
 static inline bool vq_available(enum vec_type type, unsigned int vq)
 {
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 22542fb81812..83984cb3f21a 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -129,6 +129,7 @@ __ro_after_init struct vl_info vl_info[ARM64_VEC_MAX] = {
 		.min_vl			= SVE_VL_MIN,
 		.max_vl			= SVE_VL_MIN,
 		.max_virtualisable_vl	= SVE_VL_MIN,
+		.max_cpu_vl		= SVE_VL_MIN,
 	},
 #endif
 #ifdef CONFIG_ARM64_SME
@@ -1041,8 +1042,13 @@ static void vec_probe_vqs(struct vl_info *info,
 void __init vec_init_vq_map(enum vec_type type)
 {
 	struct vl_info *info = &vl_info[type];
+	unsigned long b;
+
 	vec_probe_vqs(info, info->vq_map);
 	bitmap_copy(info->vq_partial_map, info->vq_map, SVE_VQ_MAX);
+
+	b = find_first_bit(info->vq_map, SVE_VQ_MAX);
+	info->max_cpu_vl = __bit_to_vl(b);
 }
 
 /*
@@ -1054,11 +1060,16 @@ void vec_update_vq_map(enum vec_type type)
 {
 	struct vl_info *info = &vl_info[type];
 	DECLARE_BITMAP(tmp_map, SVE_VQ_MAX);
+	unsigned long b;
 
 	vec_probe_vqs(info, tmp_map);
 	bitmap_and(info->vq_map, info->vq_map, tmp_map, SVE_VQ_MAX);
 	bitmap_or(info->vq_partial_map, info->vq_partial_map, tmp_map,
 		  SVE_VQ_MAX);
+
+	b = find_first_bit(tmp_map, SVE_VQ_MAX);
+	if (__bit_to_vl(b) > info->max_cpu_vl)
+		info->max_cpu_vl = __bit_to_vl(b);
 }
 
 /*
@@ -1069,10 +1080,23 @@ int vec_verify_vq_map(enum vec_type type)
 {
 	struct vl_info *info = &vl_info[type];
 	DECLARE_BITMAP(tmp_map, SVE_VQ_MAX);
-	unsigned long b;
+	unsigned long b, max_vl;
 
 	vec_probe_vqs(info, tmp_map);
 
+	/*
+	 * Currently the maximum VL is only used for pKVM which
+	 * doesn't allow late CPUs but we don't expect asymmetry and
+	 * if we encounter any then future users will need handling so
+	 * warn if we see anything.
+	 */
+	max_vl = __bit_to_vl(find_first_bit(tmp_map, SVE_VQ_MAX));
+	if (max_vl > info->max_cpu_vl) {
+		pr_warn("%s: cpu%d: increases maximum VL to %lu\n",
+			info->name, smp_processor_id(), max_vl);
+		info->max_cpu_vl = max_vl;
+	}
+
 	bitmap_complement(tmp_map, tmp_map, SVE_VQ_MAX);
 	if (bitmap_intersects(tmp_map, info->vq_map, SVE_VQ_MAX)) {
 		pr_warn("%s: cpu%d: Required vector length(s) missing\n",

-- 
2.39.2

[PATCH v4 3/4] KVM: arm64: Fix FFR offset calculation for pKVM host state save and restore

[Mark Brown]

When saving and restoring the SVE state for the host we configure the
hardware for the maximum VL it supports, but when calculating offsets in
memory we use the maximum usable VL for the host. Since these two values
may not be the same this may result in data corruption.  We can just
read the current VL from the hardware with an instruction so do that
instead of a saved value, we need to correct the value and this makes
the consistency obvious.

Fixes: b5b9955617bc ("KVM: arm64: Eagerly restore host fpsimd/sve state in pKVM")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/kvm_hyp.h        | 1 +
 arch/arm64/kvm/hyp/fpsimd.S             | 5 +++++
 arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
 arch/arm64/kvm/hyp/nvhe/hyp-main.c      | 2 +-
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index b05bceca3385..7510383d78a6 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -113,6 +113,7 @@ void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
 void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
 void __sve_save_state(void *sve_pffr, u32 *fpsr, int save_ffr);
 void __sve_restore_state(void *sve_pffr, u32 *fpsr, int restore_ffr);
+int __sve_get_vl(void);
 
 u64 __guest_enter(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
index e950875e31ce..d272dbf36da8 100644
--- a/arch/arm64/kvm/hyp/fpsimd.S
+++ b/arch/arm64/kvm/hyp/fpsimd.S
@@ -31,3 +31,8 @@ SYM_FUNC_START(__sve_save_state)
 	sve_save 0, x1, x2, 3
 	ret
 SYM_FUNC_END(__sve_save_state)
+
+SYM_FUNC_START(__sve_get_vl)
+	_sve_rdvl	0, 1
+	ret
+SYM_FUNC_END(__sve_get_vl)
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 0c4de44534b7..06efcca765cc 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -327,7 +327,7 @@ static inline void __hyp_sve_save_host(void)
 
 	sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR);
 	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);
-	__sve_save_state(sve_state->sve_regs + sve_ffr_offset(kvm_host_sve_max_vl),
+	__sve_save_state(sve_state->sve_regs + sve_ffr_offset(__sve_get_vl()),
 			 &sve_state->fpsr,
 			 true);
 }
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index f43d845f3c4e..bd8f671e848c 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -49,7 +49,7 @@ static void __hyp_sve_restore_host(void)
 	 * supported by the system (or limited at EL3).
 	 */
 	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);
-	__sve_restore_state(sve_state->sve_regs + sve_ffr_offset(kvm_host_sve_max_vl),
+	__sve_restore_state(sve_state->sve_regs + sve_ffr_offset(__sve_get_vl()),
 			    &sve_state->fpsr,
 			    true);
 	write_sysreg_el1(sve_state->zcr_el1, SYS_ZCR);

-- 
2.39.2

Re: [PATCH v4 3/4] KVM: arm64: Fix FFR offset calculation for pKVM host state save and restore

[Marc Zyngier]

On Thu, 04 Jul 2024 18:28:18 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> When saving and restoring the SVE state for the host we configure the
> hardware for the maximum VL it supports, but when calculating offsets in
> memory we use the maximum usable VL for the host. Since these two values
> may not be the same this may result in data corruption.  We can just
> read the current VL from the hardware with an instruction so do that
> instead of a saved value, we need to correct the value and this makes
> the consistency obvious.

Which value are we correcting?

> 
> Fixes: b5b9955617bc ("KVM: arm64: Eagerly restore host fpsimd/sve state in pKVM")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_hyp.h        | 1 +
>  arch/arm64/kvm/hyp/fpsimd.S             | 5 +++++
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c      | 2 +-
>  4 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index b05bceca3385..7510383d78a6 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -113,6 +113,7 @@ void __fpsimd_save_state(struct user_fpsimd_state *fp_regs);
>  void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
>  void __sve_save_state(void *sve_pffr, u32 *fpsr, int save_ffr);
>  void __sve_restore_state(void *sve_pffr, u32 *fpsr, int restore_ffr);
> +int __sve_get_vl(void);

Is there any case where you'd consider returning a signed value? Given
the multiplier of '1', I don't see it likely to happen.

>  
>  u64 __guest_enter(struct kvm_vcpu *vcpu);
>  
> diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
> index e950875e31ce..d272dbf36da8 100644
> --- a/arch/arm64/kvm/hyp/fpsimd.S
> +++ b/arch/arm64/kvm/hyp/fpsimd.S
> @@ -31,3 +31,8 @@ SYM_FUNC_START(__sve_save_state)
>  	sve_save 0, x1, x2, 3
>  	ret
>  SYM_FUNC_END(__sve_save_state)
> +
> +SYM_FUNC_START(__sve_get_vl)
> +	_sve_rdvl	0, 1
> +	ret
> +SYM_FUNC_END(__sve_get_vl)
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 0c4de44534b7..06efcca765cc 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -327,7 +327,7 @@ static inline void __hyp_sve_save_host(void)
>  
>  	sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR);
>  	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);
> -	__sve_save_state(sve_state->sve_regs + sve_ffr_offset(kvm_host_sve_max_vl),
> +	__sve_save_state(sve_state->sve_regs + sve_ffr_offset(__sve_get_vl()),
>  			 &sve_state->fpsr,
>  			 true);
>  }
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index f43d845f3c4e..bd8f671e848c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -49,7 +49,7 @@ static void __hyp_sve_restore_host(void)
>  	 * supported by the system (or limited at EL3).
>  	 */
>  	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);
> -	__sve_restore_state(sve_state->sve_regs + sve_ffr_offset(kvm_host_sve_max_vl),
> +	__sve_restore_state(sve_state->sve_regs + sve_ffr_offset(__sve_get_vl()),
>  			    &sve_state->fpsr,
>  			    true);
>  	write_sysreg_el1(sve_state->zcr_el1, SYS_ZCR);
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4 3/4] KVM: arm64: Fix FFR offset calculation for pKVM host state save and restore

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On Fri, Jul 05, 2024 at 02:27:01PM +0100, Marc Zyngier wrote:
> On Thu, 04 Jul 2024 18:28:18 +0100,
> Mark Brown <broonie@kernel.org> wrote:

> > When saving and restoring the SVE state for the host we configure the
> > hardware for the maximum VL it supports, but when calculating offsets in
> > memory we use the maximum usable VL for the host. Since these two values
> > may not be the same this may result in data corruption.  We can just
> > read the current VL from the hardware with an instruction so do that
> > instead of a saved value, we need to correct the value and this makes
> > the consistency obvious.

> Which value are we correcting?

The vector length used when laying out the storage, especially in the
case where the hardware VL is larger than the largest VL used by Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v4 4/4] KVM: arm64: Avoid underallocating storage for host SVE state

[Mark Brown]

We size the allocation for the host SVE state using the maximum VL
shared by all CPUs in the host.  As observed during review on an
asymmetric system this may be less than the maximum VL supported on some
of the CPUs.  Since the pKVM hypervisor saves and restores the host
state using the maximum VL for the current CPU this may lead to buffer
overflows, fix this by changing pKVM to use the maximum VL for any CPU
to size allocations and limit host configurations.

Fixes: 66d5b53e20a6 ("KVM: arm64: Allocate memory mapped at hyp for host sve state in pKVM")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h  | 2 +-
 arch/arm64/include/asm/kvm_hyp.h   | 2 +-
 arch/arm64/include/asm/kvm_pkvm.h  | 2 +-
 arch/arm64/kvm/hyp/nvhe/hyp-main.c | 4 ++--
 arch/arm64/kvm/hyp/nvhe/pkvm.c     | 2 +-
 arch/arm64/kvm/reset.c             | 6 +++---
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 36b8e97bf49e..a28fae10596f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -76,7 +76,7 @@ static inline enum kvm_mode kvm_get_mode(void) { return KVM_MODE_NONE; };
 DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
 
 extern unsigned int __ro_after_init kvm_sve_max_vl;
-extern unsigned int __ro_after_init kvm_host_sve_max_vl;
+extern unsigned int __ro_after_init kvm_host_sve_max_cpu_vl;
 int __init kvm_arm_init_sve(void);
 
 u32 __attribute_const__ kvm_target_cpu(void);
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 7510383d78a6..47426df69875 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -144,6 +144,6 @@ extern u64 kvm_nvhe_sym(id_aa64smfr0_el1_sys_val);
 
 extern unsigned long kvm_nvhe_sym(__icache_flags);
 extern unsigned int kvm_nvhe_sym(kvm_arm_vmid_bits);
-extern unsigned int kvm_nvhe_sym(kvm_host_sve_max_vl);
+extern unsigned int kvm_nvhe_sym(kvm_host_sve_max_cpu_vl);
 
 #endif /* __ARM64_KVM_HYP_H__ */
diff --git a/arch/arm64/include/asm/kvm_pkvm.h b/arch/arm64/include/asm/kvm_pkvm.h
index cd56acd9a842..6fc0cf42fca3 100644
--- a/arch/arm64/include/asm/kvm_pkvm.h
+++ b/arch/arm64/include/asm/kvm_pkvm.h
@@ -134,7 +134,7 @@ static inline size_t pkvm_host_sve_state_size(void)
 		return 0;
 
 	return size_add(sizeof(struct cpu_sve_state),
-			SVE_SIG_REGS_SIZE(sve_vq_from_vl(kvm_host_sve_max_vl)));
+			SVE_SIG_REGS_SIZE(sve_vq_from_vl(kvm_host_sve_max_cpu_vl)));
 }
 
 #endif	/* __ARM64_KVM_PKVM_H__ */
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index bd8f671e848c..d232775b72c9 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -90,8 +90,8 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
 	hyp_vcpu->vcpu.arch.ctxt	= host_vcpu->arch.ctxt;
 
 	hyp_vcpu->vcpu.arch.sve_state	= kern_hyp_va(host_vcpu->arch.sve_state);
-	/* Limit guest vector length to the maximum supported by the host.  */
-	hyp_vcpu->vcpu.arch.sve_max_vl	= min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl);
+	/* Limit guest vector length to the maximum supported by any CPU.  */
+	hyp_vcpu->vcpu.arch.sve_max_vl	= min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_cpu_vl);
 
 	hyp_vcpu->vcpu.arch.hw_mmu	= host_vcpu->arch.hw_mmu;
 
diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
index 95cf18574251..08e825de09d1 100644
--- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
+++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
@@ -18,7 +18,7 @@ unsigned long __icache_flags;
 /* Used by kvm_get_vttbr(). */
 unsigned int kvm_arm_vmid_bits;
 
-unsigned int kvm_host_sve_max_vl;
+unsigned int kvm_host_sve_max_cpu_vl;
 
 /*
  * Set trap register values based on features in ID_AA64PFR0.
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 3fc8ca164dbe..59cccb477cf3 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -32,7 +32,7 @@
 
 /* Maximum phys_shift supported for any VM on this host */
 static u32 __ro_after_init kvm_ipa_limit;
-unsigned int __ro_after_init kvm_host_sve_max_vl;
+unsigned int __ro_after_init kvm_host_sve_max_cpu_vl;
 
 /*
  * ARMv8 Reset Values
@@ -52,8 +52,8 @@ int __init kvm_arm_init_sve(void)
 {
 	if (system_supports_sve()) {
 		kvm_sve_max_vl = sve_max_virtualisable_vl();
-		kvm_host_sve_max_vl = sve_max_vl();
-		kvm_nvhe_sym(kvm_host_sve_max_vl) = kvm_host_sve_max_vl;
+		kvm_host_sve_max_cpu_vl = sve_max_cpu_vl();
+		kvm_nvhe_sym(kvm_host_sve_max_cpu_vl) = kvm_host_sve_max_cpu_vl;
 
 		/*
 		 * The get_sve_reg()/set_sve_reg() ioctl interface will need

-- 
2.39.2

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Marc Zyngier]

On Thu, 04 Jul 2024 18:28:15 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> As observed during review the pKVM support for saving host SVE state is
> broken if an asymmetric system has VLs larger than the maximum shared
> VL, fix this by discovering then using the maximum VL for allocations
> and using RDVL during the save/restore process.

I really don't see why we need such complexity here.

Fuad did post something[1] that did the trick with a far less invasive
change, and it really feels like we are putting the complexity at the
wrong place.

So what's wrong with that approach? I get that you want to shout about
secondary CPUs, but that's an orthogonal problem.

	M.

[1] https://lore.kernel.org/r/20240606092623.2236172-1-tabba@google.com

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On Fri, Jul 05, 2024 at 02:20:05PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > As observed during review the pKVM support for saving host SVE state is
> > broken if an asymmetric system has VLs larger than the maximum shared
> > VL, fix this by discovering then using the maximum VL for allocations
> > and using RDVL during the save/restore process.

> I really don't see why we need such complexity here.

> Fuad did post something[1] that did the trick with a far less invasive
> change, and it really feels like we are putting the complexity at the
> wrong place.

> So what's wrong with that approach? I get that you want to shout about
> secondary CPUs, but that's an orthogonal problem.

As I've said from a clarity/fragility point of view I'm not happy with
configuring the vector length to one value then immediately doing things
that assume another value, even if everything is actually lined up
in a way that works.  Having uncommented code where you have to go and
check correctness when you see it isn't great, seeing an inconsistency
just raises alarm bells.  It is much clearer to write the code in a way
that makes it obvious that the VL we are using is the one the hardware
is using, for the host save/restore reading the actual VL back seemed
like the most straightforward way to do that.

A similar thing applies with the enumeration code - like I said in reply
to one of Fuad's postings I originally wrote something that's basically
the same as the patch Faud posted but because it is not consistent with
the surrounding code in how it approaches things it was just raising
questions about if the new code was missing something, or if there was
some problem that should be addressed in the existing code.  Rather than
write an extensive changelog and/or comments covering these
considerations it seemed more better to just write the code in a
consistent manner so the questions aren't prompted.  Keeping the
approach consistent is a bit more code right now but makes the result
much easier to reason about.

The late CPUs thing is really just an answer to the initial "why is this
different, what might we have missed?" question rather than a particular
goal itself.  Adding a warning is as much about documenting the handling
of late CPUs as it is about highlighting any unfortunate implementation
choices we run into.

Basically it's maintainability concerns, especially with the enumeration
code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Dave Martin]

Hi all,

On Fri, Jul 05, 2024 at 06:18:50PM +0100, Mark Brown wrote:
> On Fri, Jul 05, 2024 at 02:20:05PM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > As observed during review the pKVM support for saving host SVE state is
> > > broken if an asymmetric system has VLs larger than the maximum shared
> > > VL, fix this by discovering then using the maximum VL for allocations
> > > and using RDVL during the save/restore process.
> 
> > I really don't see why we need such complexity here.

The first patch is orthogonal cleanup, and the rest doesn't really add
complexity IIUC.

> > Fuad did post something[1] that did the trick with a far less invasive
> > change, and it really feels like we are putting the complexity at the
> > wrong place.
> 
> > So what's wrong with that approach? I get that you want to shout about
> > secondary CPUs, but that's an orthogonal problem.
> 
> As I've said from a clarity/fragility point of view I'm not happy with
> configuring the vector length to one value then immediately doing things
> that assume another value, even if everything is actually lined up
> in a way that works.  Having uncommented code where you have to go and
> check correctness when you see it isn't great, seeing an inconsistency
> just raises alarm bells.  It is much clearer to write the code in a way
> that makes it obvious that the VL we are using is the one the hardware
> is using, for the host save/restore reading the actual VL back seemed
> like the most straightforward way to do that.
> 
> A similar thing applies with the enumeration code - like I said in reply
> to one of Fuad's postings I originally wrote something that's basically
> the same as the patch Faud posted but because it is not consistent with
> the surrounding code in how it approaches things it was just raising
> questions about if the new code was missing something, or if there was
> some problem that should be addressed in the existing code.  Rather than
> write an extensive changelog and/or comments covering these
> considerations it seemed more better to just write the code in a
> consistent manner so the questions aren't prompted.  Keeping the
> approach consistent is a bit more code right now but makes the result
> much easier to reason about.
> 
> The late CPUs thing is really just an answer to the initial "why is this
> different, what might we have missed?" question rather than a particular
> goal itself.  Adding a warning is as much about documenting the handling
> of late CPUs as it is about highlighting any unfortunate implementation
> choices we run into.
> 
> Basically it's maintainability concerns, especially with the enumeration
> code.

I tend to agree here.

It's probably best to stick to one convention everywhere about how the
SVE regs are laid out for a given VL.  There's nothing wrong with Fuad's
fixed sve_ffr_offset(), but it's different from the VL-dependent offset
already used elsewhere and so risks causing confusion further down the
line.


One thing confuses me:

The host could never use over-max VLs except in non-preemptible kernel
code, since code doing that would be non-migratable to other physical
CPUs.  This is done to probe SVE only, and the extra bits in the vector
registers are never used at all.

Can't pKVM just hide the non symmetrically supported VLs using ZCR_EL2,
just as regular KVM does for the guest?

(I may be making bad assumptions about pKVM's relationship with the host
kernel.)

Cheers
---Dave

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On Mon, Jul 08, 2024 at 04:30:30PM +0100, Dave Martin wrote:

> One thing confuses me:

> The host could never use over-max VLs except in non-preemptible kernel
> code, since code doing that would be non-migratable to other physical
> CPUs.  This is done to probe SVE only, and the extra bits in the vector
> registers are never used at all.

> Can't pKVM just hide the non symmetrically supported VLs using ZCR_EL2,
> just as regular KVM does for the guest?

> (I may be making bad assumptions about pKVM's relationship with the host
> kernel.)

That'd be another way to do it, constrain the VLs the host can set - I
assume there's something about how pKVM or hVHE does things that makes
problems for that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On Mon, Jul 08, 2024 at 04:30:30PM +0100, Dave Martin wrote:
> On Fri, Jul 05, 2024 at 06:18:50PM +0100, Mark Brown wrote:
> > On Fri, Jul 05, 2024 at 02:20:05PM +0100, Marc Zyngier wrote:
> > > Mark Brown <broonie@kernel.org> wrote:

> > > > As observed during review the pKVM support for saving host SVE state is
> > > > broken if an asymmetric system has VLs larger than the maximum shared
> > > > VL, fix this by discovering then using the maximum VL for allocations
> > > > and using RDVL during the save/restore process.

> > > I really don't see why we need such complexity here.

> The first patch is orthogonal cleanup, and the rest doesn't really add
> complexity IIUC.

...

> > Basically it's maintainability concerns, especially with the enumeration
> > code.

> I tend to agree here.

Did anyone have any further thoughts on this?  It's been roughly a
release cycle since I originally posted this, and there's been no
changes requested since -rc1 (which was itself just a rebase).

> The host could never use over-max VLs except in non-preemptible kernel
> code, since code doing that would be non-migratable to other physical
> CPUs.  This is done to probe SVE only, and the extra bits in the vector
> registers are never used at all.

> Can't pKVM just hide the non symmetrically supported VLs using ZCR_EL2,
> just as regular KVM does for the guest?

> (I may be making bad assumptions about pKVM's relationship with the host
> kernel.)

That's one for the pKVM people.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Fuad Tabba]

Hi,

> > Can't pKVM just hide the non symmetrically supported VLs using ZCR_EL2,
> > just as regular KVM does for the guest?
>
> > (I may be making bad assumptions about pKVM's relationship with the host
> > kernel.)
>
> That's one for the pKVM people.

Yes, but that's not really the issue here, unless I'm missing
something else. The issue is that pKVM needs to store the host's SVE
state too, to be able to restore it. So hiding non-symmetrically
supported VLs for the guests shouldn't be relevant.

Cheers,
/fuad

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

On Fri, Sep 06, 2024 at 04:35:29PM +0100, Fuad Tabba wrote:

> > > Can't pKVM just hide the non symmetrically supported VLs using ZCR_EL2,
> > > just as regular KVM does for the guest?

> > > (I may be making bad assumptions about pKVM's relationship with the host
> > > kernel.)

> > That's one for the pKVM people.

> Yes, but that's not really the issue here, unless I'm missing
> something else. The issue is that pKVM needs to store the host's SVE
> state too, to be able to restore it. So hiding non-symmetrically
> supported VLs for the guests shouldn't be relevant.

If the host kernel is also running at EL1 and it's only the hypervisor
running at EL2 then the hypervisor can control the VLs that the host
kernel is able to access?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Fuad Tabba]

On Fri, 6 Sept 2024 at 17:10, Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 06, 2024 at 04:35:29PM +0100, Fuad Tabba wrote:
>
> > > > Can't pKVM just hide the non symmetrically supported VLs using ZCR_EL2,
> > > > just as regular KVM does for the guest?
>
> > > > (I may be making bad assumptions about pKVM's relationship with the host
> > > > kernel.)
>
> > > That's one for the pKVM people.
>
> > Yes, but that's not really the issue here, unless I'm missing
> > something else. The issue is that pKVM needs to store the host's SVE
> > state too, to be able to restore it. So hiding non-symmetrically
> > supported VLs for the guests shouldn't be relevant.
>
> If the host kernel is also running at EL1 and it's only the hypervisor
> running at EL2 then the hypervisor can control the VLs that the host
> kernel is able to access?

Yes it can. But do we want to impose limits on host VLs when running
pKVM that might not exist without pKVM?

Although AFAIK, such hardware doesn't exist in practice, the reason we
went down this rabbit hole from the beginning was to ensure that we
wouldn't run into problems if it were to happen.

Thanks,
/fuad

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On Fri, Sep 06, 2024 at 05:14:09PM +0100, Fuad Tabba wrote:
> On Fri, 6 Sept 2024 at 17:10, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Sep 06, 2024 at 04:35:29PM +0100, Fuad Tabba wrote:

> > > Yes, but that's not really the issue here, unless I'm missing
> > > something else. The issue is that pKVM needs to store the host's SVE
> > > state too, to be able to restore it. So hiding non-symmetrically
> > > supported VLs for the guests shouldn't be relevant.

> > If the host kernel is also running at EL1 and it's only the hypervisor
> > running at EL2 then the hypervisor can control the VLs that the host
> > kernel is able to access?

> Yes it can. But do we want to impose limits on host VLs when running
> pKVM that might not exist without pKVM?

I mean, at the minute the host kernel will just not configure any
non-shared VLs so pKVM isn't making a difference anyway.  Even when we
add kernel mode SVE usage kernel mode FP is preemptable and schedulable
so we'd not likely want to use non-shared VLs there either.  If someone
ever felt moved to add support for using any non-shared VLs the most
likely usage would be for userspace and we'd constrain any tasks using
SVE to the cores that support their VLs similar to how we handle CPUs
with no 32 bit support.  Hopefully the scheduler would cope well with
that.

> Although AFAIK, such hardware doesn't exist in practice, the reason we
> went down this rabbit hole from the beginning was to ensure that we
> wouldn't run into problems if it were to happen.

Yes, it's not an issue with any presently known hardware - the issue is
leaving nasty surprises should someone build it rather than anything
immediately practical.  Ideally they won't.

My general feeling is that it would have been perfectly fine for pKVM to
enforce what the host kernel wants to do anyway, if we ever do add
support for using asymmetric VLs and care about doing so on a system
running pKVM then dealing with pKVM imposed limits at that time seems
more than reasonable.  It probably wouldn't be the largest part of the
work.  Equally we now have the code so we may as well use it?  It's not
imposing huge overheads.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Fuad Tabba]

Hi,

On Fri, 6 Sept 2024 at 19:03, Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 06, 2024 at 05:14:09PM +0100, Fuad Tabba wrote:
> > On Fri, 6 Sept 2024 at 17:10, Mark Brown <broonie@kernel.org> wrote:
> > > On Fri, Sep 06, 2024 at 04:35:29PM +0100, Fuad Tabba wrote:
>
> > > > Yes, but that's not really the issue here, unless I'm missing
> > > > something else. The issue is that pKVM needs to store the host's SVE
> > > > state too, to be able to restore it. So hiding non-symmetrically
> > > > supported VLs for the guests shouldn't be relevant.
>
> > > If the host kernel is also running at EL1 and it's only the hypervisor
> > > running at EL2 then the hypervisor can control the VLs that the host
> > > kernel is able to access?
>
> > Yes it can. But do we want to impose limits on host VLs when running
> > pKVM that might not exist without pKVM?
>
> I mean, at the minute the host kernel will just not configure any
> non-shared VLs so pKVM isn't making a difference anyway.  Even when we
> add kernel mode SVE usage kernel mode FP is preemptable and schedulable
> so we'd not likely want to use non-shared VLs there either.  If someone
> ever felt moved to add support for using any non-shared VLs the most
> likely usage would be for userspace and we'd constrain any tasks using
> SVE to the cores that support their VLs similar to how we handle CPUs
> with no 32 bit support.  Hopefully the scheduler would cope well with
> that.
>
> > Although AFAIK, such hardware doesn't exist in practice, the reason we
> > went down this rabbit hole from the beginning was to ensure that we
> > wouldn't run into problems if it were to happen.
>
> Yes, it's not an issue with any presently known hardware - the issue is
> leaving nasty surprises should someone build it rather than anything
> immediately practical.  Ideally they won't.
>
> My general feeling is that it would have been perfectly fine for pKVM to
> enforce what the host kernel wants to do anyway, if we ever do add
> support for using asymmetric VLs and care about doing so on a system
> running pKVM then dealing with pKVM imposed limits at that time seems
> more than reasonable.  It probably wouldn't be the largest part of the
> work.  Equally we now have the code so we may as well use it?  It's not
> imposing huge overheads.

From pKVM, this would work and other than the potential for future
support for using asymmetric VLs, I don't really see a problem. Much
of the complexity was an attempt not to make pKVM more restrictive
than other modes.

Cheers,
/fuad

Re: [PATCH v4 0/4] KVM: arm64: Fix underallocation of storage for SVE state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

On Tue, Sep 10, 2024 at 01:49:37PM +0100, Fuad Tabba wrote:
> On Fri, 6 Sept 2024 at 19:03, Mark Brown <broonie@kernel.org> wrote:

> > My general feeling is that it would have been perfectly fine for pKVM to
> > enforce what the host kernel wants to do anyway, if we ever do add
> > support for using asymmetric VLs and care about doing so on a system
> > running pKVM then dealing with pKVM imposed limits at that time seems
> > more than reasonable.  It probably wouldn't be the largest part of the
> > work.  Equally we now have the code so we may as well use it?  It's not
> > imposing huge overheads.

> From pKVM, this would work and other than the potential for future
> support for using asymmetric VLs, I don't really see a problem. Much
> of the complexity was an attempt not to make pKVM more restrictive
> than other modes.

Right, so it's just a question of if we want to use the code that
doesn't impose the limit given that we have it already.  I'm throwing a
patch that limits the host VL into CI, should be out today.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]