From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932905AbbERUf4 (ORCPT ); Mon, 18 May 2015 16:35:56 -0400 Received: from www.linutronix.de ([62.245.132.108]:51049 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932494AbbERUfy (ORCPT ); Mon, 18 May 2015 16:35:54 -0400 Date: Mon, 18 May 2015 22:36:01 +0200 (CEST) From: Thomas Gleixner To: Dave Hansen cc: linux-kernel@vger.kernel.org, x86@kernel.org, dave.hansen@linux.intel.com, oleg@redhat.com, bp@alien8.de, riel@redhat.com, sbsiddha@gmail.com, luto@amacapital.net, mingo@redhat.com, hpa@zytor.com, fenghua.yu@intel.com Subject: Re: [PATCH 03/19] x86, mpx: use new tsk_get_xsave_addr() In-Reply-To: <20150508185949.3250100D@viggo.jf.intel.com> Message-ID: References: <20150508185948.4C19F4B0@viggo.jf.intel.com> <20150508185949.3250100D@viggo.jf.intel.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 8 May 2015, Dave Hansen wrote: > Subject: x86, mpx: use new tsk_get_xsave_addr() -NOSUCHFUNCTION And please let the sentence start with an upper case letter. x86, mpx: Use ... > #ifdef CONFIG_X86_INTEL_MPX > siginfo_t *mpx_generate_siginfo(struct pt_regs *regs, > - struct xsave_struct *xsave_buf); > -int mpx_handle_bd_fault(struct xsave_struct *xsave_buf); > + struct task_struct *tsk); > +int mpx_handle_bd_fault(struct task_struct *tsk); So you still keep *tsk and remove it in the next patch. I guess either ordering has its value, so no problem to do it this way. > static inline int kernel_managing_mpx_tables(struct mm_struct *mm) > { > return (mm->bd_addr != MPX_INVALID_BOUNDS_DIR); > @@ -78,11 +78,11 @@ void mpx_notify_unmap(struct mm_struct * > unsigned long start, unsigned long end); > #else > static inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs, > - struct xsave_struct *xsave_buf) > + struct task_struct *tsk) But that makes no sense. Introduce a new instance just to remove it one patch later. So maybe cleaning up the *tsk thing first might be the better approach, but I let you decide. > #include > #include > #include > +#include > > #ifdef CONFIG_X86_64 > #include > @@ -372,7 +373,6 @@ dotraplinkage void do_double_fault(struc > dotraplinkage void do_bounds(struct pt_regs *regs, long error_code) > { > struct task_struct *tsk = current; > - struct xsave_struct *xsave_buf; > enum ctx_state prev_state; > struct bndcsr *bndcsr; > siginfo_t *info; > @@ -393,12 +393,11 @@ dotraplinkage void do_bounds(struct pt_r > > /* > * We need to look at BNDSTATUS to resolve this exception. > - * It is not directly accessible, though, so we need to > - * do an xsave and then pull it out of the xsave buffer. > + * A NULL here might mean that it is in its 'init state', > + * which is all zeros which indicates MPX was not > + * responsible for the exception. > */ > - fpu_save_init(&tsk->thread.fpu); > - xsave_buf = &(tsk->thread.fpu.state->xsave); > - bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR); > + bndcsr = get_xsave_field(XSTATE_BNDCSR); Hmm, I did not notice when I looked at the patch which introduced get_xsave_field(). But with the removed get_xsave_addr() I really had to look twice. get_xsave_field() suggest that you get the field content. So wouldn't it be better to name it get_xsave_field_ptr() or such? > if (!bndcsr) > goto exit_trap; > > @@ -409,11 +408,11 @@ dotraplinkage void do_bounds(struct pt_r > */ > switch (bndcsr->bndstatus & MPX_BNDSTA_ERROR_CODE) { > case 2: /* Bound directory has invalid entry. */ > - if (mpx_handle_bd_fault(xsave_buf)) > + if (mpx_handle_bd_fault(tsk)) > goto exit_trap; > break; /* Success, it was handled */ > case 1: /* Bound violation. */ > - info = mpx_generate_siginfo(regs, xsave_buf); > + info = mpx_generate_siginfo(regs, tsk); > if (IS_ERR(info)) { > /* > * We failed to decode the MPX instruction. Act as if > diff -puN arch/x86/mm/mpx.c~use-new-tsk_get_xsave_addr arch/x86/mm/mpx.c > --- a/arch/x86/mm/mpx.c~use-new-tsk_get_xsave_addr 2015-05-08 11:46:11.388599581 -0700 > +++ b/arch/x86/mm/mpx.c 2015-05-08 11:46:11.393599807 -0700 > @@ -273,7 +273,7 @@ bad_opcode: > * The caller is expected to kfree() the returned siginfo_t. > */ > siginfo_t *mpx_generate_siginfo(struct pt_regs *regs, > - struct xsave_struct *xsave_buf) > + struct task_struct *tsk) Looking at the implementation makes me wonder about the extra *tsk argument some more .... > { > struct bndreg *bndregs, *bndreg; > siginfo_t *info = NULL; > @@ -295,8 +295,8 @@ siginfo_t *mpx_generate_siginfo(struct p > err = -EINVAL; > goto err_out; > } > - /* get the bndregs _area_ of the xsave structure */ > - bndregs = get_xsave_addr(xsave_buf, XSTATE_BNDREGS); > + /* get bndregs field from urrent task's xsave area */ urrent? > + bndregs = get_xsave_field(XSTATE_BNDREGS); > if (!bndregs) { > err = -EINVAL; > goto err_out; Still no user of *tsk .... > @@ -358,8 +358,7 @@ static __user void *task_get_bounds_dir( > -int mpx_handle_bd_fault(struct xsave_struct *xsave_buf) > +int mpx_handle_bd_fault(struct task_struct *tsk) > { > /* > * Userspace never asked us to manage the bounds tables, > @@ -535,7 +534,7 @@ int mpx_handle_bd_fault(struct xsave_str > if (!kernel_managing_mpx_tables(current->mm)) > return -EINVAL; > > - if (do_mpx_bt_fault(xsave_buf)) { > + if (do_mpx_bt_fault(tsk)) { Same problem actually with do_mpx_bt_fault(). Thanks, tglx