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

[Simon Glass]

Hi Bin,

On 1 June 2015 at 06:31, Bin Meng <bmeng.cn at 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 at gmail.com>
> ---
>
>  arch/x86/cpu/cpu.c                | 18 ++++++++++++++++++
>  arch/x86/cpu/start16.S            |  1 +
>  arch/x86/include/asm/u-boot-x86.h |  4 ++++
>  arch/x86/lib/fsp/fsp_support.c    |  3 +++
>  4 files changed, 26 insertions(+)
>
> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
> index bb4a110..a0b101c 100644
> --- a/arch/x86/cpu/cpu.c
> +++ b/arch/x86/cpu/cpu.c
> @@ -164,6 +164,24 @@ void setup_gdt(gd_t *id, u64 *gdt_addr)
>         load_fs(X86_GDT_ENTRY_32BIT_FS);
>  }
>
> +/*
> + * 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 + BOOT_SEG), 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);
> +}
> +
>  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 be103c0..faa5182 100644
> --- a/arch/x86/include/asm/u-boot-x86.h
> +++ b/arch/x86/include/asm/u-boot-x86.h
> @@ -8,12 +8,16 @@
>  #ifndef _U_BOOT_I386_H_
>  #define _U_BOOT_I386_H_        1
>
> +#define BOOT_SEG       0xffff0000

We have RESET_SEG_START in arch/x86/cpu/config.mk. We could move this
to Kconfig if you like? But it would be good to avoid the duplication.

> +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);
>  void panic_puts(const char *str);
> diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c
> index 5f96da1..cf6ed04 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();
> +

Where does the GDT get set back to the normal U-Boot one?

>         /*
>          * Use ASM code to ensure the register value in EAX & ECX
>          * will be passed into BlContinuationFunc
> --
> 1.8.2.1
>

Regards,
Simon