> Also that makes me wonder, if the device path is the one to the booted > partition, wouldn't blkio point to the booted partition instead of the > full device as well?
Yes, there are a blkio which point the same device path of the booted partion's device path. But our boot loader does I/O against disks (not partitions), so we need to find the device path of the disk. --yasuoka On Tue, 28 Feb 2017 16:48:19 +0100 Patrick Wildt <[email protected]> wrote: > On Tue, Feb 28, 2017 at 04:33:14PM +0100, Patrick Wildt wrote: >> On Wed, Mar 01, 2017 at 12:17:02AM +0900, YASUOKA Masahiko wrote: >> > Hi, >> > >> > On Tue, 28 Feb 2017 15:16:58 +0100 >> > Patrick Wildt <[email protected]> wrote: >> > > On Tue, Feb 28, 2017 at 10:41:21PM +0900, YASUOKA Masahiko wrote: >> > >> 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. >> > > >> > > First of all, I've been having a look at supporting CDROMs in efiboot >> > > and saw that FreeBSD has a nice "devpath.c" file which implements a few >> > > good helpers. I've been using efi_devpath_match(), efi_devpath_trim() >> > > and efi_devpath_last_node(). Those have been particularly helpful and >> > > simplify some code. I wonder if it would make sense to pull that one >> > > in instead of writing another completely new implementation of tha same >> > > thing. >> > > >> > >> 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; >> > > >> > > This part makes sense to me. >> > > >> > >> 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) >> > > >> > > This part makes sense to me as well. >> > > >> > >> 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); >> > >> +} >> > > >> > > I don't like this. Have a look at the FreeBSD implementation, it's >> > > nicer sind you don't have to pass some device path type but instead >> > > it does a full path comparison. >> > >> > How will you fix by efi_devpath_match()? >> > >> > AFAIK, we need to find the device path of the booted disk. (not the >> > booted parition) So I belive "full path comparison" is not usable. >> > >> > --yasuoka >> > >> >> I see the problem. The way people do it with hard drives and cdroms is >> to trim/cut away the last node of the device path, because then you get >> the device path to the entire device instead of just a subnode. Then >> you can compare the pathes. >> > > Also that makes me wonder, if the device path is the one to the booted > partition, wouldn't blkio point to the booted partition instead of the > full device as well?
