Re: [PATCH v5] KVM: x86: avoid large stack allocations in em_fxrstor

2017-06-01 Thread Nick Desaulniers 
On Thu, Jun 01, 2017 at 09:36:18AM +0200, Paolo Bonzini wrote:
> On 01/06/2017 03:05, Nick Desaulniers wrote:
> > On Wed, May 31, 2017 at 07:01:29AM -0400, Paolo Bonzini wrote:
> >>> + size = offsetof(struct fxregs_state, xmm_space[16]);
> >> This still has the same issue (it should be multiplied by 4).
> > 
> > I'm still misunderstanding the math here.
> > 
> > Why multiplied by four, in this case? 8 * 16 / 4 is used in other cases.
> 
> *16/4 is the same as *4. :)

I meant the use of an expression full of literals rather than either a
single literal or an expression formed from well named variables seemed
kind of like a code smell, but w/e.  Patch inbound.

Re: [PATCH v5] KVM: x86: avoid large stack allocations in em_fxrstor

2017-06-01 Thread Paolo Bonzini 


On 01/06/2017 03:05, Nick Desaulniers wrote:
> On Wed, May 31, 2017 at 07:01:29AM -0400, Paolo Bonzini wrote:
>>> +   size = offsetof(struct fxregs_state, xmm_space[16]);
>> This still has the same issue (it should be multiplied by 4).
> 
> I'm still misunderstanding the math here.
> 
> Why multiplied by four, in this case? 8 * 16 / 4 is used in other cases.

*16/4 is the same as *4. :)

Paolo

Re: [PATCH v5] KVM: x86: avoid large stack allocations in em_fxrstor

2017-05-31 Thread Nick Desaulniers 
On Wed, May 31, 2017 at 07:01:29AM -0400, Paolo Bonzini wrote:
> > +   size = offsetof(struct fxregs_state, xmm_space[16]);
> This still has the same issue (it should be multiplied by 4).

I'm still misunderstanding the math here.

Why multiplied by four, in this case? 8 * 16 / 4 is used in other cases.

Also, previously Radim wrote:

>> +  size = offsetof(struct fxregs_state, xmm_space[8]);
> This should be the size of first 8 XMM registers, but xmm_space is of
> type u32, so the correct size is
>   xmm_space[8 * 16/sizeof(*fx_state.xmm_space)].

So I think my calculation is off in xmm_offset still?  Can we make use
of well-named variables, in place of these constants? Otherwise the math
is hard to follow.

> Thanks Nick for the patches and Radim for the reviews!
> Paolo

Thanks for the code review!

Re: [PATCH v5] KVM: x86: avoid large stack allocations in em_fxrstor

2017-05-31 Thread Paolo Bonzini 

> + size = offsetof(struct fxregs_state, xmm_space[16]);

This still has the same issue (it should be multiplied by 4).  Here's my
take on it; I checked the compiled code and it's pretty good too (the
compiler knows to do the fxsave if and only if ctxt->mode <
X86EMUL_MODE_PROT64, because that's when the size is smaller):

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 55470ad07c2a..b76f19d2684d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3944,10 +3944,19 @@ static int check_fxsr(struct x86_emulate_ctxt *ctxt)
  * Hardware doesn't save and restore XMM 0-7 without CR4.OSFXSR, but does save
  * and restore MXCSR.
  */
-static size_t xmm_offset(struct x86_emulate_ctxt *ctxt)
+static size_t __fxstate_size(int nregs)
 {
-   bool b = ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSFXSR;
-   return offsetof(struct fxregs_state, xmm_space[b ? 8 * 16 / 4 : 0]);
+   return offsetof(struct fxregs_state, xmm_space[0]) + nregs * 16;
+}
+
+static inline size_t fxstate_size(struct x86_emulate_ctxt *ctxt)
+{
+   bool cr4_osfxsr;
+   if (ctxt->mode == X86EMUL_MODE_PROT64)
+   return __fxstate_size(16);
+
+   cr4_osfxsr = ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSFXSR;
+   return __fxstate_size(cr4_osfxsr ? 8 : 0);
 }
 
 /*
@@ -3987,7 +3996,7 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt)
return rc;
 
return segmented_write_std(ctxt, ctxt->memop.addr.mem, &fx_state,
-   xmm_offset(ctxt));
+  fxstate_size(ctxt));
 }
 
 static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
@@ -4002,13 +4011,11 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
 
ctxt->ops->get_fpu(ctxt);
 
-   if (ctxt->mode < X86EMUL_MODE_PROT64) {
+   size = fxstate_size(ctxt);
+   if (size < __fxstate_size(16)) {
rc = asm_safe("fxsave %[fx]", , [fx] "+m"(fx_state));
if (rc != X86EMUL_CONTINUE)
goto out;
-   size = xmm_offset(ctxt);
-   } else {
-   size = offsetof(struct fxregs_state, xmm_space[16]);
}
 
rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, size);

Thanks Nick for the patches and Radim for the reviews!

Paolo