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
-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98088): https://edk2.groups.io/g/devel/message/98088
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]
-=-=-=-=-=-=-=-=-=-=-=-
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
>>
>>
>>
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98139): https://edk2.groups.io/g/devel/message/98139
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]
-=-=-=-=-=-=-=-=-=-=-=-
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
> >>
> >>
> >>
> >>
> >>
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98146): https://edk2.groups.io/g/devel/message/98146
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]
-=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98147): https://edk2.groups.io/g/devel/message/98147 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] -=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98354): https://edk2.groups.io/g/devel/message/98354 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] -=-=-=-=-=-=-=-=-=-=-=-
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
>
> );
>
> }
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97711): https://edk2.groups.io/g/devel/message/97711
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]
-=-=-=-=-=-=-=-=-=-=-=-
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
> );
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97692): https://edk2.groups.io/g/devel/message/97692
Mute This Topic: https://groups.io/mt/95809780/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.