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

[Tom Rini <trini@konsulko.com>]

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

On Thu, Aug 15, 2024 at 09:33:18PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 14 Aug 2024 at 11:56, Tom Rini <trini@konsulko.com> wrote:
> >
> > 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@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@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@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.
> 
> Basically the virtio block device is probed by EFI (for no useful
> purpose), but doesn't actually work.
> 
> >
> > > 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.
> 
> Indeed, but we have to live with it.

Which gets back to the question I was asking, is the sandbox virtio
driver deficient here? It sounds like that doesn't work, and that's the
problem.

> > > > 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).
> 
> Right, I normally use 'make qcheck' which skips the FS test and some
> other slow ones
> 
> Some notes from a little bit of digging: There are almost 1000 sandbox
> tests (17-20mins), but qemu_arm only runs 62 (2mins). With 'make
> qcheck' it runs about 2500 tests in about 4 minutes. I just got 'make
> pcheck' going again and that is a little faster (2.5 mins). Binman
> tests run in parallel if you use 'pip install concurrencytest'

Yes, I find it frustrating that I only tend to run ~50 tests on real
hardware each iteration and skip ~400. Of course, 250 of them are skips
because they only support sandbox. I guess I need to find time to dig in
to UNIT_TEST compiling on more platforms again.

-- 
Tom

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