> 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?
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);
