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@gmx.de wrote:
Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass sjg@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-ra...
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.
Best regards
Heinrich
Even without the hang, this still fixes a bug. We should be using device_remove() to quiesce devices. There is no need to unbind them.
Is this what the patch does?
BTW another patch that suffered a similar fate was [1]. I just applied it based on a review from Bin.
Please, ping Ilias and me if you don't get the feedback that you deserve.
Best regards
Heinrich
[..]
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20231001191444.v3.1.I9f7f37...