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

    * 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


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