Re: [PATCH 1/2] x86-32: Fix crashes with CPU hotplug on AMD machines

["Roedel, Joerg" <Joerg.Roedel@amd.com>]

On Wed, Aug 04, 2010 at 07:05:47PM -0400, H. Peter Anvin wrote:
> On 08/04/2010 09:45 AM, Borislav Petkov wrote:
> > 
> >         2. Do not use swapper_pg_dir to boot secondary CPUs like 64-bit
> >         does.
> > 
> > This patch implements solution 2. It introduces a trampoline_pg_dir
> > which has the same layout as swapper_pg_dir with low_mappings. This page
> > table is used as the initial page table of the booting CPU. Later in the
> > bringup process, it switches to swapper_pg_dir and does a global TLB
> > flush. This fixes the crashes in our test cases.
> > 
> 
> I would like to keep around a page directory with the low mappings
> around -- and not use it for kernel threads -- at all times *anyway*.
> This means we can remove any current hacks that we have to do around S3
> entry and exit, for example.

Yeah, the page table with the low mappings is trampoline_pg_dir
introduced in this patch :-)

> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -328,7 +328,7 @@ ENTRY(startup_32_smp)
>  /*
>   * Enable paging
>   */
> -	movl $pa(swapper_pg_dir),%eax
> +	movl pa(initial_page_table), %eax
>  	movl %eax,%cr3		/* set the page table pointer.. */
>  	movl %cr0,%eax
>  	orl  $X86_CR0_PG,%eax
> @@ -608,6 +608,8 @@ ignore_int:
>  .align 4
>  ENTRY(initial_code)
>  	.long i386_start_kernel
> +ENTRY(initial_page_table)
> +	.long pa(swapper_pg_dir)
> 
>  /*
>   * BSS section
> @@ -623,6 +625,10 @@ ENTRY(swapper_pg_dir)
>  #endif
>  swapper_pg_fixmap:
>  	.fill 1024,4,0
> +#ifdef CONFIG_X86_TRAMPOLINE
> +ENTRY(trampoline_pg_dir)
> +	.fill 1024,4,0
> +#endif
> 
> I don't really see why this makes sense, though.  It would make more
> sense that the initial page table we set up becomes trampoline_pg_dir;
> we can then set up and change to swapper_pg_dir almost immediately.

To make sure I understand correctly, you suggest to initialize
tramponline_pg_dir in the boot sequence of the first cpu and fork
swapper_pg_dir from it later on?

> I realize this isn't how the 64-bit code works at the moment, but in a
> lot of ways I think it would be better if it did.

Yeah, may make sense. This patch already brings the 32 bit
implementation closer to the 64 bit one. On 64 bit things are somewhat
simpler because the tramponline page table can be defined at
compile-time there (contains only 2 pgd_t entries) while on 32 bit we
have to initialize it at runtime.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632