On Wed, Jun 23, 2021 at 12:22:23PM +0200, Borislav Petkov wrote:

quoted

On Tue, Jun 22, 2021 at 11:30:43PM -0500, Michael Roth wrote:

quoted

Quoting Borislav Petkov (2021-06-18 10:05:28)

quoted

On Fri, Jun 18, 2021 at 08:57:12AM -0500, Brijesh Singh wrote:

quoted

Don't have any strong reason to keep it separate, I can define a new
type and use the setup_data to pass this information.

setup_data is exactly for use cases like that - pass a bunch of data
to the kernel. So there's no need for a separate thing. Also see that
kernel_info thing which got added recently for read_only data.

Hi Boris,

There's one side-effect to this change WRT the CPUID page (which I think
we're hoping to include in RFC v4).

With CPUID page we need to access it very early in boot, for both
boot/compressed kernel, and the uncompressed kernel. At first this was
implemented by moving the early EFI table parsing code from
arch/x86/kernel/boot/compressed/acpi.c into a little library to handle early
EFI table parsing to fetch the Confidential Computing blob to get the CPUID
page address.

This was a bit messy since we needed to share that library between
boot/compressed and uncompressed, and at that early stage things like
fixup_pointer() are needed in some places, else even basic things like
accessing EFI64_LOADER_SIGNATURE and various EFI helper functions could crash
in uncompressed otherwise, so the library code needed to be fixed up
accordingly.

To simplify things we ended up simply keeping the early EFI table parsing in
boot/compressed, and then having boot/compressed initialize
setup_data.cc_blob_address so that the uncompressed kernel could access it
from there (acpi does something similar with rdsp address).

Yes, except the rsdp address is not vendor-specific but an x86 ACPI
thing, so pretty much omnipresent.

Also, acpi_rsdp_addr is part of boot_params and that struct is full
of padding holes and obsolete members so reusing a u32 there is a lot
"easier" than changing the setup_header. So can you put that address in
boot_params instead?

Thanks for the suggestion! I tried something like the below and that seems to
work pretty well. I'm not sure if that's the best spot or not though, it
seems like it might be a good idea to leave some padding after eddbuf in
case it needs to grow in the future. I'll look into that a bit more.

One downside to this is we still need something in the boot protocol,
either via setup_data, or setup_header directly. Having it in
setup_header avoids the need to also have to add a field to boot_params
for the boot/compressed->uncompressed passing, but maybe that's not a good
enough justification. Perhaps if the TDX folks have similar needs though.

diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index 1ac5acca72ce..0824c8646861 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h

@@ -218,7 +218,8 @@ struct boot_params {
         struct boot_e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */
         __u8  _pad8[48];                                /* 0xcd0 */
         struct edd_info eddbuf[EDDMAXNR];               /* 0xd00 */
-       __u8  _pad9[276];                               /* 0xeec */
+       __u32 cc_blob_address;							/* 0xeec */
+       __u8  _pad9[272];                               /* 0xef0 */
  } __attribute__((packed));

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 981fe923a59f..53e9b0620d96 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h

@@ -74,6 +74,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
                         BOOT_PARAM_PRESERVE(hdr),
                         BOOT_PARAM_PRESERVE(e820_table),
                         BOOT_PARAM_PRESERVE(eddbuf),
+                       BOOT_PARAM_PRESERVE(cc_blob_address),
                 };

                 memset(&scratch, 0, sizeof(scratch));

quoted

quoted

Now that we're moving it to setup_data, this becomes a bit more awkward,
since we need to reserve memory in boot/compressed to store the setup_data
entry, then add it to the linked list to pass along to uncompressed kernel.
In turn that also means we need to add an identity mapping for this in
ident_map_64.c, so I'm not sure that's the best approach.

So just trying to pin what the best approach is:

a) move cc_blob to setup_data, and do the above-described to pass
    cc_blob_address from boot/compressed to uncompressed to avoid early
    EFI parsing in uncompressed
b) move cc_blob to setup_data, and do the EFI table parsing in both
    boot/compressed. leave setup_data allocation/init for BIOS/bootloader
c) keep storing cc_blob_address in setup_header.cc_blob_address
d) something else?

Leaving in the whole text for newly CCed TDX folks in case they're going
to need something like that.

And if so, then both vendors should even share the field definition.

Dave, Sathya, you can find the whole subthread here:

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20210602140416.23573-21-brijesh.singh%40amd.com&data=04%7C01%7Cmichael.roth%40amd.com%7C3b352c4b944c4d95bbdb08d93630d0eb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637600405622460196%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=464O7JxibsbjC3bc0LkGcztdb4kCYH7kcQAcqohJhug%3D&reserved=0

in case you need background info on the topic at hand.

Thx.

-- 
Regards/Gruss,
     Boris.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C3b352c4b944c4d95bbdb08d93630d0eb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637600405622460196%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ruCM7CNgPCNPkrOoiNts1ZKi5k7JSUumln7qQMP%2BMi0%3D&reserved=0