| Message ID | 20140523150331.GS4179@bivouac.eciton.net |
|---|---|
| State | New |
| Headers | show |
On 23 May 2014 17:03, Leif Lindholm <leif.lindholm@linaro.org> wrote: > 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.marinas@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. > 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?
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
On Fri, May 23, 2014 at 8:45 AM, Leif Lindholm <leif.lindholm@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
On 23 May 2014 17:45, Leif Lindholm <leif.lindholm@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.
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
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. 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(). / Leif
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
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
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)