On Wed, Oct 31, 2018 at 2:30 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> On 10/31/18 2:24 PM, Marc Orr wrote:
> >> It can get set to sizeof(struct fregs_state) for systems where XSAVE is
> >> not in use. I was neglecting to mention those when I said the "~500
> >> byte" number.
> >>
> >> My point was that it can vary wildly and that any static allocation
> >> scheme will waste lots of memory when we have small hardware-supported
> >> buffers.
> >
> > Got it. Then I think we need to set the size for the kmem cache to
> > max(fpu_kernel_xstate_size, sizeof(fxregs_state)), unless I'm missing
> > something. I'll send out a version of the patch that does this in a
> > bit. Thanks!
>
> Despite its name, fpu_kernel_xstate_size *should* always be the "size of
> the hardware buffer we need to back 'struct fpu'". That's true for all
> of the various formats we support: XSAVE, fxregs, swregs, etc...
>
> fpu__init_system_xstate_size_legacy() does that when XSAVE itself is not
> in play.
That makes sense. But my specific concern is the code I've copied
below, from arch/x86/kvm/x86.c. Notice on a system where
guest_fpu.state is a fregs_state, this code would generate garbage for
some fields. With the new code we're talking about, it will cause
memory corruption. But maybe it's not possible to run this code on a
system with an fregs_state, because such systems would predate VMX?
8382 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
8383 {
8384 struct fxregs_state *fxsave;
8385
8386 vcpu_load(vcpu);
8387
8388 fxsave = &vcpu->arch.guest_fpu->state.fxsave;
8389 memcpy(fpu->fpr, fxsave->st_space, 128);
8390 fpu->fcw = fxsave->cwd;
8391 fpu->fsw = fxsave->swd;
8392 fpu->ftwx = fxsave->twd;
8393 fpu->last_opcode = fxsave->fop;
8394 fpu->last_ip = fxsave->rip;
8395 fpu->last_dp = fxsave->rdp;
8396 memcpy(fpu->xmm, fxsave->xmm_space, sizeof fxsave->xmm_space);
8397
8398 vcpu_put(vcpu);
8399 return 0;
8400 }
