8139 loader: efi multiboot2 update

Review Request #461 — Created May 1, 2017 and submitted

tsoome
illumos-gate
8139
462
793d99b...
general
8139 loader: efi multiboot2 update


  • 0
  • 0
  • 22
  • 10
  • 32
Description From Last Updated
rm
  1. 
      
  2. Is there a reason that all these need to be extern and aren't in a header file?
  3. Is there any reason this isn't in a common header file?
    1. it actually is fixed in later update, but moved it into this patch now.

  4. Should there be a warning here that this has happened?
    1. And setting error code.

  5. Can we either comment this or actually just drop the #else block entirely and make the #else a #endif?
  6. Should this be a macro maybe?
  7. Do we need to align this after adding the sizeof (multiboot_tag_efi_mmap_t)?
    1. hm, it actually was missing indeed, just that I adjust the alignment in mbi_size() itself.

  8. Should we make this an explicit 64-bit ifdef as well to make it clear that we're only supporting the 64-bit case?
    1. There is no need - the UEFI firmware will only load and run the native apps, and does provide only "native" resources. Meaning that as long as we only provide 64-bit loader, we will not get 32-bit systab.

  9. Comment the endif?
  10. This works because of the assumption that these are all contiguous allocations via mb_alloc right?
    1. Yes, the MB2 info tag list is contiguous chunk of memory, for UEFI we do calculate the size and allocate the chunk in efi_loadaddr() with AllocatePages() Boot Services callback; for BIOS we will pick next free address after loaded modules. And while populating the MBI tag list, we are using mb_malloc as simple helper to receive the next unallocated address.

  11. Should we cast this to a uintptr_t first?
  12. I think I understand the point of this, maybe worth a comment that the msb is directly correlated to the bits per pixel present?
  13. Maybe worth a comment on how this is derived?
  14. Can we get a comment explaining how this is related to the field positions. While I think I might understand it, I'm not certain.
  15. I guess if for some reason the status was OK, that'd be a problem and it makes sense to error here. Right?
    1. Yes, we call GetMemoryMap() with size 0 (the first argument), and we are expecting to get EFI_BUFFER_TOO_SMALL as an response, as we should get the suggested size for the memory map.

  16. How do we know that the contents of map are valid? Presumably there's some way to indicate that the last one is reached, right?
    1. On successful GetMemoryMap() call, we will get three important values - the memory map size in bytes (size), the map is array of EFI_MEMORY_DESCRIPTORs and we will get the size of the EFI_MEMORY_DESCRIPTORs (desc_size). So the number of the EFI_MEMORY_DESCRIPTORs in the array is total size / desc_size.

  17. So, if we don't find something what will the value of map be? It seems we assume we'll always find something, is that a safe assumption?
    1. Apparently we should get memory map starting with conventional memory at 1MB boundary, so at this point the code is using this assumption. This is done to simplify the search for the blocks and the relocater itself. It definitely is not the best way, but so far the idea seems to hold.

  18. We don't seem to do anything with status. Should we?
  19. Is there a bound on the size of chunklist that we can use?
    1. Actually yes, (4096 - offsetof (struct relocator, rel_chunklist)) / sizeof (struct chunk), should be about 127 entries. Did add the check...

  20. Maybe worth a macro to transform the size?
  21. Is this just something that happened upstream?
    1. The fbsd UFS does have two versions, ufs1 and ufs2; our ufs is almost the same as fbsd ufs1, the ufs reader code is implemented to support either specific versions or both, but we only need ufs1, and we have no need to include the bits for ufs2 which we do not support anyhow.

  22. Does this describe the state when we get here? If so, I think it'd be useful to explicitly mention that.
    1. Actually thats the machine state we are expected to have on kernel entry. Comment updated.

  23. The point of the iretq here is to make sure that we end up translating the state that we're in, right?
    1. Somehow the reply is not showing up, so posting it again:

      It is a bit more complicated - this specific iret is used to switch to the 64-bit compatibility mode - where the code segment is in low memory. This can not be done with jump, but can be done with ret or iret. Once in compatibility mode, we can switch off the long mode.

      Here I got the most ideas from our fastboot code (see uts/i86pc/ml/fb_swtch_src.s) - for simple reason; it does basically the same thing, set up the memory, switch from long mode to 32-bit protected mode and jump to dboot (our kernel).

      So I did use iret, as it also does have nice side effect of helping to set up the (part) of the state. Part because setting up the segment selectors etc can be cone after we are back in compatibility mode. The process is also nicely described here: https://www.codeproject.com/Articles/45788/The-Real-Protected-Long-mode-assembly-tutorial-for

  24. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1)
     
     
    Does EFI assume a 4k page size or should we be using a macro for the 12?
    1. I totally did miss there is the macro.

  25. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1)
     
     
    For this and copyout, is it worth asserting something about the EFI address and length and making sure it's less than 4 GiB given the earlier constraint. This is in a similar fashion to what copyin() does if you're below kernelbase on debug.
  26. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1)
     
     

    Do we need to cast this through a uintptr_t first?

  27. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1)
     
     
    Do we need to cast this through a uintptr_t first?
  28. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1)
     
     
    Maybe you can describe the algorithm in a bit more detail here? what exactly happens if there are conflicts?
  29. Maybe in a future revision we can get structure prefixes for both of these structs?
  30. usr/src/boot/sys/boot/efi/loader/main.c (Diff revision 1)
     
     
    Presumably we need to check argp for malloc failure. Also, do we need to clean it up in the failure case?
  31. Can we get structure prefixes in the future, please?
  32. Can you please include the table name? As the table number changes from release to release.
  33. Shouldn't the default be AddressRangeReserved?
    1. SMAP_TYPE_RESERVED, but yes, this is better indeed.

  34. Should this be consistent with return values being a separate line as the previous function?
  35. This can fail, right?

    1. Yes, but in that case we will get EFI_INVALID_PARAMETER (size is not 0, but buffer is NULL) from BS->GetMemoryMap() in next line and we will print the error and return.

  36. So is it okay if we don't allocate all the ndesc entries? I understand that the logic later on handles not having all of them, but it seems a bit unclear as to what we'd expect.
    1. the malloc() as such can fail, but since the memory map (currently smap) is critical resource to be presented to kernel, we call efi_getsmap() in loader main() as early as possible - right after the console is set up and before we are going to probe the disks, therefore we expect never get out of memory while building the smap list; also we are aggregating the smap entries, so we should not have that large list (my vmware fusion vm has 8 entries).

      From another hand, if we get out of heap memory this early, it is fatal problem in any case - something must be very wrong.

  37. I have the same question about knowing that we haven't gone off the end of the memory descriptors here as earlier.
    1. we are counting the descriptors with i and the while loop has condition i < ndesc and ndesc * desc_size is the size of the buffer. NextMemoryDescriptor() macro is advancing the pointer by desc_size. Since we have no inner loop, we are only testing one desctiptor at the time and every increment is tested by the while loop condition.

  38. It looks like this calls something which can fail but that's not propagated at all.
    1. Sigh, so true. I think it is better to address this as an separate issue (+upstream update), the file_addmetadata() is called in so many places...

  39. 
      
tsoome
  1. 
      
  2. It is a bit more complicated - this specific iret is used to switch to the 64-bit compatibility mode - where the code segment is in low memory. This can not be done with jump, but can be done with ret or iret. Once in compatibility mode, we can switch off the long mode.

    Here I got the most ideas from our fastboot code (see uts/i86pc/ml/fb_swtch_src.s) - for simple reason; it does basically the same thing, set up the memory, switch from long mode to 32-bit protected mode and jump to dboot (our kernel).

    So I did use iret, as it also does have nice side effect of helping to set up the (part) of the state. Part because setting up the segment selectors etc can be cone after we are back in compatibility mode. The process is also nicely described here: https://www.codeproject.com/Articles/45788/The-Real-Protected-Long-mode-assembly-tutorial-for

  3. 
      
tsoome
rm
  1. Thanks for all of the clean up!

  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...