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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2024 11:28 am, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index fa21024042..80bba6ff21 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>  
>  __pvh_start:
> -        cld
> +        mov     $BOOT_TYPE_PVH, %dl
> +        jmp     .Lcommon_bios_pvh
> +#endif /* CONFIG_PVH_GUEST */
> +
> +__start:
> +        mov     $BOOT_TYPE_BIOS, %dl

Even if we're generally using %dl, these must be full %edx writes.

%edx commonly contains FMS on entry, and we don't want part of FMS left
in the upper half of the register.

> +
> +.Lcommon_bios_pvh:
>          cli
> +        cld
>  
>          /*
> -         * We need one call (i.e. push) to determine the load address.  See
> -         * __start for a discussion on how to do this safely using the PVH
> -         * info structure.
> +         * Multiboot (both 1 and 2) and PVH specify the stack pointer as
> +         * undefined.  This is unhelpful for relocatable images, where one
> +         * call (i.e. push) is required to calculate the image's load 
> address.
> +         *
> +         * Durig BIOS 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.
> +         *
> +         * During PVH boot use info structure in %ebx.
>           */
>  
>          /* Preserve the field we're about to clobber. */
> -        mov     (%ebx), %edx
> +        mov     (%ebx), %ecx

Both here, and ...

>          lea     4(%ebx), %esp
>  
>          /* Calculate the load base address. */
> @@ -449,62 +459,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)
> -
> -        /* 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 selector to unused segment registers. */
> +        xor     %ecx, %ecx
> +        mov     %ecx, %fs
> +        mov     %ecx, %gs
>  
> -.Linitialise_bss:
>       /* Initialise the BSS. */
> -        mov     %eax, %edx
> -
> +        mov     %eax, %ebp

... here, we've got changes caused by now using %edx for a long-lived
purpose (and a change in linebreaks.)

For this, %ebp should be used straight away in patch 1.  I've not
committed it yet, so can fix that up.


I have to admit that I think this patch would be easier if the "use %ebx
for BOOT_TYPE_*" change was split out of "better merge the BIOS/PVH
paths".  That would at least get the incidental %edx changes out of the way.

Also, inserting

#ifdef CONFIG_PVH_GUEST
        cmp     $BOOT_TYPE_PVH, %dl
        jne     1f
1:
#endif /* CONFIG_PVH_GUEST */

in the same patch will probably make the subsequent diff far more legible.

Thoughts?

I might give this a quick go, and see how it ends up looking...

~Andrew