| Message ID | 20210820151933.22401-31-brijesh.singh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Part1,v5,01/38] x86/mm: Add sev_feature_enabled() helper | expand |
On Fri, Aug 20, 2021 at 10:19:25AM -0500, Brijesh Singh wrote: > From: Michael Roth <michael.roth@amd.com> > > When the Confidential Computing blob is located by the boot/compressed > kernel, store a pointer to it in bootparams->cc_blob_address to avoid > the need for the run-time kernel to rescan the EFI config table to find > it again. > > Since this function is also shared by the run-time kernel, this patch Here's "this patch" again... but you know what to do. > also adds the logic to make use of bootparams->cc_blob_address when it > has been initialized. > > Signed-off-by: Michael Roth <michael.roth@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kernel/sev-shared.c | 40 ++++++++++++++++++++++++++---------- > 1 file changed, 29 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index 651980ddbd65..6f70ba293c5e 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -868,7 +868,6 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb, > return ES_OK; > } > > -#ifdef BOOT_COMPRESSED > static struct setup_data *get_cc_setup_data(struct boot_params *bp) > { > struct setup_data *hdr = (struct setup_data *)bp->hdr.setup_data; > @@ -888,6 +887,16 @@ static struct setup_data *get_cc_setup_data(struct boot_params *bp) > * 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 > + * 3) Return a pointer to the CC blob, NULL otherwise. > + * > + * For run-time/uncompressed kernel: > + * > + * 1) Search for CC blob in the following order/precedence: > + * - via linux boot protocol / setup_data entry Why would you do this again if the boot/compressed kernel has already searched for it? > + * - via boot_params->cc_blob_address Yes, that is the only thing you need to do in the runtime kernel - see if cc_blob_address is not 0. And all the work has been done by the decompressor kernel already. > * 2) Return a pointer to the CC blob, NULL otherwise. > */ > static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) > @@ -897,9 +906,11 @@ static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) > struct setup_data header; > u32 cc_blob_address; > } *sd; > +#ifdef __BOOT_COMPRESSED > unsigned long conf_table_pa; > unsigned int conf_table_len; > bool efi_64; > +#endif That function turns into an unreadable mess with that #ifdef __BOOT_COMPRESSED slapped everywhere. It seems the cleanest thing to do is to do what we do with acpi_rsdp_addr: do all the parsing in boot/compressed/ and pass it on through boot_params. Kernel proper simply reads the pointer. Which means, you can stick all that cc_blob figuring out functionality in arch/x86/boot/compressed/sev.c instead. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Aug 27, 2021 at 04:15:14PM +0200, Borislav Petkov wrote: > On Fri, Aug 20, 2021 at 10:19:25AM -0500, Brijesh Singh wrote: > > From: Michael Roth <michael.roth@amd.com> > > > > When the Confidential Computing blob is located by the boot/compressed > > kernel, store a pointer to it in bootparams->cc_blob_address to avoid > > the need for the run-time kernel to rescan the EFI config table to find > > it again. > > > > Since this function is also shared by the run-time kernel, this patch > > Here's "this patch" again... but you know what to do. > > > also adds the logic to make use of bootparams->cc_blob_address when it > > has been initialized. > > > > Signed-off-by: Michael Roth <michael.roth@amd.com> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > --- > > arch/x86/kernel/sev-shared.c | 40 ++++++++++++++++++++++++++---------- > > 1 file changed, 29 insertions(+), 11 deletions(-) > > > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > > index 651980ddbd65..6f70ba293c5e 100644 > > --- a/arch/x86/kernel/sev-shared.c > > +++ b/arch/x86/kernel/sev-shared.c > > @@ -868,7 +868,6 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb, > > return ES_OK; > > } > > > > -#ifdef BOOT_COMPRESSED > > static struct setup_data *get_cc_setup_data(struct boot_params *bp) > > { > > struct setup_data *hdr = (struct setup_data *)bp->hdr.setup_data; > > @@ -888,6 +887,16 @@ static struct setup_data *get_cc_setup_data(struct boot_params *bp) > > * 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 > > + * 3) Return a pointer to the CC blob, NULL otherwise. > > + * > > + * For run-time/uncompressed kernel: > > + * > > + * 1) Search for CC blob in the following order/precedence: > > + * - via linux boot protocol / setup_data entry > > Why would you do this again if the boot/compressed kernel has already > searched for it? In some cases it's possible to boot directly to kernel proper without going through decompression kernel (e.g. CONFIG_PVH), so this is to allow a way for boot loaders of this sort to provide a CC blob without relying on EFI. It could be relevant for things like fast/virtualized containers. > > > + * - via boot_params->cc_blob_address > > Yes, that is the only thing you need to do in the runtime kernel - see > if cc_blob_address is not 0. And all the work has been done by the > decompressor kernel already. > > > * 2) Return a pointer to the CC blob, NULL otherwise. > > */ > > static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) > > @@ -897,9 +906,11 @@ static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) > > struct setup_data header; > > u32 cc_blob_address; > > } *sd; > > +#ifdef __BOOT_COMPRESSED > > unsigned long conf_table_pa; > > unsigned int conf_table_len; > > bool efi_64; > > +#endif > > That function turns into an unreadable mess with that #ifdef > __BOOT_COMPRESSED slapped everywhere. > > It seems the cleanest thing to do is to do what we do with > acpi_rsdp_addr: do all the parsing in boot/compressed/ and pass it on > through boot_params. Kernel proper simply reads the pointer. > > Which means, you can stick all that cc_blob figuring out functionality > in arch/x86/boot/compressed/sev.c instead. Most of the #ifdef'ery is due to the EFI scan, so I moved that part out to a separate helper, snp_probe_cc_blob_efi(), that lives in boot/compressed.sev.c. Still not pretty, but would this be acceptable? /* * 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 * 3) Return a pointer to the CC blob, NULL otherwise. * * For run-time/uncompressed kernel: * * 1) Search for CC blob in the following order/precedence: * - via boot_params->cc_blob_address * - via linux boot protocol / setup_data entry * 2) Return a pointer to the CC blob, NULL otherwise. */ static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) { struct cc_blob_sev_info *cc_info = NULL; struct cc_setup_data *sd; #ifndef __BOOT_COMPRESSED /* * CC blob isn't in setup_data, see if boot kernel passed it via * boot_params. */ if (bp->cc_blob_address) { cc_info = (struct cc_blob_sev_info *)(unsigned long)bp->cc_blob_address; goto out_verify; } #endif /* Try to get CC blob via setup_data */ sd = get_cc_setup_data(bp); if (sd) { cc_info = (struct cc_blob_sev_info *)(unsigned long)sd->cc_blob_address; goto out_verify; } #ifdef __BOOT_COMPRESSED cc_info = snp_probe_cc_blob_efi(bp); #endif out_verify: /* CC blob should be either valid or not present. Fail otherwise. */ if (cc_info && cc_info->magic != CC_BLOB_SEV_HDR_MAGIC) sev_es_terminate(1, GHCB_SNP_UNSUPPORTED); #ifdef __BOOT_COMPRESSED /* * Pass run-time kernel a pointer to CC info via boot_params for easier * access during early boot. */ bp->cc_blob_address = (u32)(unsigned long)cc_info; #endif return cc_info; } > > 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%7C1c87c1e207d64d80bae308d9696503b4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637656704870745741%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=R3pm8Xf3f5B%2Fm7IDpL%2BiFS0kUdCMmUlhtJFXCROh4YA%3D&reserved=0
On Fri, Aug 27, 2021 at 02:09:55PM -0500, Michael Roth wrote: > Most of the #ifdef'ery is due to the EFI scan, so I moved that part out > to a separate helper, snp_probe_cc_blob_efi(), that lives in > boot/compressed.sev.c. Still not pretty, but would this be acceptable? It is still ugly... :) I guess you should simply do two separate functions - one doing the compressed dance and the other the later parsing and keep 'em separate. It's not like you're duplicating a ton of code so... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index 651980ddbd65..6f70ba293c5e 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -868,7 +868,6 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb, return ES_OK; } -#ifdef BOOT_COMPRESSED static struct setup_data *get_cc_setup_data(struct boot_params *bp) { struct setup_data *hdr = (struct setup_data *)bp->hdr.setup_data; @@ -888,6 +887,16 @@ static struct setup_data *get_cc_setup_data(struct boot_params *bp) * 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 + * 3) Return a pointer to the CC blob, NULL otherwise. + * + * For run-time/uncompressed kernel: + * + * 1) Search for CC blob in the following order/precedence: + * - via linux boot protocol / setup_data entry + * - via boot_params->cc_blob_address * 2) Return a pointer to the CC blob, NULL otherwise. */ static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) @@ -897,9 +906,11 @@ static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) struct setup_data header; u32 cc_blob_address; } *sd; +#ifdef __BOOT_COMPRESSED unsigned long conf_table_pa; unsigned int conf_table_len; bool efi_64; +#endif /* Try to get CC blob via setup_data */ sd = (struct setup_data_cc *)get_cc_setup_data(bp); @@ -908,29 +919,36 @@ static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) goto out_verify; } +#ifdef __BOOT_COMPRESSED /* CC blob isn't in setup_data, see if it's in the EFI config table */ if (!efi_get_conf_table(bp, &conf_table_pa, &conf_table_len, &efi_64)) (void)efi_find_vendor_table(conf_table_pa, conf_table_len, EFI_CC_BLOB_GUID, efi_64, (unsigned long *)&cc_info); +#else + /* + * CC blob isn't in setup_data, see if boot kernel passed it via + * boot_params. + */ + if (bp->cc_blob_address) + cc_info = (struct cc_blob_sev_info *)(unsigned long)bp->cc_blob_address; +#endif out_verify: /* CC blob should be either valid or not present. Fail otherwise. */ if (cc_info && cc_info->magic != CC_BLOB_SEV_HDR_MAGIC) sev_es_terminate(1, GHCB_SNP_UNSUPPORTED); +#ifdef __BOOT_COMPRESSED + /* + * Pass run-time kernel a pointer to CC info via boot_params for easier + * access during early boot. + */ + bp->cc_blob_address = (u32)(unsigned long)cc_info; +#endif + return cc_info; } -#else -/* - * Probing for CC blob for run-time kernel will be enabled in a subsequent - * patch. For now we need to stub this out. - */ -static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp) -{ - return NULL; -} -#endif /* * Initial set up of CPUID table when running identity-mapped.