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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7800 bytes --]

On Fri, Sep 20, 2024 at 09:25:29AM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 19 Sept 2024 at 19:45, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Sep 19, 2024 at 04:10:12PM +0200, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 17 Sept 2024 at 19:03, Tom Rini <trini@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.glpk@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.
> >
> > Yes, it's just my recollection of the time and I could be misremembering
> > it as one of the other times we've had this discussion.
> >
> > > > > > 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 <sjg@chromium.org>
> > > > > > > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed")
> > > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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.
> >
> > OK, but I can't tell if your answer to my point here is:
> > - Yes, sandbox virtio is broken / incomplete
> > - No, sandbox virtio is fine, there's some other mismatch between how
> >   it's used for sandbox and how it's used for QEMU. This will get
> >   resolved later.
> 
> It is incomplete, in that the block device is not fully implemented.

Then please disable it until you can complete it.

> No other test enables it, but EFI does, since it blinding tries to
> access all block devices without any control.
> 
> >
> > > 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.
> >
> > I frankly get really puzzled about why testing all of this, in QEMU,
> > where we could actually design the test to see if the OS has booted (and
> > if we leave things configurable well enough, do this on real hardware)
> > is wrong but sandbox, where we can't boot the OS, is good. Especially
> > for the device that's only present in an emulator. We're emulating an
> > emulator and not getting matching behavior in our emulator.
> 
> There are many reasons:

To be clear, I'm not saying sandbox tests have no value, or are
unimportant. I apologize for imply as much.

> - with sandbox we can test the operation of the bootmeth, including
> under failure conditions

Yes, state machine tests are useful. But we can test for xFAIL on other
platforms, yes?

> - we can test what happens within U-Boot itself when
> exit-boot-services is called, which is the bug that provoked me to
> write the test

I honestly don't recall the state of the discussion around that patch,
positive/negative/neutral.

> - we can build on this test to cover other bootmeths without needing
> to install a full OS just to run a test

Counter point, we can't test that an OS actually boots. One of the most
valuable personally tests we've added recently is 
test/py/tests/test_net_boot.py which makes the network load and boot an
OS image, and test for (some) failure modes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]