* [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline [not found] <41021d66db2ab427c14255d2a24bb4517c8b58fd.1617126961.git.danielwa@cisco.com> @ 2021-03-30 17:57 ` Daniel Walker 2021-03-31 16:10 ` Ard Biesheuvel 2021-04-02 17:36 ` Christophe Leroy 0 siblings, 2 replies; 5+ messages in thread From: Daniel Walker @ 2021-03-30 17:57 UTC (permalink / raw) To: Will Deacon, Christophe Leroy, ob Herring, Daniel Gimpelevich, Andrew Morton, x86, linux-mips, linuxppc-dev Cc: xe-linux-external, Ard Biesheuvel, linux-efi, linux-kernel This adds code to handle the generic command line changes. The efi code appears that it doesn't benefit as much from this design as it could. For example, if you had a prepend command line with "nokaslr" then you might be helpful to re-enable it in the boot loader or dts, but there appears to be no way to re-enable kaslr or some of the other options. Cc: xe-linux-external@cisco.com Signed-off-by: Daniel Walker <danielwa@cisco.com> --- .../firmware/efi/libstub/efi-stub-helper.c | 35 +++++++++++++++++++ drivers/firmware/efi/libstub/efi-stub.c | 7 ++++ drivers/firmware/efi/libstub/efistub.h | 1 + drivers/firmware/efi/libstub/x86-stub.c | 13 +++++-- 4 files changed, 54 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index aa8da0a49829..c155837cedc9 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -13,6 +13,7 @@ #include <linux/efi.h> #include <linux/kernel.h> #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */ +#include <linux/cmdline.h> #include <asm/efi.h> #include <asm/setup.h> @@ -172,6 +173,40 @@ int efi_printk(const char *fmt, ...) return printed; } +/** + * efi_handle_cmdline() - handle adding in building parts of the command line + * @cmdline: kernel command line + * + * Add in the generic parts of the commandline and start the parsing of the + * command line. + * + * Return: status code + */ +efi_status_t efi_handle_cmdline(char const *cmdline) +{ + efi_status_t status; + + status = efi_parse_options(CMDLINE_PREPEND); + if (status != EFI_SUCCESS) { + efi_err("Failed to parse options\n"); + return status; + } + + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); + if (status != EFI_SUCCESS) { + efi_err("Failed to parse options\n"); + return status; + } + + status = efi_parse_options(CMDLINE_APPEND); + if (status != EFI_SUCCESS) { + efi_err("Failed to parse options\n"); + return status; + } + + return EFI_SUCCESS; +} + /** * efi_parse_options() - Parse EFI command line options * @cmdline: kernel command line diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c index 26e69788f27a..760480248adf 100644 --- a/drivers/firmware/efi/libstub/efi-stub.c +++ b/drivers/firmware/efi/libstub/efi-stub.c @@ -172,6 +172,12 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, goto fail; } +#ifdef CONFIG_GENERIC_CMDLINE + status = efi_handle_cmdline(cmdline_ptr); + if (status != EFI_SUCCESS) { + goto fail_free_cmdline; + } +#else if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || IS_ENABLED(CONFIG_CMDLINE_FORCE) || cmdline_size == 0) { @@ -189,6 +195,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, goto fail_free_cmdline; } } +#endif efi_info("Booting Linux Kernel...\n"); diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index cde0a2ef507d..07c7f9fdfffc 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -800,6 +800,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, unsigned long alignment, unsigned long min_addr); +efi_status_t efi_handle_cmdline(char const *cmdline); efi_status_t efi_parse_options(char const *cmdline); void efi_parse_option_graphics(char *option); diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index f14c4ff5839f..30ad8fb7122d 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -673,6 +673,8 @@ unsigned long efi_main(efi_handle_t handle, unsigned long bzimage_addr = (unsigned long)startup_32; unsigned long buffer_start, buffer_end; struct setup_header *hdr = &boot_params->hdr; + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | + ((u64)boot_params->ext_cmd_line_ptr << 32)); efi_status_t status; efi_system_table = sys_table_arg; @@ -735,6 +737,14 @@ unsigned long efi_main(efi_handle_t handle, image_offset = 0; } +#ifdef CONFIG_GENERIC_CMDLINE + status = efi_handle_cmdline((char *)cmdline_paddr); + if (status != EFI_SUCCESS) { + efi_err("Failed to parse options\n"); + goto fail; + } +#else /* CONFIG_GENERIC_CMDLINE */ + #ifdef CONFIG_CMDLINE_BOOL status = efi_parse_options(CONFIG_CMDLINE); if (status != EFI_SUCCESS) { @@ -743,8 +753,6 @@ unsigned long efi_main(efi_handle_t handle, } #endif if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | - ((u64)boot_params->ext_cmd_line_ptr << 32)); status = efi_parse_options((char *)cmdline_paddr); if (status != EFI_SUCCESS) { efi_err("Failed to parse options\n"); @@ -752,6 +760,7 @@ unsigned long efi_main(efi_handle_t handle, } } +#endif /* * At this point, an initrd may already have been loaded by the * bootloader and passed via bootparams. We permit an initrd loaded -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline 2021-03-30 17:57 ` [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline Daniel Walker @ 2021-03-31 16:10 ` Ard Biesheuvel 2021-03-31 18:21 ` Daniel Walker 2021-04-02 17:36 ` Christophe Leroy 1 sibling, 1 reply; 5+ messages in thread From: Ard Biesheuvel @ 2021-03-31 16:10 UTC (permalink / raw) To: Daniel Walker, Arvind Sankar Cc: Will Deacon, Christophe Leroy, ob Herring, Daniel Gimpelevich, Andrew Morton, X86 ML, open list:MIPS, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), xe-linux-external, linux-efi, Linux Kernel Mailing List (+ Arvind) On Tue, 30 Mar 2021 at 19:57, Daniel Walker <danielwa@cisco.com> wrote: > > This adds code to handle the generic command line changes. > The efi code appears that it doesn't benefit as much from this design > as it could. > > For example, if you had a prepend command line with "nokaslr" then > you might be helpful to re-enable it in the boot loader or dts, > but there appears to be no way to re-enable kaslr or some of the > other options. > > Cc: xe-linux-external@cisco.com > Signed-off-by: Daniel Walker <danielwa@cisco.com> > --- > .../firmware/efi/libstub/efi-stub-helper.c | 35 +++++++++++++++++++ > drivers/firmware/efi/libstub/efi-stub.c | 7 ++++ > drivers/firmware/efi/libstub/efistub.h | 1 + > drivers/firmware/efi/libstub/x86-stub.c | 13 +++++-- > 4 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index aa8da0a49829..c155837cedc9 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -13,6 +13,7 @@ > #include <linux/efi.h> > #include <linux/kernel.h> > #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */ > +#include <linux/cmdline.h> > #include <asm/efi.h> > #include <asm/setup.h> > > @@ -172,6 +173,40 @@ int efi_printk(const char *fmt, ...) > return printed; > } > > +/** > + * efi_handle_cmdline() - handle adding in building parts of the command line > + * @cmdline: kernel command line > + * > + * Add in the generic parts of the commandline and start the parsing of the > + * command line. > + * > + * Return: status code > + */ > +efi_status_t efi_handle_cmdline(char const *cmdline) > +{ > + efi_status_t status; > + > + status = efi_parse_options(CMDLINE_PREPEND); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } Even though I am not a fan of the 'success handling' pattern, duplicating the exact same error handling three times is not great either. Could we reuse more of the code here? > + > + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); What is the point of calling efi_parse_options() with an empty string? > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + status = efi_parse_options(CMDLINE_APPEND); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + return EFI_SUCCESS; > +} > + > /** > * efi_parse_options() - Parse EFI command line options > * @cmdline: kernel command line > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index 26e69788f27a..760480248adf 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -172,6 +172,12 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > goto fail; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_cmdline(cmdline_ptr); > + if (status != EFI_SUCCESS) { > + goto fail_free_cmdline; > + } > +#else > if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || > IS_ENABLED(CONFIG_CMDLINE_FORCE) || Does this mean CONFIG_GENERIC_CMDLINE does not replace CMDLINE_EXTEND / CMDLINE_FORCE etc, but introduces yet another variant on top of those? That does not seem like an improvement to me. I think it is great that you are cleaning this up, but only if it means we can get rid of the old implementation. > cmdline_size == 0) { > @@ -189,6 +195,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > goto fail_free_cmdline; > } > } > +#endif > > efi_info("Booting Linux Kernel...\n"); > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index cde0a2ef507d..07c7f9fdfffc 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -800,6 +800,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, > unsigned long alignment, > unsigned long min_addr); > > +efi_status_t efi_handle_cmdline(char const *cmdline); > efi_status_t efi_parse_options(char const *cmdline); > > void efi_parse_option_graphics(char *option); > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index f14c4ff5839f..30ad8fb7122d 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -673,6 +673,8 @@ unsigned long efi_main(efi_handle_t handle, > unsigned long bzimage_addr = (unsigned long)startup_32; > unsigned long buffer_start, buffer_end; > struct setup_header *hdr = &boot_params->hdr; > + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > + ((u64)boot_params->ext_cmd_line_ptr << 32)); > efi_status_t status; > > efi_system_table = sys_table_arg; > @@ -735,6 +737,14 @@ unsigned long efi_main(efi_handle_t handle, > image_offset = 0; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_cmdline((char *)cmdline_paddr); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + goto fail; > + } > +#else /* CONFIG_GENERIC_CMDLINE */ > + > #ifdef CONFIG_CMDLINE_BOOL > status = efi_parse_options(CONFIG_CMDLINE); > if (status != EFI_SUCCESS) { > @@ -743,8 +753,6 @@ unsigned long efi_main(efi_handle_t handle, > } > #endif > if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { > - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > - ((u64)boot_params->ext_cmd_line_ptr << 32)); > status = efi_parse_options((char *)cmdline_paddr); > if (status != EFI_SUCCESS) { > efi_err("Failed to parse options\n"); > @@ -752,6 +760,7 @@ unsigned long efi_main(efi_handle_t handle, > } > } > > +#endif > /* > * At this point, an initrd may already have been loaded by the > * bootloader and passed via bootparams. We permit an initrd loaded > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline 2021-03-31 16:10 ` Ard Biesheuvel @ 2021-03-31 18:21 ` Daniel Walker 0 siblings, 0 replies; 5+ messages in thread From: Daniel Walker @ 2021-03-31 18:21 UTC (permalink / raw) To: Ard Biesheuvel Cc: Arvind Sankar, Will Deacon, Christophe Leroy, ob Herring, Daniel Gimpelevich, Andrew Morton, X86 ML, open list:MIPS, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), xe-linux-external, linux-efi, Linux Kernel Mailing List On Wed, Mar 31, 2021 at 06:10:08PM +0200, Ard Biesheuvel wrote: > (+ Arvind) > > On Tue, 30 Mar 2021 at 19:57, Daniel Walker <danielwa@cisco.com> wrote: > > > > This adds code to handle the generic command line changes. > > The efi code appears that it doesn't benefit as much from this design > > as it could. > > > > For example, if you had a prepend command line with "nokaslr" then > > you might be helpful to re-enable it in the boot loader or dts, > > but there appears to be no way to re-enable kaslr or some of the > > other options. > > > > Cc: xe-linux-external@cisco.com > > Signed-off-by: Daniel Walker <danielwa@cisco.com> > > --- > > .../firmware/efi/libstub/efi-stub-helper.c | 35 +++++++++++++++++++ > > drivers/firmware/efi/libstub/efi-stub.c | 7 ++++ > > drivers/firmware/efi/libstub/efistub.h | 1 + > > drivers/firmware/efi/libstub/x86-stub.c | 13 +++++-- > > 4 files changed, 54 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > > index aa8da0a49829..c155837cedc9 100644 > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > @@ -13,6 +13,7 @@ > > #include <linux/efi.h> > > #include <linux/kernel.h> > > #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */ > > +#include <linux/cmdline.h> > > #include <asm/efi.h> > > #include <asm/setup.h> > > > > @@ -172,6 +173,40 @@ int efi_printk(const char *fmt, ...) > > return printed; > > } > > > > +/** > > + * efi_handle_cmdline() - handle adding in building parts of the command line > > + * @cmdline: kernel command line > > + * > > + * Add in the generic parts of the commandline and start the parsing of the > > + * command line. > > + * > > + * Return: status code > > + */ > > +efi_status_t efi_handle_cmdline(char const *cmdline) > > +{ > > + efi_status_t status; > > + > > + status = efi_parse_options(CMDLINE_PREPEND); > > + if (status != EFI_SUCCESS) { > > + efi_err("Failed to parse options\n"); > > + return status; > > + } > > Even though I am not a fan of the 'success handling' pattern, > duplicating the exact same error handling three times is not great > either. Could we reuse more of the code here? How about efi_status_t status = 0; status |= efi_parse_options(CMDLINE_PREPEND); then error checking once ? > > + > > + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); > > What is the point of calling efi_parse_options() with an empty string? I could change it to if ((IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) ? > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > @@ -172,6 +172,12 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > > goto fail; > > } > > > > +#ifdef CONFIG_GENERIC_CMDLINE > > + status = efi_handle_cmdline(cmdline_ptr); > > + if (status != EFI_SUCCESS) { > > + goto fail_free_cmdline; > > + } > > +#else > > if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || > > IS_ENABLED(CONFIG_CMDLINE_FORCE) || > > Does this mean CONFIG_GENERIC_CMDLINE does not replace CMDLINE_EXTEND > / CMDLINE_FORCE etc, but introduces yet another variant on top of > those? > > That does not seem like an improvement to me. I think it is great that > you are cleaning this up, but only if it means we can get rid of the > old implementation. It does replace extend and force. I was under the impression this code was shared between arm64 and arm32. If that's not the case I can delete the extend and force section. I haven't submitted a conversion for arm32 yet. Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline 2021-03-30 17:57 ` [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline Daniel Walker 2021-03-31 16:10 ` Ard Biesheuvel @ 2021-04-02 17:36 ` Christophe Leroy 2021-04-06 16:42 ` Daniel Walker 1 sibling, 1 reply; 5+ messages in thread From: Christophe Leroy @ 2021-04-02 17:36 UTC (permalink / raw) To: Daniel Walker, Will Deacon, ob Herring, Daniel Gimpelevich, Andrew Morton, x86, linux-mips, linuxppc-dev Cc: xe-linux-external, Ard Biesheuvel, linux-efi, linux-kernel Le 30/03/2021 à 19:57, Daniel Walker a écrit : > This adds code to handle the generic command line changes. > The efi code appears that it doesn't benefit as much from this design > as it could. > > For example, if you had a prepend command line with "nokaslr" then > you might be helpful to re-enable it in the boot loader or dts, > but there appears to be no way to re-enable kaslr or some of the > other options. > > Cc: xe-linux-external@cisco.com > Signed-off-by: Daniel Walker <danielwa@cisco.com> > --- > .../firmware/efi/libstub/efi-stub-helper.c | 35 +++++++++++++++++++ > drivers/firmware/efi/libstub/efi-stub.c | 7 ++++ > drivers/firmware/efi/libstub/efistub.h | 1 + > drivers/firmware/efi/libstub/x86-stub.c | 13 +++++-- > 4 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index aa8da0a49829..c155837cedc9 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -13,6 +13,7 @@ > #include <linux/efi.h> > #include <linux/kernel.h> > #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */ > +#include <linux/cmdline.h> > #include <asm/efi.h> > #include <asm/setup.h> > > @@ -172,6 +173,40 @@ int efi_printk(const char *fmt, ...) > return printed; > } > > +/** > + * efi_handle_cmdline() - handle adding in building parts of the command line > + * @cmdline: kernel command line > + * > + * Add in the generic parts of the commandline and start the parsing of the > + * command line. > + * > + * Return: status code > + */ > +efi_status_t efi_handle_cmdline(char const *cmdline) > +{ > + efi_status_t status; > + > + status = efi_parse_options(CMDLINE_PREPEND); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + status = efi_parse_options(CMDLINE_APPEND); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + return status; > + } > + > + return EFI_SUCCESS; > +} I think we can refactor to first build the final command line, then call efi_parse_options() only once after that. The big advantage of GENERIC_CMDLINE should be to not address anymore CONFIG_CMDLINE_XXX options at all outside of linux/cmdline.h > + > /** > * efi_parse_options() - Parse EFI command line options > * @cmdline: kernel command line > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index 26e69788f27a..760480248adf 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -172,6 +172,12 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > goto fail; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_cmdline(cmdline_ptr); > + if (status != EFI_SUCCESS) { > + goto fail_free_cmdline; > + } > +#else Why an alternative ? > if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || > IS_ENABLED(CONFIG_CMDLINE_FORCE) || > cmdline_size == 0) { > @@ -189,6 +195,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > goto fail_free_cmdline; > } > } > +#endif > > efi_info("Booting Linux Kernel...\n"); > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index cde0a2ef507d..07c7f9fdfffc 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -800,6 +800,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr, > unsigned long alignment, > unsigned long min_addr); > > +efi_status_t efi_handle_cmdline(char const *cmdline); > efi_status_t efi_parse_options(char const *cmdline); > > void efi_parse_option_graphics(char *option); > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index f14c4ff5839f..30ad8fb7122d 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -673,6 +673,8 @@ unsigned long efi_main(efi_handle_t handle, > unsigned long bzimage_addr = (unsigned long)startup_32; > unsigned long buffer_start, buffer_end; > struct setup_header *hdr = &boot_params->hdr; > + unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > + ((u64)boot_params->ext_cmd_line_ptr << 32)); > efi_status_t status; > > efi_system_table = sys_table_arg; > @@ -735,6 +737,14 @@ unsigned long efi_main(efi_handle_t handle, > image_offset = 0; > } > > +#ifdef CONFIG_GENERIC_CMDLINE > + status = efi_handle_cmdline((char *)cmdline_paddr); > + if (status != EFI_SUCCESS) { > + efi_err("Failed to parse options\n"); > + goto fail; > + } > +#else /* CONFIG_GENERIC_CMDLINE */ > + > #ifdef CONFIG_CMDLINE_BOOL > status = efi_parse_options(CONFIG_CMDLINE); > if (status != EFI_SUCCESS) { > @@ -743,8 +753,6 @@ unsigned long efi_main(efi_handle_t handle, > } > #endif > if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { > - unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr | > - ((u64)boot_params->ext_cmd_line_ptr << 32)); > status = efi_parse_options((char *)cmdline_paddr); > if (status != EFI_SUCCESS) { > efi_err("Failed to parse options\n"); > @@ -752,6 +760,7 @@ unsigned long efi_main(efi_handle_t handle, > } > } > > +#endif > /* > * At this point, an initrd may already have been loaded by the > * bootloader and passed via bootparams. We permit an initrd loaded > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline 2021-04-02 17:36 ` Christophe Leroy @ 2021-04-06 16:42 ` Daniel Walker 0 siblings, 0 replies; 5+ messages in thread From: Daniel Walker @ 2021-04-06 16:42 UTC (permalink / raw) To: Christophe Leroy Cc: Will Deacon, ob Herring, Daniel Gimpelevich, Andrew Morton, x86, linux-mips, linuxppc-dev, xe-linux-external, Ard Biesheuvel, linux-efi, linux-kernel On Fri, Apr 02, 2021 at 07:36:53PM +0200, Christophe Leroy wrote: > > > Le 30/03/2021 à 19:57, Daniel Walker a écrit : > > This adds code to handle the generic command line changes. > > The efi code appears that it doesn't benefit as much from this design > > as it could. > > > > For example, if you had a prepend command line with "nokaslr" then > > you might be helpful to re-enable it in the boot loader or dts, > > but there appears to be no way to re-enable kaslr or some of the > > other options. > > > > Cc: xe-linux-external@cisco.com > > Signed-off-by: Daniel Walker <danielwa@cisco.com> > > --- > > .../firmware/efi/libstub/efi-stub-helper.c | 35 +++++++++++++++++++ > > drivers/firmware/efi/libstub/efi-stub.c | 7 ++++ > > drivers/firmware/efi/libstub/efistub.h | 1 + > > drivers/firmware/efi/libstub/x86-stub.c | 13 +++++-- > > 4 files changed, 54 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > > index aa8da0a49829..c155837cedc9 100644 > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > @@ -13,6 +13,7 @@ > > #include <linux/efi.h> > > #include <linux/kernel.h> > > #include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */ > > +#include <linux/cmdline.h> > > #include <asm/efi.h> > > #include <asm/setup.h> > > @@ -172,6 +173,40 @@ int efi_printk(const char *fmt, ...) > > return printed; > > } > > +/** > > + * efi_handle_cmdline() - handle adding in building parts of the command line > > + * @cmdline: kernel command line > > + * > > + * Add in the generic parts of the commandline and start the parsing of the > > + * command line. > > + * > > + * Return: status code > > + */ > > +efi_status_t efi_handle_cmdline(char const *cmdline) > > +{ > > + efi_status_t status; > > + > > + status = efi_parse_options(CMDLINE_PREPEND); > > + if (status != EFI_SUCCESS) { > > + efi_err("Failed to parse options\n"); > > + return status; > > + } > > + > > + status = efi_parse_options(IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ? "" : cmdline); > > + if (status != EFI_SUCCESS) { > > + efi_err("Failed to parse options\n"); > > + return status; > > + } > > + > > + status = efi_parse_options(CMDLINE_APPEND); > > + if (status != EFI_SUCCESS) { > > + efi_err("Failed to parse options\n"); > > + return status; > > + } > > + > > + return EFI_SUCCESS; > > +} > > I think we can refactor to first build the final command line, then call > efi_parse_options() only once after that. I tried this, like what you did in your v4 .. The issues are similar to the prom_init.c problems. The environment is delicate and requires careful programming to get it done correctly. > The big advantage of GENERIC_CMDLINE should be to not address anymore > CONFIG_CMDLINE_XXX options at all outside of linux/cmdline.h I agree , but not I've found that it's not likely to get this all changed in a single series. Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-06 16:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <41021d66db2ab427c14255d2a24bb4517c8b58fd.1617126961.git.danielwa@cisco.com>
2021-03-30 17:57 ` [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline Daniel Walker
2021-03-31 16:10 ` Ard Biesheuvel
2021-03-31 18:21 ` Daniel Walker
2021-04-02 17:36 ` Christophe Leroy
2021-04-06 16:42 ` Daniel Walker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox