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

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

Le 10/11/2023 à 02:38, 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.

So what can we do to improve that ?


> 
> 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,

s/you/it

> but there appears to be no way to re-enable kaslr or some of the
> other options.
> 
> The efi command line handling is incorrect. x86 and arm have an append
> system however the efi code prepends the command line.
> 
> For example, you could have a non-upgradable bios which sends
> 
> efi=disable_early_pci_dma
> 
> This hypothetically could have been set because early pci dma caused
> issues on early versions of the product.
> 
> Then later the early pci dma was made to work and the company desired
> to start using it. To override the bios you could set the CONFIG_CMDLINE
> to,
> 
> efi=no_disable_early_pci_dma
> 
> then parsing would normally start with the bios command line, then move
> to the CONFIG_CMDLINE and you would end up with early pci dma turned on.
> 
> however, current efi code keeps early pci dma off because the bios
> arguments always override the built in.
> 
> Per my reading this is different from the main body of x86, arm, and
> arm64.
> 
> The generic command line provides both append and prepend, so it
> alleviates this issue if it's used. However not all architectures use
> it.
> 
> It would be desirable to allow the efi stub to have it's builtin command
> line to be modified after compile, but I don't see a feasible way to do
> that currently.

Would be desirable or is desirable ? Your explanations are unclear.


> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>   .../firmware/efi/libstub/efi-stub-helper.c    | 29 +++++++++++++++++++
>   drivers/firmware/efi/libstub/efi-stub.c       |  9 ++++++
>   drivers/firmware/efi/libstub/efistub.h        |  1 +
>   drivers/firmware/efi/libstub/x86-stub.c       | 14 +++++++--
>   4 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index bfa30625f5d0..952fa2cdff51 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -11,6 +11,7 @@
>   
>   #include <linux/efi.h>
>   #include <linux/kernel.h>
> +#include <linux/cmdline.h>
>   #include <asm/efi.h>
>   #include <asm/setup.h>
>   
> @@ -29,6 +30,34 @@ bool __pure __efi_soft_reserve_enabled(void)
>   	return !efi_nosoftreserve;
>   }
>   
> +/**
> + * efi_handle_cmdline() - handle adding in built-in 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_builtin_cmdline(char const *cmdline)
> +{
> +	efi_status_t status = EFI_SUCCESS;
> +
> +	if (sizeof(CMDLINE_STATIC_PREPEND) > 1)
> +		status |= efi_parse_options(CMDLINE_STATIC_PREPEND);
> +
> +	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
> +		status |= efi_parse_options(cmdline);
> +
> +	if (sizeof(CMDLINE_STATIC_APPEND) > 1)
> +		status |= efi_parse_options(CMDLINE_STATIC_APPEND);
> +
> +	if (status != EFI_SUCCESS)
> +		efi_err("Failed to parse options\n");
> +
> +	return status;
> +}
> +
>   /**
>    * 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 f9c1e8a2bd1d..770abe95c0ee 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -127,6 +127,14 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>   		return EFI_OUT_OF_RESOURCES;
>   	}
>   
> +#ifdef CONFIG_GENERIC_CMDLINE
> +	status = efi_handle_builtin_cmdline(cmdline);
> +	if (status != EFI_SUCCESS) {
> +		goto fail_free_cmdline;
> +	}
> +#endif
> +
> +#ifdef CONFIG_CMDLINE
>   	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
>   	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
>   	    cmdline_size == 0) {
> @@ -144,6 +152,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
>   			goto fail_free_cmdline;
>   		}
>   	}
> +#endif
>   
>   	*cmdline_ptr = cmdline;
>   	return EFI_SUCCESS;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 212687c30d79..1ac6631905c5 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -996,6 +996,7 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
>   				 unsigned long alignment,
>   				 unsigned long min_addr);
>   
> +efi_status_t efi_handle_builtin_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 9d5df683f882..273a8a9c8bbb 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -847,6 +847,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   	struct setup_header *hdr = &boot_params->hdr;
>   	const struct linux_efi_initrd *initrd = NULL;
>   	unsigned long kernel_entry;
> +	unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
> +				       ((u64)boot_params->ext_cmd_line_ptr << 32));
>   	efi_status_t status;
>   
>   	boot_params_pointer = boot_params;
> @@ -877,6 +879,14 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   		goto fail;
>   	}
>   
> +#ifdef CONFIG_GENERIC_CMDLINE
> +	status = efi_handle_builtin_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) {
> @@ -885,8 +895,6 @@ void __noreturn efi_stub_entry(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");
> @@ -894,6 +902,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>   		}
>   	}
>   
> +#endif
> +
>   	status = efi_decompress_kernel(&kernel_entry);
>   	if (status != EFI_SUCCESS) {
>   		efi_err("Failed to decompress kernel\n");