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
