Message-ID: <CAMzpN2h+rs1tU0x4B=w146uAgEWje5+1azxoC8uQsAbzT1d3DQ@mail.gmail.com>
Date:	Tue, 21 Jul 2015 11:30:09 -0400
From:	Brian Gerst <brgerst@...il.com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	"the arch/x86 maintainers" <x86@...nel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

On Tue, Jul 21, 2015 at 3:11 AM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Brian Gerst <brgerst@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists