| Message ID | 20240318233352.2728327-10-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: allow customizing VMSA features | expand |
On Mon, Mar 18, 2024 at 07:33:46PM -0400, Paolo Bonzini wrote: > SEV-ES allows passing custom contents for x87, SSE and AVX state into the VMSA. > Allow userspace to do that with the usual KVM_SET_XSAVE API and only mark > FPU contents as confidential after it has been copied and encrypted into > the VMSA. > > Since the XSAVE state for AVX is the first, it does not need the > compacted-state handling of get_xsave_addr(). However, there are other > parts of XSAVE state in the VMSA that currently are not handled, and > the validation logic of get_xsave_addr() is pointless to duplicate > in KVM, so move get_xsave_addr() to public FPU API; it is really just > a facility to operate on XSAVE state and does not expose any internal > details of arch/x86/kernel/fpu. > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/fpu/api.h | 3 +++ > arch/x86/kernel/fpu/xstate.h | 2 -- > arch/x86/kvm/svm/sev.c | 36 ++++++++++++++++++++++++++++++++++ > arch/x86/kvm/svm/svm.c | 8 -------- > 4 files changed, 39 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h > index a2be3aefff9f..f86ad3335529 100644 > --- a/arch/x86/include/asm/fpu/api.h > +++ b/arch/x86/include/asm/fpu/api.h > @@ -143,6 +143,9 @@ extern void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfe > > extern u64 xstate_get_guest_group_perm(void); > > +extern void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr); I get a linker error if I don't add an EXPORT_SYMBOL_GPL(get_xsave_addr) -Mike
On Tue, Mar 19, 2024 at 2:42 PM Michael Roth <michael.roth@amd.com> wrote: > > Since the XSAVE state for AVX is the first, it does not need the > > compacted-state handling of get_xsave_addr(). However, there are other > > parts of XSAVE state in the VMSA that currently are not handled, and > > the validation logic of get_xsave_addr() is pointless to duplicate > > in KVM, so move get_xsave_addr() to public FPU API; it is really just > > a facility to operate on XSAVE state and does not expose any internal > > details of arch/x86/kernel/fpu. > > > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > arch/x86/include/asm/fpu/api.h | 3 +++ > > arch/x86/kernel/fpu/xstate.h | 2 -- > > arch/x86/kvm/svm/sev.c | 36 ++++++++++++++++++++++++++++++++++ > > arch/x86/kvm/svm/svm.c | 8 -------- > > 4 files changed, 39 insertions(+), 10 deletions(-) > > > > diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h > > index a2be3aefff9f..f86ad3335529 100644 > > --- a/arch/x86/include/asm/fpu/api.h > > +++ b/arch/x86/include/asm/fpu/api.h > > @@ -143,6 +143,9 @@ extern void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfe > > > > extern u64 xstate_get_guest_group_perm(void); > > > > +extern void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr); > > I get a linker error if I don't add an EXPORT_SYMBOL_GPL(get_xsave_addr) Indeed, and also the format for the 10-byte x87 registers is... unusual. I sent a follow up at the end of this thread that includes a fixup for this patch and the FPU/XSAVE test for SEV-ES. Paolo
On 3/18/24 16:33, Paolo Bonzini wrote: > Since the XSAVE state for AVX is the first, it does not need the > compacted-state handling of get_xsave_addr(). However, there are other > parts of XSAVE state in the VMSA that currently are not handled, and > the validation logic of get_xsave_addr() is pointless to duplicate > in KVM, so move get_xsave_addr() to public FPU API; it is really just > a facility to operate on XSAVE state and does not expose any internal > details of arch/x86/kernel/fpu. We don't want to grow _too_ many users of get_xsave_addr() since it's hard to use it right. But this seems to be a legitimate user. Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
On Mon, Mar 18, 2024 at 07:33:46PM -0400, Paolo Bonzini wrote: > SEV-ES allows passing custom contents for x87, SSE and AVX state into the VMSA. > Allow userspace to do that with the usual KVM_SET_XSAVE API and only mark > FPU contents as confidential after it has been copied and encrypted into > the VMSA. > > Since the XSAVE state for AVX is the first, it does not need the > compacted-state handling of get_xsave_addr(). However, there are other > parts of XSAVE state in the VMSA that currently are not handled, and > the validation logic of get_xsave_addr() is pointless to duplicate > in KVM, so move get_xsave_addr() to public FPU API; it is really just > a facility to operate on XSAVE state and does not expose any internal > details of arch/x86/kernel/fpu. > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/include/asm/fpu/api.h | 3 +++ > arch/x86/kernel/fpu/xstate.h | 2 -- > arch/x86/kvm/svm/sev.c | 36 ++++++++++++++++++++++++++++++++++ > arch/x86/kvm/svm/svm.c | 8 -------- > 4 files changed, 39 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index a8300646a280..800e836a69fb 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > + xsave = &vcpu->arch.guest_fpu.fpstate->regs.xsave; > + save->x87_dp = xsave->i387.rdp; > + save->mxcsr = xsave->i387.mxcsr; > + save->x87_ftw = xsave->i387.twd; > + save->x87_fsw = xsave->i387.swd; > + save->x87_fcw = xsave->i387.cwd; > + save->x87_fop = xsave->i387.fop; > + save->x87_ds = 0; > + save->x87_cs = 0; > + save->x87_rip = xsave->i387.rip; > + > + for (i = 0; i < 8; i++) { > + d = save->fpreg_x87 + i * 10; > + s = ((u8 *)xsave->i387.st_space) + i * 16; > + memcpy(d, s, 10); > + } > + memcpy(save->fpreg_xmm, xsave->i387.xmm_space, 256); > + > + s = get_xsave_addr(xsave, XFEATURE_YMM); > + if (s) > + memcpy(save->fpreg_ymm, s, 256); > + else > + memset(save->fpreg_ymm, 0, 256); > + > pr_debug("Virtual Machine Save Area (VMSA):\n"); > print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); > > @@ -657,6 +686,13 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, > if (ret) > return ret; > > + /* > + * SEV-ES guests maintain an encrypted version of their FPU > + * state which is restored and saved on VMRUN and VMEXIT. > + * Mark vcpu->arch.guest_fpu->fpstate as scratch so it won't > + * do xsave/xrstor on it. > + */ > + fpstate_set_confidential(&vcpu->arch.guest_fpu); > vcpu->arch.guest_state_protected = true; > return 0; > } > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index c22e87ebf0de..03108055a7b0 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -1433,14 +1433,6 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu) > vmsa_page = snp_safe_alloc_page(vcpu); > if (!vmsa_page) > goto error_free_vmcb_page; > - > - /* > - * SEV-ES guests maintain an encrypted version of their FPU > - * state which is restored and saved on VMRUN and VMEXIT. > - * Mark vcpu->arch.guest_fpu->fpstate as scratch so it won't > - * do xsave/xrstor on it. > - */ > - fpstate_set_confidential(&vcpu->arch.guest_fpu); There may have be userspaces that previously relied on KVM_SET_XSAVE being silently ignored when calculating the expected VMSA measurement. Granted, that's sort of buggy behavior on the part of userspace, but QEMU for instance does this. In that case, it just so happens that QEMU's reset values don't appear to affect the VMSA measurement/contents, but there may be userspaces where it would. To avoid this, and have parity with the other interfaces where the new behavior is gated on the new vm_type/KVM_SEV_INIT2 stuff (via has_protected_state), maybe should limited XSAVE/FPU sync'ing to has_protected_state as well? -Mike > } > > err = avic_init_vcpu(svm); > -- > 2.43.0 > >
On Mon, Mar 25, 2024 at 12:43 AM Michael Roth <michael.roth@amd.com> wrote: > There may have be userspaces that previously relied on KVM_SET_XSAVE > being silently ignored when calculating the expected VMSA measurement. > Granted, that's sort of buggy behavior on the part of userspace, but QEMU > for instance does this. In that case, it just so happens that QEMU's reset > values don't appear to affect the VMSA measurement/contents, but there may > be userspaces where it would. > > To avoid this, and have parity with the other interfaces where the new > behavior is gated on the new vm_type/KVM_SEV_INIT2 stuff (via > has_protected_state), maybe should limited XSAVE/FPU sync'ing to > has_protected_state as well? Yes, in particular I am kinda surprised that MXCSR (whose default value after reset is 0x1F80) does not affect the measurement. Paolo
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h index a2be3aefff9f..f86ad3335529 100644 --- a/arch/x86/include/asm/fpu/api.h +++ b/arch/x86/include/asm/fpu/api.h @@ -143,6 +143,9 @@ extern void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfe extern u64 xstate_get_guest_group_perm(void); +extern void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr); + + /* KVM specific functions */ extern bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu); extern void fpu_free_guest_fpstate(struct fpu_guest *gfpu); diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h index 3518fb26d06b..4ff910545451 100644 --- a/arch/x86/kernel/fpu/xstate.h +++ b/arch/x86/kernel/fpu/xstate.h @@ -54,8 +54,6 @@ extern int copy_sigframe_from_user_to_xstate(struct task_struct *tsk, const void extern void fpu__init_cpu_xstate(void); extern void fpu__init_system_xstate(unsigned int legacy_size); -extern void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr); - static inline u64 xfeatures_mask_supervisor(void) { return fpu_kernel_cfg.max_features & XFEATURE_MASK_SUPERVISOR_SUPPORTED; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index a8300646a280..800e836a69fb 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -23,6 +23,7 @@ #include <asm/pkru.h> #include <asm/trapnr.h> #include <asm/fpu/xcr.h> +#include <asm/fpu/xstate.h> #include <asm/debugreg.h> #include "mmu.h" @@ -577,6 +578,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) struct kvm_vcpu *vcpu = &svm->vcpu; struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info; struct sev_es_save_area *save = svm->sev_es.vmsa; + struct xregs_state *xsave; + const u8 *s; + u8 *d; + int i; /* Check some debug related fields before encrypting the VMSA */ if (svm->vcpu.guest_debug || (svm->vmcb->save.dr7 & ~DR7_FIXED_1)) @@ -619,6 +624,30 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) save->sev_features = sev->vmsa_features; + xsave = &vcpu->arch.guest_fpu.fpstate->regs.xsave; + save->x87_dp = xsave->i387.rdp; + save->mxcsr = xsave->i387.mxcsr; + save->x87_ftw = xsave->i387.twd; + save->x87_fsw = xsave->i387.swd; + save->x87_fcw = xsave->i387.cwd; + save->x87_fop = xsave->i387.fop; + save->x87_ds = 0; + save->x87_cs = 0; + save->x87_rip = xsave->i387.rip; + + for (i = 0; i < 8; i++) { + d = save->fpreg_x87 + i * 10; + s = ((u8 *)xsave->i387.st_space) + i * 16; + memcpy(d, s, 10); + } + memcpy(save->fpreg_xmm, xsave->i387.xmm_space, 256); + + s = get_xsave_addr(xsave, XFEATURE_YMM); + if (s) + memcpy(save->fpreg_ymm, s, 256); + else + memset(save->fpreg_ymm, 0, 256); + pr_debug("Virtual Machine Save Area (VMSA):\n"); print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false); @@ -657,6 +686,13 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, if (ret) return ret; + /* + * SEV-ES guests maintain an encrypted version of their FPU + * state which is restored and saved on VMRUN and VMEXIT. + * Mark vcpu->arch.guest_fpu->fpstate as scratch so it won't + * do xsave/xrstor on it. + */ + fpstate_set_confidential(&vcpu->arch.guest_fpu); vcpu->arch.guest_state_protected = true; return 0; } diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index c22e87ebf0de..03108055a7b0 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -1433,14 +1433,6 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu) vmsa_page = snp_safe_alloc_page(vcpu); if (!vmsa_page) goto error_free_vmcb_page; - - /* - * SEV-ES guests maintain an encrypted version of their FPU - * state which is restored and saved on VMRUN and VMEXIT. - * Mark vcpu->arch.guest_fpu->fpstate as scratch so it won't - * do xsave/xrstor on it. - */ - fpstate_set_confidential(&vcpu->arch.guest_fpu); } err = avic_init_vcpu(svm);
SEV-ES allows passing custom contents for x87, SSE and AVX state into the VMSA. Allow userspace to do that with the usual KVM_SET_XSAVE API and only mark FPU contents as confidential after it has been copied and encrypted into the VMSA. Since the XSAVE state for AVX is the first, it does not need the compacted-state handling of get_xsave_addr(). However, there are other parts of XSAVE state in the VMSA that currently are not handled, and the validation logic of get_xsave_addr() is pointless to duplicate in KVM, so move get_xsave_addr() to public FPU API; it is really just a facility to operate on XSAVE state and does not expose any internal details of arch/x86/kernel/fpu. Cc: Dave Hansen <dave.hansen@linux.intel.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/include/asm/fpu/api.h | 3 +++ arch/x86/kernel/fpu/xstate.h | 2 -- arch/x86/kvm/svm/sev.c | 36 ++++++++++++++++++++++++++++++++++ arch/x86/kvm/svm/svm.c | 8 -------- 4 files changed, 39 insertions(+), 10 deletions(-)