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.lenda...@amd.com>; Ni, Ray >> <ray...@intel.com>; Dov Murik <dovmu...@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 <dovmu...@linux.ibm.com> >> Suggested-by: Dov Murik <dovmu...@linux.ibm.com> >> Signed-off-by: Michael Roth <michael.r...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
