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

Wed Aug 14 19:56:01 CEST 2024

On Sun, Aug 11, 2024 at 08:50:21AM -0600, Simon Glass wrote:
> 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?

Maybe? I guess I'm missing how the problem isn't a problem with the
sandbox emulation of it.

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

That commit seems fairly intentional and I kinda recall that being one
of those challenge points, conceptually. In order for the EFI_LOADER to
do what it needs to do correctly, it needs to know what all exists. This
is contrary to the usual U-Boot practice of not probing something until
it's specifically needed.

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

Everything has it's place, yes. For us, sandbox tests take 15-20 minutes
per run in CI and most QEMU platforms finish up in about a minute. And
of course, it would be real nice if we could easily build those
platforms in CI with CONFIG_UNIT_TEST enabled, but I don't think that
would cause the run time to balloon up (nor are they the cause of the
overall time on sandbox, that would be filesystem tests).

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

OK, thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240814/45c0bf65/attachment.sig>