Re: EFI_STUB fails to boot non-EFI on arm64

2014-07-08 Thread Catalin Marinas 
I forgot about this thread. I think we need it sorted in some way.

On Fri, May 23, 2014 at 04:03:31PM +0100, Leif Lindholm wrote:
 On Fri, May 23, 2014 at 02:47:20PM +0100, Catalin Marinas wrote:
  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().

So, to be in line with 32-bit arm, the only way to tell the uncompressed
kernel image that it was started as an EFI application is via FDT.

 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.
[...]
 diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
 index cd36deb..4bb42e1e 100644
 --- a/drivers/firmware/efi/efi.c
 +++ b/drivers/firmware/efi/efi.c
 @@ -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;

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

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index eff1a2f22f09..dc79346689e6 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EFI_STUB fails to boot non-EFI on arm64

2014-07-08 Thread Leif Lindholm 
On Tue, Jul 08, 2014 at 10:21:00AM +0100, Catalin Marinas wrote:
 I forgot about this thread. I think we need it sorted in some way.

Agreed.
 
 On Fri, May 23, 2014 at 04:03:31PM +0100, Leif Lindholm wrote:
  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.
 [...]
  diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
  index cd36deb..4bb42e1e 100644
  --- a/drivers/firmware/efi/efi.c
  +++ b/drivers/firmware/efi/efi.c
  @@ -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;
 
 I'm ok with the idea but I don't particularly like the implementation.
 Does this look any better (functionally the same as yours)?

It solves the problem, so sure.
Acked-by: Leif Lindholm leif.lindh...@linaro.org
Tested-by: Leif Lindholm leif.lindh...@linaro.org

 diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
 index eff1a2f22f09..dc79346689e6 100644
 --- a/drivers/firmware/efi/efi.c
 +++ b/drivers/firmware/efi/efi.c
 @@ -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@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-28 Thread Will Deacon 
On Fri, May 23, 2014 at 04:03:31PM +0100, Leif Lindholm wrote:
 On Fri, May 23, 2014 at 02:47:20PM +0100, Catalin Marinas wrote:
  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?

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..

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

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-28 Thread Will Deacon 
On Wed, May 28, 2014 at 07:05:26PM +0100, Leif Lindholm wrote:
 On Wed, May 28, 2014 at 04:59:31PM +0100, Will Deacon wrote:
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?
  
  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.

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?

 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?

 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? ;)

 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.

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

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-28 Thread Leif Lindholm 
On Wed, May 28, 2014 at 07:40:43PM +0100, Will Deacon wrote:
  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?

The pr_err would now only be triggered if some UEFI properties were
present and not others. I.e. if the UEFI stub loader failed miserably
without detecting it, or something corrupted the DT later on. We would
then be the proud owners of a system in an undefined state.

  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?

Having a partial/invalid DT is going to cause undefined behaviour
regardless of your method of booting.

And if the data provided proved inaccessible or broken, the symptom
would be a complete lack of memblocks. So it may in fact not be
possible for that pr_err to ever make it to the console :)

  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? ;)

Name cargo-culted from ia64/x86 by me, and from me by Mark :)

  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.

And I still don't see how an extra call site in setup_arch would make
this more possible than it already is (with my suggested patch to the
current version of the code).

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

EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Catalin Marinas 
Hi,

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?


Linux version 3.15.0-rc1+ (cmarinas@e102109-lin) (gcc version 4.8.3 20140401 
(prerelease) (0.12.310) ) #451 SMP PREEMPT Fri May 23 10:40:10 BST 2014
CPU: AArch64 Processor [410fd0f0] revision 0
bootconsole [earlycon0] enabled
efi: Getting parameters from FDT:
efi: Can't find System Table in device tree!
cma: CMA: reserved 16 MiB at 8ff00
Trying ::1...
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
Unable to handle kernel NULL pointer dereference at virtual address 0020
pgd = ffc7d000
[0020] *pgd=
Internal error: Oops: 9605 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.15.0-rc1+ #451
task: ffc0005f4890 ti: ffc0005e8000 task.ti: ffc0005e8000
PC is at efi_idmap_init+0x74/0xc8
LR is at efi_idmap_init+0x4c/0xc8
pc : [ffc0005b7f54] lr : [ffc0005b7f2c] pstate: 62c5
sp : ffc0005ebee0
x29: ffc0005ebee0 x28: 00408000 
x27: ffc0005f60c8 x26: ffc0005f6000 
x25: ffc00053f618 x24: ffc0005f8fa8 
x23: ffc00060 x22: ffc000629000 
x21: ffc00060 x20: ffc000629338 
x19:  x18:  
x17: ffc000635d48 x16: 0001 
x15: 000a x14: 0009 
x13: 0018 x12: 00088000 
x11: ffc0 x10: 0040 
x9 : 02c00713 x8 : 4000 
x7 : 0008 x6 : ffc000629390 
x5 : ffc000629000 x4 :  
x3 : 0008ffe00711 x2 :  
x1 :  x0 :  

Process swapper (pid: 0, stack limit = 0xffc0005e8058)
Stack: (0xffc0005ebee0 to 0xffc0005ec000)
bee0: 005ebf10 ffc0 005b667c ffc0 7effdf80 ffc8 00635d78 ffc0
bf00: 005f6108 ffc0 005b65ec ffc0 005ebfa0 ffc0 005b4528 ffc0
bf20: 00629000 ffc0 0001  005d7fd8 ffc0 410fd0f0 
bf40: 805f6000  8000  8007b000  8007d000 
bf60: 00080590 ffc0 0044d088 ffc0 0001  8800 
bf80: 0062f278 ffc0 0002  0062ee32 ffc0  
bfa0:   80080330    0e12 
bfc0: 8800  410fd0f0  805f6000   
bfe0:   005d7fd8 ffc0    
Call trace:
[ffc0005b7f54] efi_idmap_init+0x74/0xc8
[ffc0005b6678] setup_arch+0x3d4/0x57c
[ffc0005b4524] start_kernel+0x88/0x364
Code: f9401a80 cb20 eb00027f 54000248 (f9401260) 
---[ end trace f3ac502fe4422ad7 ]---
Kernel panic - not syncing: Attempted to kill the idle task!
---[ end Kernel panic - not syncing: Attempted to kill the idle task!
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Leif Lindholm 
On Fri, May 23, 2014 at 10:45:13AM +0100, Catalin Marinas wrote:
 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?

Apparently not sufficuently...
Sorry about that.

Fix appended:

From 98433920394730d835f0061474832909c0740f29 Mon Sep 17 00:00:00 2001
From: Leif Lindholm leif.lindh...@linaro.org
Date: Fri, 23 May 2014 11:23:23 +0100
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 leif.lindh...@linaro.org
---
 arch/arm64/kernel/efi.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 7bfd650..14db1f6 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -333,6 +333,9 @@ void __init efi_init(void)
 
 void __init efi_idmap_init(void)
 {
+   if (!efi_enabled(EFI_BOOT))
+   return;
+
/* boot time idmap_pg_dir is incomplete, so fill in missing parts */
efi_setup_idmap();
 }
-- 
1.7.10.4


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

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Matt Fleming 
On Fri, 23 May, at 02:16:56PM, Leif Lindholm wrote:
 On Fri, May 23, 2014 at 10:45:13AM +0100, Catalin Marinas wrote:
  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?
 
 Apparently not sufficuently...
 Sorry about that.
 
 Fix appended:

This looks pretty straight forward. I've picked it up and shoved it on
my arm64-efi branch.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Catalin Marinas 
On Fri, May 23, 2014 at 02:16:56PM +0100, Leif Lindholm wrote:
 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 leif.lindh...@linaro.org
 ---
  arch/arm64/kernel/efi.c |3 +++
  1 file changed, 3 insertions(+)
 
 diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
 index 7bfd650..14db1f6 100644
 --- a/arch/arm64/kernel/efi.c
 +++ b/arch/arm64/kernel/efi.c
 @@ -333,6 +333,9 @@ void __init efi_init(void)
  
  void __init efi_idmap_init(void)
  {
 + if (!efi_enabled(EFI_BOOT))
 + return;
 +

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

Acked-by: Catalin Marinas catalin.mari...@arm.com

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().

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

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Leif Lindholm 
On Fri, May 23, 2014 at 02:47:20PM +0100, Catalin Marinas wrote:
 That's a first (possibly temporary) step and I think it's fine:
 
 Acked-by: Catalin Marinas catalin.mari...@arm.com
 
 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.

 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 leif.lindh...@linaro.org
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 leif.lindh...@linaro.org
---
 drivers/firmware/efi/efi.c |   15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd36deb..4bb42e1e 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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

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

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Leif Lindholm 
On Fri, May 23, 2014 at 05:17:39PM +0200, Ard Biesheuvel wrote:
  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?

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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Roy Franz 
On Fri, May 23, 2014 at 8:45 AM, Leif Lindholm leif.lindh...@linaro.org wrote:
 On Fri, May 23, 2014 at 05:17:39PM +0200, Ard Biesheuvel wrote:
  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?

 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

Also, for arm32 the decompressor is still used, so the kernel proper
only knows that it was started via the stub
based on device tree entries.  In the arm32 case the stub load the
initrd and sets up the device tree for the compressed
kernel just like any linux loader.

(I had a previous response to Ard's message that was html and got bounced.)

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

Re: EFI_STUB fails to boot non-EFI on arm64

2014-05-23 Thread Ard Biesheuvel 
On 23 May 2014 17:45, Leif Lindholm leif.lindh...@linaro.org wrote:

 On Fri, May 23, 2014 at 05:17:39PM +0200, Ard Biesheuvel wrote:
   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?

 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?


Yeah, you're right. Also, the ARM requirements are sufficiently
different (as Roy points out) that the presence of the FDT nodes is
the most reliable indicator of whether you can proceed booting in EFI
mode.

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