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