Hi,
On Mon, 27 Feb 2017 11:10:31 +0100
Patrick Wildt <[email protected]> wrote:
> I'm surprised this didn't come up earlier, but I think the for-loop is a
> bit wrong. What the code is supposed to be doing is going over each
> device path node for the loaded image, which is supposed to be the path
> to the device that efiboot was loaded from, and check for a node that
> is a medium (as in hard drive, cdrom) and look for a hard drive.
Yes, you are right it's wrong.
> As it turns out, the code would actually skip all device paths that
> are mediums and instead match on all non-media types. It's surprising
> to me that this seems to work for people.
>
> On Qemu and VMware the first node in the device path is ACPI (2) with
> subtype (1). Interestingly, the hard drive subtype in the media type
> has the subtype (1) as well. So that might be a reason it did work.
As far as my test on QEMU and VAIO, the first node is ACPI and subtype
is 1.
But we had another bug,
87 if (status == EFI_SUCCESS) {
88 for (dp = dp0; !IsDevicePathEnd(dp);
89 dp = NextDevicePathNode(dp)) {
90 if (DevicePathType(dp) == MEDIA_DEVICE_PATH)
91 continue;
92 if (DevicePathSubType(dp) ==
MEDIA_HARDDRIVE_DP) {
93 bios_bootdev = 0x80;
94 efi_bootdp = dp;
95 break;
96 }
97 break;
98 }
99 }
if we fix the bug at #90, at #94 efi_bootdp will not become the first
dp node(dp0). Then efi_diskprobe() will fail to reorder hd? properly
since the function assumes efi_bootdp is the first dp node.
So I'd like to commit the diff below.
diff --git a/sys/arch/amd64/stand/efiboot/efiboot.c
b/sys/arch/amd64/stand/efiboot/efiboot.c
index 1903862d271..f82ea34f66e 100644
--- a/sys/arch/amd64/stand/efiboot/efiboot.c
+++ b/sys/arch/amd64/stand/efiboot/efiboot.c
@@ -41,7 +41,8 @@
EFI_SYSTEM_TABLE *ST;
EFI_BOOT_SERVICES *BS;
EFI_RUNTIME_SERVICES *RS;
-EFI_HANDLE IH, efi_bootdp = NULL;
+EFI_HANDLE IH;
+EFI_DEVICE_PATH *efi_bootdp = NULL;
EFI_PHYSICAL_ADDRESS heap;
EFI_LOADED_IMAGE *loadedImage;
UINTN heapsiz = 1 * 1024 * 1024;
@@ -51,6 +52,7 @@ static EFI_GUID blkio_guid = BLOCK_IO_PROTOCOL;
static EFI_GUID devp_guid = DEVICE_PATH_PROTOCOL;
u_long efi_loadaddr;
+static int efi_device_path_cmp(EFI_DEVICE_PATH *, EFI_DEVICE_PATH *, int);
static void efi_heap_init(void);
static void efi_memprobe_internal(void);
static void efi_video_init(void);
@@ -87,14 +89,12 @@ efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *systab)
if (status == EFI_SUCCESS) {
for (dp = dp0; !IsDevicePathEnd(dp);
dp = NextDevicePathNode(dp)) {
- if (DevicePathType(dp) == MEDIA_DEVICE_PATH)
- continue;
- if (DevicePathSubType(dp) == MEDIA_HARDDRIVE_DP) {
+ if (DevicePathType(dp) == MEDIA_DEVICE_PATH &&
+ DevicePathSubType(dp) == MEDIA_HARDDRIVE_DP) {
bios_bootdev = 0x80;
- efi_bootdp = dp;
+ efi_bootdp = dp0;
break;
}
- break;
}
}
@@ -166,7 +166,7 @@ efi_diskprobe(void)
EFI_BLOCK_IO *blkio;
EFI_BLOCK_IO_MEDIA *media;
struct diskinfo *di;
- EFI_DEVICE_PATH *dp, *bp;
+ EFI_DEVICE_PATH *dp;
TAILQ_INIT(&efi_disklist);
@@ -193,23 +193,14 @@ efi_diskprobe(void)
di = alloc(sizeof(struct diskinfo));
efid_init(di, blkio);
- if (efi_bootdp == NULL)
- goto next;
- status = EFI_CALL(BS->HandleProtocol, handles[i], &devp_guid,
- (void **)&dp);
- if (EFI_ERROR(status))
- goto next;
- bp = efi_bootdp;
- while (1) {
- if (IsDevicePathEnd(dp)) {
+ if (efi_bootdp != NULL) {
+ status = EFI_CALL(BS->HandleProtocol, handles[i],
&devp_guid,
+ (void **)&dp);
+ if (EFI_ERROR(status))
+ goto next;
+ if (efi_device_path_cmp(efi_bootdp, dp,
MESSAGING_DEVICE_PATH)
+ == 0)
bootdev = 1;
- break;
- }
- if (memcmp(dp, bp, sizeof(EFI_DEVICE_PATH)) != 0 ||
- memcmp(dp, bp, DevicePathNodeLength(dp)) != 0)
- break;
- dp = NextDevicePathNode(dp);
- bp = NextDevicePathNode(bp);
}
next:
if (bootdev)
@@ -221,6 +212,35 @@ next:
free(handles, sz);
}
+static int
+efi_device_path_cmp(EFI_DEVICE_PATH *dpa, EFI_DEVICE_PATH *dpb, int dptype)
+{
+ int cmp;
+ EFI_DEVICE_PATH *dp, *dpt_a = NULL, *dpt_b = NULL;
+
+ for (dp = dpa; !IsDevicePathEnd(dp); dp = NextDevicePathNode(dp)) {
+ if (DevicePathType(dp) == dptype) {
+ dpt_a = dp;
+ break;
+ }
+ }
+ for (dp = dpb; !IsDevicePathEnd(dp); dp = NextDevicePathNode(dp)) {
+ if (DevicePathType(dp) == dptype) {
+ dpt_b = dp;
+ break;
+ }
+ }
+
+ if (dpt_a && dpt_b) {
+ cmp = DevicePathNodeLength(dpt_a) - DevicePathNodeLength(dpt_b);
+ if (cmp)
+ return (cmp);
+ return (memcmp(dpt_a, dpt_b, DevicePathNodeLength(dpt_a)));
+ }
+
+ return ((uintptr_t)dpt_a - (uintptr_t)dpt_b);
+}
+
/***********************************************************************
* Memory
***********************************************************************/
