| Message ID | 20240605-kvm-arm64-fix-pkvm-sve-vl-v1-3-680d6b43b4c1@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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 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. > > 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); > } If my understanding of the spec is correct (which more often than not it isn't), I don't think we have an issue as long as we use the same value in the offset on saving/restoring, and that that value represents the maximum possible value. On the other hand, if my understanding is wrong, then we might need to also fix __efi_fpsimd_begin()/__efi_fpsimd_end() in arch/arm64/kernel/fpsimd.c, as well as vcpu_sve_pffr() in arch/arm64/include/asm/kvm_host.h What do you think? /fuad > 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 >
On Wed, Jun 05, 2024 at 01:12:34PM +0100, Fuad Tabba wrote: > If my understanding of the spec is correct (which more often than not > it isn't), I don't think we have an issue as long as we use the same > value in the offset on saving/restoring, and that that value > represents the maximum possible value. Yes, we could also correct the issue by switching to use the system enumerated maximum but that still leaves us setting one value and then immediately assuming a different value. Reading the actual VL from the hardware makes the code more obviously self consistent.
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);
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. 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(-)