lkml.org 
[lkml]   [2005]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] fix i386 double fault handler
    >On Fri, 09 Sep 2005 at 09:58:29 +0200, Jan Beulich wrote:
    >
    >> Make the double fault handler use CPU-specific stacks, add some
    >> abstraction to simplify future change of other exception handlers
    >> to go through task gates. Change the pointer validity checks in
    >> the double fault handler to account for the fact that both GDT
    >> and TSS aren't in static kernel space anymore.
    >
    > This is really nice. Some comments, though:

    Thanks!

    >> --- 2.6.13/arch/i386/kernel/cpu/common.c 2005-08-29
    01:41:01.000000000 +0200
    >> +++
    2.6.13-i386-double-fault/arch/i386/kernel/cpu/common.c 2005-09-08
    15:00:54.000000000 +0200
    >>...
    >> + panic("Cannot allocate exception stack
    %u %d\n",
    >> + i,
    >> + cpu);
    >
    > Indent gone wild?

    Not really. There doesn't seem to be a clear indentation rule for
    function arguments not all fitting on one line - I re-checked a few
    instances (namely calls to do_fork), and the most consistent thing I can
    conclude from this would be "indent using spaces to match the first
    arguments position, and replace groups of 8 spaces with tabs". The code
    here seems to match this, except that I dislike putting two arguments on
    the same line if I have to split the line anyway.

    >>...
    >> + exception_tss[cpu][i].esp = exception_tss[cpu][i].esp0 =
    stack;

    > Please don't cascade assignments like that. It's much more obvious
    >when it's like this:
    >
    > exception_tss[cpu][i].esp = stack;
    > exception_tss[cpu][i].esp0 = stack;

    Changed.

    >>...
    >> + if (EXCEPTION_STACK_ORDER > THREAD_ORDER)
    >> + free_bootmem(stack, THREAD_SIZE -
    EXCEPTION_STKSZ);
    >
    > This comparison should be "if (EXCEPTION_STACK_ORDER <
    THREAD_ORDER)"

    Indeed. However, the whole piece is unnecessary as it turns out (I
    added it shortly before submitting the patch, thinking I had overlooked
    the freeing part for the bootmem case, but in reality there's nothing to
    be freed in that case at all).

    >>...
    >> --- 2.6.13/arch/i386/kernel/head.S 2005-08-29 01:41:01.000000000
    +0200
    >> +++
    2.6.13-i386-double-fault/arch/i386/kernel/head.S 2005-09-01
    13:06:01.000000000 +0200
    >>...
    >> - .quad 0x0000000000000000 /* 0xd8 - unused */
    >> - .quad 0x0000000000000000 /* 0xe0 - unused */
    >> - .quad 0x0000000000000000 /* 0xe8 - unused */
    >> - .quad 0x0000000000000000 /* 0xf0 - unused */
    >
    > Could you leave these in? They make it easier to see they're
    available.

    No problem.

    >> - .quad 0x0000000000000000 /* 0xf8 - GDT entry 31:
    double-fault TSS */
    >> -
    >> + /* remaining entries run-time initialized */
    >
    > A better comment would help here.

    Hopefully the new version will be liked better.

    New patch attached.

    Thanks again, Jan
    [unhandled content-type:application/octet-stream]
    \
     
     \ /
      Last update: 2005-09-12 10:55    [W:0.047 / U:3.564 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site