Re: [kvm PATCH v5 2/4] kvm: x86: Dynamically allocate guest_fpu

[Marc Orr <marcorr@google.com>]

On Wed, Oct 31, 2018 at 2:30 PM Dave Hansen <dave.hansen@intel.com> 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 }