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.
--- 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 .LhaltOn 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
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
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.
© 2016 - 2024 Red Hat, Inc.