[BUG] issues with new bootflow, uefi and virtio

[Simon Glass]

Hi Mathew,

On Thu, 6 Apr 2023 at 12:25, Mathew McBride <matt at traverse.com.au> wrote:
>
> Hi Vincent,
>
> On Thu, Apr 6, 2023, at 1:04 AM, Vincent Stehlé wrote:
> > Hi,
> >
> > I am hitting an issue with the new bootflow when booting with UEFI from a
> > virtio device on Qemu Arm.
> >
> > It seems the device number computation in efiload_read_file() does not work
> > in the general virtio case, where it will pick the virtio device number
> > instead of the block device index. On Qemu arm virt machine, many virtio
> > mmio devices are provisioned in the memory map and no assumption can be
> > made on the number of the actual virtio device in use in the general case.
> >
> > This is an extract of the output of `dm tree' on this platform, focused on
> > the virtio device from which I would like to boot:
> >
> >   virtio    31  [ + ]   virtio-mmio     |-- virtio_mmio at a003e00
> >   blk        0  [ + ]   virtio-blk      |   |-- virtio-blk#31
> >   partition  0  [ + ]   blk_partition   |   |   |-- virtio-blk#31:1
> >   partition  1  [ + ]   blk_partition   |   |   `-- virtio-blk#31:2
> >   bootdev    2  [ + ]   virtio_bootdev  |   `-- virtio-blk#31.bootdev
> >
> > In this extract the actual virtio device number is 31, as will be picked by
> > efiload_read_file(), but the desired block device index is zero, as would
> > be used with e.g. `ls virtio 0'.
>
> I came across the exact same issue a few days ago. Below is a patch which I believe fixes the problem, by using the devnum of blk uclass (virtio 0) instead of the sequence number of the parent udevice (e.g virtio-blk#35).
>
> Separately, the devnum was previously being parsed as a hex which meant "virtio_blk#35" was actually being parsed as "virtio_blk#23". That confused me for a while.
>
> If the patch looks good I can re-post it directly to the ML. I'm not 100% sure that I got it right.
>
> In case the email mangles the patch, you can grab a diff here as well: https://gitlab.com/traversetech/ls1088firmware/u-boot/-/commit/5ed3315b4a297f143fb84f44117b5b31e5617af5
>
> - Matt
>
> ------------
>
> From 5ed3315b4a297f143fb84f44117b5b31e5617af5 Mon Sep 17 00:00:00 2001
> From: Mathew McBride <matt at traverse.com.au>
> Date: Wed, 5 Apr 2023 02:44:48 +0000
> Subject: [PATCH] bootstd: use blk uclass device numbers to set efi bootdev
>
> When loading a file from a block device, efiload_read_file
> was using the seq_num of the device (e.g "35" of virtio_blk#35)
> instead of the block device id (e.g what you get from running
> the corresponding device scan command, like "virtio 0")
>
> This cause EFI booting from these devices to fail as an
> invalid device number is passed to blk_get_device_part_str:
>
> Scanning bootdev 'virtio-blk#35.bootdev':
> distro_efi_read_bootflow_file start (efi,fname=<NULL>)
> distro_efi_read_bootflow_file start (efi,fname=<NULL>)
> setting bootdev virtio, 35, efi/boot/bootaa64.efi, 00000000beef9a40, 170800
> efi_dp_from_name calling blk_get_device_part_str
> dev=virtio devnr=35 path=efi/boot/bootaa64.efi
> blk_get_device_part_str (virtio,35)
> blk_get_device_by_str (virtio, 35)
> ** Bad device specification virtio 35 **
> Using default device tree: dtb/qemu-arm.dtb
> No device tree available
> 0  efi          ready   virtio       1  virtio-blk#35.bootdev.par efi/boot/bootaa64.efi
> ** Booting bootflow 'virtio-blk#35.bootdev.part_1' with efi
> blk_get_device_part_str (virtio,0:1)
> blk_get_device_by_str (virtio, 0)
> No UEFI binary known at beef9a40 (image buf=00000000beef9a40,addr=0000000000000000)
> Boot failed (err=-22)
>
> Signed-off-by: Mathew McBride <matt at traverse.com.au>
> ---
>  boot/bootmeth_efi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index 6a97ac02ff..bc106aa736 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -117,7 +117,7 @@ static int efiload_read_file(struct blk_desc *desc, struct bootflow *bflow)
>          * this can go away.
>          */
>         media_dev = dev_get_parent(bflow->dev);
> -       snprintf(devnum_str, sizeof(devnum_str), "%x", dev_seq(media_dev));
> +       snprintf(devnum_str, sizeof(devnum_str), "%d", desc->devnum);

Yes this looks right to me.  I am actually changing the same line in a
current series, so I can pick up this patch and include it in the
series.

>
>         strlcpy(dirname, bflow->fname, sizeof(dirname));
>         last_slash = strrchr(dirname, '/');
> --
> 2.30.1
>
>
>

Regards,
Simon