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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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.

>
> 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".
>

Done

> 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.)
>

IMHO it makes code less readable, it's hard to understand which
registers are in use or not, I prefer to compute one more time
instead, this code is not in an hard path and it's going to be
discarded after initialization.

> Jan

Frediano