On Tue, Feb 28, 2017 at 03:16:58PM +0100, Patrick Wildt wrote:
> On Tue, Feb 28, 2017 at 10:41:21PM +0900, YASUOKA Masahiko wrote:
> > 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.
>
> 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.
I just now realized that CDROM support is actually already implemented
with a commit you did some time ago, I had an older checkout. Still
interesting to see you did this by hijacking the disklabel code. My
other point still stands.
>
> >
> > 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.
>
> Patrick
>
> > +
> > /***********************************************************************
> > * Memory
> > ***********************************************************************/
> >
