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

[Jan Beulich <jbeulich@suse.com>]

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

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

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

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

Jan