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

Fri Sep 20 09:25:29 CEST 2024

Hi Tom,

On Thu, 19 Sept 2024 at 19:45, Tom Rini <trini at 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 at 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 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.
> > >
> > > 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 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.
> > >
> > > 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.
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:
- with sandbox we can test the operation of the bootmeth, including
under failure conditions
- 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
- we can build on this test to cover other bootmeths without needing
to install a full OS just to run a test

Just the sheer number of bugs found and fixed by this test should show
its value. One of the bugs was an access at location 0 which many
boards happily accept, but sandbox catches.

Also, someone should keep track of all the bugs we never see, because
a test covers them. For example, driver model is well-covered by tests
and we seldom see a bug. If all we had was QEMU, then all our tests
would be 'happy path' and we would be in a very different position.

One lesson I see here is that I should have pushed for a proper block
device with virtio at the time. But who could have foreseen that EFI
would throw away the 'lazy init' approach of U-Boot and just scan all
the devices?

Regards,
Simon