[RFC PATCH 02/14] OvmfPkg/PlatformPei: Mark SEC GHCB page in the page encrpytion bitmap.


Tobin Feldman-Fitzthum <tobin@...>
 

From: Ashish Kalra <ashish.kalra@...>

Mark the SEC GHCB page that is mapped as unencrypted in
ResetVector code in the hypervisor page encryption bitmap.

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>

Signed-off-by: Ashish Kalra <ashish.kalra@...>
---
OvmfPkg/PlatformPei/AmdSev.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda..c72eeb37c5 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -15,6 +15,7 @@
#include <Library/HobLib.h>
#include <Library/MemEncryptSevLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MemEncryptHypercallLib.h>
#include <Library/PcdLib.h>
#include <PiPei.h>
#include <Register/Amd/Msr.h>
@@ -52,6 +53,15 @@ AmdSevEsInitialize (
PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
ASSERT_RETURN_ERROR (PcdStatus);

+ //
+ // GHCB_BASE setup during reset-vector needs to be marked as
+ // decrypted in the hypervisor page encryption bitmap.
+ //
+ SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase),
+ EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)),
+ FALSE
+ );
+
//
// Allocate GHCB and per-CPU variable pages.
// Since the pages must survive across the UEFI to OS transition
--
2.20.1


Ashish Kalra
 

Hello Tobin,

Just a high level question, why is this patch included in this
patch series, i don't think you are supporting SEV-ES platform
migration in this patch-set ?

Thanks,
Ashish

On Tue, Mar 02, 2021 at 03:48:27PM -0500, Tobin Feldman-Fitzthum wrote:
From: Ashish Kalra <ashish.kalra@...>

Mark the SEC GHCB page that is mapped as unencrypted in
ResetVector code in the hypervisor page encryption bitmap.

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>

Signed-off-by: Ashish Kalra <ashish.kalra@...>
---
OvmfPkg/PlatformPei/AmdSev.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda..c72eeb37c5 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -15,6 +15,7 @@
#include <Library/HobLib.h>
#include <Library/MemEncryptSevLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MemEncryptHypercallLib.h>
#include <Library/PcdLib.h>
#include <PiPei.h>
#include <Register/Amd/Msr.h>
@@ -52,6 +53,15 @@ AmdSevEsInitialize (
PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
ASSERT_RETURN_ERROR (PcdStatus);

+ //
+ // GHCB_BASE setup during reset-vector needs to be marked as
+ // decrypted in the hypervisor page encryption bitmap.
+ //
+ SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase),
+ EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)),
+ FALSE
+ );
+
//
// Allocate GHCB and per-CPU variable pages.
// Since the pages must survive across the UEFI to OS transition
--
2.20.1


Tobin Feldman-Fitzthum <tobin@...>
 

Hello Tobin,

Just a high level question, why is this patch included in this
patch series, i don't think you are supporting SEV-ES platform
migration in this patch-set ?
You are correct that we don't support migration for SEV-ES machines, although our approach can potentially be adapted for SEV-ES. I was on the fence about including this patch, because we don't strictly need it for migration. I'm not sure if the SEC GHCB would be significant even if we did support SEV-ES migration. Ultimately it seemed like a good idea because the SEV firmware build does otherwise support SEV-ES. Since I was introducing the hypercall in an environment where SEV-ES can be enabled, it seemed reasonable to include. Syncing page encryption status hypothetically has uses beyond migration.

Note that I am not adding full support for the hypercall in OVMF, which might be a good idea at some point.

-Tobin

Thanks,
Ashish

On Tue, Mar 02, 2021 at 03:48:27PM -0500, Tobin Feldman-Fitzthum wrote:
From: Ashish Kalra <ashish.kalra@...>

Mark the SEC GHCB page that is mapped as unencrypted in
ResetVector code in the hypervisor page encryption bitmap.

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>

Signed-off-by: Ashish Kalra <ashish.kalra@...>
---
OvmfPkg/PlatformPei/AmdSev.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda..c72eeb37c5 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -15,6 +15,7 @@
#include <Library/HobLib.h>
#include <Library/MemEncryptSevLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/MemEncryptHypercallLib.h>
#include <Library/PcdLib.h>
#include <PiPei.h>
#include <Register/Amd/Msr.h>
@@ -52,6 +53,15 @@ AmdSevEsInitialize (
PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
ASSERT_RETURN_ERROR (PcdStatus);
+ //
+ // GHCB_BASE setup during reset-vector needs to be marked as
+ // decrypted in the hypervisor page encryption bitmap.
+ //
+ SetMemoryEncDecHypercall3 (FixedPcdGet32 (PcdOvmfSecGhcbBase),
+ EFI_SIZE_TO_PAGES(FixedPcdGet32 (PcdOvmfSecGhcbSize)),
+ FALSE
+ );
+
//
// Allocate GHCB and per-CPU variable pages.
// Since the pages must survive across the UEFI to OS transition
--
2.20.1


Ashish Kalra
 

On Wed, Mar 03, 2021 at 09:56:00AM -0500, Tobin Feldman-Fitzthum wrote:

Hello Tobin,

Just a high level question, why is this patch included in this
patch series, i don't think you are supporting SEV-ES platform
migration in this patch-set ?
You are correct that we don't support migration for SEV-ES machines,
although our approach can potentially be adapted for SEV-ES. I was on the
fence about including this patch, because we don't strictly need it for
migration. I'm not sure if the SEC GHCB would be significant even if we did
support SEV-ES migration. Ultimately it seemed like a good idea because the
SEV firmware build does otherwise support SEV-ES. Since I was introducing
the hypercall in an environment where SEV-ES can be enabled, it seemed
reasonable to include. Syncing page encryption status hypothetically has
uses beyond migration.

Note that I am not adding full support for the hypercall in OVMF, which
might be a good idea at some point.
Yes, i don't see the assembler code stub for the hypercall in the OVMF
patches, so i was wondering the same.

Thanks,
Ashish