[PATCH v2 37/39] efi: Avoid using sandbox virtio devices

[Simon Glass]

Hi Tom,

On Thu, 8 Aug 2024 at 14:06, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Aug 08, 2024 at 12:44:05PM -0600, Simon Glass wrote:
> > Hi Heinrick, Tom,
> >
> > On Tue, 6 Aug 2024 at 19:56, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Wed, Aug 07, 2024 at 03:47:21AM +0200, Heinrich Schuchardt wrote:
> > > > On 06.08.24 14:58, 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 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.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >   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;
> > > >
> > > > We should avoid depending on the sandbox everywhere.
> > > >
> > > > Please, fix the problem in the sandbox driver.
> > > >
> > > > If you cannot fix it, run the tests involving virtio on QEMU instead of
> > > > the sandbox.
> >
> > Which test? All of the EFI tests fail due to this problem. The test
> > actually has nothing to do with virtio, it is just that EFI goes and
> > probes every single block device, since [1].
>
> Aren't we running "the tests" on other platforms such as QEMU today?
>
> > > This is an area we go back-and-forth on but, yes, IMHO, if we can't
> > > easily provide a virtio device for sandbox, QEMU is right there and what
> > > this is for, so I see sandbox as more useful as build rather than
> > > runtime checking in this case.
> >
> > The best solution would be to implement a simple emulator, like we do
> > in other places for sandbox. At present virtio_sandbox_notify() is
> > empty.
> >
> > I don't mind working on that, but would like to get a temporary
> > solution here so this test can land.
> >
> > Talking about virtio for QEMU is missing the point of this test, which
> > is after all a test of booting an EFI app. I do wish more people would
> > see the value in these unit tests. There is a talk at [2] which shows
> > how emulators are used in Zephyr.
>
> So that talk is interesting, yes. So, yes, implement the bus driver for
> sandbox for virtio, and until then we shouldn't have the tests running
> on sandbox? Or am I still missing something?

Sure, but perhaps there is a way to get this test landed without doing
that work right away?

I'm not sure if we actually need d5391bf02b9 ("efi_loader: ensure all
block devices are probed"). It seems to fix a real problem, though.

>
> But I also still say that given that we as a project are more resource
> constrained than Zephyr, for things that are QEMU-centric, there's
> already a wealth of information on debugging QEMU since it too is
> software. There's only so many hours in the day after all.

One of Zephyr's challenges is that it relied on QEMU for almost all
testing for a long time. As a result it takes a huge amount of CPU
power to run tests - last I checked it was something like an hour on a
64-core machine. U-Boot's unit tests ('ut all') run in about 12
seconds on my machine. I could go on for hours about the different
types of tests and the benefits of one versus the other, but the
cheapest answer is not necessarily just to do a 'happy path' test and
call it good.

For this particular test, I do want to have tests for each bootmeth,
to the point of running the target, so far as possible. It turns out
that we can launch an EFI app on sandbox, so connecting it up to
bootstd seems valuable to me.

I've filed an issue for the virtio improvements.

Regards,
Simon