Messages in this thread | | | Date | Tue, 21 Jul 2015 11:30:09 -0400 | Subject | Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct | From | Brian Gerst <> |
| |
On Tue, Jul 21, 2015 at 3:11 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Brian Gerst <brgerst@gmail.com> 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
| |