On 09.08.2024 14:48, Frediano Ziglio wrote: > On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeul...@suse.com> wrote: >> 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 ... >> > > About similarity is that some part of EFI code expect xen_phys_start > to be initialized so this change make sure that if in the future these > paths are called even for this case they won't break. > >>> --- 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. > > Restored this code.
Was my analysis wrong then and it's actually needed for some specific case? Jan
