From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751150AbdE3KPp (ORCPT ); Tue, 30 May 2017 06:15:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41408 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750912AbdE3KPn (ORCPT ); Tue, 30 May 2017 06:15:43 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 06C8C3680F Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=pbonzini@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 06C8C3680F Subject: Re: [PATCH v4] KVM: x86: avoid large stack allocations in em_fxrstor To: Nick Desaulniers Cc: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org References: <20170529203908.10775-1-nick.desaulniers@gmail.com> <20170529224858.9942-1-nick.desaulniers@gmail.com> From: Paolo Bonzini Message-ID: <241100ad-9049-6a4f-078d-c64e0fbba417@redhat.com> Date: Tue, 30 May 2017 12:15:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <20170529224858.9942-1-nick.desaulniers@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 30 May 2017 10:15:43 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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)); > >