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.desaulni...@gmail.com>
> ---
> 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));
>
>
