[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 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>

Tested on Minnowboard MAX:
Tested-by: Simon Glass <sjg at 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