arch/x86: Add better error logging to efi main

Review

Quoted 9 lines · On Thu, 10 Jul, at 02:12:41AM, ulf@... wrote:

<at> <at> -1376,7 +1376,10 <at> <at> struct boot_params *efi_main(struct efi_config *c,

    setup_graphics(boot_params);

(quoted diff) +4 -1

-   setup_efi_pci(boot_params);
+   status = setup_efi_pci(boot_params);
+   if (status != EFI_SUCCESS) {
+           efi_printk(sys_table, "setup_efi_pci() failed!\n");
+   }

Actually, it turns out that a lot of people are seeing this error message in what would be an otherwise quiet boot,

https://bugzilla.kernel.org/show_bug.cgi?id=81891

Maybe we should distinguish between EFI_PCI_IO_PROTOCOL_GUID not being found and all other errors, i.e. memory allocation failure. We obviously do want to know if we failed to allocate memory, but we're less bothered if we've no PCI devices.

Something like below? Folks on Cc, could you apply this snippet and see whether you're still seeing the error message?

With a Thinkpad T420 (pre-2.0 EFI), I do run into the failure condition for another reason:

setup_efi_pci64 gets called,
the loop therein is executed nr_pci=11 (!?) times.
The second call run calls into __setup_efi_pci64 successfully, the memcpy takes place; the free_struct: path is not used. Back in setup_efi_pci64, the non-data path is walked.

For all the other 10 runs of the loop, __setup_efi_pci64 fails at if (!pci->romimage || !pci->romsize)

        return EFI_INVALID_PARAMETER;

so that is the status here for any run but the second.

Quoted 2 lines · On Sat, 6 Sep 2014 23:02:51 +0200 Andre <andre.muller-S0/[email protected].…

With a Thinkpad T420 (pre-2.0 EFI), I do run into the failure condition for another reason:

Quoted 8 lines

setup_efi_pci64 gets called,
the loop therein is executed nr_pci=11 (!?) times.
The second call run calls into __setup_efi_pci64 successfully, the memcpy takes place; the free_struct: path is not used. Back in setup_efi_pci64, the non-data path is walked.

For all the other 10 runs of the loop, __setup_efi_pci64 fails at if (!pci->romimage || !pci->romsize)

        return EFI_INVALID_PARAMETER;

so that is the status here for any run but the second.

Following up with code...

If it is OK to just leave the loop in setup_efi_pci64, this can be done simply by

x86/boot/compressed/eboot_break.c renamed from x86/boot/compressed/eboot.c +1 -0

@@ -504,6 +504,7 @@
 			params->hdr.setup_data = (unsigned long)rom;
 
 		data = (struct setup_data *)rom;
+		break;
 
 	}
 

But I've got severe doubts about this.

A solution limited to this particular failure mode could look like the below. It has all the charm of introducing a helper var. Hmpf.

x86/boot/compressed/eboot_preserve.c renamed from x86/boot/compressed/eboot.c +1 -1

@@ -474,6 +474,7 @@
 	unsigned long nr_pci;
 	struct setup_data *data;
 	int i;
+	int setup_pci_worked_once = 0;
 
 	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
 

@@ -495,7 +496,13 @@
 			continue;
 
 		status = __setup_efi_pci64(pci, &rom);
-		if (status != EFI_SUCCESS)

Quoted 4 lines · Andre

Quoted 2 lines · On Sat, 6 Sep 2014 23:02:51 +0200 Andre <andre.muller-S0/[email protected].…

With a Thinkpad T420 (pre-2.0 EFI), I do run into the failure condition for another reason:

Quoted 8 lines

Quoted 8 lines

setup_efi_pci64 gets called,
the loop therein is executed nr_pci=11 (!?) times.
The second call run calls into __setup_efi_pci64 successfully, the memcpy takes place; the free_struct: path is not used. Back in setup_efi_pci64, the non-data path is walked.

For all the other 10 runs of the loop, __setup_efi_pci64 fails at if (!pci->romimage || !pci->romsize)

        return EFI_INVALID_PARAMETER;

so that is the status here for any run but the second.

Thanks for doing the analysis.

Quoted 13 lines

Following up with code...

If it is OK to just leave the loop in setup_efi_pci64, this can be done simply by

x86/boot/compressed/eboot_break.c renamed from x86/boot/compressed/eboot.c +1 -0

@@ -504,6 +504,7 @@
 			params->hdr.setup_data = (unsigned long)rom;
 
 		data = (struct setup_data *)rom;
+		break;
 

Quoted 3 lines

x86/boot/compressed/eboot_break.c renamed from x86/boot/compressed/eboot.c +0 -0

 	}
 

But I've got severe doubts about this.

Right, we can't do this because we want to add all the PCI devices that we can find with EFI, not just the first one.

Quoted 3 lines

A solution limited to this particular failure mode could look like the below. It has all the charm of introducing a helper var. Hmpf.

The code is generally fine as-is.

Where I think we could improve things
is by adding efi_printk() message in certain error paths. Clearly, not all error paths need such messages, e.g. the EFI_INVALID_PARAMETER path you highlighted above, but it makes sense for memory allocation and PCI read failures.

Care to take a whack at that?

commit fb86b2440de0ec10fe0272eb19d262ae7a01adb8

Author: Ulf Winkelvos <[email protected]>
Date:   Thu Jul 10 02:12:41 2014 +0200

x86/efi: Add better error logging to EFI boot stub

Where I think we could improve things
is by adding efi_printk() message in certain error paths. Clearly, not all error paths need such messages, e.g. the EFI_INVALID_PARAMETER path you highlighted above, but it makes sense for memory allocation and PCI read failures.

All other calls to allocate memory seem to make some noise already, with the exception of two calls (for gop, uga) in the setup_graphics path.

The purpose is to be noisy on worrysome errors immediately.

Quoted 5 lines

commit fb86b2440de0ec10fe0272eb19d262ae7a01adb8

Author: Ulf Winkelvos <[email protected]>
Date:   Thu Jul 10 02:12:41 2014 +0200

x86/efi: Add better error logging to EFI boot stub

introduces printing false alarms for lots of hardware. Rather than playing Whack a Mole with non-fatal exit conditions, try the other way round.

This is per Matt Fleming's suggestion:

Quoted 5 lines

Where I think we could improve things
is by adding efi_printk() message in certain error paths. Clearly, not all error paths need such messages, e.g. the EFI_INVALID_PARAMETER path you highlighted above, but it makes sense for memory allocation and PCI read failures.

http://article.gmane.org/gmane.linux.kernel.efi/4628

After this, the status check on the setup_efi_pci() function can go away again.

Signed-off-by Andre Müller <andre.muller-S0/[email protected]>

---
 arch/x86/boot/compressed/eboot.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)