| Message ID | 20221109205353.984745-2-ltykernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/hyperv/sev: Add AMD sev-snp enlightened guest support on hyperv | expand |
On Wed, Nov 09, 2022 at 03:53:36PM -0500, Tianyu Lan wrote: > From: Tianyu Lan <tiala@microsoft.com> > > Hypervisor may pass cc blob address directly into boot param's cc > blob address in the direct boot mode. Check cc blcb hdr magic first > in the sev_enable() and use it as cc blob address if check successfully. > > Signed-off-by: Tianyu Lan <tiala@microsoft.com> > --- > arch/x86/boot/compressed/sev.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c > index c93930d5ccbd..960968f8bf75 100644 > --- a/arch/x86/boot/compressed/sev.c > +++ b/arch/x86/boot/compressed/sev.c > @@ -272,17 +272,24 @@ static void enforce_vmpl0(void) > > void sev_enable(struct boot_params *bp) > { > + struct cc_blob_sev_info *cc_info; > unsigned int eax, ebx, ecx, edx; > struct msr m; > bool snp; > > /* > - * bp->cc_blob_address should only be set by boot/compressed kernel. > - * Initialize it to 0 to ensure that uninitialized values from > - * buggy bootloaders aren't propagated. > + * bp->cc_blob_address should only be set by boot/compressed > + * kernel and hypervisor with direct boot mode. Initialize it > + * to 0 after checking in order to ensure that uninitialized > + * values from buggy bootloaders aren't propagated. > */ > - if (bp) > - bp->cc_blob_address = 0; > + if (bp) { > + cc_info = (struct cc_blob_sev_info *)(unsigned long) > + bp->cc_blob_address; > + > + if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC) > + bp->cc_blob_address = 0; It doesn't seem great to rely on SEV_HDR_MAGIC to determine whether bp->cc_blob_address is valid or not since it is only a 32-bit value. Would it be possible to use a setup_data entry of type SETUP_CC_BLOB in bp->hdr.setup_data instead? There's already handling for that in find_cc_blob_setup_data() so it should "just work". -Mike
On 11/10/2022 7:39 AM, Michael Roth wrote: >> - * bp->cc_blob_address should only be set by boot/compressed kernel. >> - * Initialize it to 0 to ensure that uninitialized values from >> - * buggy bootloaders aren't propagated. >> + * bp->cc_blob_address should only be set by boot/compressed >> + * kernel and hypervisor with direct boot mode. Initialize it >> + * to 0 after checking in order to ensure that uninitialized >> + * values from buggy bootloaders aren't propagated. >> */ >> - if (bp) >> - bp->cc_blob_address = 0; >> + if (bp) { >> + cc_info = (struct cc_blob_sev_info *)(unsigned long) >> + bp->cc_blob_address; >> + >> + if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC) >> + bp->cc_blob_address = 0; > It doesn't seem great to rely on SEV_HDR_MAGIC to determine whether > bp->cc_blob_address is valid or not since it is only a 32-bit value. > > Would it be possible to use a setup_data entry of type SETUP_CC_BLOB > in bp->hdr.setup_data instead? There's already handling for that in > find_cc_blob_setup_data() so it should "just work". Hi Michael: Thanks for your review. I will have a try. Hypervisor may set cc_blob_address directly and so propose this.
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index c93930d5ccbd..960968f8bf75 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -272,17 +272,24 @@ static void enforce_vmpl0(void) void sev_enable(struct boot_params *bp) { + struct cc_blob_sev_info *cc_info; unsigned int eax, ebx, ecx, edx; struct msr m; bool snp; /* - * bp->cc_blob_address should only be set by boot/compressed kernel. - * Initialize it to 0 to ensure that uninitialized values from - * buggy bootloaders aren't propagated. + * bp->cc_blob_address should only be set by boot/compressed + * kernel and hypervisor with direct boot mode. Initialize it + * to 0 after checking in order to ensure that uninitialized + * values from buggy bootloaders aren't propagated. */ - if (bp) - bp->cc_blob_address = 0; + if (bp) { + cc_info = (struct cc_blob_sev_info *)(unsigned long) + bp->cc_blob_address; + + if (cc_info->magic != CC_BLOB_SEV_HDR_MAGIC) + bp->cc_blob_address = 0; + } /* * Setup/preliminary detection of SNP. This will be sanity-checked @@ -374,6 +381,10 @@ static struct cc_blob_sev_info *find_cc_blob(struct boot_params *bp) { struct cc_blob_sev_info *cc_info; + /* Boot kernel would have passed the CC blob via boot_params. */ + if (bp->cc_blob_address) + return (struct cc_blob_sev_info *)(unsigned long)bp->cc_blob_address; + cc_info = find_cc_blob_efi(bp); if (cc_info) goto found_cc_info; @@ -416,9 +427,11 @@ bool snp_init(struct boot_params *bp) /* * Pass run-time kernel a pointer to CC info via boot_params so EFI * config table doesn't need to be searched again during early startup - * phase. + * phase. Hypervisor also may popualte cc_blob_address directyly + * in direct boot mode. */ - bp->cc_blob_address = (u32)(unsigned long)cc_info; + if (!bp->cc_blob_address) + bp->cc_blob_address = (u32)(unsigned long)cc_info; return true; }