svn commit: r322941 - head/sys/boot/efi/boot1

[Warner Losh]

On Sat, Sep 2, 2017 at 11:25 AM, Warner Losh <imp at bsdimp.com> wrote:

>
>
> On Sat, Sep 2, 2017 at 1:43 AM, Tomoaki AOKI <junchoon at dec.sakura.ne.jp>
> wrote:
>
>> Hi.
>>
>> This broke boot drive selection functionality by smh@ at r295320 on
>> Feb.5, 2016. [1]
>> Now, even if I forcibly select 2nd HDD via UEFI firmware, boot1.efi
>> (as bootx64.efi) selects /boot/loader.efi on 1st HDD. Each boot
>> partition are ZFS pools. Confirmed previous rev was OK.
>>
>> Attached is the boot log (with EFI_DEBUG). Pool zsysS02 should be
>> selected there instead of zsysS01. As I have no serial console, there
>> can be some typos. (Took video and hand-typed.)
>>
>> The boot order should be as below (what smh@ implemented).
>>
>>  1. ZFS pool on which drive boot1.efi is read from.
>>  2. UFS partition on which drive boot1.efi is read from.
>>  3. If both 1 and 2 are missed, try from UEFI 1st drive and later.
>>
>> P.S. Another possibility (not tested yet):
>>      Not forcibly prefer 1st drive, but reversed selection.
>>      (If boot1.efi is on 1st drive, loader.efi on 2nd drive is read.)
>>
>> [1]
>> https://lists.freebsd.org/pipermail/svn-src-head/2016-Februa
>> ry/082215.html
>
>
> Looks like the matching function that I replaced this stuff with wasn't
> quite the same. Will fix. I hadn't thought I'd broken it, honestly, since
> the setup I have still worked. The intent was to keep things as they were,
> which clearly didn't happen in at least your case. I'll take a look at the
> logs to see if I can spot the differences between the two setups. The
> intent was to keep functionality, for now, the same.
>
> In the long term, though, this guessing and matching is going to end up
> first as deprecated and then as removed. To be replaced by a boot1.efi that
> follows the EFI Boot Manager protocol where exactly what to load is
> contained in EFI env variables. That's what all my changes to boot1 have
> been working towards.
>
> Warner
>

P.S.  This is the doc I put together for discussion. I'm keeping it updated
as each stage is implemented.

https://docs.google.com/document/d/1aK9IqF-60JPEbUeSAUAkYjF2W_8EnmczFs6RqCT90Jg/edit

Lemme know if you have any comments.

Warner

> > Author: imp
>> > Date: Sun Aug 27 03:10:16 2017
>> > New Revision: 322941
>> > URL: https://svnweb.freebsd.org/changeset/base/322941
>> >
>> > Log:
>> >   Eliminate redunant device path matching.
>> >
>> >   Use efi_devpath_match instead of device_paths_match. They are
>> >   functionally the same. Remove device_paths_match from boot1.c and call
>> >   efi_devpath_match instead.
>> >
>> >   Sponsored by: Netflix
>> >
>> > Modified:
>> >   head/sys/boot/efi/boot1/boot1.c
>> >
>> > Modified: head/sys/boot/efi/boot1/boot1.c
>> > ============================================================
>> ==================
>> > --- head/sys/boot/efi/boot1/boot1.c   Sat Aug 26 23:13:18 2017
>> (r322940)
>> > +++ head/sys/boot/efi/boot1/boot1.c   Sun Aug 27 03:10:16 2017
>> (r322941)
>> > @@ -76,53 +76,6 @@ Free(void *buf, const char *file __unused, int line
>> __ }
>> >
>> >  /*
>> > - * nodes_match returns TRUE if the imgpath isn't NULL and the nodes
>> match,
>> > - * FALSE otherwise.
>> > - */
>> > -static BOOLEAN
>> > -nodes_match(EFI_DEVICE_PATH *imgpath, EFI_DEVICE_PATH *devpath)
>> > -{
>> > -     size_t len;
>> > -
>> > -     if (imgpath == NULL || imgpath->Type != devpath->Type ||
>> > -         imgpath->SubType != devpath->SubType)
>> > -             return (FALSE);
>> > -
>> > -     len = DevicePathNodeLength(imgpath);
>> > -     if (len != DevicePathNodeLength(devpath))
>> > -             return (FALSE);
>> > -
>> > -     return (memcmp(imgpath, devpath, (size_t)len) == 0);
>> > -}
>> > -
>> > -/*
>> > - * device_paths_match returns TRUE if the imgpath isn't NULL and all
>> nodes
>> > - * in imgpath and devpath match up to their respective occurrences of a
>> > - * media node, FALSE otherwise.
>> > - */
>> > -static BOOLEAN
>> > -device_paths_match(EFI_DEVICE_PATH *imgpath, EFI_DEVICE_PATH *devpath)
>> > -{
>> > -
>> > -     if (imgpath == NULL)
>> > -             return (FALSE);
>> > -
>> > -     while (!IsDevicePathEnd(imgpath) && !IsDevicePathEnd(devpath))
>> {
>> > -             if (IsDevicePathType(imgpath, MEDIA_DEVICE_PATH) &&
>> > -                 IsDevicePathType(devpath, MEDIA_DEVICE_PATH))
>> > -                     return (TRUE);
>> > -
>> > -             if (!nodes_match(imgpath, devpath))
>> > -                     return (FALSE);
>> > -
>> > -             imgpath = NextDevicePathNode(imgpath);
>> > -             devpath = NextDevicePathNode(devpath);
>> > -     }
>> > -
>> > -     return (FALSE);
>> > -}
>> > -
>> > -/*
>> >   * devpath_last returns the last non-path end node in devpath.
>> >   */
>> >  static EFI_DEVICE_PATH *
>> > @@ -318,7 +271,7 @@ probe_handle(EFI_HANDLE h, EFI_DEVICE_PATH
>> *imgpath, B
>> >  if (!blkio->Media->LogicalPartition)
>> >               return (EFI_UNSUPPORTED);
>> >
>> > -     *preferred = device_paths_match(imgpath, devpath);
>> > +     *preferred = efi_devpath_match(imgpath, devpath);
>> >
>> >       /* Run through each module, see if it can load this partition
>> */
>> >  for (i = 0; i < NUM_BOOT_MODULES; i++) {
>>
>> --
>> Tomoaki AOKI    <junchoon at dec.sakura.ne.jp>
>>
>
>