[PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

On 11/16/23 02:42, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> On 11/15/23 17:23, Heinrich Schuchardt wrote:
>>> On 11/15/23 16:50, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
>>>>> <sjg at chromium.org>:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
>>>>>> <xypron.glpk at gmx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
>>>>>>> <sjg at chromium.org>:
>>>>>>>> When a USB device is unbound, it causes any bootflows attached to
>>>>>>>> it to
>>>>>>>> be removed, via a call to bootdev_clear_bootflows() from
>>>>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
>>>>>>>> bootflow.
>>>>>>>>
>>>>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
>>>>>>>> called, which unbinds the device. For EFI, this happens in
>>>>>>>> efi_exit_boot_services() which means that the bootflow disappears
>>>>>>>> before it is finished with.
>>>>>>>
>>>>>>> After ExitBootServices() no driver of U-Boot can be used as the
>>>>>>> operating system is in charge.
>>>>>>>
>>>>>>> Any bootflow inside U-Boot is terminated by definition when
>>>>>>> reaching ExitBootServices.
>>>>>>>
>>>>>>>>
>>>>>>>> There is no need to unbind all the USB devices just to quiesce them.
>>>>>>>> Add a new usb_pause() call which removes them but leaves them bound.
>>>>>>>
>>>>>>> As U-Boot must not access any device after ExitBootServices() it is
>>>>>>> unclear to me what you want to achieve.
>>>>>>
>>>>>> I can't remember exactly what is needed from the bootflow, but
>>>>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
>>>>>> have been better if I had mentioned this explicitly,  But then this
>>>>>> patch has been sitting around for ages...
>>>>>>
>>>>>> In any case, the intent of exit-boot-services is not to free all the
>>>>>> memory used, since some of it may have been used to hold data that the
>>>>>> app continues to use.
>>>>>
>>>>> The EFI application reads the memory map and receives an ID which it
>>>>> passes to ExitBootServiced() after this point any memory not marked
>>>>> as EFI runtime can be used by the EFI app. This includes the memory
>>>>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
>>>>> here.
>>>>>
>>>>> Starting the EFI app via StartImage() must  terminate any running
>>>>> U-Boot "boot flow".
>>>>>
>>>>> After ExitBootServices() the EFI application cannot return to U-Boot
>>>>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
>>>>>
>>>>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Also there is U-Boot code between when the devices are unbound and
>>>>>> when U-Boot actually exits back to the app.
>>>>>>
>>>>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
>>>>>> quite a while to find.
>>>>>
>>>>> We need a better understanding of the problem that you experience to
>>>>> find an adequate solution. Why does removing all devices lead to
>>>>> hanging the system?
>>>>
>>>> Are you able to test this? With your better knowledge of EFI you might
>>>> be able to figure out something else that is going on. But in my case
>>>> it causes some memory to be freed (perhaps the memory holding the EFI
>>>> app?), which is then overwritten by something else being malloc()'d,
>>>
>>> The memory for the EFI app is not assigned via malloc(). It is allocated
>>> by AllocatedPages().
>>>
>>> Reading "some memory freed" does not give me confidence that the problem
>>> is sufficiently analyzed.
>>>
>>>> so the boot hangs. It is hard to see what is going on after the app
>>>> starts.
>>>>
>>>> This patch was sent almost two months ago and fixes a real bug. The
>>>> first few versions attracted no comment now it is being blocked
>>>> because you don't understand how it fixes anything.
>>>
>>> Do you understand why unbinding the devices causes the problem?
>>>
>>>>
>>>> I can get the hardware up again and try this but it will take a while.
>>>
>>> Digging a bit deeper seems to be the right approach.
>>
>> Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
>> https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/
>>
>> points to a probable root cause:
>>
>> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
>> free(bflow->buf)
>>
>> In the EFI boot bflow->buf points to $kernel_addr_r which never was
>> allocated and therefore must not be freed.
>
> Yes, this is the root cause of the crash.
>
> However, this patch should still be applied. I can update the commit
> message if you like.
>
> Freeing the FDT and kernel before booting them is just not safe,
> whether EFI or anything else. Freed memory is not guaranteed to hang
> around for any length of time. For example, with truetype fonts,
> malloc() is called during any console output and will likely corrupt
> the images just as they are being booted.

Please, observe that malloc() and efi_allocate_pages() use completely
separate parts of the memory.

When reaching ExitBootServices() the memory used by the EFI binary
(relocated in efi_load_pe()) and for the configuration table with the
device-tree have been allocated by efi_allocate_pages(). These addresses
cannot neither allocated by malloc() nor freed with free().

Best regards

Heinrich

>
> We should leave the devices bound when booting.
>
> Regards,
> Simon