[PATCH v9 30/32] OvmfPkg/PlatformPei: mark cpuid and secrets memory reserved in EFI map


Brijesh Singh
 

When SEV-SNP is active, the CPUID and Secrets memory range contains the
information that is used during the VM boot. The content need to be persist
across the kexec boot. Mark the memory range as Reserved in the EFI map
so that guest OS or firmware does not use the range as a system RAM.

Cc: Michael Roth <michael.roth@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 4 ++++
OvmfPkg/PlatformPei/Platform.h | 5 +++++
OvmfPkg/PlatformPei/AmdSev.c | 31 +++++++++++++++++++++++++++++
OvmfPkg/PlatformPei/MemDetect.c | 2 ++
4 files changed, 42 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index d048b692f155..0a89e1a0760e 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -112,6 +112,8 @@ [Pcd]


[FixedPcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
@@ -122,6 +124,8 @@ [FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize

[FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable
diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 8b1d270c2b0b..4169019b4c07 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -102,6 +102,11 @@ AmdSevInitialize (
VOID
);

+VOID
+SevInitializeRam (
+ VOID
+ );
+
extern EFI_BOOT_MODE mBootMode;

extern BOOLEAN mS3Supported;
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index b71a4a7304f7..133382407bc5 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -414,3 +414,34 @@ AmdSevInitialize (
ASSERT_RETURN_ERROR (PcdStatus);

}
+
+/**
+ The function performs SEV specific region initialization.
+
+ **/
+VOID
+SevInitializeRam (
+ VOID
+ )
+{
+ if (MemEncryptSevSnpIsEnabled ()) {
+ //
+ // If SEV-SNP is enabled, reserve the Secrets and CPUID memory area.
+ //
+ // This memory range is given to the PSP by the hypervisor to populate
+ // the information used during the SNP VM boots, and it need to persist
+ // across the kexec boots. Mark it as EfiReservedMemoryType so that
+ // the guest firmware and OS does not use it as a system memory.
+ //
+ BuildMemoryAllocationHob (
+ (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfSnpSecretsBase),
+ (UINT64)(UINTN) PcdGet32 (PcdOvmfSnpSecretsSize),
+ EfiReservedMemoryType
+ );
+ BuildMemoryAllocationHob (
+ (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfCpuidBase),
+ (UINT64)(UINTN) PcdGet32 (PcdOvmfCpuidSize),
+ EfiReservedMemoryType
+ );
+ }
+}
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index d736b85e0d90..058bb394f0df 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -821,6 +821,8 @@ InitializeRamRegions (
{
QemuInitializeRam ();

+ SevInitializeRam ();
+
if (mS3Supported && mBootMode != BOOT_ON_S3_RESUME) {
//
// This is the memory range that will be used for PEI on S3 resume
--
2.25.1


Gerd Hoffmann
 

On Wed, Oct 13, 2021 at 11:57:11AM -0500, Brijesh Singh wrote:
When SEV-SNP is active, the CPUID and Secrets memory range contains the
information that is used during the VM boot. The content need to be persist
across the kexec boot. Mark the memory range as Reserved in the EFI map
so that guest OS or firmware does not use the range as a system RAM.
Why is this needed? Isn't the complete firmware memory tagged as
reserved anyway?

take care,
Gerd


Brijesh Singh
 

On 10/14/21 1:58 AM, Gerd Hoffmann wrote:
On Wed, Oct 13, 2021 at 11:57:11AM -0500, Brijesh Singh wrote:
When SEV-SNP is active, the CPUID and Secrets memory range contains the
information that is used during the VM boot. The content need to be persist
across the kexec boot. Mark the memory range as Reserved in the EFI map
so that guest OS or firmware does not use the range as a system RAM.
Why is this needed? Isn't the complete firmware memory tagged as
reserved anyway?
PlatformPei detects all the guest memory and marks it as a SYSTEM_RAM
unless its an MMIO or added as reserved in e820 map file. Since the
Secrets and CPUID pages are part of system RAM so we need to explicitly
exclude these region.

thanks


Gerd Hoffmann
 

On Thu, Oct 14, 2021 at 05:11:22PM -0500, Brijesh Singh wrote:

On 10/14/21 1:58 AM, Gerd Hoffmann wrote:
On Wed, Oct 13, 2021 at 11:57:11AM -0500, Brijesh Singh wrote:
When SEV-SNP is active, the CPUID and Secrets memory range contains the
information that is used during the VM boot. The content need to be persist
across the kexec boot. Mark the memory range as Reserved in the EFI map
so that guest OS or firmware does not use the range as a system RAM.
Why is this needed? Isn't the complete firmware memory tagged as
reserved anyway?
PlatformPei detects all the guest memory and marks it as a SYSTEM_RAM
unless its an MMIO or added as reserved in e820 map file. Since the
Secrets and CPUID pages are part of system RAM so we need to explicitly
exclude these region.
secret and cpuid are in memfd which in turn is part of the firmware
image mapping which is reserved in the e820 map:

kraxel@rhel8 ~# dmesg | grep -i e820
[ ... some lines snipped ... ]
[ 0.000000] BIOS-e820: [mem 0x000000007ff7c000-0x000000007fffffff] reserved
[ 0.000000] BIOS-e820: [mem 0x00000000b0000000-0x00000000bfffffff] reserved
[ 0.000000] BIOS-e820: [mem 0x00000000ffc00000-0x00000000ffffffff] reserved <= here
[ 0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000027fffffff] usable

I think they should be covered already ...

take care,
Gerd


Brijesh Singh
 

On 10/14/21 10:26 PM, Gerd Hoffmann wrote:
On Thu, Oct 14, 2021 at 05:11:22PM -0500, Brijesh Singh wrote:
On 10/14/21 1:58 AM, Gerd Hoffmann wrote:
On Wed, Oct 13, 2021 at 11:57:11AM -0500, Brijesh Singh wrote:
When SEV-SNP is active, the CPUID and Secrets memory range contains the
information that is used during the VM boot. The content need to be persist
across the kexec boot. Mark the memory range as Reserved in the EFI map
so that guest OS or firmware does not use the range as a system RAM.
Why is this needed? Isn't the complete firmware memory tagged as
reserved anyway?
PlatformPei detects all the guest memory and marks it as a SYSTEM_RAM
unless its an MMIO or added as reserved in e820 map file. Since the
Secrets and CPUID pages are part of system RAM so we need to explicitly
exclude these region.
secret and cpuid are in memfd which in turn is part of the firmware
image mapping which is reserved in the e820 map:

kraxel@rhel8 ~# dmesg | grep -i e820
[ ... some lines snipped ... ]
[ 0.000000] BIOS-e820: [mem 0x000000007ff7c000-0x000000007fffffff] reserved
[ 0.000000] BIOS-e820: [mem 0x00000000b0000000-0x00000000bfffffff] reserved
[ 0.000000] BIOS-e820: [mem 0x00000000ffc00000-0x00000000ffffffff] reserved <= here
[ 0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000027fffffff] usable

I think they should be covered already ...
The MEMFD range is outside of the firmware image map,  MEMFD begins with
0x800000 [1] and in my boots I don't see it reserved in e820. Here is
the snippet.

[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009ffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000100000-0x00000000007fffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000800000-0x0000000000807fff]
ACPI NVS
[    0.000000] BIOS-e820: [mem 0x0000000000808000-0x000000000080afff] usable
[    0.000000] BIOS-e820: [mem 0x000000000080b000-0x000000000080bfff]
ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000000080c000-0x000000000080ffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000810000-0x00000000008fffff]
ACPI NVS
[    0.000000] BIOS-e820: [mem 0x0000000000900000-0x000000007f4eefff] usable
[    0.000000] BIOS-e820: [mem 0x000000007f4ef000-0x000000007f76efff]
reserved
[    0.000000] BIOS-e820: [mem 0x000000007f76f000-0x000000007f77efff]
ACPI data
[    0.000000] BIOS-e820: [mem 0x000000007f77f000-0x000000007f7fefff]
ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000007f7ff000-0x000000007fcfbfff] usable
[    0.000000] BIOS-e820: [mem 0x000000007fcfc000-0x000000007fd7ffff]
reserved
[    0.000000] BIOS-e820: [mem 0x000000007fd80000-0x000000007fffffff]
ACPI NVS
[    0.000000] BIOS-e820: [mem 0x00000000b0000000-0x00000000bfffffff]
reserved
[    0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000017fffffff] usable

[1]
https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgDefines.fdf.inc#L97


Gerd Hoffmann
 

Hi,

The MEMFD range is outside of the firmware image map,  MEMFD begins with
0x800000 [1] and in my boots I don't see it reserved in e820.
Ah, ok.

Here is the snippet.

[ ... ]
[    0.000000] BIOS-e820: [mem 0x0000000000800000-0x0000000000807fff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x0000000000808000-0x000000000080afff] usable
[    0.000000] BIOS-e820: [mem 0x000000000080b000-0x000000000080bfff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000000080c000-0x000000000080ffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000810000-0x00000000008fffff] ACPI NVS
[ ... ]
Hmm. Confused. memfd size is 0xD00000, so should the block from 800000
to 8cffff be reserved? Why does it end at 8fffff instead?

The first hole is this:

0x008000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
0x009000|0x002000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize

The second hole is this (git master) ...

0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

And IIRC the cpuid + secrets pages are added there.

So, yes, they must be reserved indeed. What about the other pages?
Shouldn't they be reserved too? Or will they not be used any more
at runtime?

thanks,
Gerd


Brijesh Singh
 

On 10/18/21 1:01 AM, Gerd Hoffmann wrote:
Hi,

The MEMFD range is outside of the firmware image map,  MEMFD begins with
0x800000 [1] and in my boots I don't see it reserved in e820.
Ah, ok.

Here is the snippet.

[ ... ]
[    0.000000] BIOS-e820: [mem 0x0000000000800000-0x0000000000807fff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x0000000000808000-0x000000000080afff] usable
[    0.000000] BIOS-e820: [mem 0x000000000080b000-0x000000000080bfff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000000080c000-0x000000000080ffff] usable
[    0.000000] BIOS-e820: [mem 0x0000000000810000-0x00000000008fffff] ACPI NVS
[ ... ]
Hmm. Confused. memfd size is 0xD00000, so should the block from 800000
to 8cffff be reserved? Why does it end at 8fffff instead?
There is no strong reason for block all of the MEMFD. What I see in the
current code is some selective pages gets marked reserved or other
memory type. As system boots some pages may get released as a system RAM.


The first hole is this:

0x008000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
0x009000|0x002000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize

The second hole is this (git master) ...

0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

And IIRC the cpuid + secrets pages are added there.

So, yes, they must be reserved indeed. What about the other pages?
Shouldn't they be reserved too? Or will they not be used any more
at runtime?
As I indicated above, the other part of the code (such MemDetect.c)
makes the pages reserved as system boot. Some page can be may not be
used at all during the runtime and thus gets released.

thanks