| Message ID | 20211210154332.11526-28-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On 12/10/21 7:43 AM, Brijesh Singh wrote: > +/* AMD SEV Confidential computing blob structure */ > +#define CC_BLOB_SEV_HDR_MAGIC 0x45444d41 > +struct cc_blob_sev_info { > + u32 magic; > + u16 version; > + u16 reserved; > + u64 secrets_phys; > + u32 secrets_len; > + u64 cpuid_phys; > + u32 cpuid_len; > +}; This is an ABI structure rather than some purely kernel construct, right? I searched through all of the specs to which you linked in the cover letter. I looked for "blob", "guid", the magic and part of the GUID itself trying to find where this is defined to see if the struct is correct. I couldn't find anything. Where is the spec for this blob? How large is it? Did you mean to leave a 4-byte hole after secrets_len and before cpuid_phys?
On 12/10/21 1:12 PM, Dave Hansen wrote: > On 12/10/21 7:43 AM, Brijesh Singh wrote: >> +/* AMD SEV Confidential computing blob structure */ >> +#define CC_BLOB_SEV_HDR_MAGIC 0x45444d41 >> +struct cc_blob_sev_info { >> + u32 magic; >> + u16 version; >> + u16 reserved; >> + u64 secrets_phys; >> + u32 secrets_len; >> + u64 cpuid_phys; >> + u32 cpuid_len; >> +}; > This is an ABI structure rather than some purely kernel construct, right? This is ABI between the guest BIOS and Guest OS. It is defined in the OVMF. https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h SEV-SNP FW spec does not have it documented; it's up to the guest BIOS on how it wants to communicate the Secrets and CPUID page location to guest OS. > I searched through all of the specs to which you linked in the cover > letter. I looked for "blob", "guid", the magic and part of the GUID > itself trying to find where this is defined to see if the struct is correct. > > I couldn't find anything. > > Where is the spec for this blob? How large is it? Did you mean to > leave a 4-byte hole after secrets_len and before cpuid_phys? Yes, the length is never going to be > 4GB.
On 12/10/21 12:18 PM, Brijesh Singh wrote: > On 12/10/21 1:12 PM, Dave Hansen wrote: >> On 12/10/21 7:43 AM, Brijesh Singh wrote: >>> +/* AMD SEV Confidential computing blob structure */ >>> +#define CC_BLOB_SEV_HDR_MAGIC 0x45444d41 >>> +struct cc_blob_sev_info { >>> + u32 magic; >>> + u16 version; >>> + u16 reserved; >>> + u64 secrets_phys; >>> + u32 secrets_len; >>> + u64 cpuid_phys; >>> + u32 cpuid_len; >>> +}; >> This is an ABI structure rather than some purely kernel construct, right? > > This is ABI between the guest BIOS and Guest OS. It is defined in the OVMF. > > https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h > > SEV-SNP FW spec does not have it documented; it's up to the guest BIOS > on how it wants to communicate the Secrets and CPUID page location to > guest OS. Well, no matter where it is defined, could we please make it a bit easier for folks to find it in the future? >> I searched through all of the specs to which you linked in the cover >> letter. I looked for "blob", "guid", the magic and part of the GUID >> itself trying to find where this is defined to see if the struct is correct. >> >> I couldn't find anything. >> >> Where is the spec for this blob? How large is it? Did you mean to >> leave a 4-byte hole after secrets_len and before cpuid_phys? > Yes, the length is never going to be > 4GB. I was more concerned that this structure could change sizes if it were compiled on 32-bit versus 64-bit code. For kernel ABIs, we try not to do that. Is this somehow OK when talking to firmware? Or can a 32-bit OS and 64-bit firmware never interact?
On 12/10/21 2:30 PM, Dave Hansen wrote: > On 12/10/21 12:18 PM, Brijesh Singh wrote: >> On 12/10/21 1:12 PM, Dave Hansen wrote: >>> On 12/10/21 7:43 AM, Brijesh Singh wrote: >>>> +/* AMD SEV Confidential computing blob structure */ >>>> +#define CC_BLOB_SEV_HDR_MAGIC 0x45444d41 >>>> +struct cc_blob_sev_info { >>>> + u32 magic; >>>> + u16 version; >>>> + u16 reserved; >>>> + u64 secrets_phys; >>>> + u32 secrets_len; >>>> + u64 cpuid_phys; >>>> + u32 cpuid_len; >>>> +}; >>> This is an ABI structure rather than some purely kernel construct, right? >> >> This is ABI between the guest BIOS and Guest OS. It is defined in the OVMF. >> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FOvmfPkg%2FInclude%2FGuid%2FConfidentialComputingSevSnpBlob.h&data=04%7C01%7Cbrijesh.singh%40amd.com%7C460f6abff7f04e065c9108d9bc1bfcf7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637747650681544593%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GI1fAngRJ%2Bj4hcM91UutVXlS1F7kfk2xxtG6I%2BL%2FRYc%3D&reserved=0 >> >> SEV-SNP FW spec does not have it documented; it's up to the guest BIOS >> on how it wants to communicate the Secrets and CPUID page location to >> guest OS. > > Well, no matter where it is defined, could we please make it a bit > easier for folks to find it in the future? > Noted, I will add a comment so that readers can find it easily. Additionally, I will create a doc and get it published on developer.amd.com/sev so that information is documented outside the source code files. >>> I searched through all of the specs to which you linked in the cover >>> letter. I looked for "blob", "guid", the magic and part of the GUID >>> itself trying to find where this is defined to see if the struct is correct. >>> >>> I couldn't find anything. >>> >>> Where is the spec for this blob? How large is it? Did you mean to >>> leave a 4-byte hole after secrets_len and before cpuid_phys? >> Yes, the length is never going to be > 4GB. > > I was more concerned that this structure could change sizes if it were > compiled on 32-bit versus 64-bit code. For kernel ABIs, we try not to > do that. > > Is this somehow OK when talking to firmware? Or can a 32-bit OS and > 64-bit firmware never interact? > For SNP, both the firmware and OS need to be 64-bit. IIRC, both the Linux and OVMF do not enable the memory encryption for the 32-bit. thanks
On 12/13/21 6:49 AM, Brijesh Singh wrote: >> I was more concerned that this structure could change sizes if it were >> compiled on 32-bit versus 64-bit code. For kernel ABIs, we try not to >> do that. >> >> Is this somehow OK when talking to firmware? Or can a 32-bit OS and >> 64-bit firmware never interact? > > For SNP, both the firmware and OS need to be 64-bit. IIRC, both the > Linux and OVMF do not enable the memory encryption for the 32-bit. Could you please make the structure's size invariant? That's great if there's no problem in today's implementation, but it's best no to leave little land mines like this around. Let's say someone copies your code as an example of something that interacts with a firmware table a few years or months down the road.
On 12/13/21 9:08 AM, Dave Hansen wrote: > On 12/13/21 6:49 AM, Brijesh Singh wrote: >>> I was more concerned that this structure could change sizes if it were >>> compiled on 32-bit versus 64-bit code. For kernel ABIs, we try not to >>> do that. >>> >>> Is this somehow OK when talking to firmware? Or can a 32-bit OS and >>> 64-bit firmware never interact? >> >> For SNP, both the firmware and OS need to be 64-bit. IIRC, both the >> Linux and OVMF do not enable the memory encryption for the 32-bit. > > Could you please make the structure's size invariant? Ack. I will make the required changes. That's great if > there's no problem in today's implementation, but it's best no to leave > little land mines like this around. Let's say someone copies your code > as an example of something that interacts with a firmware table a few > years or months down the road. >
On 2021-12-10 09:43:19 -0600, Brijesh Singh wrote: > While launching the encrypted guests, the hypervisor may need to provide > some additional information during the guest boot. When booting under the > EFI based BIOS, the EFI configuration table contains an entry for the > confidential computing blob that contains the required information. > > To support booting encrypted guests on non-EFI VM, the hypervisor needs to > pass this additional information to the kernel with a different method. > > For this purpose, introduce SETUP_CC_BLOB type in setup_data to hold the > physical address of the confidential computing blob location. The boot > loader or hypervisor may choose to use this method instead of EFI > configuration table. The CC blob location scanning should give preference > to setup_data data over the EFI configuration table. > > In AMD SEV-SNP, the CC blob contains the address of the secrets and CPUID > pages. The secrets page includes information such as a VM to PSP > communication key and CPUID page contains PSP filtered CPUID values. > Define the AMD SEV confidential computing blob structure. > > While at it, define the EFI GUID for the confidential computing blob. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> Reviewed-by: Venu Busireddy <venu.busireddy@oracle.com> > --- > arch/x86/include/asm/sev.h | 12 ++++++++++++ > arch/x86/include/uapi/asm/bootparam.h | 1 + > include/linux/efi.h | 1 + > 3 files changed, 14 insertions(+) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index f7cbd5164136..f42fbe3c332f 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -44,6 +44,18 @@ struct es_em_ctxt { > > void do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code); > > +/* AMD SEV Confidential computing blob structure */ > +#define CC_BLOB_SEV_HDR_MAGIC 0x45444d41 > +struct cc_blob_sev_info { > + u32 magic; > + u16 version; > + u16 reserved; > + u64 secrets_phys; > + u32 secrets_len; > + u64 cpuid_phys; > + u32 cpuid_len; > +}; > + > static inline u64 lower_bits(u64 val, unsigned int bits) > { > u64 mask = (1ULL << bits) - 1; > diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h > index b25d3f82c2f3..1ac5acca72ce 100644 > --- a/arch/x86/include/uapi/asm/bootparam.h > +++ b/arch/x86/include/uapi/asm/bootparam.h > @@ -10,6 +10,7 @@ > #define SETUP_EFI 4 > #define SETUP_APPLE_PROPERTIES 5 > #define SETUP_JAILHOUSE 6 > +#define SETUP_CC_BLOB 7 > > #define SETUP_INDIRECT (1<<31) > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index dbd39b20e034..a022aed7adb3 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -344,6 +344,7 @@ void efi_native_runtime_setup(void); > #define EFI_CERT_SHA256_GUID EFI_GUID(0xc1c41626, 0x504c, 0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28) > #define EFI_CERT_X509_GUID EFI_GUID(0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72) > #define EFI_CERT_X509_SHA256_GUID EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed) > +#define EFI_CC_BLOB_GUID EFI_GUID(0x067b1f5f, 0xcf26, 0x44c5, 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42) > > /* > * This GUID is used to pass to the kernel proper the struct screen_info > -- > 2.25.1 >
On Mon, Dec 13, 2021 at 07:08:07AM -0800, Dave Hansen wrote: > Could you please make the structure's size invariant? That's great if > there's no problem in today's implementation, but it's best no to leave > little land mines like this around. Let's say someone copies your code > as an example of something that interacts with a firmware table a few > years or months down the road. Btw, about that cc blob thing: is TDX going to need something like that too and if so, can they use it too?
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index f7cbd5164136..f42fbe3c332f 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -44,6 +44,18 @@ struct es_em_ctxt { void do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code); +/* AMD SEV Confidential computing blob structure */ +#define CC_BLOB_SEV_HDR_MAGIC 0x45444d41 +struct cc_blob_sev_info { + u32 magic; + u16 version; + u16 reserved; + u64 secrets_phys; + u32 secrets_len; + u64 cpuid_phys; + u32 cpuid_len; +}; + static inline u64 lower_bits(u64 val, unsigned int bits) { u64 mask = (1ULL << bits) - 1; diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index b25d3f82c2f3..1ac5acca72ce 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -10,6 +10,7 @@ #define SETUP_EFI 4 #define SETUP_APPLE_PROPERTIES 5 #define SETUP_JAILHOUSE 6 +#define SETUP_CC_BLOB 7 #define SETUP_INDIRECT (1<<31) diff --git a/include/linux/efi.h b/include/linux/efi.h index dbd39b20e034..a022aed7adb3 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -344,6 +344,7 @@ void efi_native_runtime_setup(void); #define EFI_CERT_SHA256_GUID EFI_GUID(0xc1c41626, 0x504c, 0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28) #define EFI_CERT_X509_GUID EFI_GUID(0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72) #define EFI_CERT_X509_SHA256_GUID EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed) +#define EFI_CC_BLOB_GUID EFI_GUID(0x067b1f5f, 0xcf26, 0x44c5, 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42) /* * This GUID is used to pass to the kernel proper the struct screen_info
While launching the encrypted guests, the hypervisor may need to provide some additional information during the guest boot. When booting under the EFI based BIOS, the EFI configuration table contains an entry for the confidential computing blob that contains the required information. To support booting encrypted guests on non-EFI VM, the hypervisor needs to pass this additional information to the kernel with a different method. For this purpose, introduce SETUP_CC_BLOB type in setup_data to hold the physical address of the confidential computing blob location. The boot loader or hypervisor may choose to use this method instead of EFI configuration table. The CC blob location scanning should give preference to setup_data data over the EFI configuration table. In AMD SEV-SNP, the CC blob contains the address of the secrets and CPUID pages. The secrets page includes information such as a VM to PSP communication key and CPUID page contains PSP filtered CPUID values. Define the AMD SEV confidential computing blob structure. While at it, define the EFI GUID for the confidential computing blob. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev.h | 12 ++++++++++++ arch/x86/include/uapi/asm/bootparam.h | 1 + include/linux/efi.h | 1 + 3 files changed, 14 insertions(+)