Page MenuHomeFreeBSD

loader.efi: efipart should exclude iPXE stub block protocol
ClosedPublic

Authored by tsoome on Nov 29 2017, 6:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 21, 11:08 AM
Unknown Object (File)
Wed, Dec 4, 12:46 PM
Unknown Object (File)
Sun, Dec 1, 7:35 PM
Unknown Object (File)
Sun, Dec 1, 7:01 PM
Unknown Object (File)
Nov 17 2024, 7:09 AM
Unknown Object (File)
Nov 9 2024, 8:31 PM
Unknown Object (File)
Nov 8 2024, 11:50 PM
Unknown Object (File)
Nov 8 2024, 9:52 AM
Subscribers

Details

Summary

iPXE does insert stub BLOCK IO protocol handler, it is unusable and the
access will result with error. We need to exclude it from the list of disks.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=223969

Test Plan

Tested with iPXE boot.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 13176
Build 13420: arc lint + arc unit

Event Timeline

So basicaly LGTM, however two things to think about...

#define might be nice. but other than that it looks cool to me.

BlockSize is defined in the standard as:

The intrinsic block size of the device. ... Returns the number of bytes per logical block....

This suggests to me the proper filter is < 512, and maybe even a power of 2 test.

This revision is now accepted and ready to land.Nov 29 2017, 6:26 PM
This revision now requires review to proceed.Nov 29 2017, 6:52 PM
stand/efi/libefi/efipart.c
269

Still think we should just replace this whole thing with:

if (blkio->Media->BlockSize != 512 || blkio->Media->BlockSize != 4096)

stand/efi/libefi/efipart.c
269

This is not as simple as that.

UEFI 2.7 does state about BlockSize:
The intrinsic block size of the device. If the media changes, then this field is updated.Returns the number of bytes per logical block. For ATA devices, this is reported in IDENTIFY DEVICE data words 117-118 (i.e., Words per Logical Sector) (see ATA8-ACS). For SCSI devices, this is reported in the READ CAPACITY (16) parameter data Logical Block Length In Bytes field (see SBC-3).

Now ATA IDENTIFY DEVICE:
7.18.7.61 Words 117..118: Logical sector size
Words 117..118 are a DWord field that indicates the size of device logical sectors in words. The value of logical sector size shall be equal to or greater than 256. The value of logical sector size shall be valid when bit 12 of word 106 is set to one. All logical sectors on a device shall be this length. When bit 12 of word 106 is cleared to zero words 117..118 shall be cleared to zero.

So from ATA, we have at least 256 bytes.

and SCSI READ CAPACITY:
The BLOCK LENGTH IN BYTES field contains the number of bytes of user data in the logical block indicated by the RETURNED LOGICAL BLOCK ADDRESS field. This value does not include protection information or additional information (e.g., ECC bytes) recorded on the medium.

For iPXE case I did confirm the stub (src/interface/efi/efi_file.c in ipxe source), they do implement FILE interface and to work around other issues, they indeed do insert the dummy block io into the stack, so this test is valid and does fix the issue.

I am still a bit hesitant about validating the BlockSize for other values, and mostly because it may help to identify some weird issues if those should arise.

stand/efi/libefi/efipart.c
269

Does our code work with 256 sector size? No. I doubt it works for < 512 for various reasons (not least that we read a sector, then access data > 256 unconditionally). Just because an ATA might possibly be a weird number doesn't mean we should accept it here. We can't use it anyway, and I doubt there's been more than a half a dozen drives made with 256 byte sectors, all in the early days of ATA.

I'm more inclined to filter all bogus values. If we know the code can't possibly work with odd-ball sector sizes, we should fail early and tell people they lose. If it's important, they'll let us know. It's been my experience in the past that the harder I tried to support oddball configurations, especially ones I could only lightly test, the result was rarely that the oddballs just worked and more often that the attempt interfered with normal device support.

For better or worse, 512, 4k and soon 64k are the only sector sizes that we'll see in the wild.

Update to check BlockSize

stand/efi/libefi/efipart.c
277

I don't think this is a power of 2 test, but rather just an even / odd test.

#define ispow2(x) ((x) != 0 && !((x) & ((x) - 1)))

is the canonical one-liner...

stand/efi/libefi/efipart.c
277

#define powerof2(x) ((((x)-1)&(x))==0)

can be found in sys/param.h and could be used. It falsely identifies 0 as a power of 2, but that's OK for this purpose.

tsoome added inline comments.
stand/efi/libefi/efipart.c
277

indeed, I obviously was way too sleepy already:D

tsoome marked an inline comment as done.

use proper powerof2

This revision is now accepted and ready to land.Dec 1 2017, 6:28 AM
This revision was automatically updated to reflect the committed changes.