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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/09/2024 7:00 am, Frediano Ziglio wrote:
> The 2 code paths were sharing quite some common code, reuse it instead
> of having duplications.
> Use %dl register to store boot type before running common code.
> Using a 8 bit register reduces code size.

These final two lines are stale and can be dropped.

> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 267207e5a2..2d2f56ad22 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -409,13 +411,27 @@ cs32_switch:
>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>  
>  __pvh_start:
> -        cld
> +        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
> +        /*
> +         * Fall through into BIOS code.
> +         * We will use %eax to distinguish we came from PHV entry point.

PVH.

It occurs to me that we could actually have:

        mov     $XEN_HVM_START_MAGIC_VALUE, %eax

Given the cross-check against 0(%ebx) later, that's marginally more robust.

> @@ -449,62 +458,40 @@ __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)
> +        /* Load null selector to unused segment registers. */
> +        xor     %ecx, %ecx
> +        mov     %ecx, %fs
> +        mov     %ecx, %gs

Honestly, the more I look at this, the more bizarre it is.

We should just set up %fs/gs like we do %ds/es, which in this case is
simply to drop the comment and the xor.

>  
> -        /* 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 */
> -
> -.Linitialise_bss:
>          /* Initialise the BSS.  Preserve %eax (BOOTLOADER_MAGIC). */
>          mov     %eax, %ebp
> -
>          lea     sym_esi(__bss_start), %edi
>          lea     sym_esi(__bss_end), %ecx
>          sub     %edi, %ecx
>          xor     %eax, %eax
>          shr     $2, %ecx
>          rep stosl
> -
>          mov     %ebp, %eax

Are these two dropped lines intentional?

I did ask on the previous version; it's weird to introduce the code with
them present, then delete them in the subsequent patch.

> -        ret
> -
> -__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     $XEN_HVM_START_MAGIC_VALUE, %eax
> +        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    .Linitialise_bss
> +        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 
> 0 */
> +        movl    $PAGE_SIZE, sym_esi(trampoline_phys)
> +        mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */

This isn't needed any more.  It was previously reloading %eax after REP
STOSL, but that's sorted elsehow now.


Overall, this bit of the diff is still hard to read, but it's the best
we're going to get I think.  The end result is good.

I'm happy to adjust all of these on commit.

~Andrew