>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
Attachment:
linux-2.6.13-i386-double-fault.patch
Description: Binary data
- References:
- Re: [PATCH] fix i386 double fault handler
- From: Chuck Ebbert <[email protected]>
- Re: [PATCH] fix i386 double fault handler
- Prev by Date: Re: [RFC][MEGAPATCH] Change __ASSEMBLY__ to __ASSEMBLER__ (defined by GCC from 2.95 to current CVS)
- Next by Date: Re: [PATCH 2.6.13 2/14] sas-class: README
- Previous by thread: Re: [PATCH] fix i386 double fault handler
- Next by thread: kernel 2.6.13 buffer strangeness
- Index(es):
