In-Reply-To: <[EMAIL PROTECTED]>
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:
> --- 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
> @@ -4,6 +4,7 @@
> #include <linux/smp.h>
> #include <linux/module.h>
> #include <linux/percpu.h>
> +#include <linux/bootmem.h>
> #include <asm/semaphore.h>
> #include <asm/processor.h>
> #include <asm/i387.h>
> @@ -571,6 +572,7 @@ void __init early_cpu_init(void)
> void __devinit cpu_init(void)
> {
> int cpu = smp_processor_id();
> + unsigned i;
> struct tss_struct * t = &per_cpu(init_tss, cpu);
> struct thread_struct *thread = ¤t->thread;
> __u32 stk16_off = (__u32)&per_cpu(cpu_16bit_stack, cpu);
> @@ -635,8 +637,57 @@ void __devinit cpu_init(void)
> load_TR_desc();
> load_LDT(&init_mm.context);
>
> - /* Set up doublefault TSS pointer in the GDT */
> - __set_tss_desc(cpu, GDT_ENTRY_DOUBLEFAULT_TSS, &doublefault_tss);
> +#if EXCEPTION_STACK_ORDER > THREAD_ORDER
> +# error Assertion failed: EXCEPTION_STACK_ORDER <= THREAD_ORDER
> +#endif
> + for (i = 0; i < N_EXCEPTION_TSS; ++i) {
> + unsigned long stack;
> +
> + /* Set up exception handling TSS */
> + exception_tss[cpu][i].esp2 = cpu;
> + exception_tss[cpu][i].esi = (unsigned
> long)&exception_tss[cpu][i];
> +
> + /* Set up exception handling stacks */
> +#ifdef CONFIG_SMP
> + if (cpu) {
> + stack = __get_free_pages(GFP_ATOMIC, THREAD_ORDER);
> + if (!stack)
> + panic("Cannot allocate exception stack %u %d\n",
> + i,
> + cpu);
Indent gone wild?
> + }
> + else
> +#endif
> + stack = (unsigned long)__alloc_bootmem(EXCEPTION_STKSZ,
> + THREAD_SIZE,
> +
> __pa(MAX_DMA_ADDRESS));
> + stack += EXCEPTION_STKSZ;
> + 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;
> +#ifdef CONFIG_SMP
> + if (cpu) {
> + unsigned j;
> +
> + for (j = EXCEPTION_STACK_ORDER; j < THREAD_ORDER; ++j) {
> + /* set_page_refs sets the page count only for
> the first
> + page, but since we split the larger-order
> page here,
> + we need to adjust the page count before
> freeing the
> + pieces. */
> + struct page * page = virt_to_page((void
> *)stack);
> +
> + BUG_ON(page_count(page));
> + set_page_count(page, 1);
> + free_pages(stack, j);
> + stack += (PAGE_SIZE << j);
> + }
> + }
> + else
> +#endif
> + if (EXCEPTION_STACK_ORDER > THREAD_ORDER)
> + free_bootmem(stack, THREAD_SIZE - EXCEPTION_STKSZ);
This comparison should be "if (EXCEPTION_STACK_ORDER < THREAD_ORDER)"
> +
> + /* Set up exception handling TSS pointer in the GDT */
> + __set_tss_desc(cpu, GDT_ENTRY_EXCEPTION_TSS + i,
> &exception_tss[cpu][i]);
> + }
>
> /* Clear %fs and %gs. */
> asm volatile ("xorl %eax, %eax; movl %eax, %fs; movl %eax, %gs");
> --- 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
> @@ -475,9 +475,9 @@ ENTRY(boot_gdt_table)
> .quad 0x00cf92000000ffff /* kernel 4GB data at 0x00000000 */
>
> /*
> - * The Global Descriptor Table contains 28 quadwords, per-CPU.
> + * The Global Descriptor Table contains at least 31 quadwords, per-CPU.
> */
> - .align PAGE_SIZE_asm
> + .align L1_CACHE_BYTES
> ENTRY(cpu_gdt_table)
> .quad 0x0000000000000000 /* NULL descriptor */
> .quad 0x0000000000000000 /* 0x0b reserved */
> @@ -515,9 +515,5 @@ ENTRY(cpu_gdt_table)
> .quad 0x0040920000000000 /* 0xc8 APM DS data */
>
> .quad 0x0000920000000000 /* 0xd0 - ESPFIX 16-bit SS */
> - .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.
> - .quad 0x0000000000000000 /* 0xf8 - GDT entry 31: double-fault
> TSS */
> -
> + /* remaining entries run-time initialized */
A better comment would help here.
> + .fill GDT_ENTRIES - (. - cpu_gdt_table) / 8, 8, 0
__
Chuck
Subliminal URL: www.sluggy.com/daily.php?date=050905
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
