| Message ID | 20211011223611.249593446@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/fpu: Preparatory cleanups for AMX support (part 1) | expand |
On Tue, Oct 12, 2021 at 02:00:22AM +0200, Thomas Gleixner wrote:
> Similar to the copy from user function the FPU core has this already
implemented with all bells and whistels.
"whistles"
And also, same nitpicks as here:
https://lore.kernel.org/r/YWW/PEQyQAwS9/qv@zn.tnic
On 12/10/21 02:00, Thomas Gleixner wrote: > > - if (boot_cpu_has(X86_FEATURE_XSAVE)) { > - memset(guest_xsave, 0, sizeof(struct kvm_xsave)); > - fill_xsave((u8 *) guest_xsave->region, vcpu); > - } else { > - memcpy(guest_xsave->region, > - &vcpu->arch.guest_fpu->state.fxsave, > - sizeof(struct fxregs_state)); > - *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] = > - XFEATURE_MASK_FPSSE; > - } After the patch, this final assignment is not done in the else case: > + > + if (cpu_feature_enabled(X86_FEATURE_XSAVE)) { > + __copy_xstate_to_uabi_buf(mb, &kstate->xsave, pkru, > + XSTATE_COPY_XSAVE); > + } else { > + memcpy(&ustate->fxsave, &kstate->fxsave, sizeof(ustate->fxsave)); > + } > +} This leaves the xstate_bv set to 0 instead of XFEATURE_MASK_FPSSE. Resuming a VM then fails if you save on a non-XSAVE machine and restore it on an XSAVE machine. The memset(guest_xsave, 0, sizeof(struct kvm_xsave)) also is not reproduced, you can make it unconditional for simplicity; this is not a fast path. Paolo
On Tue, Oct 12 2021 at 19:36, Paolo Bonzini wrote: > On 12/10/21 02:00, Thomas Gleixner wrote: >> >> - if (boot_cpu_has(X86_FEATURE_XSAVE)) { >> - memset(guest_xsave, 0, sizeof(struct kvm_xsave)); >> - fill_xsave((u8 *) guest_xsave->region, vcpu); >> - } else { >> - memcpy(guest_xsave->region, >> - &vcpu->arch.guest_fpu->state.fxsave, >> - sizeof(struct fxregs_state)); >> - *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] = >> - XFEATURE_MASK_FPSSE; >> - } > > After the patch, this final assignment is not done in the else case: Doh. >> + >> + if (cpu_feature_enabled(X86_FEATURE_XSAVE)) { >> + __copy_xstate_to_uabi_buf(mb, &kstate->xsave, pkru, >> + XSTATE_COPY_XSAVE); >> + } else { >> + memcpy(&ustate->fxsave, &kstate->fxsave, sizeof(ustate->fxsave)); >> + } >> +} > > This leaves the xstate_bv set to 0 instead of XFEATURE_MASK_FPSSE. > Resuming a VM then fails if you save on a non-XSAVE machine and restore > it on an XSAVE machine. Yup. > The memset(guest_xsave, 0, sizeof(struct kvm_xsave)) also is not > reproduced, you can make it unconditional for simplicity; this is not a > fast path. Duh, I should have mentioned that in the changelog. The buffer is allocated with kzalloc() soe the memset is redundant, right? Thanks, tglx
On 12/10/21 19:47, Thomas Gleixner wrote: >> The memset(guest_xsave, 0, sizeof(struct kvm_xsave)) also is not >> reproduced, you can make it unconditional for simplicity; this is not a >> fast path. > Duh, I should have mentioned that in the changelog. The buffer is > allocated with kzalloc() soe the memset is redundant, right? Yes, I always confuse the __user pointers with the temporary ones that are allocated in the callers. Paolo
--- a/arch/x86/include/asm/fpu/api.h +++ b/arch/x86/include/asm/fpu/api.h @@ -116,7 +116,7 @@ extern void fpu_init_fpstate_user(struct /* KVM specific functions */ extern void fpu_swap_kvm_fpu(struct fpu *save, struct fpu *rstor, u64 restore_mask); -struct kvm_vcpu; extern int fpu_copy_kvm_uabi_to_vcpu(struct fpu *fpu, const void *buf, u64 xcr0, u32 *pkru); +extern void fpu_copy_vcpu_to_kvm_uabi(struct fpu *fpu, void *buf, unsigned int size, u32 pkru); #endif /* _ASM_X86_FPU_API_H */ --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -175,6 +175,22 @@ void fpu_swap_kvm_fpu(struct fpu *save, } EXPORT_SYMBOL_GPL(fpu_swap_kvm_fpu); +void fpu_copy_vcpu_to_kvm_uabi(struct fpu *fpu, void *buf, + unsigned int size, u32 pkru) +{ + union fpregs_state *kstate = &fpu->state; + union fpregs_state *ustate = buf; + struct membuf mb = { .p = buf, .left = size }; + + if (cpu_feature_enabled(X86_FEATURE_XSAVE)) { + __copy_xstate_to_uabi_buf(mb, &kstate->xsave, pkru, + XSTATE_COPY_XSAVE); + } else { + memcpy(&ustate->fxsave, &kstate->fxsave, sizeof(ustate->fxsave)); + } +} +EXPORT_SYMBOL_GPL(fpu_copy_vcpu_to_kvm_uabi); + int fpu_copy_kvm_uabi_to_vcpu(struct fpu *fpu, const void *buf, u64 xcr0, u32 *vpkru) { --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4695,65 +4695,15 @@ static int kvm_vcpu_ioctl_x86_set_debugr return 0; } -static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu) -{ - struct xregs_state *xsave = &vcpu->arch.guest_fpu->state.xsave; - u64 xstate_bv = xsave->header.xfeatures; - u64 valid; - - /* - * Copy legacy XSAVE area, to avoid complications with CPUID - * leaves 0 and 1 in the loop below. - */ - memcpy(dest, xsave, XSAVE_HDR_OFFSET); - - /* Set XSTATE_BV */ - xstate_bv &= vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE; - *(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv; - - /* - * Copy each region from the possibly compacted offset to the - * non-compacted offset. - */ - valid = xstate_bv & ~XFEATURE_MASK_FPSSE; - while (valid) { - u32 size, offset, ecx, edx; - u64 xfeature_mask = valid & -valid; - int xfeature_nr = fls64(xfeature_mask) - 1; - void *src; - - cpuid_count(XSTATE_CPUID, xfeature_nr, - &size, &offset, &ecx, &edx); - - if (xfeature_nr == XFEATURE_PKRU) { - memcpy(dest + offset, &vcpu->arch.pkru, - sizeof(vcpu->arch.pkru)); - } else { - src = get_xsave_addr(xsave, xfeature_nr); - if (src) - memcpy(dest + offset, src, size); - } - - valid -= xfeature_mask; - } -} - static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu, struct kvm_xsave *guest_xsave) { if (!vcpu->arch.guest_fpu) return; - if (boot_cpu_has(X86_FEATURE_XSAVE)) { - memset(guest_xsave, 0, sizeof(struct kvm_xsave)); - fill_xsave((u8 *) guest_xsave->region, vcpu); - } else { - memcpy(guest_xsave->region, - &vcpu->arch.guest_fpu->state.fxsave, - sizeof(struct fxregs_state)); - *(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)] = - XFEATURE_MASK_FPSSE; - } + fpu_copy_vcpu_to_kvm_uabi(vcpu->arch.guest_fpu, guest_xsave->region, + sizeof(guest_xsave->region), + vcpu->arch.pkru); } static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
Similar to the copy from user function the FPU core has this already implemented with all bells and whistels. Get rid of the duplicated code and use the core functionality. Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: kvm@vger.kernel.org Cc: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/include/asm/fpu/api.h | 2 - arch/x86/kernel/fpu/core.c | 16 +++++++++++ arch/x86/kvm/x86.c | 56 ++--------------------------------------- 3 files changed, 20 insertions(+), 54 deletions(-)