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

[Paolo Bonzini]

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));
>  
>