LinuxLists.cc - [PATCH 00/10] efi: some cleanups/refactoring for efi/next

[permalink] [raw]

Subject: [PATCH 09/10] efi/x86: Support builtin command line

Add support for the x86 CMDLINE_BOOL and CMDLINE_OVERRIDE configuration
options.

Signed-off-by: Arvind Sankar <[email protected]>
---
drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 85a924fecc87..0faba30d6406 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -680,7 +680,6 @@ unsigned long efi_main(efi_handle_t handle,
unsigned long buffer_start, buffer_end;
struct setup_header *hdr = &boot_params->hdr;
efi_status_t status;
- unsigned long cmdline_paddr;

efi_system_table = sys_table_arg;

@@ -739,9 +738,14 @@ unsigned long efi_main(efi_handle_t handle,
image_offset = 0;
}

- cmdline_paddr = ((u64)hdr->cmd_line_ptr |
- ((u64)boot_params->ext_cmd_line_ptr << 32));
- efi_parse_options((char *)cmdline_paddr);
+#ifdef CONFIG_CMDLINE_BOOL
+ efi_parse_options(CONFIG_CMDLINE);
+#endif
+ if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+ unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+ ((u64)boot_params->ext_cmd_line_ptr << 32));
+ efi_parse_options((char *)cmdline_paddr);
+ }

/*
* At this point, an initrd may already have been loaded by the
--
2.26.2

2020-04-29 17:46:28

[permalink] [raw]

Subject: [PATCH 05/10] efi/tpm: Use pr_efi_err for error messages

Use pr_efi_err instead of bare efi_printk for error messages.

Signed-off-by: Arvind Sankar <[email protected]>
---
drivers/firmware/efi/libstub/tpm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 1d59e103a2e3..8a16983fad98 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -119,7 +119,7 @@ void efi_retrieve_tpm2_eventlog(void)
sizeof(*log_tbl) + log_size, (void **)&log_tbl);

if (status != EFI_SUCCESS) {
- efi_printk("Unable to allocate memory for event log\n");
+ pr_efi_err("Unable to allocate memory for event log\n");
return;
}

--
2.26.2

2020-04-29 17:46:41

[permalink] [raw]

Subject: [PATCH 02/10] efi/libstub: Add a helper function to split 64-bit values

In several places 64-bit values need to be split up into two 32-bit
fields, in order to be backward-compatible with the old 32-bit ABIs.

Instead of open-coding this, add a helper function to set a 64-bit value
as two 32-bit fields.

Signed-off-by: Arvind Sankar <[email protected]>
---
drivers/firmware/efi/libstub/efistub.h | 7 ++++++
drivers/firmware/efi/libstub/gop.c | 6 ++---
drivers/firmware/efi/libstub/x86-stub.c | 32 +++++++++++--------------
3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 5ff63230a1f1..e8aa70ba08d5 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -87,6 +87,13 @@ extern const efi_system_table_t *efi_system_table;
((handle = efi_get_handle_at((array), i)) || true); \
i++)

+static inline
+void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
+{
+ *lo = lower_32_bits(data);
+ *hi = upper_32_bits(data);
+}
+
/*
* Allocation types for calls to boottime->allocate_pages.
*/
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 216327d0b034..64cee0febae0 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -422,7 +422,6 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto,
efi_graphics_output_protocol_t *gop;
efi_graphics_output_protocol_mode_t *mode;
efi_graphics_output_mode_info_t *info;
- efi_physical_addr_t fb_base;

gop = find_gop(proto, size, handles);

@@ -442,9 +441,8 @@ static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto,
si->lfb_width = info->horizontal_resolution;
si->lfb_height = info->vertical_resolution;

- fb_base = efi_table_attr(mode, frame_buffer_base);
- si->lfb_base = lower_32_bits(fb_base);
- si->ext_lfb_base = upper_32_bits(fb_base);
+ efi_set_u64_split(efi_table_attr(mode, frame_buffer_base),
+ &si->lfb_base, &si->ext_lfb_base);
if (si->ext_lfb_base)
si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index d4bafd7f6f9f..677b5a1e0543 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -408,9 +408,8 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
if (!cmdline_ptr)
goto fail;

- hdr->cmd_line_ptr = (unsigned long)cmdline_ptr;
- /* Fill in upper bits of command line address, NOP on 32 bit */
- boot_params->ext_cmd_line_ptr = (u64)(unsigned long)cmdline_ptr >> 32;
+ efi_set_u64_split((u64)cmdline_ptr,
+ &hdr->cmd_line_ptr, &boot_params->ext_cmd_line_ptr);

hdr->ramdisk_image = 0;
hdr->ramdisk_size = 0;
@@ -427,10 +426,10 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
ULONG_MAX);
if (status != EFI_SUCCESS)
goto fail2;
- hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
- hdr->ramdisk_size = ramdisk_size & 0xffffffff;
- boot_params->ext_ramdisk_image = (u64)ramdisk_addr >> 32;
- boot_params->ext_ramdisk_size = (u64)ramdisk_size >> 32;
+ efi_set_u64_split(ramdisk_addr, &hdr->ramdisk_image,
+ &boot_params->ext_ramdisk_image);
+ efi_set_u64_split(ramdisk_size, &hdr->ramdisk_size,
+ &boot_params->ext_ramdisk_size);
}
}

@@ -639,17 +638,14 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
: EFI32_LOADER_SIGNATURE;
memcpy(&p->efi->efi_loader_signature, signature, sizeof(__u32));

- p->efi->efi_systab = (unsigned long)efi_system_table;
+ efi_set_u64_split((u64)efi_system_table,
+ &p->efi->efi_systab, &p->efi->efi_systab_hi);
p->efi->efi_memdesc_size = *map->desc_size;
p->efi->efi_memdesc_version = *map->desc_ver;
- p->efi->efi_memmap = (unsigned long)*map->map;
+ efi_set_u64_split((u64)*map->map,
+ &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
p->efi->efi_memmap_size = *map->map_size;

-#ifdef CONFIG_X86_64
- p->efi->efi_systab_hi = (unsigned long)efi_system_table >> 32;
- p->efi->efi_memmap_hi = (unsigned long)*map->map >> 32;
-#endif
-
return EFI_SUCCESS;
}

@@ -785,10 +781,10 @@ unsigned long efi_main(efi_handle_t handle,

status = efi_load_initrd_dev_path(&addr, &size, ULONG_MAX);
if (status == EFI_SUCCESS) {
- hdr->ramdisk_image = (u32)addr;
- hdr->ramdisk_size = (u32)size;
- boot_params->ext_ramdisk_image = (u64)addr >> 32;
- boot_params->ext_ramdisk_size = (u64)size >> 32;
+ efi_set_u64_split(addr, &hdr->ramdisk_image,
+ &boot_params->ext_ramdisk_image);
+ efi_set_u64_split(size, &hdr->ramdisk_size,
+ &boot_params->ext_ramdisk_size);
} else if (status != EFI_NOT_FOUND) {
efi_printk("efi_load_initrd_dev_path() failed!\n");
goto fail;
--
2.26.2

2020-04-29 19:10:55

[permalink] [raw]

Subject: Re: [PATCH 09/10] efi/x86: Support builtin command line

On Wed, 29 Apr 2020 at 19:41, Arvind Sankar <[email protected]> wrote:
>
> Add support for the x86 CMDLINE_BOOL and CMDLINE_OVERRIDE configuration
> options.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 85a924fecc87..0faba30d6406 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -680,7 +680,6 @@ unsigned long efi_main(efi_handle_t handle,
> unsigned long buffer_start, buffer_end;
> struct setup_header *hdr = &boot_params->hdr;
> efi_status_t status;
> - unsigned long cmdline_paddr;
>
> efi_system_table = sys_table_arg;
>
> @@ -739,9 +738,14 @@ unsigned long efi_main(efi_handle_t handle,
> image_offset = 0;
> }
>
> - cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> - ((u64)boot_params->ext_cmd_line_ptr << 32));
> - efi_parse_options((char *)cmdline_paddr);
> +#ifdef CONFIG_CMDLINE_BOOL
> + efi_parse_options(CONFIG_CMDLINE);
> +#endif

Can we use IS_ENABLED() here as well?

> + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> + ((u64)boot_params->ext_cmd_line_ptr << 32));
> + efi_parse_options((char *)cmdline_paddr);
> + }
>
> /*
> * At this point, an initrd may already have been loaded by the
> --
> 2.26.2
>

2020-04-29 21:41:49

[permalink] [raw]

Subject: Re: [PATCH 09/10] efi/x86: Support builtin command line

On Wed, Apr 29, 2020 at 09:07:32PM +0200, Ard Biesheuvel wrote:
> On Wed, 29 Apr 2020 at 19:41, Arvind Sankar <[email protected]> wrote:
> >
> > Add support for the x86 CMDLINE_BOOL and CMDLINE_OVERRIDE configuration
> > options.
> >
> > Signed-off-by: Arvind Sankar <[email protected]>
> > ---
> > drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index 85a924fecc87..0faba30d6406 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -680,7 +680,6 @@ unsigned long efi_main(efi_handle_t handle,
> > unsigned long buffer_start, buffer_end;
> > struct setup_header *hdr = &boot_params->hdr;
> > efi_status_t status;
> > - unsigned long cmdline_paddr;
> >
> > efi_system_table = sys_table_arg;
> >
> > @@ -739,9 +738,14 @@ unsigned long efi_main(efi_handle_t handle,
> > image_offset = 0;
> > }
> >
> > - cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> > - ((u64)boot_params->ext_cmd_line_ptr << 32));
> > - efi_parse_options((char *)cmdline_paddr);
> > +#ifdef CONFIG_CMDLINE_BOOL
> > + efi_parse_options(CONFIG_CMDLINE);
> > +#endif
>
> Can we use IS_ENABLED() here as well?

Unfortunately on x86, CONFIG_CMDLINE is not defined if
CONFIG_CMDLINE_BOOL isn't enabled. So turning this into an
IS_ENABLED(CONFIG_CMDLINE_BOOL) causes a compile error when it's
disabled due to CONFIG_CMDLINE being an undeclared symbol.

>
> > + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> > + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> > + ((u64)boot_params->ext_cmd_line_ptr << 32));
> > + efi_parse_options((char *)cmdline_paddr);
> > + }
> >
> > /*
> > * At this point, an initrd may already have been loaded by the
> > --
> > 2.26.2
> >

2020-04-29 21:45:31

[permalink] [raw]

Subject: Re: [PATCH 09/10] efi/x86: Support builtin command line

On Wed, 29 Apr 2020 at 23:39, Arvind Sankar <[email protected]> wrote:
>
> On Wed, Apr 29, 2020 at 09:07:32PM +0200, Ard Biesheuvel wrote:
> > On Wed, 29 Apr 2020 at 19:41, Arvind Sankar <[email protected]> wrote:
> > >
> > > Add support for the x86 CMDLINE_BOOL and CMDLINE_OVERRIDE configuration
> > > options.
> > >
> > > Signed-off-by: Arvind Sankar <[email protected]>
> > > ---
> > > drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > index 85a924fecc87..0faba30d6406 100644
> > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > @@ -680,7 +680,6 @@ unsigned long efi_main(efi_handle_t handle,
> > > unsigned long buffer_start, buffer_end;
> > > struct setup_header *hdr = &boot_params->hdr;
> > > efi_status_t status;
> > > - unsigned long cmdline_paddr;
> > >
> > > efi_system_table = sys_table_arg;
> > >
> > > @@ -739,9 +738,14 @@ unsigned long efi_main(efi_handle_t handle,
> > > image_offset = 0;
> > > }
> > >
> > > - cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> > > - ((u64)boot_params->ext_cmd_line_ptr << 32));
> > > - efi_parse_options((char *)cmdline_paddr);
> > > +#ifdef CONFIG_CMDLINE_BOOL
> > > + efi_parse_options(CONFIG_CMDLINE);
> > > +#endif
> >
> > Can we use IS_ENABLED() here as well?
>
> Unfortunately on x86, CONFIG_CMDLINE is not defined if
> CONFIG_CMDLINE_BOOL isn't enabled. So turning this into an
> IS_ENABLED(CONFIG_CMDLINE_BOOL) causes a compile error when it's
> disabled due to CONFIG_CMDLINE being an undeclared symbol.
>

What about

efi_parse_options(CONFIG_CMDLINE "");

?

2020-04-29 21:51:39

[permalink] [raw]

Subject: Re: [PATCH 09/10] efi/x86: Support builtin command line

On Wed, Apr 29, 2020 at 11:40:51PM +0200, Ard Biesheuvel wrote:
> On Wed, 29 Apr 2020 at 23:39, Arvind Sankar <[email protected]> wrote:
> >
> > On Wed, Apr 29, 2020 at 09:07:32PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 29 Apr 2020 at 19:41, Arvind Sankar <[email protected]> wrote:
> > > >
> > > > Add support for the x86 CMDLINE_BOOL and CMDLINE_OVERRIDE configuration
> > > > options.
> > > >
> > > > Signed-off-by: Arvind Sankar <[email protected]>
> > > > ---
> > > > drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++++----
> > > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > > index 85a924fecc87..0faba30d6406 100644
> > > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > > @@ -680,7 +680,6 @@ unsigned long efi_main(efi_handle_t handle,
> > > > unsigned long buffer_start, buffer_end;
> > > > struct setup_header *hdr = &boot_params->hdr;
> > > > efi_status_t status;
> > > > - unsigned long cmdline_paddr;
> > > >
> > > > efi_system_table = sys_table_arg;
> > > >
> > > > @@ -739,9 +738,14 @@ unsigned long efi_main(efi_handle_t handle,
> > > > image_offset = 0;
> > > > }
> > > >
> > > > - cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> > > > - ((u64)boot_params->ext_cmd_line_ptr << 32));
> > > > - efi_parse_options((char *)cmdline_paddr);
> > > > +#ifdef CONFIG_CMDLINE_BOOL
> > > > + efi_parse_options(CONFIG_CMDLINE);
> > > > +#endif
> > >
> > > Can we use IS_ENABLED() here as well?
> >
> > Unfortunately on x86, CONFIG_CMDLINE is not defined if
> > CONFIG_CMDLINE_BOOL isn't enabled. So turning this into an
> > IS_ENABLED(CONFIG_CMDLINE_BOOL) causes a compile error when it's
> > disabled due to CONFIG_CMDLINE being an undeclared symbol.
> >
>
> What about
>
> efi_parse_options(CONFIG_CMDLINE "");
>
> ?

That's still a syntax error if CONFIG_CMDLINE is undefined, no? It's not
defined to be empty -- it's undefined. IS_ENABLED doesn't work on
string-valued options so I can't use IS_ENABLED(CONFIG_CMDLINE) either.

2020-04-29 21:54:11

[permalink] [raw]

Subject: Re: [PATCH 09/10] efi/x86: Support builtin command line

On Wed, 29 Apr 2020 at 23:48, Arvind Sankar <[email protected]> wrote:
>
> On Wed, Apr 29, 2020 at 11:40:51PM +0200, Ard Biesheuvel wrote:
> > On Wed, 29 Apr 2020 at 23:39, Arvind Sankar <[email protected]> wrote:
> > >
> > > On Wed, Apr 29, 2020 at 09:07:32PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 29 Apr 2020 at 19:41, Arvind Sankar <[email protected]> wrote:
> > > > >
> > > > > Add support for the x86 CMDLINE_BOOL and CMDLINE_OVERRIDE configuration
> > > > > options.
> > > > >
> > > > > Signed-off-by: Arvind Sankar <[email protected]>
> > > > > ---
> > > > > drivers/firmware/efi/libstub/x86-stub.c | 12 ++++++++----
> > > > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > > > index 85a924fecc87..0faba30d6406 100644
> > > > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > > > @@ -680,7 +680,6 @@ unsigned long efi_main(efi_handle_t handle,
> > > > > unsigned long buffer_start, buffer_end;
> > > > > struct setup_header *hdr = &boot_params->hdr;
> > > > > efi_status_t status;
> > > > > - unsigned long cmdline_paddr;
> > > > >
> > > > > efi_system_table = sys_table_arg;
> > > > >
> > > > > @@ -739,9 +738,14 @@ unsigned long efi_main(efi_handle_t handle,
> > > > > image_offset = 0;
> > > > > }
> > > > >
> > > > > - cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> > > > > - ((u64)boot_params->ext_cmd_line_ptr << 32));
> > > > > - efi_parse_options((char *)cmdline_paddr);
> > > > > +#ifdef CONFIG_CMDLINE_BOOL
> > > > > + efi_parse_options(CONFIG_CMDLINE);
> > > > > +#endif
> > > >
> > > > Can we use IS_ENABLED() here as well?
> > >
> > > Unfortunately on x86, CONFIG_CMDLINE is not defined if
> > > CONFIG_CMDLINE_BOOL isn't enabled. So turning this into an
> > > IS_ENABLED(CONFIG_CMDLINE_BOOL) causes a compile error when it's
> > > disabled due to CONFIG_CMDLINE being an undeclared symbol.
> > >
> >
> > What about
> >
> > efi_parse_options(CONFIG_CMDLINE "");
> >
> > ?
>
> That's still a syntax error if CONFIG_CMDLINE is undefined, no? It's not
> defined to be empty -- it's undefined. IS_ENABLED doesn't work on
> string-valued options so I can't use IS_ENABLED(CONFIG_CMDLINE) either.

I see. Not the end of the world to have to keep it as is, I was just curious.

2020-04-30 18:32:35

[permalink] [raw]

Subject: [PATCH v2 00/11] efi: some cleanups/refactoring for efi/next

This series is on top of efi/next.

Patch 1 fixes the size allocated for x86 boot_params.
Patch 2 refactors the setting of various hi/lo 32-bit fields, mainly on x86.
Patch 3 renames pr_efi/pr_efi_err
Patches 4-6 convert the remaining uses of efi_printk to print error
messages to use efi_err instead.
Patch 7 updates dtb= ignored message to efi_err.
Patches 8-9 refactor initrd loading, moving it into efi-stub-helper.
Patch 10 adds support for x86 builtin command line.
Patch 11 adds error checking for efi_parse_options.

Changes from v1:
- Rename pr_efi/pr_efi_err
- Drop the soft_limit-removing patch
- Fix a couple of compile warnings

Arvind Sankar (11):
efi/x86: Use correct size for boot_params
efi/libstub: Add a helper function to split 64-bit values
efi/libstub: Move pr_efi/pr_efi_err into efi namespace
efi/x86: Use efi_err for error messages
efi/gop: Use efi_err for error messages
efi/tpm: Use efi_err for error messages
efi/libstub: Upgrade ignored dtb= argument message to error
efi/x86: Move command-line initrd loading to efi_main
efi/libstub: Unify initrd loading across architectures
efi/x86: Support builtin command line
efi/libstub: Check return value of efi_parse_options

drivers/firmware/efi/libstub/arm32-stub.c | 12 +-
drivers/firmware/efi/libstub/arm64-stub.c | 14 +-
.../firmware/efi/libstub/efi-stub-helper.c | 46 ++++++-
drivers/firmware/efi/libstub/efi-stub.c | 63 ++++-----
drivers/firmware/efi/libstub/efistub.h | 32 ++---
drivers/firmware/efi/libstub/fdt.c | 16 +--
drivers/firmware/efi/libstub/file.c | 12 +-
drivers/firmware/efi/libstub/gop.c | 16 +--
drivers/firmware/efi/libstub/pci.c | 8 +-
drivers/firmware/efi/libstub/relocate.c | 2 +-
drivers/firmware/efi/libstub/secureboot.c | 4 +-
drivers/firmware/efi/libstub/tpm.c | 2 +-
drivers/firmware/efi/libstub/x86-stub.c | 122 ++++++++----------
13 files changed, 186 insertions(+), 163 deletions(-)

--
2.26.2

2020-04-30 18:32:49

[permalink] [raw]

Subject: [PATCH v2 03/11] efi/libstub: Move pr_efi/pr_efi_err into efi namespace

Rename pr_efi to efi_info and pr_efi_err to efi_err to make it more
obvious that they are part of the EFI stub and not generic printk infra.

Signed-off-by: Arvind Sankar <[email protected]>
Suggested-by: Joe Perches <[email protected]>
---
drivers/firmware/efi/libstub/arm32-stub.c | 12 ++++-----
drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++-----
drivers/firmware/efi/libstub/efi-stub.c | 32 +++++++++++------------
drivers/firmware/efi/libstub/efistub.h | 4 +--
drivers/firmware/efi/libstub/fdt.c | 16 ++++++------
drivers/firmware/efi/libstub/file.c | 12 ++++-----
drivers/firmware/efi/libstub/pci.c | 8 +++---
drivers/firmware/efi/libstub/relocate.c | 2 +-
drivers/firmware/efi/libstub/secureboot.c | 4 +--
9 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
index 7826553af2ba..b038afe2ee7a 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -18,7 +18,7 @@ efi_status_t check_platform_features(void)
/* LPAE kernels need compatible hardware */
block = cpuid_feature_extract(CPUID_EXT_MMFR0, 0);
if (block < 5) {
- pr_efi_err("This LPAE kernel is not supported by your CPU\n");
+ efi_err("This LPAE kernel is not supported by your CPU\n");
return EFI_UNSUPPORTED;
}
return EFI_SUCCESS;
@@ -120,7 +120,7 @@ static efi_status_t reserve_kernel_base(unsigned long dram_base,
*/
status = efi_get_memory_map(&map);
if (status != EFI_SUCCESS) {
- pr_efi_err("reserve_kernel_base(): Unable to retrieve memory map.\n");
+ efi_err("reserve_kernel_base(): Unable to retrieve memory map.\n");
return status;
}

@@ -162,7 +162,7 @@ static efi_status_t reserve_kernel_base(unsigned long dram_base,
(end - start) / EFI_PAGE_SIZE,
&start);
if (status != EFI_SUCCESS) {
- pr_efi_err("reserve_kernel_base(): alloc failed.\n");
+ efi_err("reserve_kernel_base(): alloc failed.\n");
goto out;
}
break;
@@ -219,7 +219,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,

status = reserve_kernel_base(kernel_base, reserve_addr, reserve_size);
if (status != EFI_SUCCESS) {
- pr_efi_err("Unable to allocate memory for uncompressed kernel.\n");
+ efi_err("Unable to allocate memory for uncompressed kernel.\n");
return status;
}

@@ -232,7 +232,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
status = efi_relocate_kernel(image_addr, *image_size, *image_size,
kernel_base + MAX_UNCOMP_KERNEL_SIZE, 0, 0);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to relocate kernel.\n");
+ efi_err("Failed to relocate kernel.\n");
efi_free(*reserve_size, *reserve_addr);
*reserve_size = 0;
return status;
@@ -244,7 +244,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
* address at which the zImage is loaded.
*/
if (*image_addr + *image_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
- pr_efi_err("Failed to relocate kernel, no low memory available.\n");
+ efi_err("Failed to relocate kernel, no low memory available.\n");
efi_free(*reserve_size, *reserve_addr);
*reserve_size = 0;
efi_free(*image_size, *image_addr);
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index ba4db35015a3..7f6a57dec513 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -26,9 +26,9 @@ efi_status_t check_platform_features(void)
tg = (read_cpuid(ID_AA64MMFR0_EL1) >> ID_AA64MMFR0_TGRAN_SHIFT) & 0xf;
if (tg != ID_AA64MMFR0_TGRAN_SUPPORTED) {
if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
- pr_efi_err("This 64 KB granular kernel is not supported by your CPU\n");
+ efi_err("This 64 KB granular kernel is not supported by your CPU\n");
else
- pr_efi_err("This 16 KB granular kernel is not supported by your CPU\n");
+ efi_err("This 16 KB granular kernel is not supported by your CPU\n");
return EFI_UNSUPPORTED;
}
return EFI_SUCCESS;
@@ -59,18 +59,18 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
status = efi_get_random_bytes(sizeof(phys_seed),
(u8 *)&phys_seed);
if (status == EFI_NOT_FOUND) {
- pr_efi("EFI_RNG_PROTOCOL unavailable, no randomness supplied\n");
+ efi_info("EFI_RNG_PROTOCOL unavailable, no randomness supplied\n");
} else if (status != EFI_SUCCESS) {
- pr_efi_err("efi_get_random_bytes() failed\n");
+ efi_err("efi_get_random_bytes() failed\n");
return status;
}
} else {
- pr_efi("KASLR disabled on kernel command line\n");
+ efi_info("KASLR disabled on kernel command line\n");
}
}

if (image->image_base != _text)
- pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
+ efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");

kernel_size = _edata - _text;
kernel_memsize = kernel_size + (_end - _edata);
@@ -102,7 +102,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
ULONG_MAX, min_kimg_align);

if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to relocate kernel\n");
+ efi_err("Failed to relocate kernel\n");
*reserve_size = 0;
return status;
}
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index ee225b323687..72ffd2670f99 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -69,7 +69,7 @@ static void install_memreserve_table(void)
status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(*rsv),
(void **)&rsv);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to allocate memreserve entry!\n");
+ efi_err("Failed to allocate memreserve entry!\n");
return;
}

@@ -80,7 +80,7 @@ static void install_memreserve_table(void)
status = efi_bs_call(install_configuration_table,
&memreserve_table_guid, rsv);
if (status != EFI_SUCCESS)
- pr_efi_err("Failed to install memreserve config table!\n");
+ efi_err("Failed to install memreserve config table!\n");
}

static unsigned long get_dram_base(void)
@@ -182,13 +182,13 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
status = efi_system_table->boottime->handle_protocol(handle,
&loaded_image_proto, (void *)&image);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to get loaded image protocol\n");
+ efi_err("Failed to get loaded image protocol\n");
goto fail;
}

dram_base = get_dram_base();
if (dram_base == EFI_ERROR) {
- pr_efi_err("Failed to find DRAM base\n");
+ efi_err("Failed to find DRAM base\n");
status = EFI_LOAD_ERROR;
goto fail;
}
@@ -200,7 +200,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
*/
cmdline_ptr = efi_convert_cmdline(image, &cmdline_size, ULONG_MAX);
if (!cmdline_ptr) {
- pr_efi_err("getting command line via LOADED_IMAGE_PROTOCOL\n");
+ efi_err("getting command line via LOADED_IMAGE_PROTOCOL\n");
status = EFI_OUT_OF_RESOURCES;
goto fail;
}
@@ -213,7 +213,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_size > 0)
efi_parse_options(cmdline_ptr);

- pr_efi("Booting Linux Kernel...\n");
+ efi_info("Booting Linux Kernel...\n");

si = setup_graphics();

@@ -222,7 +222,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
&reserve_size,
dram_base, image);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to relocate kernel\n");
+ efi_err("Failed to relocate kernel\n");
goto fail_free_cmdline;
}

@@ -241,42 +241,42 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
secure_boot != efi_secureboot_mode_disabled) {
if (strstr(cmdline_ptr, "dtb="))
- pr_efi("Ignoring DTB from command line.\n");
+ efi_info("Ignoring DTB from command line.\n");
} else {
status = efi_load_dtb(image, &fdt_addr, &fdt_size);

if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to load device tree!\n");
+ efi_err("Failed to load device tree!\n");
goto fail_free_image;
}
}

if (fdt_addr) {
- pr_efi("Using DTB from command line\n");
+ efi_info("Using DTB from command line\n");
} else {
/* Look for a device tree configuration table entry. */
fdt_addr = (uintptr_t)get_fdt(&fdt_size);
if (fdt_addr)
- pr_efi("Using DTB from configuration table\n");
+ efi_info("Using DTB from configuration table\n");
}

if (!fdt_addr)
- pr_efi("Generating empty DTB\n");
+ efi_info("Generating empty DTB\n");

if (!efi_noinitrd) {
max_addr = efi_get_max_initrd_addr(dram_base, image_addr);
status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size,
max_addr);
if (status == EFI_SUCCESS) {
- pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
+ efi_info("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
} else if (status == EFI_NOT_FOUND) {
status = efi_load_initrd(image, &initrd_addr, &initrd_size,
ULONG_MAX, max_addr);
if (status == EFI_SUCCESS && initrd_size > 0)
- pr_efi("Loaded initrd from command line option\n");
+ efi_info("Loaded initrd from command line option\n");
}
if (status != EFI_SUCCESS)
- pr_efi_err("Failed to load initrd!\n");
+ efi_err("Failed to load initrd!\n");
}

efi_random_get_seed();
@@ -326,7 +326,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
/* not reached */

fail_free_initrd:
- pr_efi_err("Failed to update FDT and exit boot services\n");
+ efi_err("Failed to update FDT and exit boot services\n");

efi_free(initrd_size, initrd_addr);
efi_free(fdt_size, fdt_addr);
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index e8aa70ba08d5..8c905a1be1b4 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -49,11 +49,11 @@ extern const efi_system_table_t *efi_system_table;
#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
#endif

-#define pr_efi(msg) do { \
+#define efi_info(msg) do { \
if (!efi_quiet) efi_printk("EFI stub: "msg); \
} while (0)

-#define pr_efi_err(msg) efi_printk("EFI stub: ERROR: "msg)
+#define efi_err(msg) efi_printk("EFI stub: ERROR: "msg)

/* Helper macros for the usual case of using simple C variables: */
#ifndef fdt_setprop_inplace_var
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 3074a5e27c65..11ecf3c4640e 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -39,7 +39,7 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size,
/* Do some checks on provided FDT, if it exists: */
if (orig_fdt) {
if (fdt_check_header(orig_fdt)) {
- pr_efi_err("Device Tree header not valid!\n");
+ efi_err("Device Tree header not valid!\n");
return EFI_LOAD_ERROR;
}
/*
@@ -47,7 +47,7 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size,
* configuration table:
*/
if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
- pr_efi_err("Truncated device tree! foo!\n");
+ efi_err("Truncated device tree! foo!\n");
return EFI_LOAD_ERROR;
}
}
@@ -270,16 +270,16 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
*/
status = efi_get_memory_map(&map);
if (status != EFI_SUCCESS) {
- pr_efi_err("Unable to retrieve UEFI memory map.\n");
+ efi_err("Unable to retrieve UEFI memory map.\n");
return status;
}

- pr_efi("Exiting boot services and installing virtual address map...\n");
+ efi_info("Exiting boot services and installing virtual address map...\n");

map.map = &memory_map;
status = efi_allocate_pages(MAX_FDT_SIZE, new_fdt_addr, max_addr);
if (status != EFI_SUCCESS) {
- pr_efi_err("Unable to allocate memory for new device tree.\n");
+ efi_err("Unable to allocate memory for new device tree.\n");
goto fail;
}

@@ -296,7 +296,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
initrd_addr, initrd_size);

if (status != EFI_SUCCESS) {
- pr_efi_err("Unable to construct new device tree.\n");
+ efi_err("Unable to construct new device tree.\n");
goto fail_free_new_fdt;
}

@@ -342,7 +342,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
return EFI_SUCCESS;
}

- pr_efi_err("Exit boot services failed.\n");
+ efi_err("Exit boot services failed.\n");

fail_free_new_fdt:
efi_free(MAX_FDT_SIZE, *new_fdt_addr);
@@ -363,7 +363,7 @@ void *get_fdt(unsigned long *fdt_size)
return NULL;

if (fdt_check_header(fdt) != 0) {
- pr_efi_err("Invalid header detected on UEFI supplied FDT, ignoring ...\n");
+ efi_err("Invalid header detected on UEFI supplied FDT, ignoring ...\n");
return NULL;
}
*fdt_size = fdt_totalsize(fdt);
diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index 50aaf15f9ad5..cc177152d0df 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -46,7 +46,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,

status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to open file: ");
+ efi_err("Failed to open file: ");
efi_char16_printk(fi->filename);
efi_printk("\n");
return status;
@@ -55,7 +55,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
info_sz = sizeof(struct finfo);
status = fh->get_info(fh, &info_guid, &info_sz, fi);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to get file info\n");
+ efi_err("Failed to get file info\n");
fh->close(fh);
return status;
}
@@ -75,13 +75,13 @@ static efi_status_t efi_open_volume(efi_loaded_image_t *image,
status = efi_bs_call(handle_protocol, image->device_handle, &fs_proto,
(void **)&io);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to handle fs_proto\n");
+ efi_err("Failed to handle fs_proto\n");
return status;
}

status = io->open_volume(io, fh);
if (status != EFI_SUCCESS)
- pr_efi_err("Failed to open volume\n");
+ efi_err("Failed to open volume\n");

return status;
}
@@ -191,7 +191,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
&alloc_addr,
hard_limit);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to allocate memory for files\n");
+ efi_err("Failed to allocate memory for files\n");
goto err_close_file;
}

@@ -215,7 +215,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,

status = file->read(file, &chunksize, addr);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to read file\n");
+ efi_err("Failed to read file\n");
goto err_close_file;
}
addr += chunksize;
diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
index b025e59b94df..60af51bed573 100644
--- a/drivers/firmware/efi/libstub/pci.c
+++ b/drivers/firmware/efi/libstub/pci.c
@@ -28,21 +28,21 @@ void efi_pci_disable_bridge_busmaster(void)

if (status != EFI_BUFFER_TOO_SMALL) {
if (status != EFI_SUCCESS && status != EFI_NOT_FOUND)
- pr_efi_err("Failed to locate PCI I/O handles'\n");
+ efi_err("Failed to locate PCI I/O handles'\n");
return;
}

status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, pci_handle_size,
(void **)&pci_handle);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to allocate memory for 'pci_handle'\n");
+ efi_err("Failed to allocate memory for 'pci_handle'\n");
return;
}

status = efi_bs_call(locate_handle, EFI_LOCATE_BY_PROTOCOL, &pci_proto,
NULL, &pci_handle_size, pci_handle);
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to locate PCI I/O handles'\n");
+ efi_err("Failed to locate PCI I/O handles'\n");
goto free_handle;
}

@@ -106,7 +106,7 @@ void efi_pci_disable_bridge_busmaster(void)
status = efi_call_proto(pci, pci.write, EfiPciIoWidthUint16,
PCI_COMMAND, 1, &command);
if (status != EFI_SUCCESS)
- pr_efi_err("Failed to disable PCI busmastering\n");
+ efi_err("Failed to disable PCI busmastering\n");
}

free_handle:
diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c
index 1507d3c82884..93c04d6aaed1 100644
--- a/drivers/firmware/efi/libstub/relocate.c
+++ b/drivers/firmware/efi/libstub/relocate.c
@@ -157,7 +157,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
min_addr);
}
if (status != EFI_SUCCESS) {
- pr_efi_err("Failed to allocate usable memory for kernel.\n");
+ efi_err("Failed to allocate usable memory for kernel.\n");
return status;
}

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index a765378ad18c..5efc524b14be 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -67,10 +67,10 @@ enum efi_secureboot_mode efi_get_secureboot(void)
return efi_secureboot_mode_disabled;

secure_boot_enabled:
- pr_efi("UEFI Secure Boot is enabled.\n");
+ efi_info("UEFI Secure Boot is enabled.\n");
return efi_secureboot_mode_enabled;

out_efi_err:
- pr_efi_err("Could not determine UEFI Secure Boot status.\n");
+ efi_err("Could not determine UEFI Secure Boot status.\n");
return efi_secureboot_mode_unknown;
}
--
2.26.2

2020-04-30 19:14:31

[permalink] [raw]

Subject: [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening

Use a standard style for these output logging messages.

Miscellanea:

o Use more common macro #defines with fmt, ##__VA_ARGS__
0 Remove trailing messages periods and odd ' uses
o Remove embedded function names and use %s, __func__

Signed-off-by: Joe Perches <[email protected]>
---

Perhaps these trivialities on top of this series?

drivers/firmware/efi/libstub/arm32-stub.c | 10 +++++-----
drivers/firmware/efi/libstub/efi-stub.c | 2 +-
drivers/firmware/efi/libstub/efistub.h | 9 ++++++---
drivers/firmware/efi/libstub/fdt.c | 8 ++++----
drivers/firmware/efi/libstub/pci.c | 4 ++--
drivers/firmware/efi/libstub/relocate.c | 2 +-
drivers/firmware/efi/libstub/secureboot.c | 4 ++--
7 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
index b038afe..5795781 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -120,7 +120,7 @@ static efi_status_t reserve_kernel_base(unsigned long dram_base,
*/
status = efi_get_memory_map(&map);
if (status != EFI_SUCCESS) {
- efi_err("reserve_kernel_base(): Unable to retrieve memory map.\n");
+ efi_err("%s(): Unable to retrieve memory map\n", __func__);
return status;
}

@@ -162,7 +162,7 @@ static efi_status_t reserve_kernel_base(unsigned long dram_base,
(end - start) / EFI_PAGE_SIZE,
&start);
if (status != EFI_SUCCESS) {
- efi_err("reserve_kernel_base(): alloc failed.\n");
+ efi_err("%s(): alloc failed\n", __func__);
goto out;
}
break;
@@ -219,7 +219,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,

status = reserve_kernel_base(kernel_base, reserve_addr, reserve_size);
if (status != EFI_SUCCESS) {
- efi_err("Unable to allocate memory for uncompressed kernel.\n");
+ efi_err("Unable to allocate memory for uncompressed kernel\n");
return status;
}

@@ -232,7 +232,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
status = efi_relocate_kernel(image_addr, *image_size, *image_size,
kernel_base + MAX_UNCOMP_KERNEL_SIZE, 0, 0);
if (status != EFI_SUCCESS) {
- efi_err("Failed to relocate kernel.\n");
+ efi_err("Failed to relocate kernel\n");
efi_free(*reserve_size, *reserve_addr);
*reserve_size = 0;
return status;
@@ -244,7 +244,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
* address at which the zImage is loaded.
*/
if (*image_addr + *image_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
- efi_err("Failed to relocate kernel, no low memory available.\n");
+ efi_err("Failed to relocate kernel, no low memory available\n");
efi_free(*reserve_size, *reserve_addr);
*reserve_size = 0;
efi_free(*image_size, *image_addr);
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index c2484b..19b42b 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -251,7 +251,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
secure_boot != efi_secureboot_mode_disabled) {
if (strstr(cmdline_ptr, "dtb="))
- efi_err("Ignoring DTB from command line.\n");
+ efi_err("Ignoring DTB from command line\n");
} else {
status = efi_load_dtb(image, &fdt_addr, &fdt_size);

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 874233..369262 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -49,11 +49,14 @@ extern const efi_system_table_t *efi_system_table;
#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
#endif

-#define efi_info(msg) do { \
- if (!efi_quiet) efi_printk("EFI stub: "msg); \
+#define efi_info(fmt, ...) \
+do { \
+ if (!efi_quiet) \
+ efi_printk("EFI stub: " fmt, ##__VA_ARGS__); \
} while (0)

-#define efi_err(msg) efi_printk("EFI stub: ERROR: "msg)
+#define efi_err(fmt, ...) \
+ efi_printk("EFI stub: ERROR: " fmt, ##__VA_ARGS__)

/* Helper macros for the usual case of using simple C variables: */
#ifndef fdt_setprop_inplace_var
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 11ecf3c..7c7257 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -270,7 +270,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
*/
status = efi_get_memory_map(&map);
if (status != EFI_SUCCESS) {
- efi_err("Unable to retrieve UEFI memory map.\n");
+ efi_err("Unable to retrieve UEFI memory map\n");
return status;
}

@@ -279,7 +279,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
map.map = &memory_map;
status = efi_allocate_pages(MAX_FDT_SIZE, new_fdt_addr, max_addr);
if (status != EFI_SUCCESS) {
- efi_err("Unable to allocate memory for new device tree.\n");
+ efi_err("Unable to allocate memory for new device tree\n");
goto fail;
}

@@ -296,7 +296,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
initrd_addr, initrd_size);

if (status != EFI_SUCCESS) {
- efi_err("Unable to construct new device tree.\n");
+ efi_err("Unable to construct new device tree\n");
goto fail_free_new_fdt;
}

@@ -342,7 +342,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
return EFI_SUCCESS;
}

- efi_err("Exit boot services failed.\n");
+ efi_err("Exit boot services failed\n");

fail_free_new_fdt:
efi_free(MAX_FDT_SIZE, *new_fdt_addr);
diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
index 60af51b..111c44b 100644
--- a/drivers/firmware/efi/libstub/pci.c
+++ b/drivers/firmware/efi/libstub/pci.c
@@ -28,7 +28,7 @@ void efi_pci_disable_bridge_busmaster(void)

if (status != EFI_BUFFER_TOO_SMALL) {
if (status != EFI_SUCCESS && status != EFI_NOT_FOUND)
- efi_err("Failed to locate PCI I/O handles'\n");
+ efi_err("Failed to locate PCI I/O handles\n");
return;
}

@@ -42,7 +42,7 @@ void efi_pci_disable_bridge_busmaster(void)
status = efi_bs_call(locate_handle, EFI_LOCATE_BY_PROTOCOL, &pci_proto,
NULL, &pci_handle_size, pci_handle);
if (status != EFI_SUCCESS) {
- efi_err("Failed to locate PCI I/O handles'\n");
+ efi_err("Failed to locate PCI I/O handles\n");
goto free_handle;
}

diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c
index 93c04d..62e2d6 100644
--- a/drivers/firmware/efi/libstub/relocate.c
+++ b/drivers/firmware/efi/libstub/relocate.c
@@ -157,7 +157,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
min_addr);
}
if (status != EFI_SUCCESS) {
- efi_err("Failed to allocate usable memory for kernel.\n");
+ efi_err("Failed to allocate usable memory for kernel\n");
return status;
}

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 5efc524..796a31 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -67,10 +67,10 @@ enum efi_secureboot_mode efi_get_secureboot(void)
return efi_secureboot_mode_disabled;

secure_boot_enabled:
- efi_info("UEFI Secure Boot is enabled.\n");
+ efi_info("UEFI Secure Boot is enabled\n");
return efi_secureboot_mode_enabled;

out_efi_err:
- efi_err("Could not determine UEFI Secure Boot status.\n");
+ efi_err("Could not determine UEFI Secure Boot status\n");
return efi_secureboot_mode_unknown;
}
--
2.26.0

2020-04-30 19:15:11

[permalink] [raw]

Subject: [PATCH 2/2] efi/libstub: Correct comment typos

Fix a couple typos in comments.

Signed-off-by: Joe Perches <[email protected]>
---

Perhaps these trivialities on top of this series?

drivers/firmware/efi/libstub/pci.c | 2 +-
drivers/firmware/efi/libstub/relocate.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
index 111c44b..17a53d8 100644
--- a/drivers/firmware/efi/libstub/pci.c
+++ b/drivers/firmware/efi/libstub/pci.c
@@ -69,7 +69,7 @@ void efi_pci_disable_bridge_busmaster(void)
* access to the framebuffer. Drivers for true PCIe graphics
* controllers that are behind a PCIe root port do not use
* DMA to implement the GOP framebuffer anyway [although they
- * may use it in their implentation of Gop->Blt()], and so
+ * may use it in their implementation of Gop->Blt()], and so
* disabling DMA in the PCI bridge should not interfere with
* normal operation of the device.
*/
diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c
index 62e2d6..a7ad26 100644
--- a/drivers/firmware/efi/libstub/relocate.c
+++ b/drivers/firmware/efi/libstub/relocate.c
@@ -140,7 +140,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
* The EFI firmware loader could have placed the kernel image
* anywhere in memory, but the kernel has restrictions on the
* max physical address it can run at. Some architectures
- * also have a prefered address, so first try to relocate
+ * also have a preferred address, so first try to relocate
* to the preferred address. If that fails, allocate as low
* as possible while respecting the required alignment.
*/
--
2.26.0

2020-04-30 19:34:27

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening

On Thu, 30 Apr 2020 at 21:12, Joe Perches <[email protected]> wrote:
>
> Use a standard style for these output logging messages.
>
> Miscellanea:
>
> o Use more common macro #defines with fmt, ##__VA_ARGS__
> 0 Remove trailing messages periods and odd ' uses
> o Remove embedded function names and use %s, __func__
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
>
> Perhaps these trivialities on top of this series?
>

The EFI printing routines don't actually support format strings.

Removing trailing periods is not really necessary IMO, but i'll take a
patch that fixes those weird quotes.

Thanks,
Ard.

> drivers/firmware/efi/libstub/arm32-stub.c | 10 +++++-----
> drivers/firmware/efi/libstub/efi-stub.c | 2 +-
> drivers/firmware/efi/libstub/efistub.h | 9 ++++++---
> drivers/firmware/efi/libstub/fdt.c | 8 ++++----
> drivers/firmware/efi/libstub/pci.c | 4 ++--
> drivers/firmware/efi/libstub/relocate.c | 2 +-
> drivers/firmware/efi/libstub/secureboot.c | 4 ++--
> 7 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
> index b038afe..5795781 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -120,7 +120,7 @@ static efi_status_t reserve_kernel_base(unsigned long dram_base,
> */
> status = efi_get_memory_map(&map);
> if (status != EFI_SUCCESS) {
> - efi_err("reserve_kernel_base(): Unable to retrieve memory map.\n");
> + efi_err("%s(): Unable to retrieve memory map\n", __func__);
> return status;
> }
>
> @@ -162,7 +162,7 @@ static efi_status_t reserve_kernel_base(unsigned long dram_base,
> (end - start) / EFI_PAGE_SIZE,
> &start);
> if (status != EFI_SUCCESS) {
> - efi_err("reserve_kernel_base(): alloc failed.\n");
> + efi_err("%s(): alloc failed\n", __func__);
> goto out;
> }
> break;
> @@ -219,7 +219,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>
> status = reserve_kernel_base(kernel_base, reserve_addr, reserve_size);
> if (status != EFI_SUCCESS) {
> - efi_err("Unable to allocate memory for uncompressed kernel.\n");
> + efi_err("Unable to allocate memory for uncompressed kernel\n");
> return status;
> }
>
> @@ -232,7 +232,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> status = efi_relocate_kernel(image_addr, *image_size, *image_size,
> kernel_base + MAX_UNCOMP_KERNEL_SIZE, 0, 0);
> if (status != EFI_SUCCESS) {
> - efi_err("Failed to relocate kernel.\n");
> + efi_err("Failed to relocate kernel\n");
> efi_free(*reserve_size, *reserve_addr);
> *reserve_size = 0;
> return status;
> @@ -244,7 +244,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> * address at which the zImage is loaded.
> */
> if (*image_addr + *image_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
> - efi_err("Failed to relocate kernel, no low memory available.\n");
> + efi_err("Failed to relocate kernel, no low memory available\n");
> efi_free(*reserve_size, *reserve_addr);
> *reserve_size = 0;
> efi_free(*image_size, *image_addr);
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index c2484b..19b42b 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -251,7 +251,7 @@ efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t *sys_table_arg)
> if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
> secure_boot != efi_secureboot_mode_disabled) {
> if (strstr(cmdline_ptr, "dtb="))
> - efi_err("Ignoring DTB from command line.\n");
> + efi_err("Ignoring DTB from command line\n");
> } else {
> status = efi_load_dtb(image, &fdt_addr, &fdt_size);
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 874233..369262 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -49,11 +49,14 @@ extern const efi_system_table_t *efi_system_table;
> #define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> #endif
>
> -#define efi_info(msg) do { \
> - if (!efi_quiet) efi_printk("EFI stub: "msg); \
> +#define efi_info(fmt, ...) \
> +do { \
> + if (!efi_quiet) \
> + efi_printk("EFI stub: " fmt, ##__VA_ARGS__); \
> } while (0)
>
> -#define efi_err(msg) efi_printk("EFI stub: ERROR: "msg)
> +#define efi_err(fmt, ...) \
> + efi_printk("EFI stub: ERROR: " fmt, ##__VA_ARGS__)
>
> /* Helper macros for the usual case of using simple C variables: */
> #ifndef fdt_setprop_inplace_var
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 11ecf3c..7c7257 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -270,7 +270,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> */
> status = efi_get_memory_map(&map);
> if (status != EFI_SUCCESS) {
> - efi_err("Unable to retrieve UEFI memory map.\n");
> + efi_err("Unable to retrieve UEFI memory map\n");
> return status;
> }
>
> @@ -279,7 +279,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> map.map = &memory_map;
> status = efi_allocate_pages(MAX_FDT_SIZE, new_fdt_addr, max_addr);
> if (status != EFI_SUCCESS) {
> - efi_err("Unable to allocate memory for new device tree.\n");
> + efi_err("Unable to allocate memory for new device tree\n");
> goto fail;
> }
>
> @@ -296,7 +296,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> initrd_addr, initrd_size);
>
> if (status != EFI_SUCCESS) {
> - efi_err("Unable to construct new device tree.\n");
> + efi_err("Unable to construct new device tree\n");
> goto fail_free_new_fdt;
> }
>
> @@ -342,7 +342,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> return EFI_SUCCESS;
> }
>
> - efi_err("Exit boot services failed.\n");
> + efi_err("Exit boot services failed\n");
>
> fail_free_new_fdt:
> efi_free(MAX_FDT_SIZE, *new_fdt_addr);
> diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
> index 60af51b..111c44b 100644
> --- a/drivers/firmware/efi/libstub/pci.c
> +++ b/drivers/firmware/efi/libstub/pci.c
> @@ -28,7 +28,7 @@ void efi_pci_disable_bridge_busmaster(void)
>
> if (status != EFI_BUFFER_TOO_SMALL) {
> if (status != EFI_SUCCESS && status != EFI_NOT_FOUND)
> - efi_err("Failed to locate PCI I/O handles'\n");
> + efi_err("Failed to locate PCI I/O handles\n");
> return;
> }
>
> @@ -42,7 +42,7 @@ void efi_pci_disable_bridge_busmaster(void)
> status = efi_bs_call(locate_handle, EFI_LOCATE_BY_PROTOCOL, &pci_proto,
> NULL, &pci_handle_size, pci_handle);
> if (status != EFI_SUCCESS) {
> - efi_err("Failed to locate PCI I/O handles'\n");
> + efi_err("Failed to locate PCI I/O handles\n");
> goto free_handle;
> }
>
> diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c
> index 93c04d..62e2d6 100644
> --- a/drivers/firmware/efi/libstub/relocate.c
> +++ b/drivers/firmware/efi/libstub/relocate.c
> @@ -157,7 +157,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
> min_addr);
> }
> if (status != EFI_SUCCESS) {
> - efi_err("Failed to allocate usable memory for kernel.\n");
> + efi_err("Failed to allocate usable memory for kernel\n");
> return status;
> }
>
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index 5efc524..796a31 100644
> --- a/drivers/firmware/efi/libstub/secureboot.c
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -67,10 +67,10 @@ enum efi_secureboot_mode efi_get_secureboot(void)
> return efi_secureboot_mode_disabled;
>
> secure_boot_enabled:
> - efi_info("UEFI Secure Boot is enabled.\n");
> + efi_info("UEFI Secure Boot is enabled\n");
> return efi_secureboot_mode_enabled;
>
> out_efi_err:
> - efi_err("Could not determine UEFI Secure Boot status.\n");
> + efi_err("Could not determine UEFI Secure Boot status\n");
> return efi_secureboot_mode_unknown;
> }
> --
> 2.26.0
>

2020-04-30 19:34:58

[permalink] [raw]

Subject: Re: [PATCH 2/2] efi/libstub: Correct comment typos

On Thu, 30 Apr 2020 at 21:12, Joe Perches <[email protected]> wrote:
>
> Fix a couple typos in comments.
>
> Signed-off-by: Joe Perches <[email protected]>

Thanks, I'll queue this one up

> ---
>
> Perhaps these trivialities on top of this series?
>
> drivers/firmware/efi/libstub/pci.c | 2 +-
> drivers/firmware/efi/libstub/relocate.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
> index 111c44b..17a53d8 100644
> --- a/drivers/firmware/efi/libstub/pci.c
> +++ b/drivers/firmware/efi/libstub/pci.c
> @@ -69,7 +69,7 @@ void efi_pci_disable_bridge_busmaster(void)
> * access to the framebuffer. Drivers for true PCIe graphics
> * controllers that are behind a PCIe root port do not use
> * DMA to implement the GOP framebuffer anyway [although they
> - * may use it in their implentation of Gop->Blt()], and so
> + * may use it in their implementation of Gop->Blt()], and so
> * disabling DMA in the PCI bridge should not interfere with
> * normal operation of the device.
> */
> diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c
> index 62e2d6..a7ad26 100644
> --- a/drivers/firmware/efi/libstub/relocate.c
> +++ b/drivers/firmware/efi/libstub/relocate.c
> @@ -140,7 +140,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
> * The EFI firmware loader could have placed the kernel image
> * anywhere in memory, but the kernel has restrictions on the
> * max physical address it can run at. Some architectures
> - * also have a prefered address, so first try to relocate
> + * also have a preferred address, so first try to relocate
> * to the preferred address. If that fails, allocate as low
> * as possible while respecting the required alignment.
> */
> --
> 2.26.0
>

2020-04-30 19:39:52

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening

On Thu, 2020-04-30 at 21:29 +0200, Ard Biesheuvel wrote:
> On Thu, 30 Apr 2020 at 21:12, Joe Perches <[email protected]> wrote:
> > Use a standard style for these output logging messages.
> >
> > Miscellanea:
> >
> > o Use more common macro #defines with fmt, ##__VA_ARGS__
> > 0 Remove trailing messages periods and odd ' uses
> > o Remove embedded function names and use %s, __func__
> >
> > Signed-off-by: Joe Perches <[email protected]>
> > ---
> >
> > Perhaps these trivialities on top of this series?
> >
>
> The EFI printing routines don't actually support format strings.

Thanks. Then nevermind on that bit.

> Removing trailing periods is not really necessary IMO, but i'll take a
> patch that fixes those weird quotes.

<shrug> Your choice.

There are 60+ uses of efi_err, 12 with a trailing period.

cheers, Joe

2020-04-30 20:45:12

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening

On Thu, Apr 30, 2020 at 09:29:46PM +0200, Ard Biesheuvel wrote:
> On Thu, 30 Apr 2020 at 21:12, Joe Perches <[email protected]> wrote:
> >
> > Use a standard style for these output logging messages.
> >
> > Miscellanea:
> >
> > o Use more common macro #defines with fmt, ##__VA_ARGS__
> > 0 Remove trailing messages periods and odd ' uses
> > o Remove embedded function names and use %s, __func__
> >
> > Signed-off-by: Joe Perches <[email protected]>
> > ---
> >
> > Perhaps these trivialities on top of this series?
> >
>
> The EFI printing routines don't actually support format strings.
>

The x86 real-mode bootup code actually has a printf.o that clocks in at
under 2k. We could add it in, and it would also be nice to move it into
lib or something, since at least alpha and powerpc implement something
very similar for boot-time messages.

2020-04-30 20:45:13

[permalink] [raw]

Subject: Re: [PATCH 1/2] efi/libstub: efi_info/efi_err message neatening

On Thu, 30 Apr 2020 at 22:40, Arvind Sankar <[email protected]> wrote:
>
> On Thu, Apr 30, 2020 at 09:29:46PM +0200, Ard Biesheuvel wrote:
> > On Thu, 30 Apr 2020 at 21:12, Joe Perches <[email protected]> wrote:
> > >
> > > Use a standard style for these output logging messages.
> > >
> > > Miscellanea:
> > >
> > > o Use more common macro #defines with fmt, ##__VA_ARGS__
> > > 0 Remove trailing messages periods and odd ' uses
> > > o Remove embedded function names and use %s, __func__
> > >
> > > Signed-off-by: Joe Perches <[email protected]>
> > > ---
> > >
> > > Perhaps these trivialities on top of this series?
> > >
> >
> > The EFI printing routines don't actually support format strings.
> >
>
> The x86 real-mode bootup code actually has a printf.o that clocks in at
> under 2k. We could add it in, and it would also be nice to move it into
> lib or something, since at least alpha and powerpc implement something
> very similar for boot-time messages.

Not being able to use format strings is really quite annoying, and I
did look into reusing the ordinary one (which is hairy), not realizing
that there is already a cut down version available.

So yes, that does sound like a great idea!

2020-05-04 08:20:33

[permalink] [raw]

Subject: Re: [PATCH v2 00/11] efi: some cleanups/refactoring for efi/next

On Thu, 30 Apr 2020 at 20:28, Arvind Sankar <[email protected]> wrote:
>
> This series is on top of efi/next.
>
> Patch 1 fixes the size allocated for x86 boot_params.
> Patch 2 refactors the setting of various hi/lo 32-bit fields, mainly on x86.
> Patch 3 renames pr_efi/pr_efi_err
> Patches 4-6 convert the remaining uses of efi_printk to print error
> messages to use efi_err instead.
> Patch 7 updates dtb= ignored message to efi_err.
> Patches 8-9 refactor initrd loading, moving it into efi-stub-helper.
> Patch 10 adds support for x86 builtin command line.
> Patch 11 adds error checking for efi_parse_options.
>
> Changes from v1:
> - Rename pr_efi/pr_efi_err
> - Drop the soft_limit-removing patch
> - Fix a couple of compile warnings
>
> Arvind Sankar (11):
> efi/x86: Use correct size for boot_params
> efi/libstub: Add a helper function to split 64-bit values
> efi/libstub: Move pr_efi/pr_efi_err into efi namespace
> efi/x86: Use efi_err for error messages
> efi/gop: Use efi_err for error messages
> efi/tpm: Use efi_err for error messages
> efi/libstub: Upgrade ignored dtb= argument message to error
> efi/x86: Move command-line initrd loading to efi_main
> efi/libstub: Unify initrd loading across architectures
> efi/x86: Support builtin command line
> efi/libstub: Check return value of efi_parse_options
>

Thanks Arvind, I've queued these up now

> drivers/firmware/efi/libstub/arm32-stub.c | 12 +-
> drivers/firmware/efi/libstub/arm64-stub.c | 14 +-
> .../firmware/efi/libstub/efi-stub-helper.c | 46 ++++++-
> drivers/firmware/efi/libstub/efi-stub.c | 63 ++++-----
> drivers/firmware/efi/libstub/efistub.h | 32 ++---
> drivers/firmware/efi/libstub/fdt.c | 16 +--
> drivers/firmware/efi/libstub/file.c | 12 +-
> drivers/firmware/efi/libstub/gop.c | 16 +--
> drivers/firmware/efi/libstub/pci.c | 8 +-
> drivers/firmware/efi/libstub/relocate.c | 2 +-
> drivers/firmware/efi/libstub/secureboot.c | 4 +-
> drivers/firmware/efi/libstub/tpm.c | 2 +-
> drivers/firmware/efi/libstub/x86-stub.c | 122 ++++++++----------
> 13 files changed, 186 insertions(+), 163 deletions(-)
>
> --
> 2.26.2
>

2020-05-04 18:51:09

[permalink] [raw]

Subject: [trivial PATCH] efi/libstub: Reduce efi_printk object size

Use a few more common kernel styles.

Trivially reduce efi_printk object size by using a dereference to
a temporary instead of multiple dereferences of the same object.

Use efi_printk(const char *str) and static or static const for its
internal variables.

Use the more common form of while instead of a for loop.

Change efi_char16_printk argument to const.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++--------
drivers/firmware/efi/libstub/efistub.h | 6 +++---
2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 1c92ac231f94..dfd72a4360ac 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -26,19 +26,19 @@ bool __pure __efi_soft_reserve_enabled(void)
return !efi_nosoftreserve;
}

-void efi_printk(char *str)
+void efi_printk(const char *str)
{
- char *s8;
+ char s8;

- for (s8 = str; *s8; s8++) {
- efi_char16_t ch[2] = { 0 };
+ while ((s8 = *str++)) {
+ static efi_char16_t ch[2] = {0, 0};

- ch[0] = *s8;
- if (*s8 == '\n') {
- efi_char16_t nl[2] = { '\r', 0 };
+ if (s8 == '\n') {
+ static const efi_char16_t nl[2] = { '\r', 0 };
efi_char16_printk(nl);
}

+ ch[0] = s8;
efi_char16_printk(ch);
}
}
@@ -284,7 +284,7 @@ void *get_efi_config_table(efi_guid_t guid)
return NULL;
}

-void efi_char16_printk(efi_char16_t *str)
+void efi_char16_printk(const efi_char16_t *str)
{
efi_call_proto(efi_table_attr(efi_system_table, con_out),
output_string, str);
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 5ff63230a1f1..a03a92c665f0 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -251,7 +251,7 @@ union efi_simple_text_output_protocol {
struct {
void *reset;
efi_status_t (__efiapi *output_string)(efi_simple_text_output_protocol_t *,
- efi_char16_t *);
+ const efi_char16_t *);
void *test_string;
};
struct {
@@ -599,7 +599,7 @@ efi_status_t efi_exit_boot_services(void *handle,
void *priv,
efi_exit_boot_map_processing priv_func);

-void efi_char16_printk(efi_char16_t *);
+void efi_char16_printk(const efi_char16_t *str);

efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
unsigned long *new_fdt_addr,
@@ -624,7 +624,7 @@ efi_status_t check_platform_features(void);

void *get_efi_config_table(efi_guid_t guid);

-void efi_printk(char *str);
+void efi_printk(const char *str);

void efi_free(unsigned long size, unsigned long addr);

2020-05-05 07:53:24

[permalink] [raw]

Subject: Re: [trivial PATCH] efi/libstub: Reduce efi_printk object size

On Mon, 4 May 2020 at 20:29, Joe Perches <[email protected]> wrote:
>
> Use a few more common kernel styles.
>
> Trivially reduce efi_printk object size by using a dereference to
> a temporary instead of multiple dereferences of the same object.
>
> Use efi_printk(const char *str) and static or static const for its
> internal variables.
>
> Use the more common form of while instead of a for loop.
>
> Change efi_char16_printk argument to const.
>
> Signed-off-by: Joe Perches <[email protected]>

Thanks Joe.

> ---
> drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++--------
> drivers/firmware/efi/libstub/efistub.h | 6 +++---
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 1c92ac231f94..dfd72a4360ac 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -26,19 +26,19 @@ bool __pure __efi_soft_reserve_enabled(void)
> return !efi_nosoftreserve;
> }
>
> -void efi_printk(char *str)
> +void efi_printk(const char *str)
> {
> - char *s8;
> + char s8;
>
> - for (s8 = str; *s8; s8++) {
> - efi_char16_t ch[2] = { 0 };
> + while ((s8 = *str++)) {

I'm not sure I prefer the assignment-as-truth-value construct over the
original for () tbh

> + static efi_char16_t ch[2] = {0, 0};
>

UEFI code could potentially be reentrant, so this should not be static.

> - ch[0] = *s8;
> - if (*s8 == '\n') {
> - efi_char16_t nl[2] = { '\r', 0 };
> + if (s8 == '\n') {
> + static const efi_char16_t nl[2] = { '\r', 0 };
> efi_char16_printk(nl);

We cannot make this const, unfortunately (see below). But we can clean
this up by using L"\r" as the initializer.

> }
>
> + ch[0] = s8;
> efi_char16_printk(ch);
> }
> }
> @@ -284,7 +284,7 @@ void *get_efi_config_table(efi_guid_t guid)
> return NULL;
> }
>
> -void efi_char16_printk(efi_char16_t *str)
> +void efi_char16_printk(const efi_char16_t *str)
> {
> efi_call_proto(efi_table_attr(efi_system_table, con_out),
> output_string, str);
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 5ff63230a1f1..a03a92c665f0 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -251,7 +251,7 @@ union efi_simple_text_output_protocol {
> struct {
> void *reset;
> efi_status_t (__efiapi *output_string)(efi_simple_text_output_protocol_t *,
> - efi_char16_t *);
> + const efi_char16_t *);

This prototype comes straight from the UEFI specification, and even
though it is dumb that they forgot about const-qualified pointers
entirely, I would prefer not to deviate from this.

> void *test_string;
> };
> struct {
> @@ -599,7 +599,7 @@ efi_status_t efi_exit_boot_services(void *handle,
> void *priv,
> efi_exit_boot_map_processing priv_func);
>
> -void efi_char16_printk(efi_char16_t *);
> +void efi_char16_printk(const efi_char16_t *str);
>
> efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> unsigned long *new_fdt_addr,
> @@ -624,7 +624,7 @@ efi_status_t check_platform_features(void);
>
> void *get_efi_config_table(efi_guid_t guid);
>
> -void efi_printk(char *str);
> +void efi_printk(const char *str);
>
> void efi_free(unsigned long size, unsigned long addr);
>
>

2020-05-05 08:03:36