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

[Jan Beulich <jbeulich@suse.com>]

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

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