8796 loader.efi: efipart does not recognize partitionless disks

Review Request #731 — Created Nov. 10, 2017 and submitted

tsoome
illumos-gate
8796
730
cf7d1e6...
general
8796 loader.efi: efipart does not recognize partitionless disks

Tested in vmware fusion with floppy, cd, ata, sas, sata and nvme block devices.

  • 0
  • 0
  • 1
  • 5
  • 6
Description From Last Updated
rm
  1. 
      
    1. I did add the comment about the idea behind the efipart_hdd(). The root problem here is that we can get the list of handles for block IO devices and we have no good way to distinguish the devices. The handle can be for device itself or for partition entry. We only have distinction for removable devices and few "well known" cases.

  2. Maye make these unsigned since they can't be negative?
  3. usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 1)
     
     
     
     
    If we can't find a device path that's in the list should we really be returning false or should we instead be continuing to the next entry?
    1. Here we have list of handles provide by EFI API and we try to use EFI API to get device path corresponding to the handle. If this call is failing, we have something bad going on, and I think, we really should fail as early as possible. This case is a bit tricky in sense that normally we shall not get error here at all, but it may be possible our handle list is corrupted - but then we will also fail to get block io handle and will error out anyhow.

  4. Should we be returning false or should we continue and try other entries? Or is the theory that because we matched this device prefix from efipart_handles[i], there's no way we could match something else there? If that's the case, a comment indicating why we're doing it at this point and that it applies for the rest would be good.
    1. We need node for type/subtype test and if the node is not present, we do not have disk anyhow. I did move the type/subtype test up here and did add more comments.

  5. How should we handle the other values of DevicePathType such as:
    
    * HARDWARE_DEVICE_PATH
    * ACPI_DEVICE_PATH
    * MESSAGING_DEVICE_PATH
    1. I think the block device guid will filter out the hardware nodes (I have not seen those while testing). ACPI device nodes are filtered out too, except for floppy and we do test for floppy. So only ones we do get is MESSAGING_DEVICE_PATH, and those are for disk device itself (because of the BLOCK device guid).

  6. Is it only possible for Floppy/CD drives to have RemovableMedia set?
    1. Fortunately it seems the floppy case is covered by ACPI_DEVICE_PATH, so the question mostly is if the device is cd or something else. The answer is that we do not know for sure and can only assume for now that if the device is removable and has no media, it most likely is cd/dvd. If there is something else, we will just have to update the code. Note that grub2 is using sector size check there, but today with 4kn devices, the sector size check can not be trusted.

  7. usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 1)
     
     
     
    If we pass this last check should we actually be checking the next handle? My understanding is that the handles could exist for far more than an HDD in the list. So shouldn't we return true if we get here rather than move onto the next entry in the list?
    1. Yes, if we stop here, we will get misdetected "disk" device from cd media - tested with OI iso. So we want to walk over all handles to be sure we will not get false positive.

  8. If we have no matching handles why do we assume it's an HDD path?
    1. We can not match HDD - we can only assume, if it is not floppy nor cd/dvd, it is hdd (or "disk" device).

  9. 
      
tsoome
rm
  1. Thanks for clarifying how this functions. This really helps. I have one more comment suggest to consider.

  2. usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 2)
     
     
     
    Maybe we can add a comment here that says:
    
    "Test every EFI handle to attempt to rule this path out as another kind of handle." I dunno, but that might make it clearer.
    1. Did add a bit different wording:)

  3. 
      
tsoome
tsoome
Review request changed

Status: Closed (submitted)

Loading...