Re: [edk2-devel] [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory

["Dov Murik" <dovmurik@linux.ibm.com>]

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
>>
>>
>>
>> 
>>
>