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

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> From: Simon Glass <sjg@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.glpk@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 <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)
> >
>