8646 loader: replace EFI part devices.

Review Request #659 — Created Sept. 10, 2017 and submitted

tsoome
illumos-gate
8646
83d3cc5...
general
8646 loader: replace EFI part devices.


  • 1
  • 0
  • 11
  • 4
  • 16
Description From Last Updated
Is there a wide char strlen equivalent we can use? rm rm
tsoome
rm
  1. In general, this seems like a great cleanup. I have a few notes on this.

  2. The 'link of partitions' comment doesn't make a lot of sense to me.
    1. it should be "list of partitions".

  3. It looks like pd_unit often gets assigned from a couple of UEFI related values like PartitionNumber and the ACPI UID field which are both uint32_t values. Should this really be a signed value? It seems like we'd be truncating the value when we assign it on occasion.
  4. Should we make this unsigned since it should never be negative?
  5. Maybe we should use a boolean_t or whatever the loader equivalent type is?

  6. Should this value be unsigned since it always is coming from an unsigned value in DevicePathNodeLength?
  7. usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 2)
     
     
     
     
    Can we maybe get #define values for these IDs?
  8. Does the loader have calloc?
  9. Note this is one of the places that has the uint32_t to int issue I mentioned for the pdinfo_t structure.
  10. So, am I correct that the units for cd devices are things that we've made up in this case just based on the order that they've been added?
    1. Yes, as there is no clear distinction to CD devices, and by current knowledge they can be presented as MEDIA_CDROM_DP, MSG_ATAPI_DP, and something on USB; The list is built based on what I have found on testing and other sources, and the interpretation can depend on if there actually is media or not.

  11. How do we know this cast is safe? Are we guaranteed that this will be a hard drive device path somehow?
    1. Same as with FILEPATH_DEVICE_PATH below - the node type and subtype were checked before calling efipart_hdinfo_add().

  12. This is another place where signed/unsigned truncation could occur.
  13. Should we be checking the node type beore performing this cast to be certain?
    1. We actually did test before calling efipart_hdinfo_add_filepath(). The reason to have those checks before the call was to have viaual aid to show which kind of devices are checked. Initially only MEDIA_HARDDRIVE_DP paths were processed, but then it appeared that (some?) arm systems do use MEDIA_FILEPATH_DP devices for "hard disk" storage too, so the condition was explicitly set in efipart_updatehd(). And since there is just one point where the efipart_hdinfo_add_filepath() is called, duplicating the check does not seem to provide much benefit.

  14. usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 2)
     
     
     
    Is there a wide char strlen equivalent we can use?
    1. Not yet. There is upcoming patch for it, as it is needed elsewhere too (of course). So for now, I'll leave this comment up for an reminder, and we will fix it depending on the order of the patches landing:) In an sense, I would like to get loader.efi build activated before getting too many new updates.

  15. Does this behave like a normal libc strtol, if so you need to check errno by setting it to zero in advance to make sure we have a valid value.

  16. usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 2)
     
     
     
     
     
     
    Maybe we can use { and } in the else case here to make the intent more clear?
  17. Should this be ENXIO to represent the fact that the device is missing, but it could succeed had it been there?
  18. Does this real strategy always operate in terms of 512 byte blocks?
    1. Yes, all block IO from above is always assuming 512B blocks, so the strategy() calls have to deal with larger sectors. From read():

          if (f->f_flags & F_RAW) {
              twiddle(8);
              errno = (f->f_dev->dv_strategy)(f->f_devdata, F_READ,
                  btodb(f->f_offset), bcount, dest, &resid);
              if (errno)
                  return (-1);
              f->f_offset += resid;
              return (resid);
          } 
      

      and btodb() is just doing >> DEV_BSHIFT. So any larger media units must be handled in strategy calls.

  19. See earlier comments on strtol()
  20. 
      
tsoome
rm
  1. Thanks for following up on this and other pieces!

  2. 
      
tsoome
igork
  1. Ship It!
  2. 
      
danmcd
  1. 
      
  2. You do realize that if HID isn't any of the types you mention, you'll STILL return (acpi) because you assign acpi BEFORE you check?

    1. Right:) good catch. Of course it only means that there only is ACPI hid set with floppy (in most cases), but worth fixing anyhow.

  3. 
      
tsoome
danmcd
  1. Ship It!
  2. 
      
rm
  1. Ship It!
  2. 
      
igork
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...