[U-Boot] [PATCH v2 2/3] x86: fsp: Load GDT before calling FspInitEntry

[U-Boot] [PATCH v2 2/3] x86: fsp: Load GDT before calling FspInitEntry

[Bin Meng]

Currently the FSP execution environment GDT is setup by U-Boot in
arch/x86/cpu/start16.S, which works pretty well. But if we try to
move the FspInitEntry call a little bit later to better fit into
U-Boot's initialization sequence, FSP will fail to bring up the AP
due to #GP fault as AP's GDT is duplicated from BSP whose GDT is
now moved into CAR, and unfortunately FSP calls AP initialization
after it disables the CAR. So basically the BSP's GDT still refers
to the one in the CAR, whose content is no longer available, so
when AP starts up and loads its segment register, it blows up.

To resolve this, we load GDT before calling into FspInitEntry.
The GDT is the same one used in arch/x86/cpu/start16.S, which is
in the ROM and exists forever.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- Use CONFIG_RESET_SEG_START to avoid duplication

 arch/x86/cpu/cpu.c                | 20 ++++++++++++++++++++
 arch/x86/cpu/start16.S            |  1 +
 arch/x86/include/asm/u-boot-x86.h |  3 +++
 arch/x86/lib/fsp/fsp_support.c    |  3 +++
 4 files changed, 27 insertions(+)

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
index bb4a110..f4ebc97 100644
--- a/arch/x86/cpu/cpu.c
+++ b/arch/x86/cpu/cpu.c
@@ -164,6 +164,26 @@ void setup_gdt(gd_t *id, u64 *gdt_addr)
 	load_fs(X86_GDT_ENTRY_32BIT_FS);
 }
 
+#ifdef CONFIG_HAVE_FSP
+/*
+ * Setup FSP execution environment GDT
+ *
+ * Per Intel FSP external architecture specification, before calling any FSP
+ * APIs, we need make sure the system is in flat 32-bit mode and both the code
+ * and data selectors should have full 4GB access range. Here we reuse the one
+ * we used in arch/x86/cpu/start16.S, and reload the segement registers.
+ */
+void setup_fsp_gdt(void)
+{
+	load_gdt((const u64 *)((u32)&gdt + CONFIG_RESET_SEG_START), 4);
+	load_ds(X86_GDT_ENTRY_32BIT_DS);
+	load_ss(X86_GDT_ENTRY_32BIT_DS);
+	load_es(X86_GDT_ENTRY_32BIT_DS);
+	load_fs(X86_GDT_ENTRY_32BIT_DS);
+	load_gs(X86_GDT_ENTRY_32BIT_DS);
+}
+#endif
+
 int __weak x86_cleanup_before_linux(void)
 {
 #ifdef CONFIG_BOOTSTAGE_STASH
diff --git a/arch/x86/cpu/start16.S b/arch/x86/cpu/start16.S
index 826e2b4..a3e7ab4 100644
--- a/arch/x86/cpu/start16.S
+++ b/arch/x86/cpu/start16.S
@@ -75,6 +75,7 @@ gdt_ptr:
 
 	/* Some CPUs are picky about GDT alignment... */
 	.align	16
+.globl gdt
 gdt:
 	/*
 	 * The GDT table ...
diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
index d1d21ed..67c0098 100644
--- a/arch/x86/include/asm/u-boot-x86.h
+++ b/arch/x86/include/asm/u-boot-x86.h
@@ -8,12 +8,15 @@
 #ifndef _U_BOOT_I386_H_
 #define _U_BOOT_I386_H_	1
 
+extern u32 gdt;
+
 /* cpu/.../cpu.c */
 int arch_cpu_init(void);
 int x86_cpu_init_f(void);
 int cpu_init_f(void);
 void init_gd(gd_t *id, u64 *gdt_addr);
 void setup_gdt(gd_t *id, u64 *gdt_addr);
+void setup_fsp_gdt(void);
 int init_cache(void);
 int cleanup_before_linux(void);
 
diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
index 5809235..4585166 100644
--- a/arch/x86/lib/fsp/fsp_support.c
+++ b/arch/x86/lib/fsp/fsp_support.c
@@ -173,6 +173,9 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
 
 	post_code(POST_PRE_MRC);
 
+	/* Load GDT for FSP */
+	setup_fsp_gdt();
+
 	/*
 	 * Use ASM code to ensure the register value in EAX & ECX
 	 * will be passed into BlContinuationFunc
-- 
1.8.2.1

[U-Boot] [PATCH v2 3/3] x86: fsp: Move FspInitEntry call to board_init_f()

[Bin Meng]

The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far.
It worked pretty well but looks not that good. Apart from doing too
much work than just enabling CAR, it cannot read the configuration
data from device tree at that time. Now we want to move it a little
bit later as part of init_sequence_f[] being called by board_init_f().
This way it looks and works better in the U-Boot initialization path.

Due to FSP's design, after calling FspInitEntry it will not return to
its caller, instead it jumps to a continuation function which is given
by bootloader with a new stack in system memory. The original stack in
the CAR is gone, but its content is perserved by FSP and described by
a bootloader temporary memory HOB. Technically we can recover anything
we had before in the previous stack, but that is way too complicated.
To make life much easier, in the FSP continuation routine we just
simply call fsp_init_done() and jump back to car_init_ret() to redo
the whole board_init_f() initialization, but this time with a non-zero
HOB list pointer saved in U-Boot's global data so that we can bypass
the FspInitEntry for the second time.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Acked-by: Simon Glass <sjg@chromium.org>
Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>

---

Changes in v2: None

 arch/x86/cpu/start.S              |  6 +++++-
 arch/x86/include/asm/u-boot-x86.h |  4 ++++
 arch/x86/lib/fsp/fsp_car.S        | 26 +++++---------------------
 arch/x86/lib/fsp/fsp_common.c     |  8 ++++++++
 common/board_f.c                  |  3 +++
 5 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
index 2e5f9da..00e585e 100644
--- a/arch/x86/cpu/start.S
+++ b/arch/x86/cpu/start.S
@@ -116,12 +116,16 @@ car_init_ret:
 	rep	stosb
 
 #ifdef CONFIG_HAVE_FSP
+	test	%esi, %esi
+	jz	skip_hob
+
 	/* Store HOB list */
 	movl	%esp, %edx
 	addl	$GD_HOB_LIST, %edx
 	movl	%esi, (%edx)
-#endif
 
+skip_hob:
+#endif
 	/* Setup first parameter to setup_gdt, pointer to global_data */
 	movl	%esp, %eax
 
diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
index 67c0098..5d8a96d 100644
--- a/arch/x86/include/asm/u-boot-x86.h
+++ b/arch/x86/include/asm/u-boot-x86.h
@@ -52,6 +52,10 @@ u32 isa_map_rom(u32 bus_addr, int size);
 /* arch/x86/lib/... */
 int video_bios_init(void);
 
+#ifdef CONFIG_HAVE_FSP
+int x86_fsp_init(void);
+#endif
+
 void	board_init_f_r_trampoline(ulong) __attribute__ ((noreturn));
 void	board_init_f_r(void) __attribute__ ((noreturn));
 
diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S
index 5e09568..afbf3f9 100644
--- a/arch/x86/lib/fsp/fsp_car.S
+++ b/arch/x86/lib/fsp/fsp_car.S
@@ -56,28 +56,10 @@ temp_ram_init_ret:
 
 	/* stack grows down from top of CAR */
 	movl	%edx, %esp
+	subl	$4, %esp
 
-	/*
-	 * TODO:
-	 *
-	 * According to FSP architecture spec, the fsp_init() will not return
-	 * to its caller, instead it requires the bootloader to provide a
-	 * so-called continuation function to pass into the FSP as a parameter
-	 * of fsp_init, and fsp_init() will call that continuation function
-	 * directly.
-	 *
-	 * The call to fsp_init() may need to be moved out of the car_init()
-	 * to cpu_init_f() with the help of some inline assembly codes.
-	 * Note there is another issue that fsp_init() will setup another stack
-	 * using the fsp_init parameter stack_top after DRAM is initialized,
-	 * which means any data on the previous stack (on the CAR) gets lost
-	 * (ie: U-Boot global_data). FSP is supposed to support such scenario,
-	 * however it does not work. This should be revisited in the future.
-	 */
-	movl	$CONFIG_FSP_TEMP_RAM_ADDR, %eax
-	xorl	%edx, %edx
-	xorl	%ecx, %ecx
-	call	fsp_init
+	xor	%esi, %esi
+	jmp	car_init_done
 
 .global fsp_init_done
 fsp_init_done:
@@ -86,6 +68,8 @@ fsp_init_done:
 	 * Save eax to esi temporarily.
 	 */
 	movl	%eax, %esi
+
+car_init_done:
 	/*
 	 * Re-initialize the ebp (BIST) to zero, as we already reach here
 	 * which means we passed BIST testing before.
diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
index 001494d..5b25632 100644
--- a/arch/x86/lib/fsp/fsp_common.c
+++ b/arch/x86/lib/fsp/fsp_common.c
@@ -46,3 +46,11 @@ void board_final_cleanup(void)
 
 	return;
 }
+
+int x86_fsp_init(void)
+{
+	if (!gd->arch.hob_list)
+		fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL);
+
+	return 0;
+}
diff --git a/common/board_f.c b/common/board_f.c
index fbbad1b..21be26f 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = {
 #ifdef CONFIG_OF_CONTROL
 	fdtdec_setup,
 #endif
+#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP)
+	x86_fsp_init,
+#endif
 #ifdef CONFIG_TRACE
 	trace_early_init,
 #endif
-- 
1.8.2.1

[U-Boot] [PATCH v2 2/3] x86: fsp: Load GDT before calling FspInitEntry

[Andrew Bradford]

On 06/04 18:28, Bin Meng wrote:
> Currently the FSP execution environment GDT is setup by U-Boot in
> arch/x86/cpu/start16.S, which works pretty well. But if we try to
> move the FspInitEntry call a little bit later to better fit into
> U-Boot's initialization sequence, FSP will fail to bring up the AP
> due to #GP fault as AP's GDT is duplicated from BSP whose GDT is
> now moved into CAR, and unfortunately FSP calls AP initialization
> after it disables the CAR. So basically the BSP's GDT still refers
> to the one in the CAR, whose content is no longer available, so
> when AP starts up and loads its segment register, it blows up.
> 
> To resolve this, we load GDT before calling into FspInitEntry.
> The GDT is the same one used in arch/x86/cpu/start16.S, which is
> in the ROM and exists forever.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> 
> ---
> 
> Changes in v2:
> - Use CONFIG_RESET_SEG_START to avoid duplication
> 
>  arch/x86/cpu/cpu.c                | 20 ++++++++++++++++++++
>  arch/x86/cpu/start16.S            |  1 +
>  arch/x86/include/asm/u-boot-x86.h |  3 +++
>  arch/x86/lib/fsp/fsp_support.c    |  3 +++
>  4 files changed, 27 insertions(+)

Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>

[U-Boot] [PATCH v2 3/3] x86: fsp: Move FspInitEntry call to board_init_f()

[Andrew Bradford]

On 06/04 18:28, Bin Meng wrote:
> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far.
> It worked pretty well but looks not that good. Apart from doing too
> much work than just enabling CAR, it cannot read the configuration
> data from device tree at that time. Now we want to move it a little
> bit later as part of init_sequence_f[] being called by board_init_f().
> This way it looks and works better in the U-Boot initialization path.
> 
> Due to FSP's design, after calling FspInitEntry it will not return to
> its caller, instead it jumps to a continuation function which is given
> by bootloader with a new stack in system memory. The original stack in
> the CAR is gone, but its content is perserved by FSP and described by
> a bootloader temporary memory HOB. Technically we can recover anything
> we had before in the previous stack, but that is way too complicated.
> To make life much easier, in the FSP continuation routine we just
> simply call fsp_init_done() and jump back to car_init_ret() to redo
> the whole board_init_f() initialization, but this time with a non-zero
> HOB list pointer saved in U-Boot's global data so that we can bypass
> the FspInitEntry for the second time.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>
> 
> ---
> 
> Changes in v2: None
> 
>  arch/x86/cpu/start.S              |  6 +++++-
>  arch/x86/include/asm/u-boot-x86.h |  4 ++++
>  arch/x86/lib/fsp/fsp_car.S        | 26 +++++---------------------
>  arch/x86/lib/fsp/fsp_common.c     |  8 ++++++++
>  common/board_f.c                  |  3 +++
>  5 files changed, 25 insertions(+), 22 deletions(-)

Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>

[U-Boot] [PATCH v2 3/3] x86: fsp: Move FspInitEntry call to board_init_f()

[Simon Glass]

On 4 June 2015 at 04:28, Bin Meng <bmeng.cn@gmail.com> wrote:
> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far.
> It worked pretty well but looks not that good. Apart from doing too
> much work than just enabling CAR, it cannot read the configuration
> data from device tree at that time. Now we want to move it a little
> bit later as part of init_sequence_f[] being called by board_init_f().
> This way it looks and works better in the U-Boot initialization path.
>
> Due to FSP's design, after calling FspInitEntry it will not return to
> its caller, instead it jumps to a continuation function which is given
> by bootloader with a new stack in system memory. The original stack in
> the CAR is gone, but its content is perserved by FSP and described by
> a bootloader temporary memory HOB. Technically we can recover anything
> we had before in the previous stack, but that is way too complicated.
> To make life much easier, in the FSP continuation routine we just
> simply call fsp_init_done() and jump back to car_init_ret() to redo
> the whole board_init_f() initialization, but this time with a non-zero
> HOB list pointer saved in U-Boot's global data so that we can bypass
> the FspInitEntry for the second time.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>
>

Tested on Minnowboard MAX:
Tested-by: Simon Glass <sjg@chromium.org>

> ---
>
> Changes in v2: None
>
>  arch/x86/cpu/start.S              |  6 +++++-
>  arch/x86/include/asm/u-boot-x86.h |  4 ++++
>  arch/x86/lib/fsp/fsp_car.S        | 26 +++++---------------------
>  arch/x86/lib/fsp/fsp_common.c     |  8 ++++++++
>  common/board_f.c                  |  3 +++
>  5 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
> index 2e5f9da..00e585e 100644
> --- a/arch/x86/cpu/start.S
> +++ b/arch/x86/cpu/start.S
> @@ -116,12 +116,16 @@ car_init_ret:
>         rep     stosb
>
>  #ifdef CONFIG_HAVE_FSP
> +       test    %esi, %esi
> +       jz      skip_hob
> +
>         /* Store HOB list */
>         movl    %esp, %edx
>         addl    $GD_HOB_LIST, %edx
>         movl    %esi, (%edx)
> -#endif
>
> +skip_hob:
> +#endif
>         /* Setup first parameter to setup_gdt, pointer to global_data */
>         movl    %esp, %eax
>
> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
> index 67c0098..5d8a96d 100644
> --- a/arch/x86/include/asm/u-boot-x86.h
> +++ b/arch/x86/include/asm/u-boot-x86.h
> @@ -52,6 +52,10 @@ u32 isa_map_rom(u32 bus_addr, int size);
>  /* arch/x86/lib/... */
>  int video_bios_init(void);
>
> +#ifdef CONFIG_HAVE_FSP
> +int x86_fsp_init(void);
> +#endif

Can we drop the #ifdef here?

> +
>  void   board_init_f_r_trampoline(ulong) __attribute__ ((noreturn));
>  void   board_init_f_r(void) __attribute__ ((noreturn));
>
> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S
> index 5e09568..afbf3f9 100644
> --- a/arch/x86/lib/fsp/fsp_car.S
> +++ b/arch/x86/lib/fsp/fsp_car.S
> @@ -56,28 +56,10 @@ temp_ram_init_ret:
>
>         /* stack grows down from top of CAR */
>         movl    %edx, %esp
> +       subl    $4, %esp
>
> -       /*
> -        * TODO:
> -        *
> -        * According to FSP architecture spec, the fsp_init() will not return
> -        * to its caller, instead it requires the bootloader to provide a
> -        * so-called continuation function to pass into the FSP as a parameter
> -        * of fsp_init, and fsp_init() will call that continuation function
> -        * directly.
> -        *
> -        * The call to fsp_init() may need to be moved out of the car_init()
> -        * to cpu_init_f() with the help of some inline assembly codes.
> -        * Note there is another issue that fsp_init() will setup another stack
> -        * using the fsp_init parameter stack_top after DRAM is initialized,
> -        * which means any data on the previous stack (on the CAR) gets lost
> -        * (ie: U-Boot global_data). FSP is supposed to support such scenario,
> -        * however it does not work. This should be revisited in the future.
> -        */
> -       movl    $CONFIG_FSP_TEMP_RAM_ADDR, %eax
> -       xorl    %edx, %edx
> -       xorl    %ecx, %ecx
> -       call    fsp_init
> +       xor     %esi, %esi
> +       jmp     car_init_done
>
>  .global fsp_init_done
>  fsp_init_done:
> @@ -86,6 +68,8 @@ fsp_init_done:
>          * Save eax to esi temporarily.
>          */
>         movl    %eax, %esi
> +
> +car_init_done:
>         /*
>          * Re-initialize the ebp (BIST) to zero, as we already reach here
>          * which means we passed BIST testing before.
> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
> index 001494d..5b25632 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -46,3 +46,11 @@ void board_final_cleanup(void)
>
>         return;
>  }
> +
> +int x86_fsp_init(void)
> +{
> +       if (!gd->arch.hob_list)
> +               fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL);
> +
> +       return 0;
> +}
> diff --git a/common/board_f.c b/common/board_f.c
> index fbbad1b..21be26f 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = {
>  #ifdef CONFIG_OF_CONTROL
>         fdtdec_setup,
>  #endif
> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP)
> +       x86_fsp_init,
> +#endif

Would it be possible to put this into arch_cpu_init()? Or do you think
it is better to have a separate call?

>  #ifdef CONFIG_TRACE
>         trace_early_init,
>  #endif
> --
> 1.8.2.1
>

Regards,
Simon

[U-Boot] [PATCH v2 2/3] x86: fsp: Load GDT before calling FspInitEntry

[Simon Glass]

Hi Bin,

On 4 June 2015 at 04:28, Bin Meng <bmeng.cn@gmail.com> wrote:
> Currently the FSP execution environment GDT is setup by U-Boot in
> arch/x86/cpu/start16.S, which works pretty well. But if we try to
> move the FspInitEntry call a little bit later to better fit into
> U-Boot's initialization sequence, FSP will fail to bring up the AP
> due to #GP fault as AP's GDT is duplicated from BSP whose GDT is
> now moved into CAR, and unfortunately FSP calls AP initialization
> after it disables the CAR. So basically the BSP's GDT still refers
> to the one in the CAR, whose content is no longer available, so
> when AP starts up and loads its segment register, it blows up.
>
> To resolve this, we load GDT before calling into FspInitEntry.
> The GDT is the same one used in arch/x86/cpu/start16.S, which is
> in the ROM and exists forever.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Tested on Minnowboard MAX:
Tested-by: Simon Glass <sjg@chromium.org>

>
> ---
>
> Changes in v2:
> - Use CONFIG_RESET_SEG_START to avoid duplication
>
>  arch/x86/cpu/cpu.c                | 20 ++++++++++++++++++++
>  arch/x86/cpu/start16.S            |  1 +
>  arch/x86/include/asm/u-boot-x86.h |  3 +++
>  arch/x86/lib/fsp/fsp_support.c    |  3 +++
>  4 files changed, 27 insertions(+)
>
> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
> index bb4a110..f4ebc97 100644
> --- a/arch/x86/cpu/cpu.c
> +++ b/arch/x86/cpu/cpu.c
> @@ -164,6 +164,26 @@ void setup_gdt(gd_t *id, u64 *gdt_addr)
>         load_fs(X86_GDT_ENTRY_32BIT_FS);
>  }
>
> +#ifdef CONFIG_HAVE_FSP
> +/*
> + * Setup FSP execution environment GDT
> + *
> + * Per Intel FSP external architecture specification, before calling any FSP
> + * APIs, we need make sure the system is in flat 32-bit mode and both the code
> + * and data selectors should have full 4GB access range. Here we reuse the one
> + * we used in arch/x86/cpu/start16.S, and reload the segement registers.
> + */
> +void setup_fsp_gdt(void)
> +{
> +       load_gdt((const u64 *)((u32)&gdt + CONFIG_RESET_SEG_START), 4);
> +       load_ds(X86_GDT_ENTRY_32BIT_DS);
> +       load_ss(X86_GDT_ENTRY_32BIT_DS);
> +       load_es(X86_GDT_ENTRY_32BIT_DS);
> +       load_fs(X86_GDT_ENTRY_32BIT_DS);
> +       load_gs(X86_GDT_ENTRY_32BIT_DS);
> +}
> +#endif
> +
>  int __weak x86_cleanup_before_linux(void)
>  {
>  #ifdef CONFIG_BOOTSTAGE_STASH
> diff --git a/arch/x86/cpu/start16.S b/arch/x86/cpu/start16.S
> index 826e2b4..a3e7ab4 100644
> --- a/arch/x86/cpu/start16.S
> +++ b/arch/x86/cpu/start16.S
> @@ -75,6 +75,7 @@ gdt_ptr:
>
>         /* Some CPUs are picky about GDT alignment... */
>         .align  16
> +.globl gdt
>  gdt:

Could we rename this to gdt_initial or something like that? To me, gdt
is a bit vague for an exported symbol. We need to use a name that
indicates it is only used at the start.

>         /*
>          * The GDT table ...
> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
> index d1d21ed..67c0098 100644
> --- a/arch/x86/include/asm/u-boot-x86.h
> +++ b/arch/x86/include/asm/u-boot-x86.h
> @@ -8,12 +8,15 @@
>  #ifndef _U_BOOT_I386_H_
>  #define _U_BOOT_I386_H_        1
>
> +extern u32 gdt;

Perhaps this should be declared as char gdt[] so you don't need to
take its address later? See asm/linkage.h for how they do it.

> +
>  /* cpu/.../cpu.c */
>  int arch_cpu_init(void);
>  int x86_cpu_init_f(void);
>  int cpu_init_f(void);
>  void init_gd(gd_t *id, u64 *gdt_addr);
>  void setup_gdt(gd_t *id, u64 *gdt_addr);
> +void setup_fsp_gdt(void);

Please add function comment.

>  int init_cache(void);
>  int cleanup_before_linux(void);
>
> diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
> index 5809235..4585166 100644
> --- a/arch/x86/lib/fsp/fsp_support.c
> +++ b/arch/x86/lib/fsp/fsp_support.c
> @@ -173,6 +173,9 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>
>         post_code(POST_PRE_MRC);
>
> +       /* Load GDT for FSP */
> +       setup_fsp_gdt();
> +
>         /*
>          * Use ASM code to ensure the register value in EAX & ECX
>          * will be passed into BlContinuationFunc
> --
> 1.8.2.1
>

Regards,
Simon

[U-Boot] [PATCH v2 3/3] x86: fsp: Move FspInitEntry call to board_init_f()

[Bin Meng]

Hi Simon,

On Fri, Jun 5, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote:
> On 4 June 2015 at 04:28, Bin Meng <bmeng.cn@gmail.com> wrote:
>> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far.
>> It worked pretty well but looks not that good. Apart from doing too
>> much work than just enabling CAR, it cannot read the configuration
>> data from device tree at that time. Now we want to move it a little
>> bit later as part of init_sequence_f[] being called by board_init_f().
>> This way it looks and works better in the U-Boot initialization path.
>>
>> Due to FSP's design, after calling FspInitEntry it will not return to
>> its caller, instead it jumps to a continuation function which is given
>> by bootloader with a new stack in system memory. The original stack in
>> the CAR is gone, but its content is perserved by FSP and described by
>> a bootloader temporary memory HOB. Technically we can recover anything
>> we had before in the previous stack, but that is way too complicated.
>> To make life much easier, in the FSP continuation routine we just
>> simply call fsp_init_done() and jump back to car_init_ret() to redo
>> the whole board_init_f() initialization, but this time with a non-zero
>> HOB list pointer saved in U-Boot's global data so that we can bypass
>> the FspInitEntry for the second time.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> Acked-by: Simon Glass <sjg@chromium.org>
>> Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>
>>
>
> Tested on Minnowboard MAX:
> Tested-by: Simon Glass <sjg@chromium.org>
>
>> ---
>>
>> Changes in v2: None
>>
>>  arch/x86/cpu/start.S              |  6 +++++-
>>  arch/x86/include/asm/u-boot-x86.h |  4 ++++
>>  arch/x86/lib/fsp/fsp_car.S        | 26 +++++---------------------
>>  arch/x86/lib/fsp/fsp_common.c     |  8 ++++++++
>>  common/board_f.c                  |  3 +++
>>  5 files changed, 25 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
>> index 2e5f9da..00e585e 100644
>> --- a/arch/x86/cpu/start.S
>> +++ b/arch/x86/cpu/start.S
>> @@ -116,12 +116,16 @@ car_init_ret:
>>         rep     stosb
>>
>>  #ifdef CONFIG_HAVE_FSP
>> +       test    %esi, %esi
>> +       jz      skip_hob
>> +
>>         /* Store HOB list */
>>         movl    %esp, %edx
>>         addl    $GD_HOB_LIST, %edx
>>         movl    %esi, (%edx)
>> -#endif
>>
>> +skip_hob:
>> +#endif
>>         /* Setup first parameter to setup_gdt, pointer to global_data */
>>         movl    %esp, %eax
>>
>> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
>> index 67c0098..5d8a96d 100644
>> --- a/arch/x86/include/asm/u-boot-x86.h
>> +++ b/arch/x86/include/asm/u-boot-x86.h
>> @@ -52,6 +52,10 @@ u32 isa_map_rom(u32 bus_addr, int size);
>>  /* arch/x86/lib/... */
>>  int video_bios_init(void);
>>
>> +#ifdef CONFIG_HAVE_FSP
>> +int x86_fsp_init(void);
>> +#endif
>
> Can we drop the #ifdef here?

Yes, in v3.

>> +
>>  void   board_init_f_r_trampoline(ulong) __attribute__ ((noreturn));
>>  void   board_init_f_r(void) __attribute__ ((noreturn));
>>
>> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S
>> index 5e09568..afbf3f9 100644
>> --- a/arch/x86/lib/fsp/fsp_car.S
>> +++ b/arch/x86/lib/fsp/fsp_car.S
>> @@ -56,28 +56,10 @@ temp_ram_init_ret:
>>
>>         /* stack grows down from top of CAR */
>>         movl    %edx, %esp
>> +       subl    $4, %esp
>>
>> -       /*
>> -        * TODO:
>> -        *
>> -        * According to FSP architecture spec, the fsp_init() will not return
>> -        * to its caller, instead it requires the bootloader to provide a
>> -        * so-called continuation function to pass into the FSP as a parameter
>> -        * of fsp_init, and fsp_init() will call that continuation function
>> -        * directly.
>> -        *
>> -        * The call to fsp_init() may need to be moved out of the car_init()
>> -        * to cpu_init_f() with the help of some inline assembly codes.
>> -        * Note there is another issue that fsp_init() will setup another stack
>> -        * using the fsp_init parameter stack_top after DRAM is initialized,
>> -        * which means any data on the previous stack (on the CAR) gets lost
>> -        * (ie: U-Boot global_data). FSP is supposed to support such scenario,
>> -        * however it does not work. This should be revisited in the future.
>> -        */
>> -       movl    $CONFIG_FSP_TEMP_RAM_ADDR, %eax
>> -       xorl    %edx, %edx
>> -       xorl    %ecx, %ecx
>> -       call    fsp_init
>> +       xor     %esi, %esi
>> +       jmp     car_init_done
>>
>>  .global fsp_init_done
>>  fsp_init_done:
>> @@ -86,6 +68,8 @@ fsp_init_done:
>>          * Save eax to esi temporarily.
>>          */
>>         movl    %eax, %esi
>> +
>> +car_init_done:
>>         /*
>>          * Re-initialize the ebp (BIST) to zero, as we already reach here
>>          * which means we passed BIST testing before.
>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>> index 001494d..5b25632 100644
>> --- a/arch/x86/lib/fsp/fsp_common.c
>> +++ b/arch/x86/lib/fsp/fsp_common.c
>> @@ -46,3 +46,11 @@ void board_final_cleanup(void)
>>
>>         return;
>>  }
>> +
>> +int x86_fsp_init(void)
>> +{
>> +       if (!gd->arch.hob_list)
>> +               fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL);
>> +
>> +       return 0;
>> +}
>> diff --git a/common/board_f.c b/common/board_f.c
>> index fbbad1b..21be26f 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = {
>>  #ifdef CONFIG_OF_CONTROL
>>         fdtdec_setup,
>>  #endif
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP)
>> +       x86_fsp_init,
>> +#endif
>
> Would it be possible to put this into arch_cpu_init()? Or do you think
> it is better to have a separate call?
>

I was thinking to do that in arch_cpu_init previously, but I changed
my idea mainly for:
- I need modify every cpu codes which uses FSP. So far there are two,
not a big deal :)
- Since x86_fsp_init() will jump back to call board_init_f() again, I
feel putting it as early as possible right after fdtdec_setup() could
save some boot time, like no need to execute trace_early_init() and
initf_malloc() which are meaningless, while still could gain access to
device tree.

>>  #ifdef CONFIG_TRACE
>>         trace_early_init,
>>  #endif
>> --

Regards,
Bin

[U-Boot] [PATCH v2 2/3] x86: fsp: Load GDT before calling FspInitEntry

[Bin Meng]

Hi Simon,

On Fri, Jun 5, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 4 June 2015 at 04:28, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Currently the FSP execution environment GDT is setup by U-Boot in
>> arch/x86/cpu/start16.S, which works pretty well. But if we try to
>> move the FspInitEntry call a little bit later to better fit into
>> U-Boot's initialization sequence, FSP will fail to bring up the AP
>> due to #GP fault as AP's GDT is duplicated from BSP whose GDT is
>> now moved into CAR, and unfortunately FSP calls AP initialization
>> after it disables the CAR. So basically the BSP's GDT still refers
>> to the one in the CAR, whose content is no longer available, so
>> when AP starts up and loads its segment register, it blows up.
>>
>> To resolve this, we load GDT before calling into FspInitEntry.
>> The GDT is the same one used in arch/x86/cpu/start16.S, which is
>> in the ROM and exists forever.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> Tested on Minnowboard MAX:
> Tested-by: Simon Glass <sjg@chromium.org>
>
>>
>> ---
>>
>> Changes in v2:
>> - Use CONFIG_RESET_SEG_START to avoid duplication
>>
>>  arch/x86/cpu/cpu.c                | 20 ++++++++++++++++++++
>>  arch/x86/cpu/start16.S            |  1 +
>>  arch/x86/include/asm/u-boot-x86.h |  3 +++
>>  arch/x86/lib/fsp/fsp_support.c    |  3 +++
>>  4 files changed, 27 insertions(+)
>>
>> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
>> index bb4a110..f4ebc97 100644
>> --- a/arch/x86/cpu/cpu.c
>> +++ b/arch/x86/cpu/cpu.c
>> @@ -164,6 +164,26 @@ void setup_gdt(gd_t *id, u64 *gdt_addr)
>>         load_fs(X86_GDT_ENTRY_32BIT_FS);
>>  }
>>
>> +#ifdef CONFIG_HAVE_FSP
>> +/*
>> + * Setup FSP execution environment GDT
>> + *
>> + * Per Intel FSP external architecture specification, before calling any FSP
>> + * APIs, we need make sure the system is in flat 32-bit mode and both the code
>> + * and data selectors should have full 4GB access range. Here we reuse the one
>> + * we used in arch/x86/cpu/start16.S, and reload the segement registers.
>> + */
>> +void setup_fsp_gdt(void)
>> +{
>> +       load_gdt((const u64 *)((u32)&gdt + CONFIG_RESET_SEG_START), 4);
>> +       load_ds(X86_GDT_ENTRY_32BIT_DS);
>> +       load_ss(X86_GDT_ENTRY_32BIT_DS);
>> +       load_es(X86_GDT_ENTRY_32BIT_DS);
>> +       load_fs(X86_GDT_ENTRY_32BIT_DS);
>> +       load_gs(X86_GDT_ENTRY_32BIT_DS);
>> +}
>> +#endif
>> +
>>  int __weak x86_cleanup_before_linux(void)
>>  {
>>  #ifdef CONFIG_BOOTSTAGE_STASH
>> diff --git a/arch/x86/cpu/start16.S b/arch/x86/cpu/start16.S
>> index 826e2b4..a3e7ab4 100644
>> --- a/arch/x86/cpu/start16.S
>> +++ b/arch/x86/cpu/start16.S
>> @@ -75,6 +75,7 @@ gdt_ptr:
>>
>>         /* Some CPUs are picky about GDT alignment... */
>>         .align  16
>> +.globl gdt
>>  gdt:
>
> Could we rename this to gdt_initial or something like that? To me, gdt
> is a bit vague for an exported symbol. We need to use a name that
> indicates it is only used at the start.

Agreed. How about gdt_rom?

>>         /*
>>          * The GDT table ...
>> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
>> index d1d21ed..67c0098 100644
>> --- a/arch/x86/include/asm/u-boot-x86.h
>> +++ b/arch/x86/include/asm/u-boot-x86.h
>> @@ -8,12 +8,15 @@
>>  #ifndef _U_BOOT_I386_H_
>>  #define _U_BOOT_I386_H_        1
>>
>> +extern u32 gdt;
>
> Perhaps this should be declared as char gdt[] so you don't need to
> take its address later? See asm/linkage.h for how they do it.

Good idea!

>> +
>>  /* cpu/.../cpu.c */
>>  int arch_cpu_init(void);
>>  int x86_cpu_init_f(void);
>>  int cpu_init_f(void);
>>  void init_gd(gd_t *id, u64 *gdt_addr);
>>  void setup_gdt(gd_t *id, u64 *gdt_addr);
>> +void setup_fsp_gdt(void);
>
> Please add function comment.

Sure.

>>  int init_cache(void);
>>  int cleanup_before_linux(void);
>>
>> diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
>> index 5809235..4585166 100644
>> --- a/arch/x86/lib/fsp/fsp_support.c
>> +++ b/arch/x86/lib/fsp/fsp_support.c
>> @@ -173,6 +173,9 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>>
>>         post_code(POST_PRE_MRC);
>>
>> +       /* Load GDT for FSP */
>> +       setup_fsp_gdt();
>> +
>>         /*
>>          * Use ASM code to ensure the register value in EAX & ECX
>>          * will be passed into BlContinuationFunc
>> --

Regards,
Bin

[U-Boot] [PATCH v2 3/3] x86: fsp: Move FspInitEntry call to board_init_f()

[Simon Glass]

Hi Bin,

On 5 June 2015 at 09:57, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jun 5, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote:
>> On 4 June 2015 at 04:28, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far.
>>> It worked pretty well but looks not that good. Apart from doing too
>>> much work than just enabling CAR, it cannot read the configuration
>>> data from device tree at that time. Now we want to move it a little
>>> bit later as part of init_sequence_f[] being called by board_init_f().
>>> This way it looks and works better in the U-Boot initialization path.
>>>
>>> Due to FSP's design, after calling FspInitEntry it will not return to
>>> its caller, instead it jumps to a continuation function which is given
>>> by bootloader with a new stack in system memory. The original stack in
>>> the CAR is gone, but its content is perserved by FSP and described by
>>> a bootloader temporary memory HOB. Technically we can recover anything
>>> we had before in the previous stack, but that is way too complicated.
>>> To make life much easier, in the FSP continuation routine we just
>>> simply call fsp_init_done() and jump back to car_init_ret() to redo
>>> the whole board_init_f() initialization, but this time with a non-zero
>>> HOB list pointer saved in U-Boot's global data so that we can bypass
>>> the FspInitEntry for the second time.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> Acked-by: Simon Glass <sjg@chromium.org>
>>> Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com>
>>>
>>
>> Tested on Minnowboard MAX:
>> Tested-by: Simon Glass <sjg@chromium.org>
>>
>>> ---
>>>
>>> Changes in v2: None
>>>
>>>  arch/x86/cpu/start.S              |  6 +++++-
>>>  arch/x86/include/asm/u-boot-x86.h |  4 ++++
>>>  arch/x86/lib/fsp/fsp_car.S        | 26 +++++---------------------
>>>  arch/x86/lib/fsp/fsp_common.c     |  8 ++++++++
>>>  common/board_f.c                  |  3 +++
>>>  5 files changed, 25 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
>>> index 2e5f9da..00e585e 100644
>>> --- a/arch/x86/cpu/start.S
>>> +++ b/arch/x86/cpu/start.S
>>> @@ -116,12 +116,16 @@ car_init_ret:
>>>         rep     stosb
>>>
>>>  #ifdef CONFIG_HAVE_FSP
>>> +       test    %esi, %esi
>>> +       jz      skip_hob
>>> +
>>>         /* Store HOB list */
>>>         movl    %esp, %edx
>>>         addl    $GD_HOB_LIST, %edx
>>>         movl    %esi, (%edx)
>>> -#endif
>>>
>>> +skip_hob:
>>> +#endif
>>>         /* Setup first parameter to setup_gdt, pointer to global_data */
>>>         movl    %esp, %eax
>>>
>>> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
>>> index 67c0098..5d8a96d 100644
>>> --- a/arch/x86/include/asm/u-boot-x86.h
>>> +++ b/arch/x86/include/asm/u-boot-x86.h
>>> @@ -52,6 +52,10 @@ u32 isa_map_rom(u32 bus_addr, int size);
>>>  /* arch/x86/lib/... */
>>>  int video_bios_init(void);
>>>
>>> +#ifdef CONFIG_HAVE_FSP
>>> +int x86_fsp_init(void);
>>> +#endif
>>
>> Can we drop the #ifdef here?
>
> Yes, in v3.
>
>>> +
>>>  void   board_init_f_r_trampoline(ulong) __attribute__ ((noreturn));
>>>  void   board_init_f_r(void) __attribute__ ((noreturn));
>>>
>>> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S
>>> index 5e09568..afbf3f9 100644
>>> --- a/arch/x86/lib/fsp/fsp_car.S
>>> +++ b/arch/x86/lib/fsp/fsp_car.S
>>> @@ -56,28 +56,10 @@ temp_ram_init_ret:
>>>
>>>         /* stack grows down from top of CAR */
>>>         movl    %edx, %esp
>>> +       subl    $4, %esp
>>>
>>> -       /*
>>> -        * TODO:
>>> -        *
>>> -        * According to FSP architecture spec, the fsp_init() will not return
>>> -        * to its caller, instead it requires the bootloader to provide a
>>> -        * so-called continuation function to pass into the FSP as a parameter
>>> -        * of fsp_init, and fsp_init() will call that continuation function
>>> -        * directly.
>>> -        *
>>> -        * The call to fsp_init() may need to be moved out of the car_init()
>>> -        * to cpu_init_f() with the help of some inline assembly codes.
>>> -        * Note there is another issue that fsp_init() will setup another stack
>>> -        * using the fsp_init parameter stack_top after DRAM is initialized,
>>> -        * which means any data on the previous stack (on the CAR) gets lost
>>> -        * (ie: U-Boot global_data). FSP is supposed to support such scenario,
>>> -        * however it does not work. This should be revisited in the future.
>>> -        */
>>> -       movl    $CONFIG_FSP_TEMP_RAM_ADDR, %eax
>>> -       xorl    %edx, %edx
>>> -       xorl    %ecx, %ecx
>>> -       call    fsp_init
>>> +       xor     %esi, %esi
>>> +       jmp     car_init_done
>>>
>>>  .global fsp_init_done
>>>  fsp_init_done:
>>> @@ -86,6 +68,8 @@ fsp_init_done:
>>>          * Save eax to esi temporarily.
>>>          */
>>>         movl    %eax, %esi
>>> +
>>> +car_init_done:
>>>         /*
>>>          * Re-initialize the ebp (BIST) to zero, as we already reach here
>>>          * which means we passed BIST testing before.
>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>> index 001494d..5b25632 100644
>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>> @@ -46,3 +46,11 @@ void board_final_cleanup(void)
>>>
>>>         return;
>>>  }
>>> +
>>> +int x86_fsp_init(void)
>>> +{
>>> +       if (!gd->arch.hob_list)
>>> +               fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL);
>>> +
>>> +       return 0;
>>> +}
>>> diff --git a/common/board_f.c b/common/board_f.c
>>> index fbbad1b..21be26f 100644
>>> --- a/common/board_f.c
>>> +++ b/common/board_f.c
>>> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = {
>>>  #ifdef CONFIG_OF_CONTROL
>>>         fdtdec_setup,
>>>  #endif
>>> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP)
>>> +       x86_fsp_init,
>>> +#endif
>>
>> Would it be possible to put this into arch_cpu_init()? Or do you think
>> it is better to have a separate call?
>>
>
> I was thinking to do that in arch_cpu_init previously, but I changed
> my idea mainly for:
> - I need modify every cpu codes which uses FSP. So far there are two,
> not a big deal :)
> - Since x86_fsp_init() will jump back to call board_init_f() again, I
> feel putting it as early as possible right after fdtdec_setup() could
> save some boot time, like no need to execute trace_early_init() and
> initf_malloc() which are meaningless, while still could gain access to
> device tree.

OK, then let's leave it as it is. I have added a CPU uclass and
perhaps we can try to remove some of the arch-specific init that way.
E.g. we could have an fsp_init() method.

Regards,
Simon