Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933034AbbGUPaM (ORCPT ); Tue, 21 Jul 2015 11:30:12 -0400 Received: from mail-oi0-f45.google.com ([209.85.218.45]:34536 "EHLO mail-oi0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932933AbbGUPaJ (ORCPT ); Tue, 21 Jul 2015 11:30:09 -0400 MIME-Version: 1.0 In-Reply-To: <20150721071151.GA8367@gmail.com> References: <1437354550-25858-1-git-send-email-brgerst@gmail.com> <1437354550-25858-5-git-send-email-brgerst@gmail.com> <20150721071151.GA8367@gmail.com> Date: Tue, 21 Jul 2015 11:30:09 -0400 Message-ID: Subject: Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct From: Brian Gerst To: Ingo Molnar Cc: "the arch/x86 maintainers" , Linux Kernel Mailing List , "H. Peter Anvin" , Denys Vlasenko , Andy Lutomirski , Linus Torvalds Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4955 Lines: 151 On Tue, Jul 21, 2015 at 3:11 AM, Ingo Molnar wrote: > > * 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. Some of these field names are visible to userspace and can't change. > 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. I think there is some merit to keeping the kernel prefix to make it clear it is not the userspace version, which is similar, > 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 first is a feature flag (only used for screen bitmap), and the latter is a shadow copy of EFLAGS. > - The fields flags..vm86plus seems to be an as-is copy of 'struct > vm86plus_struct'. Could this be organized in a smarter fashion?. I specifically want to exclude retaining a copy of vm86_regs in the kernel data. It's not needed after the syscall returns to vm86 mode. I had originally copied all the fields after regs as a whole, but Andy objected to that. So I changed it to copy all the fields individually instead, so that the order of kernel vs. userspace doesn't matter. > - '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; > }; This would be a userspace visible change. The register order and field names from userspace are fixed, and I don't want it to depend on the order of fields of the in-kernel pt_regs. It was a bad ABI design choice, but it's too late to fix it now. > - 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? Will do. > > Thanks, > > Ingo -- Brian Gerst -- 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/