Hi Tom, On Tue, 17 Sept 2024 at 19:03, Tom Rini <tr...@konsulko.com> wrote: > > On Mon, Sep 16, 2024 at 05:42:31PM +0200, Simon Glass wrote: > > Hi Heinrich, > > > > On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt <xypron.g...@gmx.de> > > wrote: > > > > > > On 02.09.24 03:18, Simon Glass wrote: > > > > While sandbox supports virtio it cannot support actually using the block > > > > devices to read files, since there is nothing on the other end of the > > > > 'virtqueue'. > > > > > > > > A recent change makes EFI probe all block devices, whether used or not. > > > > This is apparently required by EFI, although it violates U-Boot's > > > > lazy-init principle. > > > > > > We always did this. > > > > Commit d5391bf02b9 dates from 2022, so I don't think that is correct. > > Yes, but I also could have sworn that was fixing the behavior having > been changed again previous to that.
I don't see any evidence of that, though. > > > > What problem do you want to fix? I have not seen any issues in our CI. > > > > The EFI test in this series hangs trying to probe a virtio block > > device. If you drop this patch and try the rest of the series in CI, > > you will see the failure. Or you could just accept that I investigated > > this, root-caused it and produced a suitable fix. This is a v5 patch > > which has had quite a bit of discussion. > > And as I noted an iteration or two back, it's entirely unclear if the > problem is "sandbox virtio is broken" or "this code is wrong here". > Which in fact gets us back to ... sandbox virtio does not support a functioning block device > > > > > We cannot just drop the virtio devices as they are used in sandbox > > > > tests. > > > > > > > > So for now just add a special case to work around this. > > > > > > > > The eventual fix is likely adding an implementation of > > > > virtio_sandbox_notify() to actually do the block read. That is tracked > > > > in [1]. > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > > > > [1] https://source.denx.de/u-boot/u-boot/-/issues/37 > > > > --- > > > > > > > > (no changes since v3) > > > > > > > > Changes in v3: > > > > - Add a Fixes tag > > > > - Mention the issue created for this problem > > > > > > > > lib/efi_loader/efi_disk.c | 14 +++++++++++++- > > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > > index 93a9a5ac025..2e1d37848fc 100644 > > > > --- a/lib/efi_loader/efi_disk.c > > > > +++ b/lib/efi_loader/efi_disk.c > > > > @@ -838,8 +838,20 @@ efi_status_t efi_disk_get_device_name(const > > > > efi_handle_t handle, char *buf, int > > > > efi_status_t efi_disks_register(void) > > > > { > > > > struct udevice *dev; > > > > + struct uclass *uc; > > > > > > > > - uclass_foreach_dev_probe(UCLASS_BLK, dev) { > > > > + uclass_id_foreach_dev(UCLASS_BLK, dev, uc) { > > > > + /* > > > > + * The virtio block-device hangs on sandbox when accessed > > > > since > > > > + * there is nothing listening to the mailbox > > > > + */ > > > > + if (IS_ENABLED(CONFIG_SANDBOX)) { > > > > + struct blk_desc *desc = dev_get_uclass_plat(dev); > > > > + > > > > + if (desc->uclass_id == UCLASS_VIRTIO) > > > > + continue; > > > > + } > > > > + device_probe(dev); > > > > } > > > > > > > > return EFI_SUCCESS; > > > > > > Please, do not spray sandbox tweaks all over the place. > > > > > > Can't you just return an error from the sandbox-virtio driver when an > > > attempt to read a queue is made? > > > > > > We are using virtio on QEMU. Why do we need sandbox virtio devices? Just > > > run the relevant tests on the real thing. > > > > Please go ahead with whatever approach to testing you wish. But > > sandbox testing is an important component of U-Boot. > > Yes, but is sandbox implementing the "just be a state machine" part here > correctly, or not? It keeps feeling like "not" and that the reasonable > course of action would be to stop testing this on sandbox until that is > fixed especially since we can test this reliably on qemu. We have to move things forward a piece at a time. Not having a proper test for the EFI bootmeth is a significant gap and is what I am trying to fix with this series. It isn't perfect, but it is a step forward and will prevent regressions. It can also be built on later. Happy-path testing with QEMU is all very well, but it only goes so far. Regards, Simon