8422 uts: basic UEFI support for illumos

Review Request #598 — Created June 23, 2017 and submitted

tsoome
illumos-gate
8422
1e11ea8...
general

8422 uts: basic UEFI support for illumos

Add basic UEFI support - data structures and MB2 info tag processing. Also enable loader efi branch build.



  • 0
  • 0
  • 22
  • 10
  • 32
Description From Last Updated
jbk
  1. 
      
  2. NIT: if one part of an if statement is using braces, the other part should probably as well (i.e. add them around the else statement) -- it also matches what some earlier code does.

  3. This doesn't appear to match the format that [GU]UIDs are normally displayed. Is this something peculiar to EFI? Also, shouldn't the 16 and 8 bit portions use %hx / %hhx?

    1. Yep, the reason actually is simple - the dboot_printf() does not really know too much about formatting. Of course it is possible to teach it a bit, but Im not sure if it is worth it for this debug output. Perhaps we even should just drop the printout here and add mdb command to output the table data - but that would be the task for separate update.

  4. Similar question as above

  5. 
      
rm
  1. 
      
  2. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    Sometimes we use 0x0 and sometimes we use 0x00, would you mind making it consistent?
    1. done. Those GUID's are ones I have seen in different systems; I am not sure at all if or how many of those should belong here, but then again, we can always split them later.

  3. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     

    I don't see this defined in the UEFI spec. Can you tell me where it comes from?

    1. https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Include/Guid/Fdt.h

  4. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    I don't see this defined in the UEFI spec. Can you tell me where it comes from?
    1. http://wiki.phoenix.com/wiki/index.php/EFI_DXE_SERVICES_TABLE_GUID

  5. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     

    I don't see this defined in the UEFI spec. Can you tell me where it comes from?

    1. http://wiki.phoenix.com/wiki/index.php/EFI_HOB_LIST_GUID

  6. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     

    I don't see this defined in the UEFI spec. Can you tell me where it comes from?

    1. https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Include/Guid/MemoryTypeInformation.h

  7. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    Do we normally use the __attribute__ syntax for this or do we normally use a pragma? Should there be a macro in sys/ccompile.h for these?
  8. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    Are these guaranteed to always be a 64-bit value on both UEFI64 and UEFI32 systems?
    1. Yes. The definition is at EFI_BOOT_SERVICES.AllocatePages() description, UEFI 2.7 page 185.

  9. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    Are these values supposed to map directly to the standard's values? Can we comment what section these values come from? From my copy it's UEFI 2.6, Section 6.2 Memory Allocation Services, Table 25 and Table 26.
  10. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     

    So, with all of these structures here should they actually be explicitly packed and then we manually make sure of the alignment required? Otherwise, it seems like these have the same problems that the loader side had.

  11. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     

    On a similar note, I guess there'll be tension between matching the spec and using structure prefixes and standard styling. I'm not sure which is better in this case.

  12. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     

    On a similar note, I guess there'll be tension between matching the spec and using structure prefixes and standard styling. I'm not sure which is better in this case.

    1. Yep. Right now, we are only reading the ACPI/SMBIOS table pointers from system tab, and it all is internal, so we have some time to make the mind there. The current approach I did use is to keep the naming as close to specification as possible, to help to keep track what is what.

  13. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    Maybe we should phrase this as:
    
    'The revision is broken into the upper 16-bit major number and the lower 16-bit minor number. The minor number has a range of 0 to 99. The revision is printed as x.y if the last digit is zero.'
    
    Though, I'm not 100% sure what we're trying to say with the last digit is zero case.
  14. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    I wonder if here and below we should have something that indicates a specific sized uintptr or not. Something to think about for the future, I guess. Just in all these places it'd be nice to know that we're trying to refer to a pointr explicitly.
    
    Maybe we should just do something like:
    
    typedef uint32_t efiptr32_t
    typedef uint64_t efiptr64_t
  15. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    We should probably consistently #pragma pack all of the strucutres here, no?
    1. For this round I did create the __packed attribute macro; I'm not really sure if we should prefer attribute or pragma based approach.

  16. usr/src/uts/common/sys/efi.h (Diff revision 1)
     
     
    I also wonder if it's worth us defining a more sane unpacked native EFI system table that has the logical bitness neutral version with real typed pointers that we can cons these both into.
    1. The actual problem there is about the combination of UEFI32 and 64-bit kernel. The pointers and NumberOfTableEntries are using native types, which in case of UEFI32 means 32-bit values. In regard to simple memory reads it means we need to be aware if we are reading 32-bit or 64-bit system table, but to access Runtime Services, it also will mean the switch to 32-bit mode. I think, the bitness neutral solution would need some more investigation and is likely good candidate for either issue of its own or to be bundled with UEFI Runtime Services support module case - this update here is just about to provide the most basic data, we still have need to implement module to access the RS.

  17. Is there a reason this isn't a bcmp?
    1. first of all, the bcmp/memcmp is not enabled in dboot build and having uuid specific function did allow better debug (to insert checkpoints).

  18. Minor nit, the pointer '*' character is off in spacing.
  19. See same not as process_efi32.
  20. Should we be predicating this on it not being set? As it seems like if we have the multiboot tag down below, we'll already have it set. Should we prefer the UEFI tags instead? If so, can we comment on that explicitly?
    1. There is possible to have different approaches. One thing is that in case of UEFI, we normally prefer to fetch table pointers from systab. But it also is possible the (acpi) table is provided via MB tag - either table from firmware or "manually" crafted. And in any case, we prefer "new" version on "old".

      So I did reverse the order in dboot_multiboot_get_fwtables() and did add simple comments.

      I am not really sure about predicating - for BIOS case it is meaningless anyhow as there we will scan the memory if no information is provided...

  21. If we're not using multiboot 2 and therefore there's no UEFI, should we have an explicit none option as opposed to indicating an unknown UEFI arch?
    1. If we have no UEFI, the systab pointer will be NULL, I figured the bi_uefi_arch to distinguish 32 versus 64 bit table; but then again, XBI_UEFI_ARCH_UNKNOWN could just as well be XBI_UEFI_ARCH_NONE.

  22. Is there a reason this was moved?
    1. Actually, I only now noticed I have sort of outsmarted myself - the whole boot_info (bi) is from BSS, so it is all zeroed and the assignments of NULL there are really not needed...

  23. usr/src/uts/i86pc/dboot/dboot_startkern.c (Diff revision 1)
     
     
     
     
     
     
     
    I'm not sure this logic is right. Consider the example in the spec:
    
    A specification with the revision value ((2<<16) | (30)) would be referred as 2.3;
    A specification with the revision value ((2<<16) | (31)) would be referred as 2.3.1
    
    In this case if we have the minor rev as 31, then we'd be in the if statement. sub would be 1 and rev would be 30. That means we'd print 30.1.
    
    In general, this handling of rev doesn't seem right. Should the minor always be EFI_REV_MINOR(ver) / 10? And the micro EFI_REV_MINOR(ver) % 10?
    1. Yep, you are correct. it really is broken.

  24. usr/src/uts/i86pc/dboot/dboot_startkern.c (Diff revision 1)
     
     
     
    Are we guaranteed that we have ascii characters here? Should we assume it's printable?
    1. the specification is only stating: "A pointer to a null terminated string that identifies the vendor that produces the system firmware for the platform."

      As the string in UEFI context is char16 array, it should be printable, but it could have more than ascii chars. So far I have not seen other than ascii (there are not that many vendors afterall), and since this is debug output, it is not that critical for now anyhow.

      I think this is another case for dboot_printf(), to add support for %S for char16 strings.

  25. usr/src/uts/i86pc/dboot/dboot_startkern.c (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Should these be formatted as we normally do UUIDs? Should we make sure we explicitly zero pad it for the right number of bytes?
    1. Same answer as to JBK - the dboot_printf() does not implement the pad+width, of course we can update dboot_printf (and perhaps it is good idea anyhow, it should not be too complicated and can make some other debug prints nicer too). I actually think, it would probably be better to update dboot_printf() in separate patch, and then depending on the integration order update this patch or create an separate update. Marking this note dropped for now.

  26. See all of the print_efi32 bits.
  27. Can we say something like:
    
    'Map framebuffer memory as PT_NOCACHE as this is memory from a device and therefore must not be cached.'?
  28. Why is this here? If we don't allocate this where is it coming from? Even if it's changing for UEFI, presumably this needs to not change for traditional booting.
    1. This is something to think about and it seems to me, this allocation and kernel move does smell something cut in middle when opensolaris was closed. So I did use #if 0 there on purpose, to draw an attention and to keep the issue in focus.

      It really is not just about UEFI; but lets start from the beginning.

      The unix has following mappings:

      for 64-bit:
      dboot: p_vaddr: 0xc00000 p_paddr: 0xc00000
      code: p_vaddr: 0xfffffffffb800000 p_paddr: 0x400000
      data: p_vaddr: 0xfffffffffbc00000 p_paddr: 0x800000

      and for 32-bit:
      dboot: p_vaddr: 0xc00000 p_paddr: 0xc00000
      code: p_vaddr: 0xfe800000 p_paddr: 0x400000
      data: p_vaddr: 0xfec00000 p_paddr: 0x800000

      The 32-bit case is currently only handled for BIOS systems, the loader will load ELF to p_paddr according to the ELF header. The 64-bit kernel is loaded as blob, and the load address is dboot p_paddr - ELF header. So for 64-bit case we will have to move kernel code and data + bss in place.

      Now we have an concern - should we move the kernel as noted in ELF header p_paddr, or should we use some other location?

      In first case, we could clash with dboot. In current kernel we have data:

      Program Header[2]:
          p_vaddr:      0xfffffffffbc00000  p_flags:    [ PF_X PF_W PF_R ]
          p_paddr:      0x800000            p_type:     [ PT_LOAD ]
          p_filesz:     0x11598             p_memsz:    0x93a20
          p_offset:     0x171000            p_align:    0
      
      Section Header[12]:  sh_name: .bss
          sh_addr:      0xfffffffffbc115c0  sh_flags:   [ SHF_WRITE SHF_ALLOC ]
          sh_size:      0x82460             sh_type:    [ SHT_NOBITS ]
          sh_offset:    0x1825c0            sh_entsize: 0
          sh_link:      0                   sh_info:    0
          sh_addralign: 0x40
      

      So we will clear from 0x800000 + 0x11598 = 0x811598 and the BSS is 0x82460 bytes, so we have still 0xc00000 - 0x8939f8 = 0x36c608 bytes, which is ~3.5MB. Not too much, but still quite plenty.

      So what the current dboot code does is, it will count the used memory (modules + MBI + kernel command line), assign the first free address as base for local allocations, will allocate the chunk for kernel and copy the kernel there. So the amd64 kernel will end up at physical address somewhere above loaded modules. The problem with this approach is that with large boot archive the kernel physical location can be in quite high place + we are assuming there actually is space for the kernel - which may not be the case. So in one way or another, the kernel allocation seems to have some issues.

      All in all, based on the calculations, in this current code, I am using the kernel physical locations based on the ELF header data and not allocating the dboot memory above the loaded modules. The only real relation to UEFI is about the fact that UEFI firmware can provide more fragmented memory map and therefore further complicate the allocations - which is the reason we will need smarter dboot/early boot allocator anyhow.

    2. Hmm. OK. I guess I wonder if we're really just undoing the change we did there and we're going back to a different set of problems (or trading off which ones we have). Which I guess is what bothers me a little bit about just ignoring it. I wonder if the cure is worse than the disease or not. I also wonder if there's any reason to believe that we'll actually have that 4 MiB even available to us (as in why is the original PA going to work)?
    3. As discussed, I have removed #if 0 for now, with recognizing we need to address the early allocation issue in dboot/locore soon.

  29. 
      
tsoome
tsoome
rm
  1. 
      
  2. usr/src/uts/common/sys/efi.h (Diff revisions 1 - 3)
     
     
    Do the system tables not need to be packed for any reason?
    1. I guess we better pack it too.

  3. usr/src/uts/i86pc/dboot/dboot_startkern.c (Diff revisions 1 - 3)
     
     
    Maybe 'The ACPI RSDP can be found by scanning the BIOS memory areas or from the EFI system table. The boot may loader may pass in the address it found the ACPI tables at.'
  4. usr/src/uts/i86pc/dboot/dboot_startkern.c (Diff revisions 1 - 3)
     
     
    I'm not sure what this sentence is trying to tell us.
  5. 
      
tsoome
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...