Re: [PATCH v2 06/10] efi: payload: always shutdown barebox when booting

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Ahmad,

On 26-10-15 11:18, Ahmad Fatoum wrote:
> When barebox is about to boot a kernel image, it calls shutdown_barebox
> and then starts the image. When the kernel image is wrapped in a UKI,
> barebox will not detect it as a kernel image and will thus not call
> shutdown_barebox beforehand, which can mean that e.g. state is not
> flushed.
> 
> We do not want to treat UKIs completely like kernels (e.g. we do not
> want to override their built-in bootargs), but we still want to properly
> shutdown barebox.

But it's still possible to pass further kernel commandline arguments to
an UKI?

Regards,
  Marco

> 
> Resolve this by shutting down barebox whenever we are in a bootm handler,
> no matter which kind of image is about to be started: bootm is the point
> of no return. Images run from the shell via binfmt keep returning to
> barebox afterwards, with the exception of EFI-stubbed kernels, which take
> over the machine and thus continue to shut barebox down as before.
> 
> Assisted-by: Claude:opus-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  efi/payload/bootm.c |  4 ++--
>  efi/payload/image.c | 21 +++++++++++++++------
>  efi/payload/image.h |  1 +
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
> index 3168e07a3dc1..2bcfd90e42fa 100644
> --- a/efi/payload/bootm.c
> +++ b/efi/payload/bootm.c
> @@ -185,7 +185,7 @@ static int do_bootm_efi_stub(struct image_data *data)
>  	if (data->dryrun)
>  		goto unload_ramdisk;
>  
> -	ret = efi_execute_image(handle, loaded_image, type);
> +	ret = efi_execute_image(handle, loaded_image, true, type);
>  
>  	/* efi_execute_image takes care to unload the image on error,
>  	 * so we set image_freed and fall through to freeing ramdisk
> @@ -225,7 +225,7 @@ static int efi_app_execute(struct image_data *data)
>  		return 0;
>  	}
>  
> -	return efi_execute_image(handle, loaded_image, type);
> +	return efi_execute_image(handle, loaded_image, true, type);
>  }
>  
>  static int linux_efi_handover = true;
> diff --git a/efi/payload/image.c b/efi/payload/image.c
> index 378709b6de95..3668a6a04cac 100644
> --- a/efi/payload/image.c
> +++ b/efi/payload/image.c
> @@ -100,15 +100,24 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>  
>  int efi_execute_image(efi_handle_t handle,
>  		      struct efi_loaded_image *loaded_image,
> +		      bool is_bootm,
>  		      enum filetype filetype)
>  {
>  	efi_status_t efiret;
>  	const char *options;
> -	bool is_driver, is_kernel = false;
> +	bool is_driver;
> +	bool no_return;
>  
>  	is_driver = (loaded_image->image_code_type == EFI_BOOT_SERVICES_CODE) ||
>  		(loaded_image->image_code_type == EFI_RUNTIME_SERVICES_CODE);
>  
> +	/*
> +	 * A bootm handler is the point of no return, but an EFI-stubbed kernel
> +	 * started from the shell takes over the machine just the same, so
> +	 * barebox needs to be shut down in both cases.
> +	 */
> +	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
> +
>  	if (filetype_is_linux_efi_image(filetype)) {
>  		options = linux_bootargs_get();
>  		printf("Booting kernel via StartImage");
> @@ -119,11 +128,11 @@ int efi_execute_image(efi_handle_t handle,
>  				(strlen(options) + 1) * sizeof(wchar_t);
>  		}
>  		printf("...\n");
> +	}
>  
> +	if (no_return) {
>  		efi_set_variable_usec("LoaderTimeExecUSec", &efi_systemd_vendor_guid,
>  				      ktime_to_us(ktime_get()));
> -
> -		is_kernel = true;
>  		shutdown_barebox();
>  	}
>  
> @@ -133,8 +142,8 @@ int efi_execute_image(efi_handle_t handle,
>  
>  	efi_continue_devices();
>  
> -	if (is_kernel) {
> -		pr_emerg("Kernel image has unexpectedly returned\n");
> +	if (no_return) {
> +		pr_emerg("Boot image has unexpectedly returned\n");
>  		BS->exit(efi_parent_image, efiret, 0, NULL);
>  	}
>  
> @@ -160,7 +169,7 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>  	if (ret)
>  		return ret;
>  
> -	return efi_execute_image(handle, loaded_image, b->type);
> +	return efi_execute_image(handle, loaded_image, false, b->type);
>  }
>  
>  static struct binfmt_hook binfmt_efi_hook = {
> diff --git a/efi/payload/image.h b/efi/payload/image.h
> index bab1be368c21..33f7e1a21b30 100644
> --- a/efi/payload/image.h
> +++ b/efi/payload/image.h
> @@ -13,6 +13,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>  
>  int efi_execute_image(efi_handle_t handle,
>  		      struct efi_loaded_image *loaded_image,
> +		      bool is_bootm,
>  		      enum filetype filetype);
>  
>  extern struct image_handler efi_x86_linux_handle_tr;
> -- 
> 2.47.3
> 
>