Hi Simon, On Fri, Jun 5, 2015 at 11:34 PM, Simon Glass <s...@chromium.org> wrote: > Hi Bin, > > On 4 June 2015 at 04:28, Bin Meng <bmeng...@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...@gmail.com> > > Tested on Minnowboard MAX: > Tested-by: Simon Glass <s...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot