diff mbox series

[Part1,RFC,v3,20/22] x86/boot: Add Confidential Computing address to setup_header

Message ID 20210602140416.23573-21-brijesh.singh@amd.com (mailing list archive)
State New, archived
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand

Commit Message

Brijesh Singh June 2, 2021, 2:04 p.m. UTC
While launching the encrypted guests, the hypervisor may need to provide
some additional information that will used during the guest boot. In the
case of AMD SEV-SNP the information includes the address of the secrets
and CPUID pages. The secrets page contains information such as a VM to
PSP communication key and CPUID page contain PSP filtered CPUID values.

When booting under the EFI based BIOS, the EFI configuration table
contains an entry for the confidential computing blob. In order to support
booting encrypted guests on non EFI VM, the hypervisor to pass these
additional information to the kernel with different method.

For this purpose expand the struct setup_header to hold the physical
address of the confidential computing blob location. Being zero means it
isn't passed.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 Documentation/x86/boot.rst            | 27 +++++++++++++++++++++++++++
 arch/x86/boot/header.S                |  7 ++++++-
 arch/x86/include/uapi/asm/bootparam.h |  1 +
 3 files changed, 34 insertions(+), 1 deletion(-)

Comments

Borislav Petkov June 18, 2021, 6:08 a.m. UTC | #1
On Wed, Jun 02, 2021 at 09:04:14AM -0500, Brijesh Singh wrote:
> While launching the encrypted guests, the hypervisor may need to provide
> some additional information that will used during the guest boot. In the
> case of AMD SEV-SNP the information includes the address of the secrets
> and CPUID pages. The secrets page contains information such as a VM to
> PSP communication key and CPUID page contain PSP filtered CPUID values.
> 
> When booting under the EFI based BIOS, the EFI configuration table
> contains an entry for the confidential computing blob. In order to support
> booting encrypted guests on non EFI VM, the hypervisor to pass these
> additional information to the kernel with different method.
> 
> For this purpose expand the struct setup_header to hold the physical
> address of the confidential computing blob location. Being zero means it
> isn't passed.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  Documentation/x86/boot.rst            | 27 +++++++++++++++++++++++++++
>  arch/x86/boot/header.S                |  7 ++++++-
>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
> index fc844913dece..9b32805617bb 100644
> --- a/Documentation/x86/boot.rst
> +++ b/Documentation/x86/boot.rst
> @@ -75,6 +75,8 @@ Protocol 2.14	BURNT BY INCORRECT COMMIT
>  		DO NOT USE!!! ASSUME SAME AS 2.13.
>  
>  Protocol 2.15	(Kernel 5.5) Added the kernel_info and kernel_info.setup_type_max.
> +
> +Protocol 2.16	(Kernel 5.14) Added the confidential computing blob address
>  =============	============================================================
>  
>  .. note::
> @@ -226,6 +228,7 @@ Offset/Size	Proto		Name			Meaning
>  0260/4		2.10+		init_size		Linear memory required during initialization
>  0264/4		2.11+		handover_offset		Offset of handover entry point
>  0268/4		2.15+		kernel_info_offset	Offset of the kernel_info
> +026C/4		2.16+		cc_blob_address	        Physical address of the confidential computing blob

Why is this a separate thing instead of being passed as setup_data?
Brijesh Singh June 18, 2021, 1:57 p.m. UTC | #2
On 6/18/2021 1:08 AM, Borislav Petkov wrote:
> On Wed, Jun 02, 2021 at 09:04:14AM -0500, Brijesh Singh wrote:
>> While launching the encrypted guests, the hypervisor may need to provide
>> some additional information that will used during the guest boot. In the
>> case of AMD SEV-SNP the information includes the address of the secrets
>> and CPUID pages. The secrets page contains information such as a VM to
>> PSP communication key and CPUID page contain PSP filtered CPUID values.
>>
>> When booting under the EFI based BIOS, the EFI configuration table
>> contains an entry for the confidential computing blob. In order to support
>> booting encrypted guests on non EFI VM, the hypervisor to pass these
>> additional information to the kernel with different method.
>>
>> For this purpose expand the struct setup_header to hold the physical
>> address of the confidential computing blob location. Being zero means it
>> isn't passed.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  Documentation/x86/boot.rst            | 27 +++++++++++++++++++++++++++
>>  arch/x86/boot/header.S                |  7 ++++++-
>>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>>  3 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
>> index fc844913dece..9b32805617bb 100644
>> --- a/Documentation/x86/boot.rst
>> +++ b/Documentation/x86/boot.rst
>> @@ -75,6 +75,8 @@ Protocol 2.14	BURNT BY INCORRECT COMMIT
>>  		DO NOT USE!!! ASSUME SAME AS 2.13.
>>  
>>  Protocol 2.15	(Kernel 5.5) Added the kernel_info and kernel_info.setup_type_max.
>> +
>> +Protocol 2.16	(Kernel 5.14) Added the confidential computing blob address
>>  =============	============================================================
>>  
>>  .. note::
>> @@ -226,6 +228,7 @@ Offset/Size	Proto		Name			Meaning
>>  0260/4		2.10+		init_size		Linear memory required during initialization
>>  0264/4		2.11+		handover_offset		Offset of handover entry point
>>  0268/4		2.15+		kernel_info_offset	Offset of the kernel_info
>> +026C/4		2.16+		cc_blob_address	        Physical address of the confidential computing blob
> 
> Why is this a separate thing instead of being passed as setup_data?
> 

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.

-Brijesh
Borislav Petkov June 18, 2021, 3:05 p.m. UTC | #3
On Fri, Jun 18, 2021 at 08:57:12AM -0500, Brijesh Singh wrote:
> 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.

Thx.
Michael Roth June 23, 2021, 4:30 a.m. UTC | #4
Quoting Borislav Petkov (2021-06-18 10:05:28)
> On Fri, Jun 18, 2021 at 08:57:12AM -0500, Brijesh Singh wrote:
> > 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).

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?

Thanks!

-Mike

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7CMichael.Roth%40amd.com%7Cc0b20041125441de743508d9326a8e96%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637596255567306700%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ninetMMQX9bQIgjIshN877BQ5xv2R7h%2FZulHd%2B8TI3c%3D&amp;reserved=0
Borislav Petkov June 23, 2021, 10:22 a.m. UTC | #5
On Tue, Jun 22, 2021 at 11:30:43PM -0500, Michael Roth wrote:
> Quoting Borislav Petkov (2021-06-18 10:05:28)
> > On Fri, Jun 18, 2021 at 08:57:12AM -0500, Brijesh Singh wrote:
> > > 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?

> 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://lkml.kernel.org/r/20210602140416.23573-21-brijesh.singh@amd.com

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

Thx.
Michael Roth June 24, 2021, 3:19 a.m. UTC | #6
On Wed, Jun 23, 2021 at 12:22:23PM +0200, Borislav Petkov wrote:
> On Tue, Jun 22, 2021 at 11:30:43PM -0500, Michael Roth wrote:
> > Quoting Borislav Petkov (2021-06-18 10:05:28)
> > > On Fri, Jun 18, 2021 at 08:57:12AM -0500, Brijesh Singh wrote:
> > > > 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));

> 
> > 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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C3b352c4b944c4d95bbdb08d93630d0eb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637600405622460196%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=464O7JxibsbjC3bc0LkGcztdb4kCYH7kcQAcqohJhug%3D&amp;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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C3b352c4b944c4d95bbdb08d93630d0eb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637600405622460196%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ruCM7CNgPCNPkrOoiNts1ZKi5k7JSUumln7qQMP%2BMi0%3D&amp;reserved=0
Borislav Petkov June 24, 2021, 7:27 a.m. UTC | #7
On Wed, Jun 23, 2021 at 10:19:11PM -0500, Michael Roth wrote:
> One downside to this is we still need something in the boot protocol,
> either via setup_data, or setup_header directly.

Huh, now I'm confused. You gave the acpi_rsdp_addr example and I thought
that should be enough, that's why I suggested boot_params.

Maybe you should point me to the code which does what you need so that I
can get a better idea...

> 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.

Yes, reportedly they do so I guess the solution should be
vendor-agnostic. Let's see what they need first.

Thx.
Michael Roth June 24, 2021, 12:26 p.m. UTC | #8
On Thu, Jun 24, 2021 at 09:27:41AM +0200, Borislav Petkov wrote:
> On Wed, Jun 23, 2021 at 10:19:11PM -0500, Michael Roth wrote:
> > One downside to this is we still need something in the boot protocol,
> > either via setup_data, or setup_header directly.
> 
> Huh, now I'm confused. You gave the acpi_rsdp_addr example and I thought
> that should be enough, that's why I suggested boot_params.

Well, that's enough for the boot/compressed->uncompressed parameter
passing, but would a bootloader be allowed to use that same approach to
pass in the CC blob (for things like non-EFI/containers)? I was under the
impression that for that case we'd still want something in
setup_header/setup_data for bootloaders to use, and that using boot_params
directly is more of a legacy/ad-hoc thing. Is that accurate?

> 
> Maybe you should point me to the code which does what you need so that I
> can get a better idea...
> 
> > 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.
> 
> Yes, reportedly they do so I guess the solution should be
> vendor-agnostic. Let's see what they need first.

Ok, good to know.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C876d84222a2d4c2fc14008d936e19194%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637601164763100440%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=XIsAM9LxAOr7LlsJBkT33OuxywezofF%2Bq7%2FEqxpJOVk%3D&amp;reserved=0
Michael Roth June 24, 2021, 12:34 p.m. UTC | #9
On Thu, Jun 24, 2021 at 09:27:41AM +0200, Borislav Petkov wrote:
> On Wed, Jun 23, 2021 at 10:19:11PM -0500, Michael Roth wrote:
> > One downside to this is we still need something in the boot protocol,
> > either via setup_data, or setup_header directly.
> 
> Huh, now I'm confused. You gave the acpi_rsdp_addr example and I thought
> that should be enough, that's why I suggested boot_params.

Well, that's sufficient for the boot/compressed->uncompressed parameter
passing, but wouldn't actual bootloaders still need something in
setup_data/setup_header to pass in the CC blob (for things like non-EFI
environments/containers)? I was under the impression that using
boot_params directly was more of a legacy/ad-hoc thing, is that
accurate?

> 
> Maybe you should point me to the code which does what you need so that I
> can get a better idea...
> 
> > 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.
> 
> Yes, reportedly they do so I guess the solution should be
> vendor-agnostic. Let's see what they need first.

Ok, good to know.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
Borislav Petkov June 24, 2021, 12:54 p.m. UTC | #10
On Thu, Jun 24, 2021 at 07:34:47AM -0500, Michael Roth wrote:
> Well, that's sufficient for the boot/compressed->uncompressed parameter
> passing, but wouldn't actual bootloaders still need something in
> setup_data/setup_header to pass in the CC blob (for things like non-EFI
> environments/containers)? I was under the impression that using
> boot_params directly was more of a legacy/ad-hoc thing, is that
> accurate?

/me goes and rereads your early mail.

I'm more confused.

You're talking about parsing an EFI table early which contains the
ccblob and in it is the CPUID page.

Now above you say, "non-EFI environments".

I'm guessing you want to support both so you want to either parse an EFI
table on EFI environments or pass the blob in a different way in non-EFI
envs. Yes, no?

Also, you want to pass the previously parsed CPUID page address to
kernel proper. For that I suggested to use boot_params.

What else?

How about you explain in a lot more detail what exactly the requirements
and the use cases are so that we can have a common base to discuss it
on.

Thx.
Kuppuswamy Sathyanarayanan June 24, 2021, 1:09 p.m. UTC | #11
+ Andi, Kirill (For TDX specific review)

On 6/23/21 8:19 PM, Michael Roth wrote:
> On Wed, Jun 23, 2021 at 12:22:23PM +0200, Borislav Petkov wrote:
>> On Tue, Jun 22, 2021 at 11:30:43PM -0500, Michael Roth wrote:
>>> Quoting Borislav Petkov (2021-06-18 10:05:28)
>>>> On Fri, Jun 18, 2021 at 08:57:12AM -0500, Brijesh Singh wrote:
>>>>> 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));
> 
>>
>>> 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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C3b352c4b944c4d95bbdb08d93630d0eb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637600405622460196%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=464O7JxibsbjC3bc0LkGcztdb4kCYH7kcQAcqohJhug%3D&amp;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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C3b352c4b944c4d95bbdb08d93630d0eb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637600405622460196%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ruCM7CNgPCNPkrOoiNts1ZKi5k7JSUumln7qQMP%2BMi0%3D&amp;reserved=0
Michael Roth June 24, 2021, 2:11 p.m. UTC | #12
On Thu, Jun 24, 2021 at 02:54:44PM +0200, Borislav Petkov wrote:
> On Thu, Jun 24, 2021 at 07:34:47AM -0500, Michael Roth wrote:
> > Well, that's sufficient for the boot/compressed->uncompressed parameter
> > passing, but wouldn't actual bootloaders still need something in
> > setup_data/setup_header to pass in the CC blob (for things like non-EFI
> > environments/containers)? I was under the impression that using
> > boot_params directly was more of a legacy/ad-hoc thing, is that
> > accurate?
> 
> /me goes and rereads your early mail.
> 
> I'm more confused.

Sorry for the confusion, hopefully I can explain better now that I've
had some coffee.

> 
> You're talking about parsing an EFI table early which contains the
> ccblob and in it is the CPUID page.
> 
> Now above you say, "non-EFI environments".
> 
> I'm guessing you want to support both so you want to either parse an EFI
> table on EFI environments or pass the blob in a different way in non-EFI
> envs. Yes, no?

Yes.

> 
> Also, you want to pass the previously parsed CPUID page address to
> kernel proper. For that I suggested to use boot_params.

Yes. (though I'm actually passing the whole CC blob address so kernel
proper can get the CPUID address from there. That gives us the option of
using that field to get at the secret page in very early boot of
uncompressed/proper kernel as well).

> 
> What else?
> 
> How about you explain in a lot more detail what exactly the requirements
> and the use cases are so that we can have a common base to discuss it
> on.

So for EFI case:

  We don't need anything in setup_data/setup_header. We can access the
  CC blob table via EFI config table. However, parsing EFI config table
  early in uncompressed/proper kernel has the complications I mentioned in my
  initial response. This is where using a new boot_params field comes into
  play (similar to acpi_rsdp_addr), so boot/compressed can pass
  uncompressed/proper kernel a pointer to the pre-parsed CC blob so it doesn't
  need to re-parse EFI config table during early boot.

For non-EFI case:

  We need a "proper" mechanism that bootloaders can use. My
  understanding is this would generally be via setup_data or
  setup_header, and that a direct boot_params field would be frowned
  upon.

So your understanding of the situation seems correct.

By bringing up the non-EFI case I only meant to point out that by using a
field in setup_header, we could re-use that field for the EFI case as well,
and wouldn't need a seperate boot_params field to handle the
boot/compressed->uncompressed passing of the pre-parsed CC blob address
in the EFI case. But I don't think it makes a big difference as far as
my stuff goes at least. Maybe for TDX though this needs more thought.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7Cd913249cd25d44e389d108d9370f40ab%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637601360942853147%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ghw22DdACcxZsfaWd%2FyAuhlr4NwJY8b63bXPvB1MvTY%3D&amp;reserved=0
Borislav Petkov June 25, 2021, 2:48 p.m. UTC | #13
On Thu, Jun 24, 2021 at 09:11:11AM -0500, Michael Roth wrote:
>   We don't need anything in setup_data/setup_header. We can access the
>   CC blob table via EFI config table. However, parsing EFI config table
>   early in uncompressed/proper kernel has the complications I mentioned in my
>   initial response.

So since you want to parse the EFI table in the boot stage, that tells
me that you need the blob in the early, boot kernel too, correct?

> This is where using a new boot_params field comes into
>   play (similar to acpi_rsdp_addr), so boot/compressed can pass
>   uncompressed/proper kernel a pointer to the pre-parsed CC blob so it doesn't
>   need to re-parse EFI config table during early boot.

Ack.

> For non-EFI case:
> 
>   We need a "proper" mechanism that bootloaders can use. My
>   understanding is this would generally be via setup_data or
>   setup_header, and that a direct boot_params field would be frowned
>   upon.

So, you need to pass only an address, right?

How workable would it be if you had a cmdline option:

	cc_blob_address=0xb1a

with which you tell the kernel where that thing is?

Because then you can use this in both cases - EFI and !EFI.

Or is this blob platform-dependent and the EFI table contains it and
something needs to get it out of there, even if it were a user to
type in the cmdline cc_blob_address or some script when it comes to
containers...?

In any case, setup_data is kinda the generic way to pass arbitrary data
to the kernel so in this case, you can prioritize the EFI table in the
EFI case and fall back to setup_data if there's no EFI table...

> So your understanding of the situation seems correct.
> 
> By bringing up the non-EFI case I only meant to point out that by using a
> field in setup_header, we could re-use that field for the EFI case as well,
> and wouldn't need a seperate boot_params field to handle the
> boot/compressed->uncompressed passing of the pre-parsed CC blob address
> in the EFI case. But I don't think it makes a big difference as far as
> my stuff goes at least. Maybe for TDX though this needs more thought.

Yah, let's see what requirements they'd come back with.

Thx.
Brijesh Singh June 25, 2021, 3:24 p.m. UTC | #14
On 6/25/2021 9:48 AM, Borislav Petkov wrote:
>> For non-EFI case:
>>
>>   We need a "proper" mechanism that bootloaders can use. My
>>   understanding is this would generally be via setup_data or
>>   setup_header, and that a direct boot_params field would be frowned
>>   upon.
> 
> So, you need to pass only an address, right?
> 
> How workable would it be if you had a cmdline option:
> 
> 	cc_blob_address=0xb1a
> 
> with which you tell the kernel where that thing is?
> 
> Because then you can use this in both cases - EFI and !EFI.
> 

In the case of EFI, the CC blob structure is dynamically allocated
and passed through the EFI configuration table. The grub will not
know what value to pass in the cmdline unless we improve it to read
the EFI configuration table and rebuild the cmdline.


> Or is this blob platform-dependent and the EFI table contains it and
> something needs to get it out of there, even if it were a user to
> type in the cmdline cc_blob_address or some script when it comes to
> containers...?
> 
> In any case, setup_data is kinda the generic way to pass arbitrary data
> to the kernel so in this case, you can prioritize the EFI table in the
> EFI case and fall back to setup_data if there's no EFI table...
>
Borislav Petkov June 25, 2021, 5:01 p.m. UTC | #15
On Fri, Jun 25, 2021 at 10:24:01AM -0500, Brijesh Singh wrote:
> In the case of EFI, the CC blob structure is dynamically allocated
> and passed through the EFI configuration table. The grub will not
> know what value to pass in the cmdline unless we improve it to read
> the EFI configuration table and rebuild the cmdline.

Or simply parse the EFI table.

To repeat my question: why do you need the CC blob in the boot kernel?

Then, how does it work then in the !EFI case?

The script glue that starts the lightweight container goes and
"prepares" that blob and passes it to guest kernel? In which case
setup_data should do the job, methinks.
Michael Roth June 25, 2021, 6:14 p.m. UTC | #16
On Fri, Jun 25, 2021 at 07:01:54PM +0200, Borislav Petkov wrote:
> On Fri, Jun 25, 2021 at 10:24:01AM -0500, Brijesh Singh wrote:
> > In the case of EFI, the CC blob structure is dynamically allocated
> > and passed through the EFI configuration table. The grub will not
> > know what value to pass in the cmdline unless we improve it to read
> > the EFI configuration table and rebuild the cmdline.
> 
> Or simply parse the EFI table.
> 
> To repeat my question: why do you need the CC blob in the boot kernel?

We basically need to be able to access the CPUID page in any place where we
might need to emulate a cpuid instruction, which both the boot kernel and
proper kernel do very early on for things like probing paging support and
checking for SEV/SME features.

> 
> Then, how does it work then in the !EFI case?

Currently, based on your v3 feedback, I have the following order of
precedence for getting at the cc blob to get the cpuid page:

For boot/compressed kernel:

  1) Search for CC blob in the following order/precedence:
     - via linux boot protocol / setup_data entry
     - via EFI configuration table
  2) If found, initialize boot_params->cc_blob_address to point to the
     blob so that uncompressed kernel can easily access it during very
     early boot without the need to re-parse EFI config table

For uncompressed/proper kernel:

  1) Search for CC blob in the following order/precedence:
     - via linux boot protocol / setup_data entry
     - via boot_params->cc_blob_address

So non-EFI case would rely purely on the setup_data entry for both (though
we could still use boot_params->cc_blob_address to avoid the need to scan
setup_data list in proper kernel as well, but scanning it early on doesn't
have the same issues as with EFI config table so it's not really
necessary there).

I opted to give setup_data precedence over EFI, since if a bootloader goes
the extra mile of packaging up a setup_data argument instead of just leaving
it to firmware/EFI config table, it might be out of some extra need.  For
example, if we do have a shared definition for both SEV and TDX, maybe the
bootloader needs to synthesize multiple EFI table entries, and a unified
setup_data will be easier for the kernel to consume than replicating that same
work, and maybe over time the fallback can be deprecated. And containers will
more than likely prefer setup_data approach, which might drive future changes
that aren't in lockstep with EFI definitions as well.

It doesn't matter much currently though since setup_data is basically just
pointing to the CC blob allocated by EFI.

> 
> The script glue that starts the lightweight container goes and
> "prepares" that blob and passes it to guest kernel? In which case
> setup_data should do the job, methinks.

Brijesh can correct me if I'm wrong, but I believe that's the intent, and the
setup_data approach definitely seems workable for that aspect.

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7CMichael.Roth%40amd.com%7Cf927ef94d7af4819892708d937faf24b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637602373266379986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=t5iRKv6RcaPazLlON1oGyyEZdxX%2FAxZz8cjJwrz7UqQ%3D&amp;reserved=0
Borislav Petkov June 28, 2021, 1:43 p.m. UTC | #17
On Fri, Jun 25, 2021 at 01:14:17PM -0500, Michael Roth wrote:
> So non-EFI case would rely purely on the setup_data entry for both (though
> we could still use boot_params->cc_blob_address to avoid the need to scan
> setup_data list in proper kernel as well, but scanning it early on doesn't
> have the same issues as with EFI config table so it's not really
> necessary there).

Yeah, sure, we can simply always use boot_params->cc_blob_address just
like acpi_rsdp_addr and always put the CC blob address there.

> I opted to give setup_data precedence over EFI, since if a bootloader goes
> the extra mile of packaging up a setup_data argument instead of just leaving
> it to firmware/EFI config table, it might be out of some extra need.  For
> example, if we do have a shared definition for both SEV and TDX, maybe the
> bootloader needs to synthesize multiple EFI table entries, and a unified
> setup_data will be easier for the kernel to consume than replicating that same
> work, and maybe over time the fallback can be deprecated. And containers will
> more than likely prefer setup_data approach, which might drive future changes
> that aren't in lockstep with EFI definitions as well.

Yah, that makes perfect sense. And you/Brijesh should put the gist of
that in a comment over the code so that people are aware. The less we
rely on firmware, the better.

> Brijesh can correct me if I'm wrong, but I believe that's the intent, and the
> setup_data approach definitely seems workable for that aspect.

Oki doki, I think we're all on the same page then. :-)

Thx.
diff mbox series

Patch

diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index fc844913dece..9b32805617bb 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -75,6 +75,8 @@  Protocol 2.14	BURNT BY INCORRECT COMMIT
 		DO NOT USE!!! ASSUME SAME AS 2.13.
 
 Protocol 2.15	(Kernel 5.5) Added the kernel_info and kernel_info.setup_type_max.
+
+Protocol 2.16	(Kernel 5.14) Added the confidential computing blob address
 =============	============================================================
 
 .. note::
@@ -226,6 +228,7 @@  Offset/Size	Proto		Name			Meaning
 0260/4		2.10+		init_size		Linear memory required during initialization
 0264/4		2.11+		handover_offset		Offset of handover entry point
 0268/4		2.15+		kernel_info_offset	Offset of the kernel_info
+026C/4		2.16+		cc_blob_address	        Physical address of the confidential computing blob
 ===========	========	=====================	============================================
 
 .. note::
@@ -1026,6 +1029,30 @@  Offset/size:	0x000c/4
 
   This field contains maximal allowed type for setup_data and setup_indirect structs.
 
+============	==================
+Field name:	cc_blob_address
+Type:		write (optional)
+Offset/size:	0x26C/4
+Protocol:	2.16+
+============	==================
+
+  This field can be set by the boot loader to tell the kernel the physical address
+  of the confidential computing blob info.
+
+  A value of 0 indicates that either the blob is not provided or use the EFI configuration
+  table to retrieve the blob location.
+
+  In the case of AMD SEV the blob look like this::
+
+  struct cc_blob_sev_info {
+        u32 magic;      /* 0x45444d41 (AMDE) */
+        u16 version;
+        u16 reserved;
+        u64 secrets_phys; /* pointer to secrets page */
+        u32 secrets_len;
+        u64 cpuid_phys;   /* pointer to cpuid page */
+        u32 cpuid_len;
+  }
 
 The Image Checksum
 ==================
diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 6dbd7e9f74c9..b4a014a18f91 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -303,7 +303,7 @@  _start:
 	# Part 2 of the header, from the old setup.S
 
 		.ascii	"HdrS"		# header signature
-		.word	0x020f		# header version number (>= 0x0105)
+		.word	0x0210		# header version number (>= 0x0105)
 					# or else old loadlin-1.5 will fail)
 		.globl realmode_swtch
 realmode_swtch:	.word	0, 0		# default_switch, SETUPSEG
@@ -577,6 +577,11 @@  pref_address:		.quad LOAD_PHYSICAL_ADDR	# preferred load addr
 init_size:		.long INIT_SIZE		# kernel initialization size
 handover_offset:	.long 0			# Filled in by build.c
 kernel_info_offset:	.long 0			# Filled in by build.c
+cc_blob_address:	.long 0			# (Header version 0x210 or later)
+						# If nonzero, a 32-bit pointer to
+						# the confidential computing blob.
+						# The blob will contain the information
+						# used while booting the encrypted VM.
 
 # End of setup header #####################################################
 
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b25d3f82c2f3..210e1a0fb4ce 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -102,6 +102,7 @@  struct setup_header {
 	__u32	init_size;
 	__u32	handover_offset;
 	__u32	kernel_info_offset;
+	__u32	cc_blob_address;
 } __attribute__((packed));
 
 struct sys_desc_table {