Re: [bisected] 2.6.31 regression: fails to boot as xen guest

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 08/25/09 11:03, Pekka Enberg wrote:
> On Tue, 2009-08-25 at 19:49 +0200, Arnd Hannemann wrote:
>   
>> Hi Pekka,
>>
>> Pekka Enberg wrote:
>>     
>>> On Tue, 2009-08-25 at 18:49 +0200, Arnd Hannemann wrote:
>>>       
>>>>> Thanks for doing the bisect! Can we also see your .config also?
>>>>>           
>>>> Config for -rc7 is attached. My bisect configs were based on that
>>>>         
>>> Thanks! While we wait for the Xen people, you can try the following
>>> patch to see if we can narrow the bug down to trap_init().
>>>       
>> Yes seems to be trap_init().
>> -rc7 with this patch applied boots up to the prompt.
>>     
> Thanks for testing! Ingo, what do you think of the following patch?
> AFAICT, x86-32 is the only architecture playing with traps in mem_init()
> so this should be the safest fix for 2.6.31.
>   

Huh, interesting.  I wonder if this is the same as the problem I'd been
chasing or separate...

    J

> 			Pekka
>
> >From b739e3c3baa6312664b4ea676bdf73df27fcecbc Mon Sep 17 00:00:00 2001
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> Date: Tue, 25 Aug 2009 20:55:25 +0300
> Subject: [PATCH] x86: Move WP bit testing to trap_init()
>
> Commit 83b519e8b9572c319c8e0c615ee5dd7272856090 ("slab: setup allocators
> earlier in the boot sequence") moved trap_init() earlier in the boot
> sequence to avoid a hard crash with 32-bit x86 in mem_init().
>
> Unfortunately the change broke Xen so make trap_init() later and move
> the WP bit test from mem_init() to trap_init().
>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  arch/x86/kernel/traps.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/mm/init_32.c   |   56 ----------------------------------------------
>  init/main.c             |    2 +-
>  3 files changed, 58 insertions(+), 57 deletions(-)
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 5204332..2084408 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -906,6 +906,60 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
>  		return;
>  	do_trap(32, SIGILL, "iret exception", regs, error_code, &info);
>  }
> +
> +static noinline int do_test_wp_bit(void);
> +
> +/*
> + * Test if the WP bit works in supervisor mode. It isn't supported on 386's
> + * and also on some strange 486's. All 586+'s are OK. This used to involve
> + * black magic jumps to work around some nasty CPU bugs, but fortunately the
> + * switch to using exceptions got rid of all that.
> + */
> +static void __init test_wp_bit(void)
> +{
> +	printk(KERN_INFO
> +  "Checking if this processor honours the WP bit even in supervisor mode...");
> +
> +	/* Any page-aligned address will do, the test is non-destructive */
> +	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_READONLY);
> +	boot_cpu_data.wp_works_ok = do_test_wp_bit();
> +	clear_fixmap(FIX_WP_TEST);
> +
> +	if (!boot_cpu_data.wp_works_ok) {
> +		printk(KERN_CONT "No.\n");
> +#ifdef CONFIG_X86_WP_WORKS_OK
> +		panic(
> +  "This kernel doesn't support CPU's with broken WP. Recompile it for a 386!");
> +#endif
> +	} else {
> +		printk(KERN_CONT "Ok.\n");
> +	}
> +}
> +
> +/*
> + * This function cannot be __init, since exceptions don't work in that
> + * section.  Put this after the callers, so that it cannot be inlined.
> + */
> +static noinline int do_test_wp_bit(void)
> +{
> +	char tmp_reg;
> +	int flag;
> +
> +	__asm__ __volatile__(
> +		"	movb %0, %1	\n"
> +		"1:	movb %1, %0	\n"
> +		"	xorl %2, %2	\n"
> +		"2:			\n"
> +		_ASM_EXTABLE(1b,2b)
> +		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
> +		 "=q" (tmp_reg),
> +		 "=r" (flag)
> +		:"2" (1)
> +		:"memory");
> +
> +	return flag;
> +}
> +
>  #endif
>  
>  void __init trap_init(void)
> @@ -982,5 +1036,8 @@ void __init trap_init(void)
>  
>  #ifdef CONFIG_X86_32
>  	x86_quirk_trap_init();
> +
> +	if (boot_cpu_data.wp_works_ok < 0)
> +		test_wp_bit();
>  #endif
>  }
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 3cd7711..e22bb8a 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -54,8 +54,6 @@
>  
>  unsigned long highstart_pfn, highend_pfn;
>  
> -static noinline int do_test_wp_bit(void);
> -
>  bool __read_mostly __vmalloc_start_set = false;
>  
>  static __init void *alloc_low_page(void)
> @@ -830,33 +828,6 @@ void __init paging_init(void)
>  	zone_sizes_init();
>  }
>  
> -/*
> - * Test if the WP bit works in supervisor mode. It isn't supported on 386's
> - * and also on some strange 486's. All 586+'s are OK. This used to involve
> - * black magic jumps to work around some nasty CPU bugs, but fortunately the
> - * switch to using exceptions got rid of all that.
> - */
> -static void __init test_wp_bit(void)
> -{
> -	printk(KERN_INFO
> -  "Checking if this processor honours the WP bit even in supervisor mode...");
> -
> -	/* Any page-aligned address will do, the test is non-destructive */
> -	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_READONLY);
> -	boot_cpu_data.wp_works_ok = do_test_wp_bit();
> -	clear_fixmap(FIX_WP_TEST);
> -
> -	if (!boot_cpu_data.wp_works_ok) {
> -		printk(KERN_CONT "No.\n");
> -#ifdef CONFIG_X86_WP_WORKS_OK
> -		panic(
> -  "This kernel doesn't support CPU's with broken WP. Recompile it for a 386!");
> -#endif
> -	} else {
> -		printk(KERN_CONT "Ok.\n");
> -	}
> -}
> -
>  static struct kcore_list kcore_mem, kcore_vmalloc;
>  
>  void __init mem_init(void)
> @@ -956,9 +927,6 @@ void __init mem_init(void)
>  	BUG_ON(VMALLOC_START				>= VMALLOC_END);
>  	BUG_ON((unsigned long)high_memory		> VMALLOC_START);
>  
> -	if (boot_cpu_data.wp_works_ok < 0)
> -		test_wp_bit();
> -
>  	save_pg_dir();
>  	zap_low_mappings(true);
>  }
> @@ -975,30 +943,6 @@ int arch_add_memory(int nid, u64 start, u64 size)
>  }
>  #endif
>  
> -/*
> - * This function cannot be __init, since exceptions don't work in that
> - * section.  Put this after the callers, so that it cannot be inlined.
> - */
> -static noinline int do_test_wp_bit(void)
> -{
> -	char tmp_reg;
> -	int flag;
> -
> -	__asm__ __volatile__(
> -		"	movb %0, %1	\n"
> -		"1:	movb %1, %0	\n"
> -		"	xorl %2, %2	\n"
> -		"2:			\n"
> -		_ASM_EXTABLE(1b,2b)
> -		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
> -		 "=q" (tmp_reg),
> -		 "=r" (flag)
> -		:"2" (1)
> -		:"memory");
> -
> -	return flag;
> -}
> -
>  #ifdef CONFIG_DEBUG_RODATA
>  const int rodata_test_data = 0xC3;
>  EXPORT_SYMBOL_GPL(rodata_test_data);
> diff --git a/init/main.c b/init/main.c
> index 2d9d6bd..5c4dacb 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -603,7 +603,6 @@ asmlinkage void __init start_kernel(void)
>  	pidhash_init();
>  	vfs_caches_init_early();
>  	sort_main_extable();
> -	trap_init();
>  	mm_init();
>  	/*
>  	 * Set up the scheduler prior starting any interrupts (such as the
> @@ -621,6 +620,7 @@ asmlinkage void __init start_kernel(void)
>  				"enabled *very* early, fixing it\n");
>  		local_irq_disable();
>  	}
> +	trap_init();
>  	rcu_init();
>  	/* init some links before init_ISA_irqs() */
>  	early_irq_init();
>