Patch to arch/x86/mm/init_32.c causes EFI-32 machines to reboot early in startup

[Hugh Dickins]

On Tue, 1 Jul 2008, Peter Jones wrote:

> Hi.  Using git bisect, I've discovered that commit
> 61165d7a035f6571c7576e7f51e7230157724c8d is the cause of 32-bit Intel Mac
> machines to reboot very early during startup when booting with EFI (but not if
> using BIOS).  The last change in that commit is:
> 
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index de236e4..ec30d10 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -438,8 +438,6 @@ void zap_low_mappings(void)
>  {
>         int i;
> 
> -       save_pg_dir();
> -
>         /*
>          * Zap initial low-memory mappings.
>          *
> @@ -663,16 +661,8 @@ void __init mem_init(void)
>                 test_wp_bit();
> 
>         cpa_init();
> -
> -       /*
> -        * Subtle. SMP is doing it's boot stuff late (because it has to
> -        * fork idle threads) - but it also needs low mappings for the
> -        * protected-mode entry to work. We zap these entries only after
> -        * the WP-bit has been tested.
> -        */
> -#ifndef CONFIG_SMP
> +       save_pg_dir();
>         zap_low_mappings();
> -#endif
>  }
> 
>  #ifdef CONFIG_MEMORY_HOTPLUG
> 
> 
> If I put the "#ifndef CONFIG_SMP" back, it no longer fails to boot.

Hmm, I'm sorry about that: thanks for letting us know.  We haven't
heard of any such failure on 2.6.26-rc, but that's probably because
Fedora is doing the wider testing for us.

> What can I do to help debug this and get it fixed?

Ooh, send me a 32-bit Intel Mac ;-?

But perhaps that'll take too long.  I would like to see the .config
involved; but if it's a CONFIG_HIGHMEM4G=y (or a CONFIG_NOHIGHMEM=y),
then please try the patch below - against 2.6.26-rc8, I hope that's
close enough to the Fedora kernel you're testing.

The EFI calls appear to be doing their own low mapping as required,
but it seems to me they've got confused between PSE and PAE: I don't
see why PSE would be relevant at this level, whereas PAE would indeed
cause each swapper_pg_dir entry to map much larger areas.  You almost
certainly have PSE active, so with the current code I believe they
would be mapping half as much as intended, so relying on early boot's
low_mappings for the other half.

If this patch does not fix it for you, then other things to try...

Does a CONFIG_SMP=n kernel boot okay with EFI?  I'd expect it to
suffer from the same problem, and it just hasn't been tried because
you've got 2 or more cores on those machines?  But confirmation or
denial would be interesting.

And I've attached a patch which in the efi_enabled case moves that
zap_low_mappings() from mem_init() to end of efi_enter_virtual_mode().

(It would be nice to merge efi_enter_virtual_mode() into mem_init(),
but I suspect that cannot be done: it looks to me like it's done
somewhat later in the start_kernel() sequence (see init/main.c) than
is necessary, but probably couldn't be done before kmem_cache_init().
Though I don't know much about it and don't have suitable box to test.)

If the patch below doesn't fix the problem, but the patch attached
does fix it, then it at least narrows the scope we have to look at,
and gives you something to run with correctly for now (the fix you
made above, putting back the #ifndef CONFIG_SMP, leaves low mappings
there forever, which caused segfaults and other weirdness later).

I've got my fingers crossed: please let us know, thanks.

Hugh

--- 2.6.26-rc8/arch/x86/kernel/efi_32.c	2008-04-17 03:49:44.000000000 +0100
+++ linux/arch/x86/kernel/efi_32.c	2008-07-02 17:02:36.000000000 +0100
@@ -49,13 +49,13 @@ void efi_call_phys_prelog(void)
 	local_irq_save(efi_rt_eflags);
 
 	/*
-	 * If I don't have PSE, I should just duplicate two entries in page
-	 * directory. If I have PSE, I just need to duplicate one entry in
+	 * If I don't have PAE, I should just duplicate two entries in page
+	 * directory. If I have PAE, I just need to duplicate one entry in
 	 * page directory.
 	 */
 	cr4 = read_cr4();
 
-	if (cr4 & X86_CR4_PSE) {
+	if (cr4 & X86_CR4_PAE) {
 		efi_bak_pg_dir_pointer[0].pgd =
 		    swapper_pg_dir[pgd_index(0)].pgd;
 		swapper_pg_dir[0].pgd =
@@ -93,7 +93,7 @@ void efi_call_phys_epilog(void)
 
 	cr4 = read_cr4();
 
-	if (cr4 & X86_CR4_PSE) {
+	if (cr4 & X86_CR4_PAE) {
 		swapper_pg_dir[pgd_index(0)].pgd =
 		    efi_bak_pg_dir_pointer[0].pgd;
 	} else {
-------------- next part --------------
--- 2.6.26-rc8/arch/x86/kernel/efi.c	2008-05-03 21:54:40.000000000 +0100
+++ linux/arch/x86/kernel/efi.c	2008-07-02 17:29:32.000000000 +0100
@@ -36,6 +36,7 @@
 #include <linux/io.h>
 #include <linux/reboot.h>
 #include <linux/bcd.h>
+#include <linux/smp.h>
 
 #include <asm/setup.h>
 #include <asm/efi.h>
@@ -485,6 +486,7 @@ void __init efi_enter_virtual_mode(void)
 		runtime_code_page_mkexec();
 	early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
 	memmap.map = NULL;
+	zap_low_mappings();
 }
 
 /*
--- 2.6.26-rc8/arch/x86/mm/init_32.c	2008-05-19 11:19:00.000000000 +0100
+++ linux/arch/x86/mm/init_32.c	2008-07-02 17:26:55.000000000 +0100
@@ -662,7 +662,8 @@ void __init mem_init(void)
 
 	cpa_init();
 	save_pg_dir();
-	zap_low_mappings();
+	if (!efi_enabled)
+		zap_low_mappings();
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG