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
