Re: [PATCH] i386: per-CPU double fault TSS and stack

Andi Kleen
2007-09-01T10:34:00Z

Can you cc the next version to Linus please? He's probably best qualified
to review the i386 double fault handler because he wrote it originally.
I must admit the code always scared me a bit.

> +#ifdef CONFIG_HOTPLUG_CPU
> +static void *noinline __init_refok
> +#else
> +static inline void *__init
> +#endif

I really wonder if there isn't a cleaner way to do that :-( These init reference checks
are starting to become a major annoyance.

> +do_alloc_bootmem(unsigned long size, unsigned long align, unsigned long goal)
> +{
> + return __alloc_bootmem(size, align, goal);
> +}
> +
> /*
> * cpu_init() initializes state that is per-CPU. Some data is already
> * initialized (naturally) in the bootstrap process, such as the GDT
> @@ -659,6 +669,9 @@ void switch_to_new_gdt(void)
> void __cpuinit cpu_init(void)
> {
> int cpu = smp_processor_id();
> +#if N_EXCEPTION_TSS
> + unsigned i;
> +#endif

Would it be that bad to have the TSS even around without CONFIG_DOUBLEFAULT?

In fact I would prefer to just eliminate CONFIG_DOUBLEFAULT (imho
it always a bad idea because the amount of code it saves is miniscule) instead of
adding such a ifdef maze.

> -#ifdef CONFIG_DOUBLEFAULT
> - /* Set up doublefault TSS pointer in the GDT */
> - __set_tss_desc(cpu, GDT_ENTRY_DOUBLEFAULT_TSS, &doublefault_tss);
> +#if N_EXCEPTION_TSS
> +#if EXCEPTION_STACK_ORDER > THREAD_ORDER
> +#error Assertion failed: EXCEPTION_STACK_ORDER <= THREAD_ORDER
> +#endif

BUILD_BUG_ON would look nicer

> +
> + /* Set up exception handling stacks */
> +#ifdef CONFIG_SMP
> + if (cpu) {

If you move the code after the gs pda setup you could use smp_processor_id() and
avoid the ifdefs (on UP it expands to 0 so the optimizer would do it cleanly)

> + BUG_ON(page_count(page));
> + init_page_count(page);
> + free_pages(stack, j);
> + stack += (PAGE_SIZE << j);

In 2.4-aa I added a alloc_pages_exact() for this. I don't think such games should
be played outside page_alloc.c. I would recommend to readd alloc_pages_exact()
and then use it.

> -#define DOUBLEFAULT_STACKSIZE (1024)
> -static unsigned long doublefault_stack[DOUBLEFAULT_STACKSIZE];
> -#define STACK_START (unsigned long)(doublefault_stack+DOUBLEFAULT_STACKSIZE)
> +extern unsigned long max_low_pfn;

No externs in .c

> +#define ptr_ok(x, l) ((x) >= PAGE_OFFSET \
> + && (x) + (l) <= PAGE_OFFSET + max_low_pfn * PAGE_SIZE - 1)
>
> -#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + MAXMEM)
> +#define THREAD_INFO_FROM(x) ((struct thread_info *)((x) & ~(THREAD_SIZE - 1)))
>
> -static void doublefault_fn(void)
> +register const struct i386_hw_tss *self __asm__("ebx");

Can't you just move that to a proper argument register in assembler code?

-Andi