Re: [PATCH v2 1/2] efi_firmware: set EFI capsule dfu_alt_info env explicitly

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Michal,

Thank you for testing this.

On lun., févr. 10, 2025 at 13:40, Michal Simek <michal.simek@amd.com> wrote:

> On 2/6/25 16:47, Jonathan Humphreys wrote:
>> The current implementation of EFI capsule update uses set_dfu_alt_info() to
>> set the dfu_alt_info environment variable with the settings it requires.
>> However, set_dfu_alt_info() is doing this for all DFU operations, even
>> those unrelated to capsule update.
>> 
>> Thus other uses of DFU, such as DFU boot which sets its own value for the
>> dfu_alt_info environment variable, will have that setting overwritten with
>> the capsule update setting. Similarly, any user defined value for the
>> dfu_alt_info environment variable would get overwritten when any DFU
>> operation was performed, including simply performing a "dfu 0 list"
>> command.
>> 
>> The solution is stop using the set_dfu_alt_info() mechanism to set the
>> dfu_alt_info environment variable and instead explicitly set it to the
>> capsule update's setting just before performing the capsule update's DFU
>> operation, and then restore the environment variable back to its original
>> value.
>> 
>> This patch implements the explicit setting and restoring of the
>> dfu_alt_info environment variable as part of the EFI capsule update
>> operation.
>> 
>> The fix is fully implemented in a subsequent patch that removes the capsule
>> update dfu_alt_info support in set_dfu_alt_info().
>> 
>> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
>> ---
>>   lib/efi_loader/efi_firmware.c | 39 ++++++++++++++++++++++++++++++++---
>>   1 file changed, 36 insertions(+), 3 deletions(-)
>> 
>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
>> index 5a754c9cd03..1a1cf3b55e1 100644
>> --- a/lib/efi_loader/efi_firmware.c
>> +++ b/lib/efi_loader/efi_firmware.c
>> @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>>   	efi_status_t (*progress)(efi_uintn_t completion),
>>   	u16 **abort_reason)
>>   {
>> +	int ret;
>>   	efi_status_t status;
>>   	struct fmp_state state = { 0 };
>> +	char *orig_dfu_env;
>>   
>>   	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>>   		  image_size, vendor_code, progress, abort_reason);
>> @@ -663,7 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>>   	if (status != EFI_SUCCESS)
>>   		return EFI_EXIT(status);
>>   
>> -	if (fit_update(image))
>> +	orig_dfu_env = strdup(env_get("dfu_alt_info"));
>> +	if (env_set("dfu_alt_info", update_info.dfu_string)) {
>
> This pretty much breaks all xilinx platforms because we actually are not 
> configuring dfu_string.
>
> I have sent RFC. If you can squash it to your patch that would be the best.
> Pretty much the part of it should be in 1/2 and the part in 2/2.

For reference, the patch that has been send as RFC is:
http://lore.kernel.org/r/c8378bd1bbc7a96ecd802897ca72e26a02bf5a2b.1739190503.git.michal.simek@amd.com

>
> M