* [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2022-12-21 16:06 [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
@ 2022-12-21 16:06 ` Roth, Michael
2022-12-21 16:48 ` Lendacky, Thomas
` (2 more replies)
2022-12-21 16:06 ` [PATCH 2/4] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition Roth, Michael
` (3 subsequent siblings)
4 siblings, 3 replies; 19+ messages in thread
From: Roth, Michael @ 2022-12-21 16:06 UTC (permalink / raw)
To: devel; +Cc: Tom Lendacky, ray.ni, Dov Murik
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 662d3c4ccb..8dfda961d7 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -21,15 +21,36 @@
#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
@@ -38,10 +59,11 @@ 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
@@ -147,6 +169,18 @@ 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.
@@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
if (MemEncryptSevSnpIsEnabled ()) {
return gBS->InstallConfigurationTable (
&gConfidentialComputingSevSnpBlobGuid,
- &mSnpBootDxeTable
+ SnpBootDxeTable
);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2022-12-21 16:06 ` [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory Roth, Michael
@ 2022-12-21 16:48 ` Lendacky, Thomas
2022-12-21 21:26 ` Dov Murik
2023-01-06 9:18 ` [edk2-devel] " Yao, Jiewen
2 siblings, 0 replies; 19+ messages in thread
From: Lendacky, Thomas @ 2022-12-21 16:48 UTC (permalink / raw)
To: Michael Roth, devel; +Cc: ray.ni, Dov Murik
On 12/21/22 10:06, Michael Roth wrote:
> 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>
Reviewed-by: Tom Lendacky <thomas.lendacky@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 662d3c4ccb..8dfda961d7 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -21,15 +21,36 @@
> #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
> @@ -38,10 +59,11 @@ 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
> @@ -147,6 +169,18 @@ 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.
> @@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
> if (MemEncryptSevSnpIsEnabled ()) {
> return gBS->InstallConfigurationTable (
> &gConfidentialComputingSevSnpBlobGuid,
> - &mSnpBootDxeTable
> + SnpBootDxeTable
> );
> }
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2022-12-21 16:06 ` [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory Roth, Michael
2022-12-21 16:48 ` Lendacky, Thomas
@ 2022-12-21 21:26 ` Dov Murik
2023-01-06 9:18 ` [edk2-devel] " Yao, Jiewen
2 siblings, 0 replies; 19+ messages in thread
From: Dov Murik @ 2022-12-21 21:26 UTC (permalink / raw)
To: Michael Roth, devel; +Cc: Tom Lendacky, ray.ni, Dov Murik
Thanks Mike for fixing this.
On 21/12/2022 18:06, Michael Roth wrote:
> 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>
Reviewed-by: Dov Murik <dovmurik@linux.ibm.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 662d3c4ccb..8dfda961d7 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -21,15 +21,36 @@
> #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
>
> @@ -38,10 +59,11 @@ 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
>
> @@ -147,6 +169,18 @@ 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.
>
> @@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
> if (MemEncryptSevSnpIsEnabled ()) {
>
> return gBS->InstallConfigurationTable (
>
> &gConfidentialComputingSevSnpBlobGuid,
>
> - &mSnpBootDxeTable
>
> + SnpBootDxeTable
>
> );
>
> }
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2022-12-21 16:06 ` [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory Roth, Michael
2022-12-21 16:48 ` Lendacky, Thomas
2022-12-21 21:26 ` Dov Murik
@ 2023-01-06 9:18 ` Yao, Jiewen
2023-01-06 20:25 ` Dov Murik
2 siblings, 1 reply; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-06 9:18 UTC (permalink / raw)
To: devel@edk2.groups.io, Michael.Roth@amd.com
Cc: Tom Lendacky, Ni, Ray, Dov Murik
Are you sure you want to use EfiACPIReclaimMemory ?
Usually EfiACPIReclaimMemory is only for ACPI table, which can be reclaimed and used by OS, after copy ACPI table.
If you want to claim the memory owned by firmware (not owned by OS), you need use ACPINvs or reserved.
Although I don't fully understand SEV, this seems suspicious.
Please double confirm if this is really you want.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Roth,
> Michael via groups.io
> Sent: Thursday, December 22, 2022 12:07 AM
> To: devel@edk2.groups.io
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray
> <ray.ni@intel.com>; Dov Murik <dovmurik@linux.ibm.com>
> Subject: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC
> blob as EfiACPIReclaimMemory
>
> 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 662d3c4ccb..8dfda961d7 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -21,15 +21,36 @@
> #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
>
> @@ -38,10 +59,11 @@ 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
>
> @@ -147,6 +169,18 @@ 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.
>
> @@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
> if (MemEncryptSevSnpIsEnabled ()) {
>
> return gBS->InstallConfigurationTable (
>
> &gConfidentialComputingSevSnpBlobGuid,
>
> - &mSnpBootDxeTable
>
> + SnpBootDxeTable
>
> );
>
> }
>
>
>
> --
> 2.25.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2023-01-06 9:18 ` [edk2-devel] " Yao, Jiewen
@ 2023-01-06 20:25 ` Dov Murik
2023-01-07 2:01 ` Yao, Jiewen
0 siblings, 1 reply; 19+ messages in thread
From: Dov Murik @ 2023-01-06 20:25 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io, Michael.Roth@amd.com,
Ard Biesheuvel
Cc: Tom Lendacky, Ni, Ray, Dov Murik
Hi Jiewen,
On 06/01/2023 11:18, Yao, Jiewen wrote:
> Are you sure you want to use EfiACPIReclaimMemory ?
>
> Usually EfiACPIReclaimMemory is only for ACPI table, which can be reclaimed and used by OS, after copy ACPI table.
>
> If you want to claim the memory owned by firmware (not owned by OS), you need use ACPINvs or reserved.
>
EfiACPIReclaimMemory type was suggested by Ard [1] for a similar fix
another SEV-related memory area that should remain in-place throughout
the guest OS lifetime (not reused by OS).
Ard -- can you please explain that choice?
[1] https://edk2.groups.io/g/devel/message/97154
-Dov
>
> Although I don't fully understand SEV, this seems suspicious.
>
> Please double confirm if this is really you want.
>
> Thank you
> Yao, Jiewen
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Roth,
>> Michael via groups.io
>> Sent: Thursday, December 22, 2022 12:07 AM
>> To: devel@edk2.groups.io
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray
>> <ray.ni@intel.com>; Dov Murik <dovmurik@linux.ibm.com>
>> Subject: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC
>> blob as EfiACPIReclaimMemory
>>
>> 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 662d3c4ccb..8dfda961d7 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> @@ -21,15 +21,36 @@
>> #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
>>
>> @@ -38,10 +59,11 @@ 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
>>
>> @@ -147,6 +169,18 @@ 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.
>>
>> @@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
>> if (MemEncryptSevSnpIsEnabled ()) {
>>
>> return gBS->InstallConfigurationTable (
>>
>> &gConfidentialComputingSevSnpBlobGuid,
>>
>> - &mSnpBootDxeTable
>>
>> + SnpBootDxeTable
>>
>> );
>>
>> }
>>
>>
>>
>> --
>> 2.25.1
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2023-01-06 20:25 ` Dov Murik
@ 2023-01-07 2:01 ` Yao, Jiewen
2023-01-07 16:52 ` Ard Biesheuvel
0 siblings, 1 reply; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-07 2:01 UTC (permalink / raw)
To: Dov Murik, devel@edk2.groups.io, Michael.Roth@amd.com,
Ard Biesheuvel
Cc: Tom Lendacky, Ni, Ray
Hi Dov/Ard
Please allow me to clarify:
EfiACPIReclaimMemory in UEFI spec means: OS may use the memory, after it copies the ACPI table to its own location. It is also called "AddressRangeACPI" in ACPI spec.
[2] https://uefi.org/specs/ACPI/6.5/15_System_Address_Map_Interfaces.html, search AddressRangeACPI.
[3] https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html, search EfiACPIReclaimMemory.
However, in the description, you mentioned "The SEV-SNP Confidential Computing blob contains metadata that should remain accessible for the life of the guest."
That requirement conflicts with the definition of ACPIReclaim memory.
I would like to suggest either of below, to meet the need "that should remain accessible for the life of the guest."
a) EfiACPIMemoryNVS in UEFI, also known as AddressRangeNVS in ACPI (or)
b) EfiReservedMemoryType in UEFI, also knowns as AddressRangeReserved in ACPI.
Please double confirm that.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: Dov Murik <dovmurik@linux.ibm.com>
> Sent: Saturday, January 7, 2023 4:26 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> Michael.Roth@amd.com; Ard Biesheuvel <ardb@kernel.org>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray
> <ray.ni@intel.com>; Dov Murik <dovmurik@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-
> SNP CC blob as EfiACPIReclaimMemory
>
> Hi Jiewen,
>
> On 06/01/2023 11:18, Yao, Jiewen wrote:
> > Are you sure you want to use EfiACPIReclaimMemory ?
> >
> > Usually EfiACPIReclaimMemory is only for ACPI table, which can be
> reclaimed and used by OS, after copy ACPI table.
> >
> > If you want to claim the memory owned by firmware (not owned by OS),
> you need use ACPINvs or reserved.
> >
>
> EfiACPIReclaimMemory type was suggested by Ard [1] for a similar fix
> another SEV-related memory area that should remain in-place throughout
> the guest OS lifetime (not reused by OS).
>
> Ard -- can you please explain that choice?
>
>
> [1] https://edk2.groups.io/g/devel/message/97154
>
>
>
> -Dov
>
>
> >
> > Although I don't fully understand SEV, this seems suspicious.
> >
> > Please double confirm if this is really you want.
> >
> > Thank you
> > Yao, Jiewen
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Roth,
> >> Michael via groups.io
> >> Sent: Thursday, December 22, 2022 12:07 AM
> >> To: devel@edk2.groups.io
> >> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray
> >> <ray.ni@intel.com>; Dov Murik <dovmurik@linux.ibm.com>
> >> Subject: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP
> CC
> >> blob as EfiACPIReclaimMemory
> >>
> >> 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 662d3c4ccb..8dfda961d7 100644
> >> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> >> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> >> @@ -21,15 +21,36 @@
> >> #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
> >>
> >> @@ -38,10 +59,11 @@ 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
> >>
> >> @@ -147,6 +169,18 @@ 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.
> >>
> >> @@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
> >> if (MemEncryptSevSnpIsEnabled ()) {
> >>
> >> return gBS->InstallConfigurationTable (
> >>
> >> &gConfidentialComputingSevSnpBlobGuid,
> >>
> >> - &mSnpBootDxeTable
> >>
> >> + SnpBootDxeTable
> >>
> >> );
> >>
> >> }
> >>
> >>
> >>
> >> --
> >> 2.25.1
> >>
> >>
> >>
> >>
> >>
> >
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2023-01-07 2:01 ` Yao, Jiewen
@ 2023-01-07 16:52 ` Ard Biesheuvel
2023-01-12 10:15 ` Yao, Jiewen
0 siblings, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2023-01-07 16:52 UTC (permalink / raw)
To: Yao, Jiewen
Cc: Dov Murik, devel@edk2.groups.io, Michael.Roth@amd.com,
Tom Lendacky, Ni, Ray
On Sat, 7 Jan 2023 at 03:01, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
> Hi Dov/Ard
> Please allow me to clarify:
>
> EfiACPIReclaimMemory in UEFI spec means: OS may use the memory, after it copies the ACPI table to its own location. It is also called "AddressRangeACPI" in ACPI spec.
>
> [2] https://uefi.org/specs/ACPI/6.5/15_System_Address_Map_Interfaces.html, search AddressRangeACPI.
> [3] https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html, search EfiACPIReclaimMemory.
>
> However, in the description, you mentioned "The SEV-SNP Confidential Computing blob contains metadata that should remain accessible for the life of the guest."
> That requirement conflicts with the definition of ACPIReclaim memory.
>
> I would like to suggest either of below, to meet the need "that should remain accessible for the life of the guest."
> a) EfiACPIMemoryNVS in UEFI, also known as AddressRangeNVS in ACPI (or)
> b) EfiReservedMemoryType in UEFI, also knowns as AddressRangeReserved in ACPI.
>
> Please double confirm that.
>
If EfiACPIMemoryNVS is considered more appropriate, I don't have any
issues with that.
But the patch is correct in the sense that it should not use
statically allocated objects. And EfiRuntimeServicesData should be
avoided as well (athough it is often used for cases like this) as it
will end up getting mapped into the firmware page tables for no
reason.
EfiReservedMemory is not suitable for this - Linux on ARM must omit
this from all its mappings of system memory, because the OS does not
know *why* it is reserved and with which attributes it is being
mapped, and the architecture does not tolerate duplicate mappings with
mismatched attributes.
The semantics of EfiAcpiReclaimMemory are also suitable for cases
where the contents of the region is only relevant to the OS, and not
to the firmware itself, and it is really up to the OS to decided
whether or not it will reclaim the region in question. So for passing
tables in memory that are similar in purpose to ACPI tables (i.e.,
describe the platform to the OS), EfiAcpiReclaimMemory is suitable
imho.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
2023-01-07 16:52 ` Ard Biesheuvel
@ 2023-01-12 10:15 ` Yao, Jiewen
0 siblings, 0 replies; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-12 10:15 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Dov Murik, devel@edk2.groups.io, Michael.Roth@amd.com,
Tom Lendacky, Ni, Ray
Never mind. I don’t have concern for SEV code.
You may merge it, if you think it is OK.
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Sunday, January 8, 2023 12:53 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io;
> Michael.Roth@amd.com; Tom Lendacky <thomas.lendacky@amd.com>; Ni,
> Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-
> SNP CC blob as EfiACPIReclaimMemory
>
> On Sat, 7 Jan 2023 at 03:01, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> > Hi Dov/Ard
> > Please allow me to clarify:
> >
> > EfiACPIReclaimMemory in UEFI spec means: OS may use the memory, after
> it copies the ACPI table to its own location. It is also called
> "AddressRangeACPI" in ACPI spec.
> >
> > [2]
> https://uefi.org/specs/ACPI/6.5/15_System_Address_Map_Interfaces.html,
> search AddressRangeACPI.
> > [3] https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html,
> search EfiACPIReclaimMemory.
> >
> > However, in the description, you mentioned "The SEV-SNP Confidential
> Computing blob contains metadata that should remain accessible for the life
> of the guest."
> > That requirement conflicts with the definition of ACPIReclaim memory.
> >
> > I would like to suggest either of below, to meet the need "that should
> remain accessible for the life of the guest."
> > a) EfiACPIMemoryNVS in UEFI, also known as AddressRangeNVS in ACPI (or)
> > b) EfiReservedMemoryType in UEFI, also knowns as AddressRangeReserved
> in ACPI.
> >
> > Please double confirm that.
> >
>
> If EfiACPIMemoryNVS is considered more appropriate, I don't have any
> issues with that.
>
> But the patch is correct in the sense that it should not use
> statically allocated objects. And EfiRuntimeServicesData should be
> avoided as well (athough it is often used for cases like this) as it
> will end up getting mapped into the firmware page tables for no
> reason.
>
> EfiReservedMemory is not suitable for this - Linux on ARM must omit
> this from all its mappings of system memory, because the OS does not
> know *why* it is reserved and with which attributes it is being
> mapped, and the architecture does not tolerate duplicate mappings with
> mismatched attributes.
>
> The semantics of EfiAcpiReclaimMemory are also suitable for cases
> where the contents of the region is only relevant to the OS, and not
> to the firmware itself, and it is really up to the OS to decided
> whether or not it will reclaim the region in question. So for passing
> tables in memory that are similar in purpose to ACPI tables (i.e.,
> describe the platform to the OS), EfiAcpiReclaimMemory is suitable
> imho.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition
2022-12-21 16:06 [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
2022-12-21 16:06 ` [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory Roth, Michael
@ 2022-12-21 16:06 ` Roth, Michael
2023-01-06 9:14 ` [edk2-devel] " Yao, Jiewen
2022-12-21 16:06 ` [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation Roth, Michael
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Roth, Michael @ 2022-12-21 16:06 UTC (permalink / raw)
To: devel; +Cc: Tom Lendacky, ray.ni
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 8dfda961d7..00bb6e5d96 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -41,11 +41,13 @@ 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 b328310fd0..83620e31b8 100644
--- a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
+++ b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
@@ -18,14 +18,16 @@
{ 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 2/4] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition
2022-12-21 16:06 ` [PATCH 2/4] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition Roth, Michael
@ 2023-01-06 9:14 ` Yao, Jiewen
0 siblings, 0 replies; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-06 9:14 UTC (permalink / raw)
To: devel@edk2.groups.io, Michael.Roth@amd.com; +Cc: Tom Lendacky, Ni, Ray
Acked-by: Jiewen Yao <jiewen.yao@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Roth,
> Michael via groups.io
> Sent: Thursday, December 22, 2022 12:07 AM
> To: devel@edk2.groups.io
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 2/4] OvmfPkg/AmdSevDxe: Update
> ConfidentialComputing blob struct definition
>
> 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 8dfda961d7..00bb6e5d96 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -41,11 +41,13 @@ 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 b328310fd0..83620e31b8 100644
> --- a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
> +++ b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
> @@ -18,14 +18,16 @@
> { 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
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation
2022-12-21 16:06 [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
2022-12-21 16:06 ` [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory Roth, Michael
2022-12-21 16:06 ` [PATCH 2/4] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition Roth, Michael
@ 2022-12-21 16:06 ` Roth, Michael
2022-12-21 16:52 ` Lendacky, Thomas
2023-01-06 8:53 ` [edk2-devel] " Yao, Jiewen
2022-12-21 16:06 ` [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP Roth, Michael
2022-12-21 17:41 ` [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
4 siblings, 2 replies; 19+ messages in thread
From: Roth, Michael @ 2022-12-21 16:06 UTC (permalink / raw)
To: devel; +Cc: Tom Lendacky, ray.ni, Pavan Kumar Paluri
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 985e547977..cd117d5a31 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
@@ -1678,9 +1678,7 @@ 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation
2022-12-21 16:06 ` [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation Roth, Michael
@ 2022-12-21 16:52 ` Lendacky, Thomas
2023-01-06 8:53 ` [edk2-devel] " Yao, Jiewen
1 sibling, 0 replies; 19+ messages in thread
From: Lendacky, Thomas @ 2022-12-21 16:52 UTC (permalink / raw)
To: Michael Roth, devel; +Cc: ray.ni, Pavan Kumar Paluri
On 12/21/22 10:06, Michael Roth wrote:
> 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>
Reviewed-by: Tom Lendacky <thomas.lendacky@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 985e547977..cd117d5a31 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -1678,9 +1678,7 @@ 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;
> }
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation
2022-12-21 16:06 ` [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation Roth, Michael
2022-12-21 16:52 ` Lendacky, Thomas
@ 2023-01-06 8:53 ` Yao, Jiewen
1 sibling, 0 replies; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-06 8:53 UTC (permalink / raw)
To: devel@edk2.groups.io, Michael.Roth@amd.com
Cc: Tom Lendacky, Ni, Ray, Pavan Kumar Paluri
Acked-by: Jiewen Yao <jiewen.yao@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Roth,
> Michael via groups.io
> Sent: Thursday, December 22, 2022 12:07 AM
> To: devel@edk2.groups.io
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray
> <ray.ni@intel.com>; Pavan Kumar Paluri <papaluri@amd.com>
> Subject: [edk2-devel] [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area
> size calculation
>
> 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 985e547977..cd117d5a31 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -1678,9 +1678,7 @@ 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
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP
2022-12-21 16:06 [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
` (2 preceding siblings ...)
2022-12-21 16:06 ` [PATCH 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation Roth, Michael
@ 2022-12-21 16:06 ` Roth, Michael
2022-12-21 16:59 ` Lendacky, Thomas
2023-01-06 8:53 ` [edk2-devel] " Yao, Jiewen
2022-12-21 17:41 ` [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
4 siblings, 2 replies; 19+ messages in thread
From: Roth, Michael @ 2022-12-21 16:06 UTC (permalink / raw)
To: devel; +Cc: Tom Lendacky, ray.ni
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 cd117d5a31..f985dcff8d 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
@@ -1647,8 +1647,6 @@ 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
@@ -1663,7 +1661,6 @@ STATIC
BOOLEAN
GetCpuidXSaveSize (
IN UINT64 XFeaturesEnabled,
- IN UINT32 XSaveBaseSize,
IN OUT UINT32 *XSaveSize,
IN BOOLEAN Compacted
)
@@ -1672,7 +1669,10 @@ 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++) {
@@ -1888,7 +1888,6 @@ GetCpuidFw (
if (!GetCpuidXSaveSize (
XCr0 | XssMsr.Uint64,
- *Ebx,
&XSaveSize,
Compacted
))
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP
2022-12-21 16:06 ` [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP Roth, Michael
@ 2022-12-21 16:59 ` Lendacky, Thomas
2023-01-06 8:53 ` [edk2-devel] " Yao, Jiewen
1 sibling, 0 replies; 19+ messages in thread
From: Lendacky, Thomas @ 2022-12-21 16:59 UTC (permalink / raw)
To: Michael Roth, devel; +Cc: ray.ni
On 12/21/22 10:06, Michael Roth wrote:
> 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>
Reviewed-by: Tom Lendacky <thomas.lendacky@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 cd117d5a31..f985dcff8d 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -1647,8 +1647,6 @@ 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
> @@ -1663,7 +1661,6 @@ STATIC
> BOOLEAN
> GetCpuidXSaveSize (
> IN UINT64 XFeaturesEnabled,
> - IN UINT32 XSaveBaseSize,
> IN OUT UINT32 *XSaveSize,
> IN BOOLEAN Compacted
> )
> @@ -1672,7 +1669,10 @@ 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++) {
> @@ -1888,7 +1888,6 @@ GetCpuidFw (
>
> if (!GetCpuidXSaveSize (
> XCr0 | XssMsr.Uint64,
> - *Ebx,
> &XSaveSize,
> Compacted
> ))
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP
2022-12-21 16:06 ` [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP Roth, Michael
2022-12-21 16:59 ` Lendacky, Thomas
@ 2023-01-06 8:53 ` Yao, Jiewen
1 sibling, 0 replies; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-06 8:53 UTC (permalink / raw)
To: devel@edk2.groups.io, Michael.Roth@amd.com; +Cc: Tom Lendacky, Ni, Ray
Acked-by: Jiewen Yao <jiewen.yao@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Roth,
> Michael via groups.io
> Sent: Thursday, December 22, 2022 12:07 AM
> To: devel@edk2.groups.io
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 4/4] OvmfPkg/CcExitLib: Use documented
> XSave area base size for SEV-SNP
>
> 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 cd117d5a31..f985dcff8d 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -1647,8 +1647,6 @@ 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
>
> @@ -1663,7 +1661,6 @@ STATIC
> BOOLEAN
>
> GetCpuidXSaveSize (
>
> IN UINT64 XFeaturesEnabled,
>
> - IN UINT32 XSaveBaseSize,
>
> IN OUT UINT32 *XSaveSize,
>
> IN BOOLEAN Compacted
>
> )
>
> @@ -1672,7 +1669,10 @@ 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++) {
>
> @@ -1888,7 +1888,6 @@ GetCpuidFw (
>
>
> if (!GetCpuidXSaveSize (
>
> XCr0 | XssMsr.Uint64,
>
> - *Ebx,
>
> &XSaveSize,
>
> Compacted
>
> ))
>
> --
> 2.25.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling
2022-12-21 16:06 [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
` (3 preceding siblings ...)
2022-12-21 16:06 ` [PATCH 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP Roth, Michael
@ 2022-12-21 17:41 ` Roth, Michael
2023-01-18 3:57 ` Yao, Jiewen
4 siblings, 1 reply; 19+ messages in thread
From: Roth, Michael @ 2022-12-21 17:41 UTC (permalink / raw)
To: devel
Cc: Tom Lendacky, ray.ni, Ard Biesheuvel, Jiewen Yao, Gerd Hoffmann,
Erdem Aktas, James Bottomley, Min Xu
On Wed, Dec 21, 2022 at 10:06:47AM -0600, Michael Roth wrote:
> 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
Adding some Cc's from Maintainers.txt that I should have included
originally:
Ard Biesheuvel <ardb+tianocore@kernel.org>
Jiewen Yao <jiewen.yao@intel.com>
Gerd Hoffmann <kraxel@redhat.com>
Erdem Aktas <erdemaktas@google.com>
James Bottomley <jejb@linux.ibm.com>
Min Xu <min.m.xu@intel.com>
Thanks,
Mike
>
> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 64 ++++++++++++++++++++++++++++++++++----------
> OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 +++--
> OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 13 ++++-----
> 3 files changed, 59 insertions(+), 24 deletions(-)
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling
2022-12-21 17:41 ` [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling Roth, Michael
@ 2023-01-18 3:57 ` Yao, Jiewen
0 siblings, 0 replies; 19+ messages in thread
From: Yao, Jiewen @ 2023-01-18 3:57 UTC (permalink / raw)
To: Michael Roth, devel@edk2.groups.io
Cc: Tom Lendacky, Ni, Ray, Ard Biesheuvel, Gerd Hoffmann,
Aktas, Erdem, James Bottomley, Xu, Min M
Hi Roth
I got weird merge conflict when I try to apply the patches from email.
Would you please resubmit the patch based on latest code base?
Once I see the new version, I will try to merge them again.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: Michael Roth <michael.roth@amd.com>
> Sent: Thursday, December 22, 2022 1:42 AM
> To: devel@edk2.groups.io
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray
> <ray.ni@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao,
> Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>
> Subject: Re: [PATCH 0/4] Fixes for SEV-SNP CC blob and CPUID table handling
>
> On Wed, Dec 21, 2022 at 10:06:47AM -0600, Michael Roth wrote:
> > 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
>
> Adding some Cc's from Maintainers.txt that I should have included
> originally:
>
> Ard Biesheuvel <ardb+tianocore@kernel.org>
> Jiewen Yao <jiewen.yao@intel.com>
> Gerd Hoffmann <kraxel@redhat.com>
> Erdem Aktas <erdemaktas@google.com>
> James Bottomley <jejb@linux.ibm.com>
> Min Xu <min.m.xu@intel.com>
>
> Thanks,
>
> Mike
>
> >
> > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 64
> ++++++++++++++++++++++++++++++++++----------
> > OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 +++--
> > OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 13 ++++-----
> > 3 files changed, 59 insertions(+), 24 deletions(-)
> >
> >
^ permalink raw reply [flat|nested] 19+ messages in thread