Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752776AbbGUHMD (ORCPT ); Tue, 21 Jul 2015 03:12:03 -0400 Received: from mail-wg0-f52.google.com ([74.125.82.52]:36779 "EHLO mail-wg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752686AbbGUHL4 (ORCPT ); Tue, 21 Jul 2015 03:11:56 -0400 Date: Tue, 21 Jul 2015 09:11:51 +0200 From: Ingo Molnar To: Brian Gerst Cc: x86@kernel.org, linux-kernel@vger.kernel.org, "H. Peter Anvin" , Denys Vlasenko , Andy Lutomirski , Linus Torvalds Subject: Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct Message-ID: <20150721071151.GA8367@gmail.com> References: <1437354550-25858-1-git-send-email-brgerst@gmail.com> <1437354550-25858-5-git-send-email-brgerst@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1437354550-25858-5-git-send-email-brgerst@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3656 Lines: 127 * Brian Gerst wrote: > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -110,6 +110,13 @@ void exit_thread(void) > kfree(bp); > } > > +#ifdef CONFIG_VM86 > + if (t->vm86) { > + kfree(t->vm86); > + t->vm86 = NULL; > + } > +#endif This should be a helper: vm86__free(t->vm86) or so. > diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c > index e6c2b47..dce0a1c 100644 > --- a/arch/x86/kernel/vm86_32.c > +++ b/arch/x86/kernel/vm86_32.c > @@ -242,9 +244,16 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus, > { > struct tss_struct *tss; > struct task_struct *tsk = current; > + struct kernel_vm86_info *vm86 = tsk->thread.vm86; > unsigned long err = 0; > > - if (tsk->thread.saved_sp0) > + if (!vm86) > + { Non-standard style. > + if (!(vm86 = kzalloc(sizeof(*vm86), GFP_KERNEL))) > + return -ENOMEM; > + tsk->thread.vm86 = vm86; > + } > + if (vm86->saved_sp0) > return -EPERM; Btw., the variable names here are crazy. I had to look twice to realize that we have 'v86' and 'vm86' which are two different things. Also, vm86plus_struct variables and fields are named wildly inconsistently: sometimes it's 'vm86.vm86_info', sometimes it's 'v86', sometimes 'user'. Ugh. Other fields have naming inconsistencies as well: for example we have thread.vm86->vm86plus.vm86dbg_active. 'vm86' is repeated _three_ times in that name, for no good reason. So please clean up the naming to make this all easier to read. Only the highest level field should have 'vm86' in it - all subsequent fields will inherit that name one way or another. At a quick glance I'd do the following renames: struct kernel_vm86_info *vm86; => struct vm86 *vm86; - it's obviously 'information' so the _info is superfluous. - and it's obviously embedded in a kernel structure, so the kernel_ is superfluous as well. Then let's look at other fields of the main structure: struct kernel_vm86_info { struct vm86plus_struct __user *vm86_info; struct pt_regs regs32; unsigned long v86flags; unsigned long v86mask; unsigned long saved_sp0; unsigned long flags; unsigned long screen_bitmap; unsigned long cpu_type; struct revectored_struct int_revectored; struct revectored_struct int21_revectored; struct vm86plus_info_struct vm86plus; }; - Why is there a vm86.flags and a vm86.v86flags field? What's the difference and can we eliminate the confusion factor? - The fields flags..vm86plus seems to be an as-is copy of 'struct vm86plus_struct'. Could this be organized in a smarter fashion?. - 'struct vm86_regs' appears to be an as-is copy of 32-bit pt_regs, plus: unsigned short es, __esh; unsigned short ds, __dsh; unsigned short fs, __fsh; unsigned short gs, __gsh; Instead of a slightly different structure copying pt_regs, why not express it as: struct vm86_regs { struct pt_regs regs; unsigned short es, __esh; unsigned short ds, __dsh; unsigned short fs, __fsh; unsigned short gs, __gsh; }; ? - There's a number of 'long' fields which are always 32-bit, which is pretty confusing even if it's only ever built on 32-bit kerenls, can we use u8/u16/u32/u64 for ABI components instead? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/