Enable booting of Slaunch-enabled EFI binaries via chainloader command

[SergiiDmytruk]

Relevant changes are in the top 2 commits, the rest are more fixes noticed in the process (to be squashed later).

[miczyg1]

I don't quite understand why we have two fields which seemingly hold the same data. Is there any difference between them?

[SergiiDmytruk]

boot_params holds virtual and boot_params_base physical address. From i386/linux.c:

    real_mode_mem = get_virtual_current_address (ch);

    if (grub_slaunch_platform_type () != SLP_NONE)
      {
        slparams.boot_params = real_mode_mem;
        slparams.boot_params_base = get_physical_target_address (ch);
      }

Why the suffix is _base and not _pa or something like that is not known to me :) Secure Launch Spec also uses "base" at least once in the same sense.

[miczyg1]

I see. Well It would be good to have boot_params_va and boot_params_pa, because right know I cannot distinguish which one is which. I will start some discussion on Matrix. RIght now closing the thread, as it should not be blocking.

[miczyg1]

Ohh wait... These fields look like something GRUB-specific:

struct grub_slaunch_params
{
  grub_uint32_t boot_type;
  grub_uint32_t platform_type;
  struct linux_kernel_params *boot_params;
  grub_uint64_t boot_params_base;

I thought it is defined in the spec... I guess it would be better to have:

struct grub_slaunch_params
{
  grub_uint32_t boot_type;
  grub_uint32_t platform_type;
  grub_uint64_t boot_params_va;
  grub_uint64_t boot_params_pa;

For less confusion. As the slaunch is not Linux-specific (will need casts everywhere to linux_kernel_params *). @rossphilipson WDYT?

[SergiiDmytruk]

Right, I mentioned the spec only because the reasoning for the suffix should be the same in both cases.

If changing a couple of the fields, then the rest should be changed too:

• efi_memmap_mem -> efi_memmap_va?
• slr_table_base -> slr_table_base
• slr_table_mem -> slr_table_va?
• mle_start -> mle_pa
• mle_ptab_target -> mle_ptab_pa
• mle_ptab_mem -> mle_ptab_va?
• ap_wake_block -> ap_wake_block_pa
• dce_base -> dce_pa
• tpm_evt_log_base -> tpm_evt_log_pa

Types are also inconsistent. Addresses should probably all be grub_addr_t.

Which is why I didn't do it. Lots of renames across lots of commits and review might result in yet other names. _target, _base, _mem are also used as parameters and local variables.

[krystian-hebel]

Addresses should probably all be grub_addr_t.

Not really. First of all, GRUB uses this for physical addresses only (not sure if that is what you meant by "all"), virtual addresses are either void * or pointer to proper type. This type is also defined differently between 64b and 32b GRUB. While it may be enough to be used internally by GRUB (it won't allocate above 4G in 32b anyway), fields in SLRT are defined to always be 64 bits.

[SergiiDmytruk]

I meant physical addresses. Consistently using grub_uint64_t for them is a good option too. Currently, mle_start and ap_wake_block are of type grub_uint32_t.

[miczyg1]

This actually breaks the build for i386-pc:

In file included from ../../include/grub/disk.h:31,
                 from ../../include/grub/file.h:26,
                 from ../../include/grub/loader.h:23,
                 from ../../grub-core/loader/slaunch/skl.c:35:
../../grub-core/loader/slaunch/skl.c: In function ‘grub_skl_set_module’:
../../grub-core/loader/slaunch/skl.c:101:21: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘unsigned int’ [-Werror=format=]
  101 |                     "Possible SKL module doesn't measure info: %u > %lu\n",
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  102 |                     module->skl_info_offset,
  103 |                     module->length - sizeof (info->uuid));
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                    |
      |                                    unsigned int
../../include/grub/misc.h:38:88: note: in definition of macro ‘grub_dprintf’
   38 | #define grub_dprintf(condition, ...) grub_real_dprintf(GRUB_FILE, __LINE__, condition, __VA_ARGS__)
      |                                                                                        ^~~~~~~~~~~
../../grub-core/loader/slaunch/skl.c:101:71: note: format string is defined here
  101 |                     "Possible SKL module doesn't measure info: %u > %lu\n",
      |                                                                     ~~^
      |                                                                       |
      |                                                                       long unsigned int
      |                                                                     %u

[SergiiDmytruk]

https://github.com/TrenchBoot/grub/compare/35c725edd5a0aca6ba09ea4fe4976cb906f54399..6965858d99a49f207be546f69289de711ad33538 should fix it.

[miczyg1]

@SergiiDmytruk can we get packages built by CI here too?

[SergiiDmytruk]

@SergiiDmytruk can we get packages built by CI here too?

I'm trying to make CI work at https://github.com/TrenchBoot/grub/commits/xen-efi-ci/.