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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Aug 12, 2024 at 9:41 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 09.08.2024 16:34, Frediano Ziglio wrote:
> > 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.
>
> In the PVH entry point it'll be, but else? Note this code in setup.c:
>
>         /* Is the region suitable for relocating Xen? */
>         if ( !xen_phys_start && e <= limit )
>
> That relocating of Xen wouldn't happen if we stored a non-zero value in
> the default (xen.gz with grub1/2) case. Also take a look at Xen before
> the EFI/MB2 path was added. xen_phys_start wasn't even written from
> head.S at that time. And if it's for the PVH entry point alone, that
> code then would want moving into the CONFIG_PVH_GUEST section (if at all
> possible). Or, if the reason for the change really is "just in case",
> another option of course is to leave these two insn in the one central
> place they are at right now.
>

Hi,
  as I said I added back the lines in the original place too (I didn't
still send that update, I want to finish other changes you suggested).
The reason I added these lines is the usage in efi-boot.h, it has
nothing to do with PVH. Yes, at the moment that part of the code is
executed only on direct EFI program so it's not impacting these paths
but better safe than sorry.

> Jan

Frediano