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

From: Paolo Bonzini
Date: Wed May 31 2017 - 07:01:35 EST



> + 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