7649 loader.efi: fix extraction of firmare table info for smbios and acpi.

Review Request #284 — Created Dec. 5, 2016 and submitted

tsoome
illumos-gate
7649
822ab82...
general
7649 loader.efi: fix extraction of firmare table info for smbios and acpi.


  • 0
  • 0
  • 10
  • 2
  • 12
Description From Last Updated
richlowe
  1. 
      
  2. usr/src/boot/sys/boot/efi/loader/acpi.c (Diff revision 1)
     
     

    Is this a rename from biosacpi.c that's not obvious in RB, or a new file? If it's new, does the other consumer of biosacpi.c need this one as well/instead?

    1. It is new file but the same segment of data extraction as it is used in biosacpi.c and efi/loader/arch/amd64/elf64_freebsd.c. I would like to avoid using libi386 files as much as possible to avoid preprocessor spaghetti. At some point it would actually make much more sense to refactor acpi/smbios common bits into common tree and have in efi and i386 (bios) trees just the platform specific bits to perform the location detection code. But I also like to move with baby steps there as more people will get better understanding about the structure and logic of this code, there will be also alternate ideas and views. biosacpi.c is providing similar interface with addition of scan code for various bios areas where acpi traditionally can be found.

  3. usr/src/boot/sys/boot/efi/loader/acpi.c (Diff revision 1)
     
     

    Either do it and remove the comment, or describe the comment to say why you aren't or whatever.

    1. I think I'll just remove it - for us this is just "nice to have" information, the kernel does the acpi processing/verification anyhow and we do not need to duplicate it here.

  4. 
      
tsoome
richlowe
  1. Ship It!
  2. 
      
rm
  1. 
      
  2. usr/src/boot/sys/boot/efi/loader/acpi.c (Diff revision 2)
     
     
    Can you explain why we have the #ifdef _LP64 bits throughout this? Is it the case that a 32-bit and 64-bit loader can only find 32-bit and 64-bit tables? It might help to clarify why we have the #ifdef's.
    1. Yes, we get efi-systab with native pointers in it, so we need to distinguish 64/32 case.

  3. usr/src/boot/sys/boot/efi/loader/acpi.c (Diff revision 2)
     
     
    Should we be using snprintf everywhere rather than sprintf?
  4. usr/src/boot/sys/boot/efi/loader/main.c (Diff revision 2)
     
     
    Is this going to result in us getting a 32-bit or 64-bit entry point for SMBIOS?
    
    I guess we'll need to update the smbios code to handle it being in a 64-bit address separately?
    1. Yes, you are corrent, I was missing 64bit SMBIOS entry point. So I would need to work on it some.. Thanks.

  5. 
      
tsoome
rm
  1. 
      
  2. SMBIOS 3.1 says that this can exist without EFI tables. Is there a reason that we're restricting this to EFI only?
    1. yes, because BIOS version of the loader is 32-bit application but the structure table can start at any 64-bit physical address. I did update the comment to point this out.

  3. In other occurrences of smbios.addr you're using the PTOV macro. Do you need to here? Though I'm not sure why any of this is translating.
    1. PTOV() is only needed for BIOS version, because there we do not have 1:1 physical to virtual memory mapping, but the user (loader) memory is shifted by 0xa000. This is to mask V86 interface from loader. In UEFI case we do have 1:1 mapping and no translation is needed and PTOV(x) macro does not do any translation for x. Since you did ask, perhaps we should include it to reduce the confusion, but there is no technical need for it.

  4. Does the rest of this code assume that smbios.count is the actual count or that it's merely an upper bound?
    1. The actual table walker, smbios_find_struct() is using this expression in for loop:

      dmi < smbios.addr + smbios.length && i < smbios.count

      And tables we need are only searched based on type, and not indexed, so it is just for upper bound; for SMBIOS3 case the actual limit is table size, as there is no other data set in entry struct, for other cases it can be both table size and entry count. The specification itself is also a bit buggy in sense that they do provide example walker in Annex B, which is also using entry count and they tell that the count is from entry table, except there is no such value in 64-bit entry table. But this is not big issue anyhow as we do have the table size. This is the reason I did use the table header size there, so we can have some sort of approximation for entry count.

  5. Missing space before the '?'?
  6. 
      
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/i386/libi386/smbios.c (Diff revisions 3 - 4)
     
     
    I wouldn't conflate SMBIOS 3.x with the 64-bit entry point. I'd phrase this whole comment maybe as follows, thoughts?:
    
    Check for the SMBIOS 64-bit entry point introduced in version 3.0.
    
    The table address is a 64-bit physical address that may appear at any 64-bit address. Because the BIOS based version of the loader is a 32-bit application, we only search for the 64-bit entry point when using UEFI.
    1. Yes, this is much better wording. However, I did figure another issue, it is not enough just to check EFI, but instead, we need to check LP64, as there is also UEFI32. I have no idea if 64-bit entry point can appear in UEFI32, but LP64 guard will keep us at safe side.

  3. 
      
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/i386/libi386/smbios.c (Diff revisions 4 - 5)
     
     
    In general, I wouldn't rely on the fact that _LP64 implies EFI. We should probably include both in the ifdef.
    1. Well, the reason for the #ifdef LP64 is about the fact that 64bit entry point can give us the table address anywhere in 64-bit address space and 32-bit loader (BIOS or EFI) can not addess the high memory. In that sense the EFI or no EFI does not really matter as it is just about the 64-bit table pointer and its usability. If the SMBIOS specification would state that in case of _SM3 entry point the 64-bit table pointer would be within 4 GB, we would not need this #ifdef.

      The only thing the EFI does contribute here is the starting address for the search, as EFI is using the address provided by efi-systab. In case of BIOS system, the starting address for the search and the range is exactly the same for both SM and SM3 tags, so we can get either entry point from the scan, depending on which one is the first one.

      Now there is another possible scenario there, if the system is only providing SM3 entry point and is supporting the BIOS emulation and we do boot 32-bit loader. In such case the loader will not find the SMBIOS and will not set the env, which by itself is no issue because the env is only for user information anyhow. Now, if the kernel does not get the smbios pointer from boot loader, it will perform the same scan as is done here and will find the smbios entry point on its own.

      My initial idea to use #ifdef EFI was from the initial EFI support, which was 64-bit only. However, we will still also need to support UEFI32 as there are many 64-bit capable systems providing only UEFI32 (intel atom for example), and therefore the preprocessor symbol EFI does not help here, as 32bit UEFI has exactly the same issue with the SM3 as 32-bit BIOS does.

      So, as I see, the EFI macro can not contribute any value here, as the only real concern is the usability of the 64-bit table pointer.

    2. This makes sense. I had forgotten as well that the 64-bit entry point was available in normal BIOS processing as well.
  3. 
      
rm
  1. Ship It!
  2. 
      
richlowe
  1. 
      
  2. "a 64-bit application"

  3. 
      
tsoome
tsoome
Review request changed

Status: Closed (submitted)

Loading...