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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2024 2:25 pm, Andrew Cooper wrote:
> 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...

Actually, why do we need BOOT_TYPE_* at all?  We've already got
BOOTLOADER_MAGIC in %eax which can be used to identify PVH.

~Andrew