Messages in this thread | | | Date | Mon, 12 Sep 2005 10:48:54 +0200 | From | "Jan Beulich" <> | Subject | Re: [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] | |