[PATCH v5 12/14] efi_loader: Avoid using sandbox virtio devices

[Simon Glass]

Hi Heinrich,

On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt <xypron.glpk at 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.

>
> 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.

>
> >
> > 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 <sjg at chromium.org>
> > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed")
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas at 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.

Regards,
Simon