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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel