Re: [PATCH] x86: fix early boot output

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.07.2023 10:06, Andrew Cooper wrote:
> On 19/07/2023 8:38 am, Jan Beulich wrote:
>> Loading the VGA base address involves sym_esi(), i.e. %esi still needs
>> to hold the relocation base address. Therefore the address of the
>> message to output cannot be "passed" in %esi. Put the message offset in
>> %ecx instead, adding it into %esi _after_ its last use as base address.
>>
>> Fixes: b28044226e1c ("x86: make Xen early boot code relocatable")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> When I was doing the label cleanup, I did wonder how this worked, given
> that it clobbered %esi.  I guess this is the answer...
> 
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

> Although it occurs to me that probably want to (optionally) use one of
> the IO-port/Hypercall protocols too to get these messages in PVH boot
> case too.

Probably.

>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -184,12 +184,15 @@ early_error: /* Here to improve the disa
>>           * Here we are on EFI IA-32 platform. Then reliable vga_text_buffer 
>> zap is
>>           * impossible in Multiboot2 scanning loop and we have to zero %edi 
>> below.
>>           */
>> -        add     $sym_offs(.Lbad_efi_msg), %esi
>> +        mov     $sym_offs(.Lbad_efi_msg), %ecx
>>          xor     %edi,%edi                       # No VGA text buffer
>>          jmp     .Lprint_err
>>  .Lget_vtb:
>>          mov     sym_esi(vga_text_buffer), %edi
>>  .Lprint_err:
>> +        add     %ecx, %esi     # Add string offset to relocation base.
>> +        # NOTE: No further use of sym_esi() till the end of the "function"!
> 
> Minor, but I'd phrase this as "Note: sym_esi() no longer useable".
> 
> It is obviously limited in scope, but "until the end of the function"
> gives an implication that it's fine thereafter which isn't really true.

It is very true. The use here is the first out of several dozen. It is
only not true for the code that immediately follows this function (for
an unrelated reason). If this really was the last use, I would have
taken the liberty of adding an #undef. That said, some re-ordering might
help the situation, but that's nothing I'd like to spend time on right
away.

Jan