| Message ID | 20240605-kvm-arm64-fix-pkvm-sve-vl-v1-2-680d6b43b4c1@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Fix underallocation of storage for SVE state | expand |
Hi Mark, On Wed, Jun 5, 2024 at 12:50 PM Mark Brown <broonie@kernel.org> wrote: > > 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. > > 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(-) Actually, I was working on fixing it and was about to send this, which I think might be a bit simpler than what you have. Let me know what you think and I'll send it as a proper patch if you agree: diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index bc69ac368d73..8bde13b7faa3 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -184,6 +184,9 @@ struct vl_info { int max_vl; int max_virtualisable_vl; + /* The maximum vl encountered for any cpu in the system. */ + int max_system_vl; + /* * Set of available vector lengths, * where length vq encoded as bit __vq_to_bit(vq): diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 82e8a6017382..e0af4c3c9a40 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -1006,7 +1006,7 @@ int sme_get_current_vl(void) static void vec_probe_vqs(struct vl_info *info, DECLARE_BITMAP(map, SVE_VQ_MAX)) { - unsigned int vq, vl; + unsigned int vq, vl, max_vl = 0; bitmap_zero(map, SVE_VQ_MAX); @@ -1031,7 +1031,12 @@ static void vec_probe_vqs(struct vl_info *info, vq = sve_vq_from_vl(vl); /* skip intervening lengths */ set_bit(__vq_to_bit(vq), map); + + if (!max_vl) + max_vl = vl; } + + info->max_system_vl = max((int) max_vl, info->max_system_vl); } /* diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c index 3fc8ca164dbe..9f751cce8081 100644 --- a/arch/arm64/kvm/reset.c +++ b/arch/arm64/kvm/reset.c @@ -52,7 +52,7 @@ 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_host_sve_max_vl = vl_info[ARM64_VEC_SVE].max_system_vl; kvm_nvhe_sym(kvm_host_sve_max_vl) = kvm_host_sve_max_vl; /*
On Wed, 05 Jun 2024 12:41:28 +0100, Mark Brown <broonie@kernel.org> wrote: > > @@ -1083,6 +1095,18 @@ int vec_verify_vq_map(enum vec_type type) > if (!IS_ENABLED(CONFIG_KVM) || !is_hyp_mode_available()) > return 0; > > + /* > + * pKVM allocates and uses storage for host state based on the > + * largest per-PE VL, reject new PEs with a larger maximum. > + */ > + if (is_protected_kvm_enabled()) { > + if (max_vl > info->max_cpu_vl) { > + pr_warn("%s: cpu%d: would increase maximum VL\n", > + info->name, smp_processor_id()); > + return -EINVAL; > + } > + } > + Once protected mode is enabled, no new CPU can be booted (see psci_relay.c::psci_cpu_on()). M.
On Wed, Jun 05, 2024 at 01:13:20PM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > + /* > > + * pKVM allocates and uses storage for host state based on the > > + * largest per-PE VL, reject new PEs with a larger maximum. > > + */ > > + if (is_protected_kvm_enabled()) { > > + if (max_vl > info->max_cpu_vl) { > > + pr_warn("%s: cpu%d: would increase maximum VL\n", > > + info->name, smp_processor_id()); > > + return -EINVAL; > > + } > > + } > Once protected mode is enabled, no new CPU can be booted (see > psci_relay.c::psci_cpu_on()). Ah, that's a bit easier. Might still be worth keeping the check just in case that changes or we acquire some further use of this value but it's not currently needed and the comment could be updated.
On Wed, Jun 05, 2024 at 01:03:16PM +0100, Fuad Tabba wrote: > Actually, I was working on fixing it and was about to send this, which > I think might be a bit simpler than what you have. Let me know what > you think and I'll send it as a proper patch if you agree: > + > + if (!max_vl) > + max_vl = vl; > } > + > + info->max_system_vl = max((int) max_vl, info->max_system_vl); Yeah, I originally had written something like this (just doing the max in the original assignment rather than staging in a local variable) but it didn't feel great and like it was more vulnerable to getting missed if we do more parallel bringup so I switched to keeping the split between enumeration and integration. I do also prefer the _cpu_ naming since _system_ could be read as the maximum that we're actually using in the system, as with any naming thing it's all a bit bikesheddy. > if (system_supports_sve()) { > kvm_sve_max_vl = sve_max_virtualisable_vl(); > - kvm_host_sve_max_vl = sve_max_vl(); > + kvm_host_sve_max_vl = vl_info[ARM64_VEC_SVE].max_system_vl; We should keep the accessor functions, it's more consistent and supports refactoring.
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..27f3593547f1 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,9 +1080,10 @@ 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); + max_vl = __bit_to_vl(find_first_bit(tmp_map, SVE_VQ_MAX)); bitmap_complement(tmp_map, tmp_map, SVE_VQ_MAX); if (bitmap_intersects(tmp_map, info->vq_map, SVE_VQ_MAX)) { @@ -1083,6 +1095,18 @@ int vec_verify_vq_map(enum vec_type type) if (!IS_ENABLED(CONFIG_KVM) || !is_hyp_mode_available()) return 0; + /* + * pKVM allocates and uses storage for host state based on the + * largest per-PE VL, reject new PEs with a larger maximum. + */ + if (is_protected_kvm_enabled()) { + if (max_vl > info->max_cpu_vl) { + pr_warn("%s: cpu%d: would increase maximum VL\n", + info->name, smp_processor_id()); + return -EINVAL; + } + } + /* * For KVM, it is necessary to ensure that this CPU doesn't * support any vector length that guests may have probed as
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. 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(-)