Re: [Xen-devel] [PATCH v2 2/6] x86/boot: Only jump into low trampoline code for real-mode boot

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Mon, 2019-08-12 at 11:10 +0200, Jan Beulich wrote:
> On 09.08.2019 17:01, David Woodhouse wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -735,7 +735,17 @@ trampoline_setup:
> >          /* Switch to low-memory stack which lives at the end of trampoline 
> > region. */
> >          mov     sym_fs(trampoline_phys),%edi
> >          lea     TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
> > +        cmpb    $0, sym_fs(skip_realmode)
> > +        jz      1f
> > +        /* If no-real-mode, jump straight to trampoline_protmode_entry */
> > +        lea     trampoline_protmode_entry-trampoline_start(%edi),%eax
> > +        /* EBX == 0 indicates we are the BP (Boot Processor). */
> > +        xor     %ebx,%ebx
> > +        jmp     2f
> > +1:
> > +        /* Go via 16-bit code in trampoline_boot_cpu_entry */
> >          lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
> > +2:
> >          pushl   $BOOT_CS32
> >          push    %eax
> 
> May I suggest to slightly streamline this into
> 
>          /* Switch to low-memory stack which lives at the end of trampoline 
> region. */
>          mov     sym_fs(trampoline_phys),%edi
>          lea     TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE(%edi),%esp
>          /* Go via 16-bit code in trampoline_boot_cpu_entry */
>          lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
>          cmpb    $0,sym_fs(skip_realmode)
>          je      1f
>          /* If no-real-mode, jump straight to trampoline_protmode_entry */
>          lea     trampoline_protmode_entry-trampoline_start(%edi),%eax
>          /* EBX == 0 indicates we are the BP (Boot Processor). */
>          xor     %ebx,%ebx
> 1:
>          pushl   $BOOT_CS32
>          push    %eax
> 
> perhaps with the second slightly adapted to it now being an override
> rather than an alternative path?

It's a *temporary* alternative path, and it's gone away by the end of
the series. Obviously we do insist on each interim commit being
buildable and working. I kind of draw the line at optimising the asm
for each intermediate step :)

> Additionally I think it would be nice if the clearing of %ebx wasn't
> replicated here and ...
> 
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -194,9 +194,6 @@ gdt_48: .word   6*8-1
> >   
> >           .code32
> >   trampoline_boot_cpu_entry:
> > -        cmpb    $0,bootsym_rel(skip_realmode,5)
> > -        jnz     .Lskip_realmode
> > -
> >           /* Load pseudo-real-mode segments. */
> >           mov     $BOOT_PSEUDORM_DS,%eax
> >           mov     %eax,%ds
> > @@ -276,7 +273,6 @@ trampoline_boot_cpu_entry:
> >           mov     %eax,%gs
> >           mov     %eax,%ss
> >   
> > -.Lskip_realmode:
> >           /* EBX == 0 indicates we are the BP (Boot Processor). */
> >           xor     %ebx,%ebx
> 
> ... here. Why don't you further do
> 
>          .code32
> trampoline_protmode_entry_bsp:
>          /* EBX == 0 indicates we are the BSP (Boot Strap Processor).
> */
>          xor     %ebx,%ebx
> trampoline_protmode_entry:
> 
> directing the BSP paths to the new label?

Yeah, I kind of see your point. But that gives us one entry point which
clears %ebx... and one which doesn't, so you still have to make sure
it's not already zero for the AP startup.

If we ended up with two simple entry points that didn't care about %ebx
for trampoline_protmode_entry_bsp and trampoline_protmode_entry_ap then
that'd be nice and simple — but I don't like the inconsistency.

I think I prefer having to set %ebx explicitly in all three separate
callers, over having one entry point that requires it and another that
doesn't.

Attachment: smime.p7s
Description: S/MIME cryptographic signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel