Page MenuHomeFreeBSD

PR216964: boot1.efi: can't boot from ZFS on 4kn HDD
ClosedPublic

Authored by tsoome on Mar 2 2017, 7:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 9 2024, 6:50 AM
Unknown Object (File)
Dec 31 2023, 8:19 PM
Unknown Object (File)
Dec 27 2023, 8:06 AM
Unknown Object (File)
Dec 22 2023, 5:52 PM
Unknown Object (File)
Dec 22 2023, 5:51 PM
Unknown Object (File)
Dec 20 2023, 12:41 AM
Unknown Object (File)
Dec 2 2023, 2:24 PM
Unknown Object (File)
Nov 30 2023, 12:09 AM

Details

Summary

The boot1.efi immediate issue from PR216964 is that we are reading into
too small buffer, from UEFI spec 2.6:

The size of the Buffer in bytes. This must be a multiple of the intrinsic block size of the device.

The secondary issue is that LBA calculation does not check reminder from
division.

Note, we quite likely will get into the same trouble with loader.efi,
however, to keep the change simple, we will just try to fix boot1.efi first.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tsoome retitled this revision from to PR216964: boot1.efi: can't boot from ZFS on 4kn HDD.
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added reviewers: allanjude, imp.
skozlov added a reviewer: skozlov.

Tested on 2 machines with 4Kn and 4Ke drives:

  • Build the boot1.efifat from the patched HEAD
  • cat the boot1.efifat onto /dev/gpt/efiboot0
  • reboot

Both machines were able to boot into vanilla FreeBSD-11.0-RELEASE-p1, multiuser mode.

This revision is now accepted and ready to land.Mar 3 2017, 2:47 PM

It seems to me the two ReadBlocks() calls could be consolidated, which would allow you to also include the buffer size in the DPRINTF(). Something like this:

size_t size, remainder, rb_size;
char *bouncebuf == NULL, rb_buf = NULL;
...
        if (remainder != 0 || size != bytes) {
                bouncebuf = malloc(size);
                if (bouncebuf == NULL) {
                        printf("vdev_read: out of memory\n");
                        return (-1);
                }
                rb_buf = bouncebuf;
                rb_size = size;
        } else {
                rb_buf = buf;
                rb_size = bytes;
        }
        status = devinfo->dev->ReadBlocks(devinfo->dev,
            devinfo->dev->Media->MediaId, lba, rb_size, rb_buf);
        if (EFI_ERROR(status)) {
                goto error;
        }
        if (rb_size == size) {
                memcpy(buf, bouncebuf+remainder, bytes);
        }

        free(bouncebuf);
        return (0);

error:
        DPRINTF("vdev_read: failed dev: %p, id: %u, lba: %jd, size: %zu,"
            " rb_size: %zu, status: %lu\n", devinfo->dev,
            devinfo->dev->Media->MediaId, (intmax_t)lba, bytes,
            rb_size, EFI_ERROR_CODE(status));
        free(bouncebuf);
        return (-1);
sys/boot/efi/boot1/zfs_module.c
48 ↗(On Diff #25912)

I think you mean "remainder", not "reminder".

tsoome edited edge metadata.

Update suggested by rpokala.

+ lba is really uint64_t, accordingly adjust DPRINTF.

This revision now requires review to proceed.Mar 3 2017, 6:04 PM
tsoome edited edge metadata.

typo fixed.

Rewrote to use single sector sized bounce buffer. Done for safety reasons,
so we wont attempt to allocate some large buffers due to chaining errors.

Please let me know if you need another round of testing done to commit this one

In D9870#206870, @kozlov.sergey.404_gmail.com wrote:

Please let me know if you need another round of testing done to commit this one

If you do not mind re-testing the updated patch - it seems to be OK and it is using the same logic as loader.efi has in efipart.c, but it would be good to get the ack from actual boot as well:)

This revision is now accepted and ready to land.Mar 15 2017, 6:07 PM
This revision was automatically updated to reflect the committed changes.