On Fri, Sep 20, 2024 at 09:25:29AM +0200, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 19 Sept 2024 at 19:45, Tom Rini <tr...@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 <tr...@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.g...@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 <s...@chromium.org>
> > > > > > > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are
> > > > > > > probed")
> > > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@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.Then please disable it until you can complete it. > 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: To be clear, I'm not saying sandbox tests have no value, or are unimportant. I apologize for imply as much. > - with sandbox we can test the operation of the bootmeth, including > under failure conditions Yes, state machine tests are useful. But we can test for xFAIL on other platforms, yes? > - 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 I honestly don't recall the state of the discussion around that patch, positive/negative/neutral. > - we can build on this test to cover other bootmeths without needing > to install a full OS just to run a test Counter point, we can't test that an OS actually boots. One of the most valuable personally tests we've added recently is test/py/tests/test_net_boot.py which makes the network load and boot an OS image, and test for (some) failure modes. -- Tom
signature.asc
Description: PGP signature
