On 07.08.2024 15:48, Alejandro Vallejo wrote: > No reason to wait, if Xen image is loaded by EFI (not multiboot > EFI path) these are set in efi_arch_load_addr_check, but > not in the multiboot EFI code path. > This change makes the 2 code paths more similar and allows > the usage of these variables if needed.
I'm afraid I'm struggling with any "similarity" argument here. Imo it would be better what, if anything, needs (is going to need) either or both of these set earlier. Which isn't to say it's wrong to do early what can be done early, just that ... > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -259,6 +259,11 @@ __efi64_mb2_start: > jmp x86_32_switch > > .Lefi_multiboot2_proto: > + /* Save Xen image load base address for later use. */ > + lea __image_base__(%rip),%rsi > + movq %rsi, xen_phys_start(%rip) > + movl %esi, trampoline_xen_phys_start(%rip) ... this path is EFI only if I'm not mistaken, while ... > @@ -605,10 +610,6 @@ trampoline_setup: > * Called on legacy BIOS and EFI platforms. > */ > > - /* Save Xen image load base address for later use. */ > - mov %esi, sym_esi(xen_phys_start) > - mov %esi, sym_esi(trampoline_xen_phys_start) ... the comment in context is pretty clear about this code also being used in the non-EFI case. It is, however, the case that %esi is 0 in that case. Yet surely you want to mention this in the description, to clarify the correctness of the change. Also in the code you move please consistently omit insn suffixes when they're not needed. Just like it was in the original code, and just like you already omit the q from "lea". Finally, if you used a register other than %rsi (say %r14) you could replace the "lea" after x86_32_switch by a 2nd "mov", similar to the one that's already there to load %edi. (You'd need to move the new code up by yet a few more lines, to cover the jump to x86_32_switch there, too.) Jan
