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    [from the cache]
©2003-2011 Jasper Spaans