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

[Simon Glass]

Hi Heinrich,

On Mon, 7 Apr 2025 at 19:55, Heinrich Schuchardt <xypron.glpk at 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.
> >
> > 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 <christian at kohlschutter.com>
> > Link: https://lore.kernel.org/u-boot/C101B675-EEE6-44CB-8A44-83F72182FBD6@kohlschutter.com/
> >
> > Signed-off-by: Simon Glass <sjg at 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)
>