Here are a number of fixes related to OVMF handling of the SEV-SNP Confidential Computing blob and CPUID table. Patch #1 is a fix for recently-reported issue that can cause significant problems with some SEV-SNP guest operating systems. Please consider applying this patch directly if the other patches in this series are held up for any reason. Patches 2-4 are minor changes for things that aren't currently triggered in practice, but make OVMF's SEV-SNP implementation more robust for different build/hypervisor environments in the future. Patch #2 was submitted previously, but refreshed here to apply cleanly on top of Patch #1, with no other functional changes since the initial review. ---------------------------------------------------------------- Michael Roth (4): OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP OvmfPkg/AmdSevDxe/AmdSevDxe.c | 64 ++++++++++++++++++++++++++++++++++---------- OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 +++-- OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 13 ++++----- 3 files changed, 59 insertions(+), 24 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97701): https://edk2.groups.io/g/devel/message/97701 Mute This Topic: https://groups.io/mt/95815539/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- The SEV-SNP Confidential Computing blob contains metadata that should remain accessible for the life of the guest. Allocate it as EfiACPIReclaimMemory to ensure the memory isn't overwritten by the guest operating system later. Reported-by: Dov Murik <dovmurik@linux.ibm.com> Suggested-by: Dov Murik <dovmurik@linux.ibm.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/AmdSevDxe/AmdSevDxe.c | 62 +++++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -XXX,XX +XXX,XX @@ #include <Guid/ConfidentialComputingSevSnpBlob.h> #include <Library/PcdLib.h> -STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = { - SIGNATURE_32 ('A', 'M', 'D', 'E'), - 1, - 0, - (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase), - FixedPcdGet32 (PcdOvmfSnpSecretsSize), - (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase), - FixedPcdGet32 (PcdOvmfCpuidSize), -}; +STATIC +EFI_STATUS +AllocateConfidentialComputingBlob ( + OUT CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION **CcBlobPtr + ) +{ + EFI_STATUS Status; + CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION *CcBlob; + + Status = gBS->AllocatePool ( + EfiACPIReclaimMemory, + sizeof (CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION), + (VOID **)&CcBlob + ); + if (EFI_ERROR (Status)) { + return Status; + } + + CcBlob->Header = SIGNATURE_32 ('A', 'M', 'D', 'E'); + CcBlob->Version = 1; + CcBlob->Reserved1 = 0; + CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase); + CcBlob->SecretsSize = FixedPcdGet32 (PcdOvmfSnpSecretsSize); + CcBlob->CpuidPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase); + CcBlob->CpuidLSize = FixedPcdGet32 (PcdOvmfCpuidSize); + + *CcBlobPtr = CcBlob; + + return EFI_SUCCESS; +} EFI_STATUS EFIAPI @@ -XXX,XX +XXX,XX @@ AmdSevDxeEntryPoint ( IN EFI_SYSTEM_TABLE *SystemTable ) { - EFI_STATUS Status; - EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; - UINTN NumEntries; - UINTN Index; + EFI_STATUS Status; + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; + UINTN NumEntries; + UINTN Index; + CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION *SnpBootDxeTable; // // Do nothing when SEV is not enabled @@ -XXX,XX +XXX,XX @@ AmdSevDxeEntryPoint ( } } + Status = AllocateConfidentialComputingBlob (&SnpBootDxeTable); + if (EFI_ERROR (Status)) { + DEBUG (( + DEBUG_ERROR, + "%a: AllocateConfidentialComputingBlob(): %r\n", + __FUNCTION__, + Status + )); + ASSERT (FALSE); + CpuDeadLoop (); + } + // // If its SEV-SNP active guest then install the CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB. // It contains the location for both the Secrets and CPUID page. @@ -XXX,XX +XXX,XX @@ AmdSevDxeEntryPoint ( if (MemEncryptSevSnpIsEnabled ()) { return gBS->InstallConfigurationTable ( &gConfidentialComputingSevSnpBlobGuid, - &mSnpBootDxeTable + SnpBootDxeTable ); } -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97702): https://edk2.groups.io/g/devel/message/97702 Mute This Topic: https://groups.io/mt/95815540/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- The Confidential Computing blob defined here is intended to match the definition defined by linux guest kernel. Previously, both definitions relied on natural alignment, but that relies on both OVMF and kernel being compiled as 64-bit. While there aren't currently any plans to enable SNP support for 32-bit compilations, the kernel definition has since been updated to use explicit padding/reserved fields to avoid this dependency. Update OVMF to match that definition. While at it, also fix up the Reserved fields to match the numbering used in the kernel. No functional changes (for currently-supported environments, at least). Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/AmdSevDxe/AmdSevDxe.c | 4 +++- OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -XXX,XX +XXX,XX @@ AllocateConfidentialComputingBlob ( CcBlob->Header = SIGNATURE_32 ('A', 'M', 'D', 'E'); CcBlob->Version = 1; - CcBlob->Reserved1 = 0; + CcBlob->Reserved = 0; CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase); CcBlob->SecretsSize = FixedPcdGet32 (PcdOvmfSnpSecretsSize); + CcBlob->Reserved1 = 0; CcBlob->CpuidPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase); CcBlob->CpuidLSize = FixedPcdGet32 (PcdOvmfCpuidSize); + CcBlob->Reserved2 = 0; *CcBlobPtr = CcBlob; diff --git a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h +++ b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h @@ -XXX,XX +XXX,XX @@ { 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42 }, \ } -typedef struct { +typedef PACKED struct { UINT32 Header; UINT16 Version; - UINT16 Reserved1; + UINT16 Reserved; UINT64 SecretsPhysicalAddress; UINT32 SecretsSize; + UINT32 Reserved1; UINT64 CpuidPhysicalAddress; UINT32 CpuidLSize; + UINT32 Reserved2; } CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION; extern EFI_GUID gConfidentialComputingSevSnpBlobGuid; -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97703): https://edk2.groups.io/g/devel/message/97703 Mute This Topic: https://groups.io/mt/95815541/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- CPUID leaf 0xD sub-leafs 0x0 and 0x1 contain cumulative sizes for the enabled XSave areas. Those sizes are calculated by tallying up all the other sub-leafs that contain per-area size information for XSave areas that are currently enabled in XCr0/XSS. The current check has the logic inverted. Fix that. This doesn't seem to cause problems currently, but could in the future if OVMF made more extensive use of XSave areas. It was noticed while implementing SNP-related tests for KVM Unit Tests, which re-uses the OVMF #VC handler in some cases. Reported-by: Pavan Kumar Paluri <papaluri@amd.com> Cc: Pavan Kumar Paluri <papaluri@amd.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c @@ -XXX,XX +XXX,XX @@ GetCpuidXSaveSize ( for (Idx = 0; Idx < CpuidInfo->Count; Idx++) { SEV_SNP_CPUID_FUNCTION *CpuidFn = &CpuidInfo->function[Idx]; - if (!((CpuidFn->EaxIn == 0xD) && - ((CpuidFn->EcxIn == 0) || (CpuidFn->EcxIn == 1)))) - { + if (!((CpuidFn->EaxIn == 0xD) && (CpuidFn->EcxIn > 1))) { continue; } -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97704): https://edk2.groups.io/g/devel/message/97704 Mute This Topic: https://groups.io/mt/95815542/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- Currently OVMF tries to rely on the base size advertised via the CPUID table entries corresponding to leaf 0xD, sub-leafs 0x0/0x1. This will generally work for KVM guests, but might not for other SEV-SNP hypervisor implementations. Make the handling more robust by simply using the base area size documented by the APM. Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c @@ -XXX,XX +XXX,XX @@ SnpEnabled ( @param[in] XFeaturesEnabled Bit-mask of enabled XSAVE features/areas as indicated by XCR0/MSR_IA32_XSS bits - @param[in] XSaveBaseSize Base/legacy XSAVE area size (e.g. when - XCR0 is 1) @param[in, out] XSaveSize Pointer to storage for calculated XSAVE area size @param[in] Compacted Whether or not the calculation is for the @@ -XXX,XX +XXX,XX @@ STATIC BOOLEAN GetCpuidXSaveSize ( IN UINT64 XFeaturesEnabled, - IN UINT32 XSaveBaseSize, IN OUT UINT32 *XSaveSize, IN BOOLEAN Compacted ) @@ -XXX,XX +XXX,XX @@ GetCpuidXSaveSize ( UINT64 XFeaturesFound = 0; UINT32 Idx; - *XSaveSize = XSaveBaseSize; + // + // The base/legacy XSave size is documented to be 0x240 in the APM. + // + *XSaveSize = 0x240; CpuidInfo = (SEV_SNP_CPUID_INFO *)(UINT64)PcdGet32 (PcdOvmfCpuidBase); for (Idx = 0; Idx < CpuidInfo->Count; Idx++) { @@ -XXX,XX +XXX,XX @@ GetCpuidFw ( if (!GetCpuidXSaveSize ( XCr0 | XssMsr.Uint64, - *Ebx, &XSaveSize, Compacted )) -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97705): https://edk2.groups.io/g/devel/message/97705 Mute This Topic: https://groups.io/mt/95815543/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- (Rebased series and resending due to merge conflict with previous submission.) Here are a number of fixes related to OVMF handling of the SEV-SNP Confidential Computing blob and CPUID table. Patch #1 is a fix for recently-reported issue that can cause significant problems with some SEV-SNP guest operating systems. Please consider applying this patch directly if the other patches in this series are held up for any reason. Patches 2-4 are minor changes for things that aren't currently triggered in practice, but make OVMF's SEV-SNP implementation more robust for different build/hypervisor environments in the future. Patch #2 was submitted previously, but refreshed here to apply cleanly on top of Patch #1, with no other functional changes since the initial review. ---------------------------------------------------------------- Michael Roth (4): OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP OvmfPkg/AmdSevDxe/AmdSevDxe.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------- OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 ++++-- OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 13 +++++-------- 3 files changed, 59 insertions(+), 24 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101244): https://edk2.groups.io/g/devel/message/101244 Mute This Topic: https://groups.io/mt/97638481/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- The SEV-SNP Confidential Computing blob contains metadata that should remain accessible for the life of the guest. Allocate it as EfiACPIReclaimMemory to ensure the memory isn't overwritten by the guest operating system later. Reported-by: Dov Murik <dovmurik@linux.ibm.com> Suggested-by: Dov Murik <dovmurik@linux.ibm.com> Reviewed-by: Dov Murik <dovmurik@linux.ibm.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/AmdSevDxe/AmdSevDxe.c | 62 +++++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -XXX,XX +XXX,XX @@ // Present, initialized, tested bits defined in MdeModulePkg/Core/Dxe/DxeMain.h #define EFI_MEMORY_INTERNAL_MASK 0x0700000000000000ULL -STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = { - SIGNATURE_32 ('A', 'M', 'D', 'E'), - 1, - 0, - (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase), - FixedPcdGet32 (PcdOvmfSnpSecretsSize), - (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase), - FixedPcdGet32 (PcdOvmfCpuidSize), -}; +STATIC +EFI_STATUS +AllocateConfidentialComputingBlob ( + OUT CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION **CcBlobPtr + ) +{ + EFI_STATUS Status; + CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION *CcBlob; + + Status = gBS->AllocatePool ( + EfiACPIReclaimMemory, + sizeof (CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION), + (VOID **)&CcBlob + ); + if (EFI_ERROR (Status)) { + return Status; + } + + CcBlob->Header = SIGNATURE_32 ('A', 'M', 'D', 'E'); + CcBlob->Version = 1; + CcBlob->Reserved1 = 0; + CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase); + CcBlob->SecretsSize = FixedPcdGet32 (PcdOvmfSnpSecretsSize); + CcBlob->CpuidPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase); + CcBlob->CpuidLSize = FixedPcdGet32 (PcdOvmfCpuidSize); + + *CcBlobPtr = CcBlob; + + return EFI_SUCCESS; +} STATIC EFI_HANDLE mAmdSevDxeHandle = NULL; @@ -XXX,XX +XXX,XX @@ AmdSevDxeEntryPoint ( IN EFI_SYSTEM_TABLE *SystemTable ) { - EFI_STATUS Status; - EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; - UINTN NumEntries; - UINTN Index; + EFI_STATUS Status; + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; + UINTN NumEntries; + UINTN Index; + CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION *SnpBootDxeTable; // // Do nothing when SEV is not enabled @@ -XXX,XX +XXX,XX @@ AmdSevDxeEntryPoint ( } } + Status = AllocateConfidentialComputingBlob (&SnpBootDxeTable); + if (EFI_ERROR (Status)) { + DEBUG (( + DEBUG_ERROR, + "%a: AllocateConfidentialComputingBlob(): %r\n", + __FUNCTION__, + Status + )); + ASSERT (FALSE); + CpuDeadLoop (); + } + if (MemEncryptSevSnpIsEnabled ()) { // // Memory acceptance began being required in SEV-SNP, so install the @@ -XXX,XX +XXX,XX @@ AmdSevDxeEntryPoint ( // return gBS->InstallConfigurationTable ( &gConfidentialComputingSevSnpBlobGuid, - &mSnpBootDxeTable + SnpBootDxeTable ); } -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101245): https://edk2.groups.io/g/devel/message/101245 Mute This Topic: https://groups.io/mt/97638492/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- The Confidential Computing blob defined here is intended to match the definition defined by linux guest kernel. Previously, both definitions relied on natural alignment, but that relies on both OVMF and kernel being compiled as 64-bit. While there aren't currently any plans to enable SNP support for 32-bit compilations, the kernel definition has since been updated to use explicit padding/reserved fields to avoid this dependency. Update OVMF to match that definition. While at it, also fix up the Reserved fields to match the numbering used in the kernel. No functional changes (for currently-supported environments, at least). Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Acked-by: Jiewen Yao <jiewen.yao@intel.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/AmdSevDxe/AmdSevDxe.c | 4 +++- OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c @@ -XXX,XX +XXX,XX @@ AllocateConfidentialComputingBlob ( CcBlob->Header = SIGNATURE_32 ('A', 'M', 'D', 'E'); CcBlob->Version = 1; - CcBlob->Reserved1 = 0; + CcBlob->Reserved = 0; CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase); CcBlob->SecretsSize = FixedPcdGet32 (PcdOvmfSnpSecretsSize); + CcBlob->Reserved1 = 0; CcBlob->CpuidPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase); CcBlob->CpuidLSize = FixedPcdGet32 (PcdOvmfCpuidSize); + CcBlob->Reserved2 = 0; *CcBlobPtr = CcBlob; diff --git a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h +++ b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h @@ -XXX,XX +XXX,XX @@ { 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42 }, \ } -typedef struct { +typedef PACKED struct { UINT32 Header; UINT16 Version; - UINT16 Reserved1; + UINT16 Reserved; UINT64 SecretsPhysicalAddress; UINT32 SecretsSize; + UINT32 Reserved1; UINT64 CpuidPhysicalAddress; UINT32 CpuidLSize; + UINT32 Reserved2; } CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION; extern EFI_GUID gConfidentialComputingSevSnpBlobGuid; -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101246): https://edk2.groups.io/g/devel/message/101246 Mute This Topic: https://groups.io/mt/97638497/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- CPUID leaf 0xD sub-leafs 0x0 and 0x1 contain cumulative sizes for the enabled XSave areas. Those sizes are calculated by tallying up all the other sub-leafs that contain per-area size information for XSave areas that are currently enabled in XCr0/XSS. The current check has the logic inverted. Fix that. This doesn't seem to cause problems currently, but could in the future if OVMF made more extensive use of XSave areas. It was noticed while implementing SNP-related tests for KVM Unit Tests, which re-uses the OVMF #VC handler in some cases. Reported-by: Pavan Kumar Paluri <papaluri@amd.com> Cc: Pavan Kumar Paluri <papaluri@amd.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Acked-by: Jiewen Yao <jiewen.yao@intel.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c @@ -XXX,XX +XXX,XX @@ GetCpuidXSaveSize ( for (Idx = 0; Idx < CpuidInfo->Count; Idx++) { SEV_SNP_CPUID_FUNCTION *CpuidFn = &CpuidInfo->function[Idx]; - if (!((CpuidFn->EaxIn == 0xD) && - ((CpuidFn->EcxIn == 0) || (CpuidFn->EcxIn == 1)))) - { + if (!((CpuidFn->EaxIn == 0xD) && (CpuidFn->EcxIn > 1))) { continue; } -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101247): https://edk2.groups.io/g/devel/message/101247 Mute This Topic: https://groups.io/mt/97638507/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- Currently OVMF tries to rely on the base size advertised via the CPUID table entries corresponding to leaf 0xD, sub-leafs 0x0/0x1. This will generally work for KVM guests, but might not for other SEV-SNP hypervisor implementations. Make the handling more robust by simply using the base area size documented by the APM. Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Acked-by: Jiewen Yao <jiewen.yao@intel.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c index XXXXXXX..XXXXXXX 100644 --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c @@ -XXX,XX +XXX,XX @@ SnpEnabled ( @param[in] XFeaturesEnabled Bit-mask of enabled XSAVE features/areas as indicated by XCR0/MSR_IA32_XSS bits - @param[in] XSaveBaseSize Base/legacy XSAVE area size (e.g. when - XCR0 is 1) @param[in, out] XSaveSize Pointer to storage for calculated XSAVE area size @param[in] Compacted Whether or not the calculation is for the @@ -XXX,XX +XXX,XX @@ STATIC BOOLEAN GetCpuidXSaveSize ( IN UINT64 XFeaturesEnabled, - IN UINT32 XSaveBaseSize, IN OUT UINT32 *XSaveSize, IN BOOLEAN Compacted ) @@ -XXX,XX +XXX,XX @@ GetCpuidXSaveSize ( UINT64 XFeaturesFound = 0; UINT32 Idx; - *XSaveSize = XSaveBaseSize; + // + // The base/legacy XSave size is documented to be 0x240 in the APM. + // + *XSaveSize = 0x240; CpuidInfo = (SEV_SNP_CPUID_INFO *)(UINT64)PcdGet32 (PcdOvmfCpuidBase); for (Idx = 0; Idx < CpuidInfo->Count; Idx++) { @@ -XXX,XX +XXX,XX @@ GetCpuidFw ( if (!GetCpuidXSaveSize ( XCr0 | XssMsr.Uint64, - *Ebx, &XSaveSize, Compacted )) -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101248): https://edk2.groups.io/g/devel/message/101248 Mute This Topic: https://groups.io/mt/97638519/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-