| Message ID | 20230502092224.52265-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: init improvements | expand |
On 02/05/2023 10:22 am, Roger Pau Monne wrote: > When booting the BSP the portion of the code executed from the > trampoline page will be using the GDT located in the hypervisor > .text.head section rather than the GDT located in the trampoline page. It's more subtle than this. gdt_boot_descr references the trampoline GDT, but by it's position in the main Xen image. > > If skip_realmode is not set the GDT located in the trampoline page > will be loaded after having executed the BIOS call, otherwise the GDT > from .text.head will be used for all the protected mode trampoline > code execution. > > Note that both gdt_boot_descr and gdt_48 contain the same entries, but > the former is located inside the hypervisor .text section, while the > later lives in the relocated trampoline page. > > This is not harmful as-is, as both GDTs contain the same entries, but > for consistency with the APs switch the BSP trampoline code to also > use the GDT on the trampoline page. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although ... > --- > xen/arch/x86/boot/trampoline.S | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S > index cdecf949b410..e4b4b9091d0c 100644 > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -164,6 +164,12 @@ GLOBAL(trampoline_cpu_started) > > .code32 > trampoline_boot_cpu_entry: > + /* > + * Load the GDT from the relocated trampoline page rather than the > + * hypervisor .text section. > + */ > + lgdt bootsym_rel(gdt_48, 4) ... I'd suggest rewording this to simply /* Switch to trampoline GDT */, or perhaps with an "alias" in there somewhere. The important point here is that we want to shed all pre-trampoline state, and unexpectedly being on the wrong GDT alias certainly complicated debugging this... > + > cmpb $0,bootsym_rel(skip_realmode,5) > jnz .Lskip_realmode >
On Tue, May 02, 2023 at 10:43:13AM +0100, Andrew Cooper wrote: > On 02/05/2023 10:22 am, Roger Pau Monne wrote: > > When booting the BSP the portion of the code executed from the > > trampoline page will be using the GDT located in the hypervisor > > .text.head section rather than the GDT located in the trampoline page. > > It's more subtle than this. > > gdt_boot_descr references the trampoline GDT, but by it's position in > the main Xen image. Right, gdt_boot_descr GDTR references gdt_48, but the instance on the Xen .text section, not the trampoline. I've tried to explain this in the commit message, but maybe I've failed to do so. > > > > If skip_realmode is not set the GDT located in the trampoline page > > will be loaded after having executed the BIOS call, otherwise the GDT > > from .text.head will be used for all the protected mode trampoline > > code execution. > > > > Note that both gdt_boot_descr and gdt_48 contain the same entries, but > > the former is located inside the hypervisor .text section, while the > > later lives in the relocated trampoline page. > > > > This is not harmful as-is, as both GDTs contain the same entries, but > > for consistency with the APs switch the BSP trampoline code to also > > use the GDT on the trampoline page. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although ... > > > --- > > xen/arch/x86/boot/trampoline.S | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S > > index cdecf949b410..e4b4b9091d0c 100644 > > --- a/xen/arch/x86/boot/trampoline.S > > +++ b/xen/arch/x86/boot/trampoline.S > > @@ -164,6 +164,12 @@ GLOBAL(trampoline_cpu_started) > > > > .code32 > > trampoline_boot_cpu_entry: > > + /* > > + * Load the GDT from the relocated trampoline page rather than the > > + * hypervisor .text section. > > + */ > > + lgdt bootsym_rel(gdt_48, 4) > > ... I'd suggest rewording this to simply /* Switch to trampoline GDT */, > or perhaps with an "alias" in there somewhere. "Switch to the relocated trampoline GDT." maybe? Thanks, Roger.
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index cdecf949b410..e4b4b9091d0c 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -164,6 +164,12 @@ GLOBAL(trampoline_cpu_started) .code32 trampoline_boot_cpu_entry: + /* + * Load the GDT from the relocated trampoline page rather than the + * hypervisor .text section. + */ + lgdt bootsym_rel(gdt_48, 4) + cmpb $0,bootsym_rel(skip_realmode,5) jnz .Lskip_realmode
When booting the BSP the portion of the code executed from the trampoline page will be using the GDT located in the hypervisor .text.head section rather than the GDT located in the trampoline page. If skip_realmode is not set the GDT located in the trampoline page will be loaded after having executed the BIOS call, otherwise the GDT from .text.head will be used for all the protected mode trampoline code execution. Note that both gdt_boot_descr and gdt_48 contain the same entries, but the former is located inside the hypervisor .text section, while the later lives in the relocated trampoline page. This is not harmful as-is, as both GDTs contain the same entries, but for consistency with the APs switch the BSP trampoline code to also use the GDT on the trampoline page. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/boot/trampoline.S | 6 ++++++ 1 file changed, 6 insertions(+)