Re: [PATCH v2 07/10] efi: payload: pass shell arguments as load options to executed images

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Marco,

On 10/15/26 12:14 PM, Marco Felsch wrote:
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>>  	if (filetype_is_linux_efi_image(filetype)) {
>> -		options = linux_bootargs_get();
>>  		printf("Booting kernel via StartImage");
>> -		if (options) {
>> -			printf("with options '%s'", options);
>> -			loaded_image->load_options = xstrdup_char_to_wchar(options);
>> -			loaded_image->load_options_size =
>> -				(strlen(options) + 1) * sizeof(wchar_t);
>> -		}
>> +		if (load_options)
>> +			printf(" with options '%s'", options);
>>  		printf("...\n");
>> +	} else if (load_options) {
>> +		pr_debug("Starting image with options '%s'\n", options);
>>  	}
> 
> Can we please make use one print style either printf() or pr_*()? IMHO
> pr_*() is better since this goes into the log buffer which could be
> pstore backed.

This is consistent with how we do it for ARM64 and ARM32. An argument in
favor of printf is that test suites often match against these strings as
these are the final strings output by barebox prior to kernel boot.

Having the log level hide them would break this. I don't think it's the
purpose of this series to deviate from this.

> Furthermore this if-else now is only responsible for handling the print
> message, can we make this cleaner?

I don't understand what you mean by this.

>> -	if (!is_driver)
>> +	/*
>> +	 * The firmware unloads an application as soon as it returns, as well as
>> +	 * a driver that failed to start, freeing the loaded image protocol with
>> +	 * it. Only a still loaded driver's protocol may be touched here, and it
>> +	 * must be, as it references the load options we are about to free.
>> +	 * Unloading an application that already returned just fails, but is
>> +	 * still needed when StartImage failed before running it.
>> +	 */
>> +	if (is_driver) {
>> +		if (!EFI_ERROR(efiret)) {
>> +			loaded_image->load_options = NULL;
> 					^
> Don't we have to free this first?

See comment above. This pointer is dangling as the firmware would free
the load options on successful termination.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>> +			loaded_image->load_options_size = 0;
>> +		}
>> +	} else {
>>  		BS->unload_image(handle);
>> +	}
>> +
>> +	free(load_options);
>>  
>>  	efi_connect_all();
>>  	efi_register_devices();
>> @@ -163,13 +182,21 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>>  {
>>  	struct efi_loaded_image *loaded_image;
>>  	efi_handle_t handle;
>> +	char *options;
>>  	int ret;
>>  
>>  	ret = efi_load_image(file, &loaded_image, &handle);
>>  	if (ret)
>>  		return ret;
>>  
>> -	return efi_execute_image(handle, loaded_image, false, b->type);
>> +	/* argv[0] is the image itself, the rest become the load options */
>> +	options = strjoin(" ", &argv[1], argc - 1);
>> +
>> +	ret = efi_execute_image(handle, loaded_image, false, b->type, options);
>> +
>> +	free(options);
>> +
>> +	return ret;
>>  }
>>  
>>  static struct binfmt_hook binfmt_efi_hook = {
>> diff --git a/efi/payload/image.h b/efi/payload/image.h
>> index 33f7e1a21b30..54494c71626a 100644
>> --- a/efi/payload/image.h
>> +++ b/efi/payload/image.h
>> @@ -14,7 +14,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);
>> +		      enum filetype filetype, const char *options);
>>  
>>  extern struct image_handler efi_x86_linux_handle_tr;
>>  extern struct image_handler efi_x86_linux_handle_handover;
>> -- 
>> 2.47.3
>>
>>

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |