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

[tsoome]

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.

[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.

[rpokala]

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

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

[tsoome]

Update suggested by rpokala.

+ lba is really uint64_t, accordingly adjust DPRINTF.

[tsoome]

typo fixed.

[tsoome]

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.

[skozlov]

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

[tsoome]

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:)

[allanjude]

Approved By: allanjude