diff mbox series

x86: fix early boot output

Message ID 3913c170-09c6-2baf-ed38-5614f8c6cb2e@suse.com (mailing list archive)
State New, archived
Headers show
Series x86: fix early boot output | expand

Commit Message

Jan Beulich July 19, 2023, 7:38 a.m. UTC
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@suse.com>
---
This also suggests that 78e693cc1232 ("x86/boot: fix early error
output") was only tested for the no-VGA case (i.e. EFI+MB2), and only
for one of the two paths which bypass the loading of %edi at .Lget_vtb
(or the offset load merely was lucky to pick up a zero).

Clearly when using "vga=current" with MB2 when the screen is already in
graphics mode, there will continue to be no visible output.

Comments

Andrew Cooper July 19, 2023, 8:06 a.m. UTC | #1
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@suse.com>

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@citrix.com>

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.

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

~Andrew
Roger Pau Monné July 19, 2023, 8:17 a.m. UTC | #2
On Wed, Jul 19, 2023 at 09:06:08AM +0100, 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@suse.com>
> 
> 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@citrix.com>
> 
> 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.

Using XEN_HVM_DEBUGCONS_IOPORT would be my preference, as it's the
same IO port that's used by QEMU as a debug console.

Roger.
Jan Beulich July 19, 2023, 8:18 a.m. UTC | #3
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@suse.com>
> 
> 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@citrix.com>

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
diff mbox series

Patch

--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -146,17 +146,17 @@  efi_platform:
 early_error: /* Here to improve the disassembly. */
 
 .Lbad_cpu:
-        add     $sym_offs(.Lbad_cpu_msg), %esi
+        mov     $sym_offs(.Lbad_cpu_msg), %ecx
         jmp     .Lget_vtb
 .Lnot_multiboot:
-        add     $sym_offs(.Lbad_ldr_msg), %esi
+        mov     $sym_offs(.Lbad_ldr_msg), %ecx
         jmp     .Lget_vtb
 .Lnot_aligned:
-        add     $sym_offs(.Lbag_alg_msg), %esi
+        mov     $sym_offs(.Lbag_alg_msg), %ecx
         jmp     .Lget_vtb
 #ifdef CONFIG_REQUIRE_NX
 .Lno_nx:
-        add     $sym_offs(.Lno_nx_msg), %esi
+        mov     $sym_offs(.Lno_nx_msg), %ecx
         jmp     .Lget_vtb
 #endif
 .Lmb2_no_st:
@@ -164,11 +164,11 @@  early_error: /* Here to improve the disa
          * Here we are on EFI platform. vga_text_buffer was zapped earlier
          * because there is pretty good chance that VGA is unavailable.
          */
-        add     $sym_offs(.Lbad_ldr_nst), %esi
+        mov     $sym_offs(.Lbad_ldr_nst), %ecx
         jmp     .Lget_vtb
 .Lmb2_no_ih:
         /* Ditto. */
-        add     $sym_offs(.Lbad_ldr_nih), %esi
+        mov     $sym_offs(.Lbad_ldr_nih), %ecx
         jmp     .Lget_vtb
 .Lmb2_no_bs:
         /*
@@ -176,7 +176,7 @@  early_error: /* Here to improve the disa
          * via start label. Then reliable vga_text_buffer zap is impossible
          * in Multiboot2 scanning loop and we have to zero %edi below.
          */
-        add     $sym_offs(.Lbad_ldr_nbs), %esi
+        mov     $sym_offs(.Lbad_ldr_nbs), %ecx
         xor     %edi,%edi                       # No VGA text buffer
         jmp     .Lprint_err
 .Lmb2_efi_ia_32:
@@ -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"!
+1:
         lodsb
         test    %al,%al        # Terminate on '\0' sentinel
         je      .Lhalt
@@ -202,11 +205,11 @@  early_error: /* Here to improve the disa
         mov     %bl,%al
         out     %al,%dx        # Send a character over the serial line
         test    %edi,%edi      # Is the VGA text buffer available?
-        jz      .Lprint_err
+        jz      1b
         stosb                  # Write a character to the VGA text buffer
         mov     $7,%al
         stosb                  # Write an attribute to the VGA text buffer
-        jmp     .Lprint_err
+        jmp     1b
 .Lhalt: hlt
         jmp     .Lhalt