diff mbox series

[v2,2/6] x86/boot: Only jump into low trampoline code for real-mode boot

Message ID 6dc2b63127f966961aeb2a7bfe89a5cdce83241b.camel@infradead.org (mailing list archive)
State New, archived
Headers show
Series [v2,1/6] x86/boot: Remove gratuitous call back into low-memory code | expand

Commit Message

David Woodhouse Aug. 9, 2019, 3:01 p.m. UTC 
From: David Woodhouse <dwmw@amazon.co.uk>

If the no-real-mode flag is set, don't go there at all. This is a prelude
to not even putting it there in the first place.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 xen/arch/x86/boot/head.S       | 10 ++++++++++
 xen/arch/x86/boot/trampoline.S |  4 ----
 2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Jan Beulich Aug. 12, 2019, 9:10 a.m. UTC | #1 
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?

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?

Jan
David Woodhouse Aug. 21, 2019, 2:04 p.m. UTC | #2 
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.
Jan Beulich Aug. 27, 2019, 8:43 a.m. UTC | #3 
On 21.08.2019 16:04, David Woodhouse wrote:
> 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.

Indeed I did notice it's temporary when making it to the later patches
of the series. If all parts go in at about the same time, I think I'm
okay with leaving the code as is.

>> 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.

I think differently, but I'm not going to insist if Andrew agrees with
your preference.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d78bed394a..e3b42e3263 100644
--- 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
 
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 7c6a2328d2..429a088b19 100644
--- 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