On 09.08.2024 15:50, Frediano Ziglio wrote: > On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <[email protected]> wrote: >> >> On 09.08.2024 14:48, Frediano Ziglio wrote: >>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <[email protected]> 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? > > Not clear to what exactly you are referring. > That later part of code (which was removed) is still needed in case of no-EFI.
Is it? Under what conditions would %esi be non-zero? As indicated by my earlier reply, I think it would never be. In which case the two stores are pointless. Jan
