Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[Matt Fleming]

On Sun, 2013-01-06 at 00:13 +0000, David Woodhouse wrote:
> When booting under OVMF we have precisely one GOP device, and it
> implements the ConOut protocol.
> 
> We break out of the loop when we look at it... and then promptly abort
> because 'first_gop' never gets set. We should set first_gop *before*
> breaking out of the loop. Yes, it doesn't really mean "first" any more,
> but that doesn't matter. It's only a flag to indicate that a suitable
> GOP was found.
> 
> In fact, we'd do just as well to initialise 'width' to zero in this
> function, then just check *that* instead of first_gop. But I'll do the
> minimal fix for now (and for stable@).
> 
> Signed-off-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index c760e07..807330a 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -314,10 +314,9 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto,
>  			 * Once we've found a GOP supporting ConOut,
>  			 * don't bother looking any further.
>  			 */
> +			first_gop = gop;
>  			if (conout_found)
>  				break;
> -
> -			first_gop = gop;
>  		}
>  	}
>  
> 
> 

This looks good to me. Matthew?

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[Matt Fleming]

On Mon, 2013-01-07 at 02:04 +0000, David Woodhouse wrote:
> On Sun, 2013-01-06 at 00:13 +0000, David Woodhouse wrote:
> > When booting under OVMF we have precisely one GOP device, and it
> > implements the ConOut protocol.
> > 
> > We break out of the loop when we look at it... and then promptly abort
> > because 'first_gop' never gets set.
> 
> Hmmm. I've just fixed OVMF so that when qemu is invoked with the
> '-kernel' argument, it uses the EFI handover protocol. And now graphics
> is broken on kernels *without* this patch. I can't even initialise the
> graphics info in OVMF (as it does for non-EFI handover) because the
> kernel helpfully zeroes it all out before failing to set it up
> correctly.
> 
> Does it make sense to bump the boot protocol to 2.12 to indicate that
> this bug is fixed, and then let things like OVMF use the EFI handover
> protocol only for boot protocol >= 2.12? Or is there a better way...?

Bumping the boot protocol seems like a sensible idea.

Peter, any objection to that?

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]

On Mon, 2013-01-07 at 17:15 +0000, Matt Fleming wrote:
> On Mon, 2013-01-07 at 02:04 +0000, David Woodhouse wrote:
> > On Sun, 2013-01-06 at 00:13 +0000, David Woodhouse wrote:
> > > When booting under OVMF we have precisely one GOP device, and it
> > > implements the ConOut protocol.
> > > 
> > > We break out of the loop when we look at it... and then promptly abort
> > > because 'first_gop' never gets set.
> > 
> > Hmmm. I've just fixed OVMF so that when qemu is invoked with the
> > '-kernel' argument, it uses the EFI handover protocol. And now graphics
> > is broken on kernels *without* this patch. I can't even initialise the
> > graphics info in OVMF (as it does for non-EFI handover) because the
> > kernel helpfully zeroes it all out before failing to set it up
> > correctly.
> > 
> > Does it make sense to bump the boot protocol to 2.12 to indicate that
> > this bug is fixed, and then let things like OVMF use the EFI handover
> > protocol only for boot protocol >= 2.12? Or is there a better way...?
> 
> Bumping the boot protocol seems like a sensible idea.
> 
> Peter, any objection to that?

I'd like to fix the 32-bit EFI entry point first. Although the PE entry
point helpfully adds 4 to $esp to effectively pop the unwanted return
address off the stack, the EFI entry point at 0x30 doesn't. So I'm doing
this for now to test OVMF, while I work out why the EFI stub is then
crashing in the PCI ROM handling:

--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -50,8 +50,10 @@ ENTRY(startup_32)
        pushl   %eax
        pushl   %esi
        pushl   %ecx
+       pushl   %ecx
 
        .org 0x30,0x90
+       add     $0x4, %esp
        call    efi_main
        cmpl    $0, %eax
        movl    %eax, %esi

On fact, the whole 32-bit vs 64-bit entry point seems entirely
confusing.

It seems that a bootloader needs to *know* whether it's loading a 64-bit
or a 32-bit kernel image. A quick check shows that grub is always
running non-EFI kernels in 32-bit mode, at the 32-bit entry point. So
that's fine. But for the EFI stub, a 64-bit grub unconditionally jumps
to the undocumented¹ 0x200 + handover_offset, while a 32-bit grub omits
the 0x200. So if you attempt to boot a 64-bit kernel from a 32-bit grub,
or vice versa, it's just going to crash.

What is the bootloader *supposed* to do?

-- 
dwmw2
¹ Not strictly true. It's documented as "THIS MAY CHANGE. BOOTLOADERS
  SHALL USE THE ELF HEADERS TO FIND IT" in .../boot/compressed/head_64.S

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[Matthew Garrett]

On Mon, Jan 07, 2013 at 05:51:20PM +0000, David Woodhouse wrote:

> What is the bootloader *supposed* to do?

Ignore this case? You can't run 32-bit kernels on 64-bit EFI, or 
vice-versa. You'll die on the first call to UEFI services.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Mon, 2013-01-07 at 17:58 +0000, Matthew Garrett wrote:
> Ignore this case? You can't run 32-bit kernels on 64-bit EFI, or 
> vice-versa. You'll die on the first call to UEFI services.

The kernel has code to avoid calling UEFI services in that case. Even if
we don't want to support that (any more?), we should at least fail
gracefully rather than just jumping to the wrong entry point and having
RandomShit™ happening.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[Matthew Garrett]

On Mon, Jan 07, 2013 at 06:36:35PM +0000, David Woodhouse wrote:
> On Mon, 2013-01-07 at 17:58 +0000, Matthew Garrett wrote:
> > Ignore this case? You can't run 32-bit kernels on 64-bit EFI, or 
> > vice-versa. You'll die on the first call to UEFI services.
> 
> The kernel has code to avoid calling UEFI services in that case. Even if
> we don't want to support that (any more?), we should at least fail
> gracefully rather than just jumping to the wrong entry point and having
> RandomShit™ happening.

The kernel does, the EFI boot stub doesn't. There's probably an argument 
for having it fail in that case, but it wouldn't even be able to print 
an error message without some gymnastics.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Mon, 2013-01-07 at 18:43 +0000, Matthew Garrett wrote:
> The kernel does, the EFI boot stub doesn't. There's probably an argument 
> for having it fail in that case, but it wouldn't even be able to print 
> an error message without some gymnastics.

Printing an error message is simple enough from assembler; you just need
to call sys_table->con_out->output_string.

What should it say?

 "Sorry, this kernel cannot boot in " (32|64) "-bit mode this way. Your
bootloader should have ignored the shiny new EFI handover protocol and
booted it the old way. It would have worked then, albeit without EFI
runtime services. Which you probably wouldn't have missed."

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[Matthew Garrett]

On Mon, Jan 07, 2013 at 07:12:51PM +0000, David Woodhouse wrote:
> On Mon, 2013-01-07 at 18:43 +0000, Matthew Garrett wrote:
> > The kernel does, the EFI boot stub doesn't. There's probably an argument 
> > for having it fail in that case, but it wouldn't even be able to print 
> > an error message without some gymnastics.
> 
> Printing an error message is simple enough from assembler; you just need
> to call sys_table->con_out->output_string.

I don't think that's guaranteed to do something unless we've set a 
console mode first, but sure, that ought to be an improvement.

>  "Sorry, this kernel cannot boot in " (32|64) "-bit mode this way. Your
> bootloader should have ignored the shiny new EFI handover protocol and
> booted it the old way. It would have worked then, albeit without EFI
> runtime services. Which you probably wouldn't have missed."

"(32|64) bit kernels cannot be booted via the EFI handover 
protocol on (64|32) bit firmware"?

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] Fix efifb initialisation when the only GOP device implements ConOut.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On Mon, 2013-01-07 at 19:16 +0000, Matthew Garrett wrote:
> On Mon, Jan 07, 2013 at 07:12:51PM +0000, David Woodhouse wrote:
> > On Mon, 2013-01-07 at 18:43 +0000, Matthew Garrett wrote:
> > > The kernel does, the EFI boot stub doesn't. There's probably an argument 
> > > for having it fail in that case, but it wouldn't even be able to print 
> > > an error message without some gymnastics.
> > 
> > Printing an error message is simple enough from assembler; you just need
> > to call sys_table->con_out->output_string.
> 
> I don't think that's guaranteed to do something unless we've set a 
> console mode first, but sure, that ought to be an improvement.

It's what we do for efi_printk() in all other cases where the EFI stub
bails out. It's certainly fair enough as a best-effort.

> >  "Sorry, this kernel cannot boot in " (32|64) "-bit mode this way. Your
> > bootloader should have ignored the shiny new EFI handover protocol and
> > booted it the old way. It would have worked then, albeit without EFI
> > runtime services. Which you probably wouldn't have missed."
> 
> "(32|64) bit kernels cannot be booted via the EFI handover 
> protocol on (64|32) bit firmware"?

Yeah. But the bootloader *could* have booted this kernel, if it had just
chosen to use the old handover protocol. And right now, there's no way
for the bootloader to *tell* what it should do.

It wouldn't be impossible to simply have both 32-bit and 64-bit stubs
(conditionally) present in the kernel, and support both simultaneously.
But if we can't do that, then I suspect we should at *least* make sure
we let the bootloader know which one *is* available, so it doesn't end
up calling the wrong one.

Perhaps we should declare that handover_offset is for the 64-bit stub,
and add a handover_offset32 for the 32-bit stub?

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] Specify ELF ABI for EFI handover protocol

[Matt Fleming]

On Mon, 2013-01-07 at 02:07 +0000, David Woodhouse wrote:
> This would have saved me some time...
> 
> Signed-off-by: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> Also, are interrupts supposed to be disabled? The efi_main() function
> will disable them, but only *after* lidt/lgdt. Shouldn't it happen
> sooner?

I was under the impression that, yes, interrupts should be disabled when
we jump to the kernel entry points. Certainly for the 32-bit boot
protocol detailed in Documentation/x86/boot.txt, interrupts must be
disabled. The 'cli' in efi_main() is likely superfluous. I can't think
of any reason you'd want to leave interrupts enabled when you enter
efi_main().

> diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
> index 9efceff..c00aa8a 100644
> --- a/Documentation/x86/boot.txt
> +++ b/Documentation/x86/boot.txt
> @@ -1054,3 +1054,7 @@ The boot loader *must* fill out the following fields in bp,
>      o hdr.ramdisk_size  (if applicable)
>  
>  All other fields should be zero.
> +
> +Note that the efi_main entry point uses Linux/ELF calling conventions, not
> +EFI calling conventions. So 'handle' is in %rdi, 'table' in %rsi and 'bp'
> +in %rdx.

Unless you're booting under 32-bit, in which case they're on the stack.
I'll take this patch and munge it a little to mention 32-bit, thanks.