EFI_STUB fails to boot non-EFI on arm64

As the EFI_STUB for arm64 got into tip and soon into -next, I thought about giving it a try (tip/arm64/efi). Using my boot-wrapper (non-EFI) is still supposed to work but I get the trace below. It looks like memmap.map is NULL but the code still assumes the kernel was started as an EFI app.

Have you guys tested these patches properly?

From 98433920394730d835f0061474832909c0740f29 Mon Sep 17 00:00:00 2001

Quoted 6 lines · Catalin Marinas

As the EFI_STUB for arm64 got into tip and soon into -next, I thought about giving it a try (tip/arm64/efi). Using my boot-wrapper (non-EFI) is still supposed to work but I get the trace below. It looks like memmap.map is NULL but the code still assumes the kernel was started as an EFI app.

Quoted 1 line

Have you guys tested these patches properly?

Apparently not sufficuently...
Sorry about that.

Fix appended:

Subject: [PATCH] arm64: efi: only attempt efi map setup if booting via EFI

Booting a kernel with CONFIG_EFI enabled on a non-EFI system caused an oops with the current UEFI support code.
Add the required test to prevent this.

Signed-off-by Leif Lindholm <[email protected]>

---
 arch/arm64/kernel/efi.c |    3 +++
 1 file changed, 3 insertions(+)

arch/arm64/kernel/efi.c +0 -0

index 7bfd650..14db1f6 100644

That's a first (possibly temporary) step and I think it's fine:

Acked-by Catalin Marinas <[email protected]>

But we need some further tweaking to the way we call efi_init(). Currently it doesn't matter whether Linux booted as an EFI application or not and efi_init() is always called, causing some pr_err() in fdt_find_uefi_params(). It's not really an error as we support the same image booting non-EFI as well.

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

From 67283e60923c14c024460b4512c49563a92acce7 Mon Sep 17 00:00:00 2001

Quoted 9 lines · Catalin Marinas

That's a first (possibly temporary) step and I think it's fine:

Acked-by Catalin Marinas <[email protected]>

But we need some further tweaking to the way we call efi_init(). Currently it doesn't matter whether Linux booted as an EFI application or not and efi_init() is always called, causing some pr_err() in fdt_find_uefi_params(). It's not really an error as we support the same image booting non-EFI as well.

OK.

Quoted 4 lines

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

My view is that this should be fixed in fdt_find_uefi_params(). A single info message that we can't find evidence of UEFI should be printed in the non-error case.

Like below?

/
Leif

From 67283e60923c14c024460b4512c49563a92acce7 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <[email protected]>
Date: Fri, 23 May 2014 15:50:51 +0100
Subject: [PATCH] efi: fdt: Drop error messages for non-error case

Change fdt_find_uefi_params() to only write error messages if actual error encountered, rather than if no UEFI information is encountered. For the non-error case, print a single info message.

Signed-off-by Leif Lindholm <[email protected]>

---
 drivers/firmware/efi/efi.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

drivers/firmware/efi/efi.c +10 -6

index cd36deb..4bb42e1e 100644
@@ -366,11 +366,8 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 
        for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
                prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
-               if (!prop) {
-                       pr_err("Can't find %s in device tree!\n",
-                              dt_params[i].name);
-                       return 0;
-               }
+               if (!prop)
+                       goto fail;
                dest = info->params + dt_params[i].offset;
 
                val = of_read_number(prop, len / sizeof(u32));

@@ -385,6 +382,14 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
                                dt_params[i].size * 2, val);
        }
        return 1;
+
+       fail:
+       if (i == 0)
+               pr_info("  UEFI not found.\n");
+       else
+               pr_err("Can't find %s in device tree!\n", dt_params[i].name);
+
+       return 0;
 }
 
 int __init efi_get_fdt_params(struct efi_fdt_params *params, int verbose)
--

1.7.10.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 10 lines

Quoted 4 lines

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

Considering that

a) the raw Image loader and the stub enter the kernel through different entry points (i.e., offset #0 and whatever entry point is specified in the PE/COFF header, respectively), and
b) there is no decompressor etc involved so we jump straight into the kernel startup code
c) head.S already deals with a similar problem, i.e., storing the CPU boot mode

I would assume it shouldn't be so difficult to set a bit somewhere indicating which case we are dealing with?

Quoted 20 lines · Ard Biesheuvel

Quoted 10 lines

Quoted 4 lines

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

Considering that

a) the raw Image loader and the stub enter the kernel through different entry points (i.e., offset #0 and whatever entry point is specified in the PE/COFF header, respectively), and
b) there is no decompressor etc involved so we jump straight into the kernel startup code
c) head.S already deals with a similar problem, i.e., storing the CPU boot mode

Quoted 2 lines

I would assume it shouldn't be so difficult to set a bit somewhere indicating which case we are dealing with?

That would certainly be possible, but what would be the benefit of having two separate mechanisms for determining whether we are booting via UEFI - which could potentially end up disagreeing?

/
Leif

Quoted 20 lines · Ard Biesheuvel

Quoted 10 lines

Quoted 4 lines

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

Considering that

a) the raw Image loader and the stub enter the kernel through different entry points (i.e., offset #0 and whatever entry point is specified in the PE/COFF header, respectively), and
b) there is no decompressor etc involved so we jump straight into the kernel startup code
c) head.S already deals with a similar problem, i.e., storing the CPU boot mode

Quoted 2 lines

I would assume it shouldn't be so difficult to set a bit somewhere indicating which case we are dealing with?

That would certainly be possible, but what would be the benefit of having two separate mechanisms for determining whether we are booting via UEFI - which could potentially end up disagreeing?

Quoted 4 lines · Catalin Marinas

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

My view is that this should be fixed in fdt_find_uefi_params(). A single info message that we can't find evidence of UEFI should be printed in the non-error case.

Like below?

Quoted 14 lines

Quoted 4 lines

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

My view is that this should be fixed in fdt_find_uefi_params(). A single info message that we can't find evidence of UEFI should be printed in the non-error case.

Quoted 1 line

Like below?

Why not move the efi_get_fdt_params call out of efi_init and into setup_arch via a wrapper? Then efi_get_fdt_params and efi_init can have useful return values, which allow us to distinguish between "My DT doesn't have the necessary UEFI properties" and "UEFI failed to initialise" without having to make some printks pr_info and others pr_err within efi_init itself..

Quoted 14 lines · Will Deacon

Quoted 14 lines

Quoted 4 lines

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

Well, I agree that we shouldn't be spewing error messages for expected operation, but efi_init() is the function we call to determine whether we _are_ booting via UEFI - and it sets flags accordingly for the efi_enabled() macro.

My view is that this should be fixed in fdt_find_uefi_params(). A single info message that we can't find evidence of UEFI should be printed in the non-error case.

Quoted 8 lines

Quoted 1 line

Like below?

Why not move the efi_get_fdt_params call out of efi_init and into setup_arch via a wrapper? Then efi_get_fdt_params and efi_init can have useful return values, which allow us to distinguish between "My DT doesn't have the necessary UEFI properties" and "UEFI failed to initialise" without having to make some printks pr_info and others pr_err within efi_init itself..

Well, but (for the output part) my patch already did that? If the "Getting parameters from FDT:\n" was too verbose, we could just drop it, and have the same effect on output.

Thing is - there is not really any error case available anywhere during the execution of efi_init() and its branches other than:
- Information required for UEFI boot cannot be found.
- Information exists, but is invalid.
- Failed to early_memremap some UEFI regions into the kernel. which all amounts to "UEFI not available or something went wrong", rather than "UEFI failed to initialise".

If efi_init returns successfully, EFI_BOOT is set, and testable using the efi_enabled() macro.

The proper "UEFI failed to initialise" bit does not come until the early_initcall arm64_enter_virtual_mode(), and is indicated not by a return value, but by setting the flag indicating that EFI_RUNTIME_SERVICES are available, which is checked later in core code using the efi_enabled() macro.

So moving the call to efi_get_fdt_params() would have little effect other than adding a third call site for UEFI bits in setup_arch().

Quoted 3 lines

Well, but (for the output part) my patch already did that? If the "Getting parameters from FDT:\n" was too verbose, we could just drop it, and have the same effect on output.

It's the pr_err which is annoying, not the "Getting parameters from FDT:\n" message. Why should I have an error logged to my console when I was never intending to boot using EFI anyway?

Quoted 7 lines

Thing is - there is not really any error case available anywhere during the execution of efi_init() and its branches other than:
- Information required for UEFI boot cannot be found.
- Information exists, but is invalid.
- Failed to early_memremap some UEFI regions into the kernel. which all amounts to "UEFI not available or something went wrong", rather than "UEFI failed to initialise".

Fine, but in this case the DT had the relevant properties which is a good indication that the user was at least *trying* to boot using EFI, no?

Quoted 8 lines

If efi_init returns successfully, EFI_BOOT is set, and testable using the efi_enabled() macro.

The proper "UEFI failed to initialise" bit does not come until the early_initcall arm64_enter_virtual_mode(), and is indicated not by a return value, but by setting the flag indicating that EFI_RUNTIME_SERVICES are available, which is checked later in core code using the efi_enabled() macro.

Sorry, I naively assumed that with a name like efi_init it might, you know, initialise EFI? ;)

Quoted 2 lines

So moving the call to efi_get_fdt_params() would have little effect other than adding a third call site for UEFI bits in setup_arch().

I don't mind having the extra call site if it allows us to distinguish errors from information.

Quoted 4 lines · Catalin Marinas

Can we add another of detecting whether it's an EFI application and avoid calling efi_init()? I can see x86 sets some efi_loader_signature string in exit_boot() and checks against it later when calling efi_init().

My view is that this should be fixed in fdt_find_uefi_params(). A single info message that we can't find evidence of UEFI should be printed in the non-error case.

drivers/firmware/efi/efi.c +2 -5

index cd36deb..4bb42e1e 100644
@@ -366,11 +366,8 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 
 	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
 		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
-		if (!prop) {
-			pr_err("Can't find %s in device tree!\n",
-			       dt_params[i].name);
-			return 0;
-		}
+		if (!prop)
+			goto fail;
 		dest = info->params + dt_params[i].offset;
 
 		val = of_read_number(prop, len / sizeof(u32));

@@ -385,6 +382,14 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 				dt_params[i].size * 2, val);

I forgot about this thread. I think we need it sorted in some way.

Quoted 3 lines

My view is that this should be fixed in fdt_find_uefi_params(). A single info message that we can't find evidence of UEFI should be printed in the non-error case.

[...]

Quoted 17 lines

drivers/firmware/efi/efi.c +2 -5

index cd36deb..4bb42e1e 100644
@@ -366,11 +366,8 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 
 	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
 		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
-		if (!prop) {
-			pr_err("Can't find %s in device tree!\n",
-			       dt_params[i].name);
-			return 0;
-		}
+		if (!prop)
+			goto fail;
 		dest = info->params + dt_params[i].offset;
 

drivers/firmware/efi/efi.c static int __init fdt_find_uefi_params(unsigned long node, const char *uname, · line 392

Quoted 13 lines

drivers/firmware/efi/efi.c +8 -0

 		val = of_read_number(prop, len / sizeof(u32));

@@ -385,6 +382,14 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 				dt_params[i].size * 2, val);
 	}
 	return 1;
+
+	fail:
+	if (i == 0)
+		pr_info("  UEFI not found.\n");
+	else
+		pr_err("Can't find %s in device tree!\n", dt_params[i].name);
+
+	return 0;

I'm ok with the idea but I don't particularly like the implementation. Does this look any better (functionally the same as yours)?

drivers/firmware/efi/efi.c +15 -8

index eff1a2f22f09..dc79346689e6 100644
@@ -346,6 +346,7 @@ static __initdata struct {
 
 struct param_info {
 	int verbose;
+	int found;
 	void *params;
 };
 

@@ -362,16 +363,12 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 	    (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen at 0") != 0))
 		return 0;
 
-	pr_info("Getting parameters from FDT:\n");
-
 	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
 		prop = of_get_flat_dt_prop(node, dt_params[i].propname, &len);
-		if (!prop) {
-			pr_err("Can't find %s in device tree!\n",
-			       dt_params[i].name);
+		if (!prop)
 			return 0;
-		}
 		dest = info->params + dt_params[i].offset;
+		info->found++;
 
 		val = of_read_number(prop, len / sizeof(u32));
 

@@ -390,10 +387,21 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 int __init efi_get_fdt_params(struct efi_fdt_params *params, int verbose)
 {
 	struct param_info info;
+	int ret;
+
+	pr_info("Getting EFI parameters from FDT:\n");
 
 	info.verbose = verbose;
+	info.found = 0;
 	info.params = params;
 
-	return of_scan_flat_dt(fdt_find_uefi_params, &info);
+	ret = of_scan_flat_dt(fdt_find_uefi_params, &info);
+	if (!info.found)
+		pr_info("UEFI not found.\n");
+	else if (!ret)
+		pr_err("Can't find '%s' in device tree!\n",
+		       dt_params[info.found].name);
+
+	return ret;
 }
 #endif /* CONFIG_EFI_PARAMS_FROM_FDT */
--

To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html