Hi Heinrich, On Mon, 7 Apr 2025 at 19:55, Heinrich Schuchardt <[email protected]> wrote: > > 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.
Yes, DMA must be stopped. > > I don't understand the virtue of the proposed change. It is described in the next two paragraphs: > > 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 See above. Also, what do you think about unifying with announce_and_cleanup() ? Regards, Simon > > > > 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 <[email protected]> > > Link: > > https://lore.kernel.org/u-boot/[email protected]/ > > > > Signed-off-by: Simon Glass <[email protected]> > > --- > > > > (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) >
