Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 09.08.2024 15:50, Frediano Ziglio wrote:
> > On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 09.08.2024 14:48, Frediano Ziglio wrote:
> >>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@xxxxxxxx> 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.
>

I really don't follow, %esi at that point should be the address where
the executable is loader, which should not be zero.

> Jan

Frediano