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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

Summary of the "quick look".

The suggested empty ifdefary doesn't really help.

However, moving the

    mov     (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */

into __pvh_entry avoids the need for any BOOT_TYPE_* being held in %edx.

The one place where it's needed can be `cmp $XEN_ ...; jne` and this
avoids needing to shuffle the register scheduling.

~Andrew