diff mbox series

[Part1,v5,29/38] x86/boot: add a pointer to Confidential Computing blob in bootparams

Message ID 20210820151933.22401-30-brijesh.singh@amd.com
State Superseded
Headers show
Series [Part1,v5,01/38] x86/mm: Add sev_feature_enabled() helper | expand

Commit Message

Brijesh Singh Aug. 20, 2021, 3:19 p.m. UTC 
From: Michael Roth <michael.roth@amd.com>

The previously defined Confidential Computing blob is provided to the
kernel via a setup_data structure or EFI config table entry. Currently
these are both checked for by boot/compressed kernel to access the
CPUID table address within it for use with SEV-SNP CPUID enforcement.

To also enable SEV-SNP CPUID enforcement for the run-time kernel,
similar early access to the CPUID table is needed early on while it's
still using the identity-mapped page table set up by boot/compressed,
where global pointers need to be accessed via fixup_pointer().

This is much of an issue for accessing setup_data, and the EFI config
table helper code currently used in boot/compressed *could* be used in
this case as well since they both rely on identity-mapping. However, it
has some reliance on EFI helpers/string constants that would need to be
accessed via fixup_pointer(), and fixing it up while making it
shareable between boot/compressed and run-time kernel is fragile and
introduces a good bit of uglyness.

Instead, this patch adds a boot_params->cc_blob_address pointer that
boot/compressed can initialize so that the run-time kernel can access
the prelocated CC blob that way instead.

Signed-off-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/include/asm/bootparam_utils.h | 1 +
 arch/x86/include/uapi/asm/bootparam.h  | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Borislav Petkov Aug. 27, 2021, 1:51 p.m. UTC | #1 
On Fri, Aug 20, 2021 at 10:19:24AM -0500, Brijesh Singh wrote:
> From: Michael Roth <michael.roth@amd.com>

> 

> The previously defined Confidential Computing blob is provided to the

> kernel via a setup_data structure or EFI config table entry. Currently

> these are both checked for by boot/compressed kernel to access the

> CPUID table address within it for use with SEV-SNP CPUID enforcement.

> 

> To also enable SEV-SNP CPUID enforcement for the run-time kernel,

> similar early access to the CPUID table is needed early on while it's

> still using the identity-mapped page table set up by boot/compressed,

> where global pointers need to be accessed via fixup_pointer().

> 

> This is much of an issue for accessing setup_data, and the EFI config

> table helper code currently used in boot/compressed *could* be used in

> this case as well since they both rely on identity-mapping. However, it

> has some reliance on EFI helpers/string constants that would need to be

> accessed via fixup_pointer(), and fixing it up while making it

> shareable between boot/compressed and run-time kernel is fragile and

> introduces a good bit of uglyness.

> 

> Instead, this patch adds a boot_params->cc_blob_address pointer that


Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> boot/compressed can initialize so that the run-time kernel can access

> the prelocated CC blob that way instead.

> 

> Signed-off-by: Michael Roth <michael.roth@amd.com>

> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

> ---

>  arch/x86/include/asm/bootparam_utils.h | 1 +

>  arch/x86/include/uapi/asm/bootparam.h  | 3 ++-

>  2 files changed, 3 insertions(+), 1 deletion(-)

> 

> 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));

> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h

> index 1ac5acca72ce..bea5cdcdf532 100644

> --- a/arch/x86/include/uapi/asm/bootparam.h

> +++ b/arch/x86/include/uapi/asm/bootparam.h

> @@ -188,7 +188,8 @@ struct boot_params {

>  	__u32 ext_ramdisk_image;			/* 0x0c0 */

>  	__u32 ext_ramdisk_size;				/* 0x0c4 */

>  	__u32 ext_cmd_line_ptr;				/* 0x0c8 */

> -	__u8  _pad4[116];				/* 0x0cc */

> +	__u8  _pad4[112];				/* 0x0cc */

> +	__u32 cc_blob_address;				/* 0x13c */


So I know I've heard grub being mentioned in conjunction with this: if
you are ever going to pass this through the boot loader, then you'd need
to update Documentation/x86/zero-page.rst too to state that this field
can be written by the boot loader too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Michael Roth Aug. 27, 2021, 6:48 p.m. UTC | #2 
On Fri, Aug 27, 2021 at 03:51:29PM +0200, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 10:19:24AM -0500, Brijesh Singh wrote:

> > From: Michael Roth <michael.roth@amd.com>

> > 

> > The previously defined Confidential Computing blob is provided to the

> > kernel via a setup_data structure or EFI config table entry. Currently

> > these are both checked for by boot/compressed kernel to access the

> > CPUID table address within it for use with SEV-SNP CPUID enforcement.

> > 

> > To also enable SEV-SNP CPUID enforcement for the run-time kernel,

> > similar early access to the CPUID table is needed early on while it's

> > still using the identity-mapped page table set up by boot/compressed,

> > where global pointers need to be accessed via fixup_pointer().

> > 

> > This is much of an issue for accessing setup_data, and the EFI config

> > table helper code currently used in boot/compressed *could* be used in

> > this case as well since they both rely on identity-mapping. However, it

> > has some reliance on EFI helpers/string constants that would need to be

> > accessed via fixup_pointer(), and fixing it up while making it

> > shareable between boot/compressed and run-time kernel is fragile and

> > introduces a good bit of uglyness.

> > 

> > Instead, this patch adds a boot_params->cc_blob_address pointer that

> 

> Avoid having "This patch" or "This commit" in the commit message. It is

> tautologically useless.

> 

> Also, do

> 

> $ git grep 'This patch' Documentation/process

> 

> for more details.

> 

> > boot/compressed can initialize so that the run-time kernel can access

> > the prelocated CC blob that way instead.

> > 

> > Signed-off-by: Michael Roth <michael.roth@amd.com>

> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

> > ---

> >  arch/x86/include/asm/bootparam_utils.h | 1 +

> >  arch/x86/include/uapi/asm/bootparam.h  | 3 ++-

> >  2 files changed, 3 insertions(+), 1 deletion(-)

> > 

> > 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));

> > diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h

> > index 1ac5acca72ce..bea5cdcdf532 100644

> > --- a/arch/x86/include/uapi/asm/bootparam.h

> > +++ b/arch/x86/include/uapi/asm/bootparam.h

> > @@ -188,7 +188,8 @@ struct boot_params {

> >  	__u32 ext_ramdisk_image;			/* 0x0c0 */

> >  	__u32 ext_ramdisk_size;				/* 0x0c4 */

> >  	__u32 ext_cmd_line_ptr;				/* 0x0c8 */

> > -	__u8  _pad4[116];				/* 0x0cc */

> > +	__u8  _pad4[112];				/* 0x0cc */

> > +	__u32 cc_blob_address;				/* 0x13c */

> 

> So I know I've heard grub being mentioned in conjunction with this: if

> you are ever going to pass this through the boot loader, then you'd need

> to update Documentation/x86/zero-page.rst too to state that this field

> can be written by the boot loader too.


Right, I think we had discussed this back in v3 or so. But for grub, or
other bootloaders, the idea would be for them to use pass the CC blob
via a struct setup_data corresponding to SETUP_CC_BLOB, introduced in:

  x86/boot: Add Confidential Computing type to setup_data

the boot_params field is only used internally to allow boot/compressed
to hand the CC blob over to kernel proper without kernel proper needing
to rescan for EFI blob (and thus needing all the efi config parsing
stuff).

> 

> -- 

> 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%7C83df94e2e42a415a515308d96961b2e8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637656690614876025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=aL8JhC82mQ59sYNfk645%2Bxv%2FrgfU95jTxBJIr8uRRZs%3D&amp;reserved=0
diff mbox series

Patch

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));
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index 1ac5acca72ce..bea5cdcdf532 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -188,7 +188,8 @@  struct boot_params {
 	__u32 ext_ramdisk_image;			/* 0x0c0 */
 	__u32 ext_ramdisk_size;				/* 0x0c4 */
 	__u32 ext_cmd_line_ptr;				/* 0x0c8 */
-	__u8  _pad4[116];				/* 0x0cc */
+	__u8  _pad4[112];				/* 0x0cc */
+	__u32 cc_blob_address;				/* 0x13c */
 	struct edid_info edid_info;			/* 0x140 */
 	struct efi_info efi_info;			/* 0x1c0 */
 	__u32 alt_mem_k;				/* 0x1e0 */