Re: [PATCH] fix i386 double fault handler

["Jan Beulich" <JBeulich@novell.com>]

>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