LinuxLists.cc - [PATCH 0/3] unify efi debug output across architectures

[permalink] [raw]

Subject: [PATCH 1/3] efi/x86: move efi=debug option parsing to core

fed6cefe3b6e ("x86/efi: Add a "debug" option to the efi= cmdline")
adds the DBG flag, but does so for x86 only. Move this early param
parsing to core code.

Signed-off-by: Leif Lindholm <[email protected]>
---
arch/x86/platform/efi/efi.c | 2 --
drivers/firmware/efi/efi.c | 3 +++
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e4308fe..9757b79 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -979,8 +979,6 @@ static int __init arch_parse_efi_cmdline(char *str)

if (parse_option_str(str, "old_map"))
set_bit(EFI_OLD_MEMMAP, &efi.flags);
- if (parse_option_str(str, "debug"))
- set_bit(EFI_DBG, &efi.flags);

return 0;
}
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index d6144e3..78de0bd 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -63,6 +63,9 @@ static int __init parse_efi_cmdline(char *str)
return -EINVAL;
}

+ if (parse_option_str(str, "debug"))
+ set_bit(EFI_DBG, &efi.flags);
+
if (parse_option_str(str, "noruntime"))
disable_runtime = true;

--
2.1.4

2015-08-26 13:25:59

[permalink] [raw]

Subject: [PATCH 2/3] arm64: use core efi=debug instead of uefi_debug command line parameter

Now that we have an efi=debug command line option in the core code, use
this instead of the arm64-specific uefi_debug option.

Signed-off-by: Leif Lindholm <[email protected]>
---
Documentation/arm/uefi.txt | 2 --
arch/arm64/kernel/efi.c | 19 +++++--------------
2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
index d60030a..7b3fdfe 100644
--- a/Documentation/arm/uefi.txt
+++ b/Documentation/arm/uefi.txt
@@ -60,5 +60,3 @@ linux,uefi-mmap-desc-ver | 32-bit | Version of the mmap descriptor format.
--------------------------------------------------------------------------------
linux,uefi-stub-kern-ver | string | Copy of linux_banner from build.
--------------------------------------------------------------------------------
-
-For verbose debug messages, specify 'uefi_debug' on the kernel command line.
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6ea..612ad5e 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -51,15 +51,6 @@ static struct mm_struct efi_mm = {
INIT_MM_CONTEXT(efi_mm)
};

-static int uefi_debug __initdata;
-static int __init uefi_debug_setup(char *str)
-{
- uefi_debug = 1;
-
- return 0;
-}
-early_param("uefi_debug", uefi_debug_setup);
-
static int __init is_normal_ram(efi_memory_desc_t *md)
{
if (md->attribute & EFI_MEMORY_WB)
@@ -171,14 +162,14 @@ static __init void reserve_regions(void)
efi_memory_desc_t *md;
u64 paddr, npages, size;

- if (uefi_debug)
+ if (efi_enabled(EFI_DBG))
pr_info("Processing EFI memory map:\n");

for_each_efi_memory_desc(&memmap, md) {
paddr = md->phys_addr;
npages = md->num_pages;

- if (uefi_debug) {
+ if (efi_enabled(EFI_DBG)) {
char buf[64];

pr_info(" 0x%012llx-0x%012llx %s",
@@ -194,11 +185,11 @@ static __init void reserve_regions(void)

if (is_reserve_region(md)) {
memblock_reserve(paddr, size);
- if (uefi_debug)
+ if (efi_enabled(EFI_DBG))
pr_cont("*");
}

- if (uefi_debug)
+ if (efi_enabled(EFI_DBG))
pr_cont("\n");
}

@@ -210,7 +201,7 @@ void __init efi_init(void)
struct efi_fdt_params params;

/* Grab UEFI information placed in FDT by stub */
- if (!efi_get_fdt_params(&params, uefi_debug))
+ if (!efi_get_fdt_params(&params, efi_enabled(EFI_DBG)))
return;

efi_system_table = params.system_table;
--
2.1.4

2015-08-26 13:25:40

[permalink] [raw]

Subject: [PATCH 3/3] efi/arm64: clean up efi_get_fdt_params() interface

As we now have a common debug infrastructure between core and arm64 efi,
drop the bit of the interface passing verbose output flags around.

Signed-off-by: Leif Lindholm <[email protected]>
---
arch/arm64/kernel/efi.c | 2 +-
drivers/firmware/efi/efi.c | 6 ++----
include/linux/efi.h | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 612ad5e..ab5eeb6 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -201,7 +201,7 @@ void __init efi_init(void)
struct efi_fdt_params params;

/* Grab UEFI information placed in FDT by stub */
- if (!efi_get_fdt_params(&params, efi_enabled(EFI_DBG)))
+ if (!efi_get_fdt_params(&params))
return;

efi_system_table = params.system_table;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 78de0bd..aca6aaf 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -492,7 +492,6 @@ static __initdata struct {
};

struct param_info {
- int verbose;
int found;
void *params;
};
@@ -523,21 +522,20 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
else
*(u64 *)dest = val;

- if (info->verbose)
+ if (efi_enabled(EFI_DBG))
pr_info(" %s: 0x%0*llx\n", dt_params[i].name,
dt_params[i].size * 2, val);
}
return 1;
}

-int __init efi_get_fdt_params(struct efi_fdt_params *params, int verbose)
+int __init efi_get_fdt_params(struct efi_fdt_params *params)
{
struct param_info info;
int ret;

pr_info("Getting EFI parameters from FDT:\n");

- info.verbose = verbose;
info.found = 0;
info.params = params;

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 85ef051..b1a5b74 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -901,7 +901,7 @@ extern void efi_initialize_iomem_resources(struct resource *code_resource,
struct resource *data_resource, struct resource *bss_resource);
extern void efi_get_time(struct timespec *now);
extern void efi_reserve_boot_services(void);
-extern int efi_get_fdt_params(struct efi_fdt_params *params, int verbose);
+extern int efi_get_fdt_params(struct efi_fdt_params *params);
extern struct efi_memory_map memmap;
extern struct kobject *efi_kobj;

--
2.1.4

2015-08-26 13:46:40

[permalink] [raw]

Subject: Re: [PATCH 0/3] unify efi debug output across architectures

On 26 August 2015 at 15:24, Leif Lindholm <[email protected]> wrote:
> efi didn't use to have much in the way of kernel command line options,
> until some point last year. Then once the efi= option was added, it has
> been expanded on.
>
> x86 had an efi=debug option added, but that would be a useful thing
> across all efi architectures.
>
> arm64 efi support (predating efi=debug) came with its own 'uefi_debug'
> option, and passed along a special verbosity flag when calling into
> some not-strictly-arch-specific functions in core efi code.
>
> So:
> - Move efi=debug into core code, to be usable by all architectures
> - Change arm64 efi code to use 'efi=debug' instead of 'uefi_debug'
> - Rework arm64 interface to core code to drop special verbosity flag
>

For the series:

Acked-by: Ard Biesheuvel <[email protected]>
Tested-by: Ard Biesheuvel <[email protected]>

> Leif Lindholm (3):
> efi/x86: move efi=debug option parsing to core
> arm64: use core efi=debug instead of uefi_debug command line parameter
> efi/arm64: clean up efi_get_fdt_params() interface
>
> Documentation/arm/uefi.txt | 2 --
> arch/arm64/kernel/efi.c | 19 +++++--------------
> arch/x86/platform/efi/efi.c | 2 --
> drivers/firmware/efi/efi.c | 9 +++++----
> include/linux/efi.h | 2 +-
> 5 files changed, 11 insertions(+), 23 deletions(-)
>
> --
> 2.1.4
>

2015-08-26 14:04:59

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi/arm64: clean up efi_get_fdt_params() interface

On 26 August 2015 at 15:24, Leif Lindholm <[email protected]> wrote:
> As we now have a common debug infrastructure between core and arm64 efi,
> drop the bit of the interface passing verbose output flags around.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> arch/arm64/kernel/efi.c | 2 +-
> drivers/firmware/efi/efi.c | 6 ++----
> include/linux/efi.h | 2 +-
> 3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 612ad5e..ab5eeb6 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -201,7 +201,7 @@ void __init efi_init(void)
> struct efi_fdt_params params;
>

Actually, one nit: struct efi_fdt_params has a verbose field that was
never used in the first place, but was added [supposedly] to convey
the same as EFI_DBG that we have now. So I think you should remove
that as well.

--
Ard.

> /* Grab UEFI information placed in FDT by stub */
> - if (!efi_get_fdt_params(&params, efi_enabled(EFI_DBG)))
> + if (!efi_get_fdt_params(&params))
> return;
>
> efi_system_table = params.system_table;
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 78de0bd..aca6aaf 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -492,7 +492,6 @@ static __initdata struct {
> };
>
> struct param_info {
> - int verbose;
> int found;
> void *params;
> };
> @@ -523,21 +522,20 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
> else
> *(u64 *)dest = val;
>
> - if (info->verbose)
> + if (efi_enabled(EFI_DBG))
> pr_info(" %s: 0x%0*llx\n", dt_params[i].name,
> dt_params[i].size * 2, val);
> }
> return 1;
> }
>
> -int __init efi_get_fdt_params(struct efi_fdt_params *params, int verbose)
> +int __init efi_get_fdt_params(struct efi_fdt_params *params)
> {
> struct param_info info;
> int ret;
>
> pr_info("Getting EFI parameters from FDT:\n");
>
> - info.verbose = verbose;
> info.found = 0;
> info.params = params;
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 85ef051..b1a5b74 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -901,7 +901,7 @@ extern void efi_initialize_iomem_resources(struct resource *code_resource,
> struct resource *data_resource, struct resource *bss_resource);
> extern void efi_get_time(struct timespec *now);
> extern void efi_reserve_boot_services(void);
> -extern int efi_get_fdt_params(struct efi_fdt_params *params, int verbose);
> +extern int efi_get_fdt_params(struct efi_fdt_params *params);
> extern struct efi_memory_map memmap;
> extern struct kobject *efi_kobj;
>
> --
> 2.1.4
>

2015-08-26 14:15:07

by Mark Salter

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi/arm64: clean up efi_get_fdt_params() interface

On Wed, 2015-08-26 at 16:04 +0200, Ard Biesheuvel wrote:
> On 26 August 2015 at 15:24, Leif Lindholm <[email protected]>
> wrote:
> > As we now have a common debug infrastructure between core and arm64
> > efi,
> > drop the bit of the interface passing verbose output flags around.
> >
> > Signed-off-by: Leif Lindholm <[email protected]>
> > ---
> > arch/arm64/kernel/efi.c | 2 +-
> > drivers/firmware/efi/efi.c | 6 ++----
> > include/linux/efi.h | 2 +-
> > 3 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> > index 612ad5e..ab5eeb6 100644
> > --- a/arch/arm64/kernel/efi.c
> > +++ b/arch/arm64/kernel/efi.c
> > @@ -201,7 +201,7 @@ void __init efi_init(void)
> > struct efi_fdt_params params;
> >
>
> Actually, one nit: struct efi_fdt_params has a verbose field that was

No it doesn't. :)

> never used in the first place, but was added [supposedly] to convey
> the same as EFI_DBG that we have now. So I think you should remove
> that as well.
>

2015-08-26 14:16:27

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi/arm64: clean up efi_get_fdt_params() interface

On 26 August 2015 at 16:15, Mark Salter <[email protected]> wrote:
> On Wed, 2015-08-26 at 16:04 +0200, Ard Biesheuvel wrote:
>> On 26 August 2015 at 15:24, Leif Lindholm <[email protected]>
>> wrote:
>> > As we now have a common debug infrastructure between core and arm64
>> > efi,
>> > drop the bit of the interface passing verbose output flags around.
>> >
>> > Signed-off-by: Leif Lindholm <[email protected]>
>> > ---
>> > arch/arm64/kernel/efi.c | 2 +-
>> > drivers/firmware/efi/efi.c | 6 ++----
>> > include/linux/efi.h | 2 +-
>> > 3 files changed, 4 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> > index 612ad5e..ab5eeb6 100644
>> > --- a/arch/arm64/kernel/efi.c
>> > +++ b/arch/arm64/kernel/efi.c
>> > @@ -201,7 +201,7 @@ void __init efi_init(void)
>> > struct efi_fdt_params params;
>> >
>>
>> Actually, one nit: struct efi_fdt_params has a verbose field that was
>
> No it doesn't. :)
>

OK, must have been something I added myself locally then.
Never mind.

2015-08-26 17:25:00

[permalink] [raw]

Subject: Re: [PATCH 2/3] arm64: use core efi=debug instead of uefi_debug command line parameter

On Wed, 26 Aug, at 02:24:57PM, Leif Lindholm wrote:
> Now that we have an efi=debug command line option in the core code, use
> this instead of the arm64-specific uefi_debug option.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> Documentation/arm/uefi.txt | 2 --
> arch/arm64/kernel/efi.c | 19 +++++--------------
> 2 files changed, 5 insertions(+), 16 deletions(-)

Are there no concerns with backwards-compatibility here? I expected
this patch to internally convert uefi_debug to efi=debug if old boot
loaders, kernel config, etc specify it.

Is that something that can be safely ignored?

--
Matt Fleming, Intel Open Source Technology Center

2015-08-26 17:25:45

[permalink] [raw]

Subject: Re: [PATCH 1/3] efi/x86: move efi=debug option parsing to core

On Wed, 26 Aug, at 02:24:56PM, Leif Lindholm wrote:
> fed6cefe3b6e ("x86/efi: Add a "debug" option to the efi= cmdline")
> adds the DBG flag, but does so for x86 only. Move this early param
> parsing to core code.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 2 --
> drivers/firmware/efi/efi.c | 3 +++
> 2 files changed, 3 insertions(+), 2 deletions(-)

Nice clean up. Thanks Leif, applied.

--
Matt Fleming, Intel Open Source Technology Center

2015-08-26 17:30:06

[permalink] [raw]

Subject: Re: [PATCH 2/3] arm64: use core efi=debug instead of uefi_debug command line parameter

On Wed, Aug 26, 2015 at 06:24:55PM +0100, Matt Fleming wrote:
> On Wed, 26 Aug, at 02:24:57PM, Leif Lindholm wrote:
> > Now that we have an efi=debug command line option in the core code, use
> > this instead of the arm64-specific uefi_debug option.
> >
> > Signed-off-by: Leif Lindholm <[email protected]>
> > ---
> > Documentation/arm/uefi.txt | 2 --
> > arch/arm64/kernel/efi.c | 19 +++++--------------
> > 2 files changed, 5 insertions(+), 16 deletions(-)
>
> Are there no concerns with backwards-compatibility here? I expected
> this patch to internally convert uefi_debug to efi=debug if old boot
> loaders, kernel config, etc specify it.
>
> Is that something that can be safely ignored?

Since it is purely a debug feature, I opted to just drop it.
I don't have strong feelings, and it could certainly be kept as an
arm64-only alias (but setting the EFI_DBG flag) if others disagree.

/
Leif

2015-08-26 18:29:29

[permalink] [raw]

Subject: Re: [PATCH 2/3] arm64: use core efi=debug instead of uefi_debug command line parameter

On 26 August 2015 at 19:30, Leif Lindholm <[email protected]> wrote:
> On Wed, Aug 26, 2015 at 06:24:55PM +0100, Matt Fleming wrote:
>> On Wed, 26 Aug, at 02:24:57PM, Leif Lindholm wrote:
>> > Now that we have an efi=debug command line option in the core code, use
>> > this instead of the arm64-specific uefi_debug option.
>> >
>> > Signed-off-by: Leif Lindholm <[email protected]>
>> > ---
>> > Documentation/arm/uefi.txt | 2 --
>> > arch/arm64/kernel/efi.c | 19 +++++--------------
>> > 2 files changed, 5 insertions(+), 16 deletions(-)
>>
>> Are there no concerns with backwards-compatibility here? I expected
>> this patch to internally convert uefi_debug to efi=debug if old boot
>> loaders, kernel config, etc specify it.
>>
>> Is that something that can be safely ignored?
>
> Since it is purely a debug feature, I opted to just drop it.
> I don't have strong feelings, and it could certainly be kept as an
> arm64-only alias (but setting the EFI_DBG flag) if others disagree.
>

Let's not bother. This is strictly a debug option that only affects
what gets printed to the kernel log, which itself is not (or should
not be) an ABI. Also, if anyone cares deeply about always having the
feature enabled both on before and after kernels, they can always pass
both options.

--
Ard.

2015-08-28 13:32:09

[permalink] [raw]

Subject: Re: [PATCH 2/3] arm64: use core efi=debug instead of uefi_debug command line parameter

2015-08-28 13:32:23