On 30/05/2017 00:48, Nick Desaulniers wrote: > em_fxstor previously called fxstor_fixup. Both created instances of > struct fxregs_state on the stack, which triggered the warning: > > arch/x86/kvm/emulate.c:4018:12: warning: stack frame size of 1080 bytes > in function > 'em_fxrstor' [-Wframe-larger-than=] > static int em_fxrstor(struct x86_emulate_ctxt *ctxt) > ^ > with CONFIG_FRAME_WARN set to 1024. > > This patch does the fixup in em_fxstor now, avoiding one additional > struct fxregs_state, and now fxstor_fixup can be removed as it has no > other call sites. > > Signed-off-by: Nick Desaulniers <nick.desaulniers@xxxxxxxxx> > --- > New in V4: > * undo changes for V3, prefer to only call segmented_read_std() when > size has been initialized. > > New in V3: > * initialized size to 0 to avoid maybe-uninitialized warning. Check > that it gets set to something other than 0 before passing size to > segmented_read_std(). > > New in V2: > * reworked patch to do what was recommended by maintainers in: > https://lkml.org/lkml/2017/5/25/391 > > arch/x86/kvm/emulate.c | 64 ++++++++++++++++++++------------------------------ > 1 file changed, 26 insertions(+), 38 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 0816ab2e8adc..0367f7f81792 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -3985,57 +3985,45 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt) > return segmented_write_std(ctxt, ctxt->memop.addr.mem, &fx_state, size); > } > > -static int fxrstor_fixup(struct x86_emulate_ctxt *ctxt, > - struct fxregs_state *new) > -{ > - int rc = X86EMUL_CONTINUE; > - struct fxregs_state old; > - > - rc = asm_safe("fxsave %[fx]", , [fx] "+m"(old)); > - if (rc != X86EMUL_CONTINUE) > - return rc; > - > - /* > - * 64 bit host will restore XMM 8-15, which is not correct on non-64 > - * bit guests. Load the current values in order to preserve 64 bit > - * XMMs after fxrstor. > - */ > -#ifdef CONFIG_X86_64 > - /* XXX: accessing XMM 8-15 very awkwardly */ > - memcpy(&new->xmm_space[8 * 16/4], &old.xmm_space[8 * 16/4], 8 * 16); > -#endif > - > - /* > - * Hardware doesn't save and restore XMM 0-7 without CR4.OSFXSR, but > - * does save and restore MXCSR. > - */ > - if (!(ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSFXSR)) > - memcpy(new->xmm_space, old.xmm_space, 8 * 16); > - > - return rc; > -} > - > static int em_fxrstor(struct x86_emulate_ctxt *ctxt) > { > struct fxregs_state fx_state; > int rc; > + unsigned int size; > > rc = check_fxsr(ctxt); > if (rc != X86EMUL_CONTINUE) > return rc; > > - rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, 512); > - if (rc != X86EMUL_CONTINUE) > - return rc; > + ctxt->ops->get_fpu(ctxt); > + > + if (ctxt->mode < X86EMUL_MODE_PROT64) { > + rc = asm_safe("fxsave %[fx]", , [fx] "+m"(fx_state)); > + if (rc != X86EMUL_CONTINUE) > + return rc; > + /* > + * Hardware doesn't save and restore XMM 0-7 without > + * CR4.OSFXSR, but does save and restore MXCSR. > + */ > + if (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSFXSR) > + size = offsetof(struct fxregs_state, xmm_space[8]); > + else > + size = offsetof(struct fxregs_state, xmm_space[0]); > + rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, > + size); > + if (rc != X86EMUL_CONTINUE) > + return rc; > + } else if (ctxt->mode == X86EMUL_MODE_PROT64) { > + size = offsetof(struct fxregs_state, xmm_space[16]); > + rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, > + size); > + if (rc != X86EMUL_CONTINUE) > + return rc; > + } You can just remove the "if (ctxt->mode == X86EMUL_MODE_PROT64)". This way, the call of segmented_read_std and the "if (rc != X86EMUL_CONTINUE)" can move outside the conditional. Thanks, Paolo > if (fx_state.mxcsr >> 16) > return emulate_gp(ctxt, 0); > > - ctxt->ops->get_fpu(ctxt); > - > - if (ctxt->mode < X86EMUL_MODE_PROT64) > - rc = fxrstor_fixup(ctxt, &fx_state); > - > if (rc == X86EMUL_CONTINUE) > rc = asm_safe("fxrstor %[fx]", : [fx] "m"(fx_state)); > >
