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

[Tom Rini <trini@konsulko.com>]

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

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?

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.

-- 
Tom

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