| Message ID | 20240902011825.746421-13-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: Add a test for EFI bootmeth | expand |
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. What problem do you want to fix? I have not seen any issues in our CI. > > 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 <sjg@chromium.org> > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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. Best regards Heinrich
Hi Heinrich, On Thu, 12 Sept 2024 at 09:12, Heinrich Schuchardt <xypron.glpk@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. > > 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. > > > > > 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 <sjg@chromium.org> > > Fixes: d5391bf02b9 ("efi_loader: ensure all block devices are probed") > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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. Regards, Simon
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;