> From: Simon Glass <s...@chromium.org> > Date: Mon, 7 Apr 2025 22:49:05 +1200
Hi Simon, Since I brought up the same objection as Heinrich... > Hi Heinrich, > > On Mon, 7 Apr 2025 at 19:55, Heinrich Schuchardt <xypron.g...@gmx.de> 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. I disagree. Stopping DMA is early as possible is good practice. > > > It also matches the code in announce_and_cleanup(), which we should at > > > some point unify with EFI_LOADER As far as I can see there is nothing that happens in between the old location and your new location in efi_exit_boot_services() that matches anything that is done in announce_and_cleanup(). > 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 <christ...@kohlschutter.com> > > > Link: > > > https://lore.kernel.org/u-boot/c101b675-eee6-44cb-8a44-83f72182f...@kohlschutter.com/ > > > > > > Signed-off-by: Simon Glass <s...@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) > > >
