7846 loader: UEFI variable support development

Review Request #392 — Created March 7, 2017 and submitted

tsoome
illumos-gate
7846
dfd2cbe...
general

This update is quite complex list of changes and the whole work is still very much open for improvement.

The problem the UEFI variable support is posing is that UEFI variables only have set GUID and name, the actual content is not typed and can contain literally any data, out of which, only variables tagged with "global" GUID, have documented values.

From operations point, eventually we like to see three operations - to see the value, to set the variable/value and to unset the variable. Since the work on variable support has been going on for some time, part of this collection is about collecting the bits to one place and cleaning up the platform specific bits from ficl tree, and providing the forth words via linker sets instead.

Since at the current state the result is entirely informational and does not affect the loading/booting, and we want to get the source tree to the similar state as the FreeBSD tree is.

Also note that there will be more and larger related updates, as the current EFI support headers are quite old and need to get updated to reflect more recent features in UEFI, to bring in v2.0+ headers and to make it possible to build not only the efi loader, but also the user space and kernel bits to access UEFI Runtime Services. However, the related work is still very much in process and is not yet even commited in FreeBSD source tree.

https://svnweb.freebsd.org/base?view=revision&revision=r300081
https://svnweb.freebsd.org/base?view=revision&revision=r300117
https://svnweb.freebsd.org/base?view=revision&revision=r300216
https://svnweb.freebsd.org/base?view=revision&revision=r300328
https://svnweb.freebsd.org/base?view=revision&revision=r300329
https://svnweb.freebsd.org/base?view=revision&revision=r300330
https://svnweb.freebsd.org/base?view=revision&revision=r300634
https://svnweb.freebsd.org/base?view=revision&revision=r306504
https://svnweb.freebsd.org/base?view=revision&revision=r307324
https://svnweb.freebsd.org/base?view=revision&revision=r313042

Implement named uuid values to ease browsing the variables and other information about EFI. Also implement some most frequent representation of the variable value printouts.

efi-show output inspection..

gate update build and install/boot on BIOS system (as at this stage we can not boot UEFI).

  • 0
  • 0
  • 15
  • 9
  • 24
Description From Last Updated
tsoome
tsoome
tsoome
rm
  1. 
      
  2. I don't see any of these defined in UEFI 2.6. Do you know where they come from?
    1. Yep, the GUIDs are one huge pain in EFI, because the specification does only list the most needed ones and to understand what is actually going on in specific system, the translation is needed - as the GUID's are used to identify both the data and the protocols.. This and two below are from intel code drop one can find from edk2 source tree.

    2. Just for an side note; I really would consider the GUID's as "development" resource in an sense that we really do not know yet which ones we will really need later, so most likely this list will need to receive either just some cleanup or perhaps preprocessor condition.

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

  3. Mind pointing me to where this GUID is defined?
    1. Same as above:
      https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Include/Guid/MtcVendor.h

  4. Mind pointing me to where this GUID is defined?

    1. Same as above:
      https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Include/Guid/ZeroGuid.h

  5. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Should the EFI_GUID here be const to match the others?
  6. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    So, I'm wondering if we should be using a strcasecmp here. I think it could be really painful to know I need IPv4 versus ipv4, etc. Otherwise I'm going to have to always come to the source, I feel.
  7. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    This should check for allocation failure and return false in those cases. All the callers today will immediately dereference it assuming they have a valid pointer if true is returned.
  8. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    I think it might be useful to leverage the logic mdb does to include addresses of the dump and the ascii decoding where appropriate, but that's a big future work and not something we should do here.
  9. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    why is the pager output string here ' = \n' as opposed to the normal '\n'? The callers don't seem to use it otherwise.
    1. Copy-paste error. Just for an side note, the pager_output should IMO eventually get implemented printf-like, so all those ugly prints could be removed - the pager_output("\n") is just an shortcut to avoid building the string buffer.

  10. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Any particular reason there are no spaces in these?
    1. I wanted to keep the UEFI memory type name strings as is - the information is useful for the developers and it is easier to track the memory types this way (IMO). Of course we can go for more user oriented output instead.

  11. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    This feels a little clever. Maybe in the future we can really separate out the checking for an empty string with adding the non-null character.
  12. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
     
     
     
    So, I'm not a fan of how this is currently constructed. This is basically assuming that we're only going to have ascii chracters and therefore that it's really a 2:1 mapping. I get why it's doing that, but that feels wrong in the long term. Is there any guarantee from firmware that we're only going to get ascii characters? I get that's what the cpy16to8 is doing today, but if it it didn't and that changed, it seems bad.
    
    Maybe in a future change we can have cpy16to8 and that family also take output sizes?
    1. Yep, the cpy16to8() and cpy8to16 are known faulty and are waiting to get replaced by proper implementations. Since the current implementations can deal with ascii, and the firmware default usually is ascii, we can live with this code for now, but sooner or later it has to be addressed. Since the char16 string management has complications of its own, I really do not want to address those in this patch and just concentrate to bring the base code in sync with upstream.

  13. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Why return OK?
  14. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Can you describe where you get the data sizes for all of these globals? Should I be looking at a specific part of the UEFI spec?
    1. The good thing is, the globals are all described in specification (and are basically only variabled described in specification). In v2.6 the descriptions can be found from 3.3 Globally Defined Variables, page 82.

  15. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    At +499 you prefix with 0x, here we don't. Shouldn't we prefix here to make it consistent?
  16. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Should we leave off the trailing space after the '=' character so that way we don't have two spaces before the first entry?
  17. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Should we explicitly check for data being NULL? It seems like data is only allowed to be NULL according to the spec if DataSize is zero, which it isn't here.
    1. This is another instance of double call approach to get the size on first call, and data on second. In the case of data == NULL, we will get EFI_INVALID_PARAMETER (as the datasz is not too small, but data is NULL). So the error is detected and reported. Of course the problem is, perhaps we should report about out of memory, and not get into the second call. I guess it would be nice thing to do... And of course there is memory leak in error case;)

  18. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Maybe we can move this to a macro in the future?
    1. This one is a bit of problem - I have figured it is to point to efi shell internal aliases, but I havent really found solid reference about this GUID. So I kept it as reference for development/identification purposes for now, I think it is good candidate to get removed eventually.

  19. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Are we guaranteed to only have ASCII?
    1. Currently we can not input anything but ascii. However, I did replace this loop by cpy8to16() so when the char16 api will change, we will not miss this bit.

  20. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    So if you specify an invalid argument here, won't getopt actually return either ':' or '?' and we need to printf optopt and not ch?
    1. Yep, actually we let getopt() to output the error, so we do not need to print anything there.

  21. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
     
    So, shouldn't we be paying attention to the return value so we can return something other than CMD_OK? Also if the other bits have errors, it seems like we don't propagate that at all.
  22. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    So, I think this highlights another issue with the copy routines. We should really consider having them indicate somehow whether or not we could copy everything or not. In this case, that could mean that the wrong variable was set, though I doubt that most folks will try to set 128 character variables.
    1. cpy8to16() actually does also check for terminating 0, but here we provide wrong size:)

  23. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    Why is strlen right here? Is the assumption that we're only ever setting character string values?
    1. It is not. We probably actually should not expose the efi-set and efi-unset commands at all for now, till we have some agreement what variables we should allow to set/unset. I'll set the temporary preprocessor guards.

      Just as we can see from printing the values, we need to verify which variable will get the value and properly prepare the value.

  24. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    I have similar comments as the earlier case.
  25. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 3)
     
     
    I think we need to look at the return value here. If we get uuid_s_invalid_string_uuid then we'll end up still having error set to true and saying that it's a memory error, which isn't true at all.
  26. So see my other notes, I don't think the copy routines are really great designs. I think they'll get by for now, but all the length checks and truncation checks always have to happen at all the callers and in some cases we've checked it and in others we haven't. It may be that we can do minor fixes for what we have for now.
    1. Yes, and the author wants them to get replaced, but as always, not all things happen overnight. Also the wchar related functions should go to libstand, the current ones are just "quick and dirty" implementations and should be replaced. Of course there is also another aspect - as the UEFI wchar is 16-bit, the userland efi bits need to deal with the same issue and obviously there is an wish to avoid the code duplication.

  27. Can you describe what this is for?
    1. The loader is already using linker set to create the set of built in commands, the ficl had the platform specific words guarded by preprocessor, but part of this change is about to move the platform specific words out to platform tree (bios bits to i386/libi386/ files for example). So we are now building two linker sets, one for builtin commands and another for ficl platform specific words. So objcopy -j set_Xficl_compile_set will ensure this section will be present in final efi application binary - the linker sets are each placed in separate named section.

  28. 
      
tsoome
rm
  1. Thanks for the various clean up here, it's appreciated.

  2. 
      
danmcd
  1. 
      
  2. Okay, this clearly comes from Intel, so my complaint about this initializer being one-endian is probably moot. I want this noted on here, however, just in case.

    There are other GUID constants that have a similar issue.

  3. usr/src/boot/sys/boot/efi/libefi/env.c (Diff revision 4)
     
     

    Not raising an issue,but want to say for the record that this GUID should be #defined somewhere, and I'm going to assume the GUID routines always case-fold down to lowercase.

  4. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...