lkml.org 
[lkml]   [2015]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct
From
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


\
 
 \ /
  Last update: 2015-07-21 17:41    [W:0.097 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site