Re: [PATCH 2/3] x86/boot: Refactor BIOS/PVH start

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2024 18:16, Frediano Ziglio wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -25,6 +25,9 @@
>  #define MB2_HT(name)      (MULTIBOOT2_HEADER_TAG_##name)
>  #define MB2_TT(name)      (MULTIBOOT2_TAG_TYPE_##name)
>  
> +#define BOOT_TYPE_BIOS 1
> +#define BOOT_TYPE_PVH 2

Did you consider using 0 and 1, to be able to use XOR on the BIOS
path and TEST for checking?

> @@ -409,13 +412,28 @@ cs32_switch:
>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>  
>  __pvh_start:
> -        cld
>          cli
> +        mov     $BOOT_TYPE_PVH, %ebp
> +        jmp     common_bios_pvh
> +#endif /* CONFIG_PVH_GUEST */
> +
> +__start:
> +        cli
> +        mov     $BOOT_TYPE_BIOS, %ebp
> +
> +common_bios_pvh:

I think labels like this one don't need to show up in the symbol
table, and hence would better start with .L.

> +        cld

So you fold the STDs but not the STIs, despite that not even having
been first on the PVH path. This decision wants explaining in the
description, even if just briefly.

> @@ -433,14 +451,9 @@ __pvh_start:
>          /* Set up stack. */
>          lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
>  
> -        call    initialise_bss

I'm little puzzled: The previous patch moved it "as early as possible"
just for it to be moved to a later point again here?

> -        mov     %ebx, sym_esi(pvh_start_info_pa)
> -
> -        /* Force xen console.  Will revert to user choice in init code. */
> -        movb    $-1, sym_esi(opt_console_xen)
> -
> -        /* Prepare gdt and segments */
> +        /*
> +         * Initialize GDTR and basic data segments.
> +         */
>          add     %esi, sym_esi(gdt_boot_base)
>          lgdt    sym_esi(gdt_boot_descr)

No real need to change the comment? In any even it wants to remain a
single-line one.

> @@ -449,67 +462,44 @@ __pvh_start:
>          mov     %ecx, %es
>          mov     %ecx, %ss
>  
> -        /* Skip bootloader setup and bios setup, go straight to trampoline */
> -        movb    $1, sym_esi(pvh_boot)
> -        movb    $1, sym_esi(skip_realmode)
> -
> -        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 
> 0 */
> -        movw    $0x1000, sym_esi(trampoline_phys)
> -        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
> -        jmp     trampoline_setup
> -
> -#endif /* CONFIG_PVH_GUEST */
> +        /* Load null descriptor to unused segment registers. */
> +        xor     %ecx, %ecx
> +        mov     %ecx, %fs
> +        mov     %ecx, %gs

The comment said "descriptor", yes, but it's a null selector that
we load here. Perhaps worth adjusting as the comment is moved.

> -initialise_bss:
>          /*
>           * Initialise the BSS.
>           *
>           * !!! WARNING - also zeroes the current stack !!!
>           */
> -        pop     %ebp
>          mov     %eax, %edx
> -
>          lea     sym_esi(__bss_start), %edi
>          lea     sym_esi(__bss_end), %ecx
>          sub     %edi, %ecx
>          xor     %eax, %eax
>          shr     $2, %ecx
>          rep stosl
> -
>          mov     %edx, %eax
> -        jmp     *%ebp
> -
> -__start:
> -        cld
> -        cli
>  
> -        /*
> -         * Multiboot (both 1 and 2) specify the stack pointer as undefined
> -         * when entering in BIOS circumstances.  This is unhelpful for
> -         * relocatable images, where one call (i.e. push) is required to
> -         * calculate the image's load address.
> -         *
> -         * This early in boot, there is one area of memory we know about with
> -         * reasonable confidence that it isn't overlapped by Xen, and that's
> -         * the Multiboot info structure in %ebx.  Use it as a temporary 
> stack.
> -         */
> -
> -        /* Preserve the field we're about to clobber. */
> -        mov     (%ebx), %edx
> -        lea     4(%ebx), %esp
> +#ifdef CONFIG_PVH_GUEST
> +        cmp     $BOOT_TYPE_PVH, %ebp
> +        jne     1f
>  
> -        /* Calculate the load base address. */
> -        call    1f
> -1:      pop     %esi
> -        sub     $sym_offs(1b), %esi
> +        mov     %ebx, sym_esi(pvh_start_info_pa)
>  
> -        /* Restore the clobbered field. */
> -        mov     %edx, (%ebx)
> +        /* Force xen console.  Will revert to user choice in init code. */
> +        movb    $-1, sym_esi(opt_console_xen)
>  
> -        /* Set up stack. */
> -        lea     STACK_SIZE - CPUINFO_sizeof + sym_esi(cpu0_stack), %esp
> +        /* Skip bootloader setup and bios setup, go straight to trampoline */
> +        movb    $1, sym_esi(pvh_boot)
> +        movb    $1, sym_esi(skip_realmode)
>  
> -        call    initialise_bss
> +        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 
> 0 */
> +        movw    $0x1000, sym_esi(trampoline_phys)

I realize you merely move this, yet I question the use of MOVW here.
You use 32-bit operations e.g. to set %ebp; perhaps the same should
be done here, even if that means a 1 byte code size increase. In any
even this would preferably use PAGE_SIZE imo.

Jan