Re: [PATCH 6/8] drivers: firmware: efi: libstub: enable generic commandline

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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
>