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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan