> Date: Mon, 31 Jul 2017 20:35:45 +1000
> From: Jonathan Gray <[email protected]>
>
> 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.
Possibly. Still we want older U-Boot versions to work as well.
> > 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);
> >
>
