| Message ID | 20240528125914.277057-6-tabba@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Fix handling of host fpsimd/sve state in protected mode | expand |
On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote: > +static inline void __hyp_sve_save_host(void) > +{ > + struct cpu_sve_state *sve_state = *host_data_ptr(sve_state); > + > + sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR); > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); As well as the sync issue Oliver mentioned on the removal of _cond_update() just doing these updates as a blind write creates a surprise if we ever get more control bits in ZCR_EL2. > +static void __hyp_sve_restore_host(void) > +{ > + struct cpu_sve_state *sve_state = *host_data_ptr(sve_state); > + > + /* > + * On saving/restoring host sve state, always use the maximum VL for > + * the host. The layout of the data when saving the sve state depends > + * on the VL, so use a consistent (i.e., the maximum) host VL. > + * > + * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length > + * supported by the system (or limited at EL3). > + */ > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); Setting ZCR_ELx_LEN_MASK sets the VL to the maximum supported/configured for the current PE, not the system. This will hopefully be the same since we really hope implementors continue to build symmetric systems but there is handling for that case in the kernel just in case. Given that we record the host's maximum VL should we use it? > +static void fpsimd_sve_flush(void) > +{ > + *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED; > +} > + That's not what I'd have expected something called fpsimd_sve_flush() to do, I'd have expected it to save the current state to memory and then mark it as free (that's what fpsimd_flush_cpu_state() does in the host kernel). Perhaps just inline this into the one user? > +static void fpsimd_sve_sync(struct kvm_vcpu *vcpu) > +{ > + if (!guest_owns_fp_regs()) > + return; > + > + cpacr_clear_set(0, CPACR_ELx_FPEN|CPACR_ELx_ZEN); > + isb(); Spaces around |. > static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu) > { > struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > > + fpsimd_sve_flush(); > + > hyp_vcpu->vcpu.arch.ctxt = host_vcpu->arch.ctxt; > > hyp_vcpu->vcpu.arch.sve_state = kern_hyp_va(host_vcpu->arch.sve_state); > - hyp_vcpu->vcpu.arch.sve_max_vl = host_vcpu->arch.sve_max_vl; > + hyp_vcpu->vcpu.arch.sve_max_vl = min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl); This needs a comment I think.
Hi Mark, On Fri, May 31, 2024 at 3:09 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote: > > > +static inline void __hyp_sve_save_host(void) > > +{ > > + struct cpu_sve_state *sve_state = *host_data_ptr(sve_state); > > + > > + sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR); > > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); > > As well as the sync issue Oliver mentioned on the removal of > _cond_update() just doing these updates as a blind write creates a > surprise if we ever get more control bits in ZCR_EL2. I'm not sure it does. The other bits are RES0, and this is always setting the length. So even if new control bits are added, this shouldn't matter. Also, one of the concerns in terms of performance is now with nested-virt support being added, and the overhead of doing the conditional update when we know that it's unlikely that anyone is implementing vectors as big as the max. > > > +static void __hyp_sve_restore_host(void) > > +{ > > + struct cpu_sve_state *sve_state = *host_data_ptr(sve_state); > > + > > + /* > > + * On saving/restoring host sve state, always use the maximum VL for > > + * the host. The layout of the data when saving the sve state depends > > + * on the VL, so use a consistent (i.e., the maximum) host VL. > > + * > > + * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length > > + * supported by the system (or limited at EL3). > > + */ > > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); > > Setting ZCR_ELx_LEN_MASK sets the VL to the maximum supported/configured > for the current PE, not the system. This will hopefully be the same > since we really hope implementors continue to build symmetric systems > but there is handling for that case in the kernel just in case. Given > that we record the host's maximum VL should we use it? You're right, but even if the current PE had a different vector length, ZCR_ELx_LEN_MASK is the default value for ZCR_EL2 when the host is running (this is the existing behavior before this patch series). It is also the value this patch series uses when saving the host SVE state. So since we are consistent I think this is correct. > > +static void fpsimd_sve_flush(void) > > +{ > > + *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED; > > +} > > + > > That's not what I'd have expected something called fpsimd_sve_flush() to > do, I'd have expected it to save the current state to memory and then > mark it as free (that's what fpsimd_flush_cpu_state() does in the host > kernel). Perhaps just inline this into the one user? > > > +static void fpsimd_sve_sync(struct kvm_vcpu *vcpu) > > +{ > > + if (!guest_owns_fp_regs()) > > + return; > > + > > + cpacr_clear_set(0, CPACR_ELx_FPEN|CPACR_ELx_ZEN); > > + isb(); > > Spaces around |. Will do. > > static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu) > > { > > struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > > > > + fpsimd_sve_flush(); > > + > > hyp_vcpu->vcpu.arch.ctxt = host_vcpu->arch.ctxt; > > > > hyp_vcpu->vcpu.arch.sve_state = kern_hyp_va(host_vcpu->arch.sve_state); > > - hyp_vcpu->vcpu.arch.sve_max_vl = host_vcpu->arch.sve_max_vl; > > + hyp_vcpu->vcpu.arch.sve_max_vl = min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl); > > This needs a comment I think. Will do. Thanks! /fuad
On Mon, Jun 03, 2024 at 09:37:16AM +0100, Fuad Tabba wrote: > On Fri, May 31, 2024 at 3:09 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote: > > > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); > > As well as the sync issue Oliver mentioned on the removal of > > _cond_update() just doing these updates as a blind write creates a > > surprise if we ever get more control bits in ZCR_EL2. > I'm not sure it does. The other bits are RES0, and this is always > setting the length. So even if new control bits are added, this > shouldn't matter. The surprise would be that if new control bits were added this would result in clearing them on restore. > Also, one of the concerns in terms of performance is now with > nested-virt support being added, and the overhead of doing the > conditional update when we know that it's unlikely that anyone is > implementing vectors as big as the max. I guess there's the option of doing a restore of a value fixed during initialisation instead? > > > + /* > > > + * On saving/restoring host sve state, always use the maximum VL for > > > + * the host. The layout of the data when saving the sve state depends > > > + * on the VL, so use a consistent (i.e., the maximum) host VL. > > > + * > > > + * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length > > > + * supported by the system (or limited at EL3). > > > + */ > > > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); > > Setting ZCR_ELx_LEN_MASK sets the VL to the maximum supported/configured > > for the current PE, not the system. This will hopefully be the same > > since we really hope implementors continue to build symmetric systems > > but there is handling for that case in the kernel just in case. Given > > that we record the host's maximum VL should we use it? > You're right, but even if the current PE had a different vector > length, ZCR_ELx_LEN_MASK is the default value for ZCR_EL2 when the > host is running (this is the existing behavior before this patch > series). It is also the value this patch series uses when saving the > host SVE state. So since we are consistent I think this is correct. The reason we just set all bits in ZCR_EL2.LEN is that we don't currently use SVE at EL2 so we're just passing everything through to EL1 and letting it worry about things. As we start adding more SVE code at EL2 we need to care more and I think we should start explicitly programming what we think we're using to use to avoid surprises. For example in this series we allocate the buffer used to store the host SVE state based on the probed maximum usable VL for the system but here we use whatever the PE has as the maximum VL. This means that in the (hopefully unlikely) case where the probed value is lower than the PE value we'll overflow the buffer.
On Mon, 03 Jun 2024 14:27:07 +0100, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 03, 2024 at 09:37:16AM +0100, Fuad Tabba wrote: > > On Fri, May 31, 2024 at 3:09 PM Mark Brown <broonie@kernel.org> wrote: > > > On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote: > > > > > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); > > > > As well as the sync issue Oliver mentioned on the removal of > > > _cond_update() just doing these updates as a blind write creates a > > > surprise if we ever get more control bits in ZCR_EL2. > > > I'm not sure it does. The other bits are RES0, and this is always > > setting the length. So even if new control bits are added, this > > shouldn't matter. > > The surprise would be that if new control bits were added this would > result in clearing them on restore. And as far as I can tell, there is no in-flight architectural change that touches this class of registers. And should this eventually happens, we will have to audit *all* the spots when ZCR_ELx is touched and turn them all into RMW accesses. Having an extra spot here isn't going to change this in a material way. > > > Also, one of the concerns in terms of performance is now with > > nested-virt support being added, and the overhead of doing the > > conditional update when we know that it's unlikely that anyone is > > implementing vectors as big as the max. > > I guess there's the option of doing a restore of a value fixed during > initialisation instead? And what do we gain from that? > > > > > + /* > > > > + * On saving/restoring host sve state, always use the maximum VL for > > > > + * the host. The layout of the data when saving the sve state depends > > > > + * on the VL, so use a consistent (i.e., the maximum) host VL. > > > > + * > > > > + * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length > > > > + * supported by the system (or limited at EL3). > > > > + */ > > > > + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); > > > > Setting ZCR_ELx_LEN_MASK sets the VL to the maximum supported/configured > > > for the current PE, not the system. This will hopefully be the same > > > since we really hope implementors continue to build symmetric systems > > > but there is handling for that case in the kernel just in case. Given > > > that we record the host's maximum VL should we use it? > > > You're right, but even if the current PE had a different vector > > length, ZCR_ELx_LEN_MASK is the default value for ZCR_EL2 when the > > host is running (this is the existing behavior before this patch > > series). It is also the value this patch series uses when saving the > > host SVE state. So since we are consistent I think this is correct. > > The reason we just set all bits in ZCR_EL2.LEN is that we don't > currently use SVE at EL2 so we're just passing everything through to EL1 > and letting it worry about things. As we start adding more SVE code at > EL2 we need to care more and I think we should start explicitly > programming what we think we're using to use to avoid surprises. For > example in this series we allocate the buffer used to store the host SVE > state based on the probed maximum usable VL for the system but here we > use whatever the PE has as the maximum VL. This means that in the > (hopefully unlikely) case where the probed value is lower than the PE > value we'll overflow the buffer. In that case, we need the *real* maximum across all CPUs, not the maximum usable. M.
On Mon, Jun 03, 2024 at 02:48:39PM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 03, 2024 at 09:37:16AM +0100, Fuad Tabba wrote: > > > Also, one of the concerns in terms of performance is now with > > > nested-virt support being added, and the overhead of doing the > > > conditional update when we know that it's unlikely that anyone is > > > implementing vectors as big as the max. > > I guess there's the option of doing a restore of a value fixed during > > initialisation instead? > And what do we gain from that? Reducing the number of places that need updating. > > programming what we think we're using to use to avoid surprises. For > > example in this series we allocate the buffer used to store the host SVE > > state based on the probed maximum usable VL for the system but here we > > use whatever the PE has as the maximum VL. This means that in the > > (hopefully unlikely) case where the probed value is lower than the PE > > value we'll overflow the buffer. > In that case, we need the *real* maximum across all CPUs, not the > maximum usable. If we stick with setting all the bits then yes, we'd need that. It'd also be more robust than trusting that the host won't set a higher length.
On Mon, 03 Jun 2024 15:15:37 +0100, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 03, 2024 at 02:48:39PM +0100, Marc Zyngier wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > On Mon, Jun 03, 2024 at 09:37:16AM +0100, Fuad Tabba wrote: > > > > > Also, one of the concerns in terms of performance is now with > > > > nested-virt support being added, and the overhead of doing the > > > > conditional update when we know that it's unlikely that anyone is > > > > implementing vectors as big as the max. > > > > I guess there's the option of doing a restore of a value fixed during > > > initialisation instead? > > > And what do we gain from that? > > Reducing the number of places that need updating. That'd be assuming that the host side value never requires any change and will be OK with a fixed value. Given that this highly hypothetical change would be likely to be a hierarchical control much like ZCR_ELx is today, the fixed value is likely to change on a regular basis. In any case, this is something we can (re)evaluate when we get to it. If ever. M.
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index d3a3f1cee668..89c52b59d2a9 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -320,6 +320,17 @@ static inline void __hyp_sve_restore_guest(struct kvm_vcpu *vcpu) write_sysreg_el1(__vcpu_sys_reg(vcpu, ZCR_EL1), SYS_ZCR); } +static inline void __hyp_sve_save_host(void) +{ + struct cpu_sve_state *sve_state = *host_data_ptr(sve_state); + + sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR); + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); + isb(); + __sve_save_state(sve_state->sve_regs + sve_ffr_offset(kvm_host_sve_max_vl), + &sve_state->fpsr); +} + static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu); /* @@ -354,7 +365,7 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) /* Valid trap. Switch the context: */ /* First disable enough traps to allow us to update the registers */ - if (sve_guest) + if (sve_guest || (is_protected_kvm_enabled() && system_supports_sve())) cpacr_clear_set(0, CPACR_ELx_FPEN|CPACR_ELx_ZEN); else cpacr_clear_set(0, CPACR_ELx_FPEN); diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c index f71394d0e32a..1088b0bd3cc5 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c @@ -23,20 +23,80 @@ DEFINE_PER_CPU(struct kvm_nvhe_init_params, kvm_init_params); void __kvm_hyp_host_forward_smc(struct kvm_cpu_context *host_ctxt); +static void __hyp_sve_save_guest(struct kvm_vcpu *vcpu) +{ + __vcpu_sys_reg(vcpu, ZCR_EL1) = read_sysreg_el1(SYS_ZCR); + /* + * On saving/restoring guest sve state, always use the maximum VL for + * the guest. The layout of the data when saving the sve state depends + * on the VL, so use a consistent (i.e., the maximum) guest VL. + */ + sve_cond_update_zcr_vq(vcpu_sve_max_vq(vcpu) - 1, SYS_ZCR_EL2); + isb(); + __sve_save_state(vcpu_sve_pffr(vcpu), &vcpu->arch.ctxt.fp_regs.fpsr); + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); +} + +static void __hyp_sve_restore_host(void) +{ + struct cpu_sve_state *sve_state = *host_data_ptr(sve_state); + + /* + * On saving/restoring host sve state, always use the maximum VL for + * the host. The layout of the data when saving the sve state depends + * on the VL, so use a consistent (i.e., the maximum) host VL. + * + * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length + * supported by the system (or limited at EL3). + */ + write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2); + isb(); + __sve_restore_state(sve_state->sve_regs + sve_ffr_offset(kvm_host_sve_max_vl), + &sve_state->fpsr); + write_sysreg_el1(sve_state->zcr_el1, SYS_ZCR); +} + +static void fpsimd_sve_flush(void) +{ + *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED; +} + +static void fpsimd_sve_sync(struct kvm_vcpu *vcpu) +{ + if (!guest_owns_fp_regs()) + return; + + cpacr_clear_set(0, CPACR_ELx_FPEN|CPACR_ELx_ZEN); + isb(); + + if (vcpu_has_sve(vcpu)) + __hyp_sve_save_guest(vcpu); + else + __fpsimd_save_state(&vcpu->arch.ctxt.fp_regs); + + if (system_supports_sve()) + __hyp_sve_restore_host(); + else + __fpsimd_restore_state(*host_data_ptr(fpsimd_state)); + + *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED; +} + static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu) { struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; + fpsimd_sve_flush(); + hyp_vcpu->vcpu.arch.ctxt = host_vcpu->arch.ctxt; hyp_vcpu->vcpu.arch.sve_state = kern_hyp_va(host_vcpu->arch.sve_state); - hyp_vcpu->vcpu.arch.sve_max_vl = host_vcpu->arch.sve_max_vl; + hyp_vcpu->vcpu.arch.sve_max_vl = min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl); hyp_vcpu->vcpu.arch.hw_mmu = host_vcpu->arch.hw_mmu; hyp_vcpu->vcpu.arch.hcr_el2 = host_vcpu->arch.hcr_el2; hyp_vcpu->vcpu.arch.mdcr_el2 = host_vcpu->arch.mdcr_el2; - hyp_vcpu->vcpu.arch.cptr_el2 = host_vcpu->arch.cptr_el2; hyp_vcpu->vcpu.arch.iflags = host_vcpu->arch.iflags; @@ -54,10 +114,11 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu) struct vgic_v3_cpu_if *host_cpu_if = &host_vcpu->arch.vgic_cpu.vgic_v3; unsigned int i; + fpsimd_sve_sync(&hyp_vcpu->vcpu); + host_vcpu->arch.ctxt = hyp_vcpu->vcpu.arch.ctxt; host_vcpu->arch.hcr_el2 = hyp_vcpu->vcpu.arch.hcr_el2; - host_vcpu->arch.cptr_el2 = hyp_vcpu->vcpu.arch.cptr_el2; host_vcpu->arch.fault = hyp_vcpu->vcpu.arch.fault; diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c index 25e9a94f6d76..feb27b4ce459 100644 --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c @@ -588,6 +588,8 @@ int __pkvm_init_vcpu(pkvm_handle_t handle, struct kvm_vcpu *host_vcpu, if (ret) unmap_donated_memory(hyp_vcpu, sizeof(*hyp_vcpu)); + hyp_vcpu->vcpu.arch.cptr_el2 = kvm_get_reset_cptr_el2(&hyp_vcpu->vcpu); + return ret; } diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 019f863922fa..bef74de7065b 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -184,7 +184,21 @@ static bool kvm_handle_pvm_sys64(struct kvm_vcpu *vcpu, u64 *exit_code) static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu) { - __fpsimd_save_state(*host_data_ptr(fpsimd_state)); + /* + * Non-protected kvm relies on the host restoring its sve state. + * Protected kvm restores the host's sve state as not to reveal that + * fpsimd was used by a guest nor leak upper sve bits. + */ + if (unlikely(is_protected_kvm_enabled() && system_supports_sve())) { + __hyp_sve_save_host(); + + /* Re-enable SVE traps if not supported for the guest vcpu. */ + if (!vcpu_has_sve(vcpu)) + cpacr_clear_set(CPACR_ELx_ZEN, 0); + + } else { + __fpsimd_save_state(*host_data_ptr(fpsimd_state)); + } } static const exit_handler_fn hyp_exit_handlers[] = {
When running in protected mode we don't want to leak protected guest state to the host, including whether a guest has used fpsimd/sve. Therefore, eagerly restore the host state on guest exit when running in protected mode, which happens only if the guest has used fpsimd/sve. Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/kvm/hyp/include/hyp/switch.h | 13 ++++- arch/arm64/kvm/hyp/nvhe/hyp-main.c | 67 +++++++++++++++++++++++-- arch/arm64/kvm/hyp/nvhe/pkvm.c | 2 + arch/arm64/kvm/hyp/nvhe/switch.c | 16 +++++- 4 files changed, 93 insertions(+), 5 deletions(-)