Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

[Ingo Molnar <mingo@kernel.org>]

* 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.

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