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

[Bin Meng]

Hi Simon,

On Thu, Jun 4, 2015 at 4:59 PM, Simon Glass <sjg at chromium.org> wrote:
> 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.

Yes, we could. Will do in v2.

>> +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?

In arch/x86/cpu/start.S where U-Boot setup its new GDT with a new FS
segment descriptor.

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

Regards,
Bin