On Sun, Jul 30, 2017 at 11:01:58PM +0200, Mark Kettenis wrote:
> > Date: Sun, 30 Jul 2017 12:20:27 +1000
> > From: Jonathan Gray <[email protected]>
> >
> > On Sat, Jul 29, 2017 at 09:01:35PM +0200, Patrick Wildt wrote:
> > > On Sat, Jul 29, 2017 at 02:59:19PM +0200, Mark Kettenis wrote:
> > > > This is apparently very hard. Caught this on arm64 where
> > > > efi_device_path_depth() returned 0, which resulted in always selecting
> > > > the first device. Clearly if the first path component (i = 0) matches
> > > > the desired type, we should return 1, not 0. Here is the amd64
> > > > version of the diff which is easier to test for people.
> > > >
> > > > ok?
> > >
> > > God damn, this feels like the tenth time this thing has to be fixed up.
> > > Fix looks good to me, please commit that on arm64 and armv7 as well.
> > >
> > > ok patrick@
> >
> > The changes to arm64 broke boot on the overdrive 1000.
>
> Damn, I thought I tested on that machine. But apparently I fucked up...
>
> Looking at this a bit closer, what I preceived as an off-by-one is
> actually deliberate. We try to get the depth up to, but not including
> the MEDIA_DEVICE_PATH node.
>
> The problem at hand is that the simplified EFI implementation in
> U-Boot represents devices by a single MEDIA_DEVICE_PATH node. In that
> context I think it makes sense to to treat the case where
> efi_device_path_depth() returns zero specially.
>
> The diff below works on both the Overdrive 1000 and the Raspberry Pi.
>
> Note that U-Boot also supports x86, so I propose we change amd64 and
> armv7 in a similar way.
>
> ok?
This diff works on overdrive 1000 here and is included in the arm64
snapshot that went up today.
There are a lot of efi_loader changes going on in u-boot at the
moment, I wonder if any correct the path behaviour.
>
> Index: arch/arm64/stand/efiboot/conf.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/stand/efiboot/conf.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 conf.c
> --- arch/arm64/stand/efiboot/conf.c 29 Jul 2017 19:51:50 -0000 1.5
> +++ arch/arm64/stand/efiboot/conf.c 30 Jul 2017 20:40:15 -0000
> @@ -35,7 +35,7 @@
> #include "efiboot.h"
> #include "efidev.h"
>
> -const char version[] = "0.5";
> +const char version[] = "0.6";
> int debug = 0;
>
> struct fs_ops file_system[] = {
> Index: arch/arm64/stand/efiboot/efiboot.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/stand/efiboot/efiboot.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 efiboot.c
> --- arch/arm64/stand/efiboot/efiboot.c 29 Jul 2017 19:51:50 -0000
> 1.9
> +++ arch/arm64/stand/efiboot/efiboot.c 30 Jul 2017 20:40:15 -0000
> @@ -192,6 +192,15 @@ efi_diskprobe(void)
> if (efi_bootdp != NULL)
> depth = efi_device_path_depth(efi_bootdp, MEDIA_DEVICE_PATH);
>
> + /*
> + * U-Boot incorrectly represents devices with a single
> + * MEDIA_DEVICE_PATH component. In that case include that
> + * component into the matching, otherwise we'll blindly select
> + * the first device.
> + */
> + if (depth == 0)
> + depth = 1;
> +
> for (i = 0; i < sz / sizeof(EFI_HANDLE); i++) {
> status = EFI_CALL(BS->HandleProtocol, handles[i], &blkio_guid,
> (void **)&blkio);
> @@ -217,6 +226,10 @@ efi_diskprobe(void)
> free(handles, sz);
> }
>
> +/*
> + * Determine the number of nodes up to, but not including, the first
> + * node of the specified type.
> + */
> static int
> efi_device_path_depth(EFI_DEVICE_PATH *dp, int dptype)
> {
> @@ -224,7 +237,7 @@ efi_device_path_depth(EFI_DEVICE_PATH *d
>
> for (i = 0; !IsDevicePathEnd(dp); dp = NextDevicePathNode(dp), i++) {
> if (DevicePathType(dp) == dptype)
> - return (i + 1);
> + return (i);
> }
>
> return (-1);
>
