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