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

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