Re: [PATCH v2 3/4] efi_loader: Move device-removal later in exit-boot-services

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 07.04.25 03:35, Simon Glass wrote:
> This removal should be the last thing done, so that U-Boot does no more
> memory allocations afterwards, thus avoiding potentially allocating
> memory which has been freed by a device that fails to de-activate its
> DMA.

The EFI application that is calling ExitBootServices() has been reading
the EFI memory map with GetMemoryMap() before. This is checked by
comparing the MapKey parameter.

Whatever allocations are done or not in ExitBootServices() is not
visible to the EFI application.

DMA has to be stopped in all cases.

I don't understand the virtue of the proposed change.

Best regards

Heinrich

>
> Of course, devices should be marked with DM_FLAG_ACTIVE_DMA or
> DM_FLAG_OS_PREPARE but this change is good practice, in any case.
>
> It also matches the code in announce_and_cleanup(), which we should at
> some point unify with EFI_LOADER
>
> So move the code and add a comment.
>
> Note that the TCG2 log is updated after this call, but I cannot see any
> allocations there.
>
> Reported-by: Christian Kohlschütter <christian@kohlschutter.com>
> Link: https://lore.kernel.org/u-boot/C101B675-EEE6-44CB-8A44-83F72182FBD6@kohlschutter.com/
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>   lib/efi_loader/efi_boottime.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index ffe43accd1e..e525662f82f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2250,14 +2250,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   			list_del(&evt->link);
>   	}
>
> -	if (!efi_st_keep_devices) {
> -		bootm_disable_interrupts();
> -		if (IS_ENABLED(CONFIG_USB_DEVICE))
> -			udc_disconnect();
> -		board_quiesce_devices();
> -		dm_remove_devices_active();
> -	}
> -
>   	/* Patch out unsupported runtime function */
>   	efi_runtime_detach();
>
> @@ -2279,6 +2271,19 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   	/* Give the payload some time to boot */
>   	efi_set_watchdog(0);
>   	schedule();
> +
> +	/*
> +	 * this should be the last thing done, to avoid memory allocations
> +	 * between removing devices and the OS taking over
> +	 */
> +	if (!efi_st_keep_devices) {
> +		bootm_disable_interrupts();
> +		if (IS_ENABLED(CONFIG_USB_DEVICE))
> +			udc_disconnect();
> +		board_quiesce_devices();
> +		dm_remove_devices_active();
> +	}
> +
>   out:
>   	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
>   		if (ret != EFI_SUCCESS)