Re: [PATCH 2/2] x86/trampoline: load the GDT located in the trampoline page

["Roger Pau Monné" <roger.pau@citrix.com>]

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.