[PATCH v2 0/4] SEV-ES TPM enablement fixes

[PATCH v2 0/4] SEV-ES TPM enablement fixes

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

This patch series provides fixes for using TPM support with an SEV-ES
guest.

The fixes include:

- Decode ModRM byte for MOVZX and MOVSX opcodes.
- Add MMIO support for MOV opcodes 0xA0-0xA3.
- Create a new TPM MMIO ready PPI guid, gOvmfTpmMmioAccessiblePpiGuid
- Mark TPM MMIO range as un-encrypted during PEI phase for an SEV-ES
  guest and install the TPM MMIO ready PPI guid.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

---

These patches are based on commit:
5b90b8abb404 ("ArmPkg: Fix typo of Manufacturer in comment in SmbiosMiscDxe")

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>

Tom Lendacky (4):
  OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
  OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
  OvmfPkg: Define a new PPI GUID to signal TPM MMIO accessability
  OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

 OvmfPkg/OvmfPkg.dec                                       |   4 +
 OvmfPkg/AmdSev/AmdSevX64.dsc                              |   1 +
 OvmfPkg/OvmfPkgIa32.dsc                                   |   1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                |   1 +
 OvmfPkg/OvmfPkgX64.dsc                                    |   1 +
 OvmfPkg/AmdSev/AmdSevX64.fdf                              |   1 +
 OvmfPkg/OvmfPkgIa32.fdf                                   |   1 +
 OvmfPkg/OvmfPkgIa32X64.fdf                                |   1 +
 OvmfPkg/OvmfPkgX64.fdf                                    |   1 +
 OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf                  |   2 +-
 OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf |  40 +++++++
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c             | 118 +++++++++++++++++++-
 OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c  |  76 +++++++++++++
 13 files changed, 245 insertions(+), 3 deletions(-)
 create mode 100644 OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
 create mode 100644 OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c

-- 
2.31.0

[PATCH v2 1/4] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

The MOVZX and MOVSX instructions use the ModRM byte in the instruction,
but the instruction decoding support was not decoding it. This resulted
in invalid decoding and failing of the MMIO operation. Also, when
performing the zero-extend or sign-extend operation, the memory operation
should be using the size, and not the size enumeration value.

Add the ModRM byte decoding for the MOVZX and MOVSX opcodes and use the
true data size to perform the extend operations. Additionally, add a
DEBUG statement identifying the MMIO address being flagged as encrypted
during the MMIO address validation.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd65..dd117f971134 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -643,6 +643,7 @@ ValidateMmioMemory (
   //
   // Any state other than unencrypted is an error, issue a #GP.
   //
+  DEBUG ((DEBUG_ERROR, "MMIO using encrypted memory: %lx\n", (UINT64) MemoryAddress));
   GpEvent.Uint64 = 0;
   GpEvent.Elements.Vector = GP_EXCEPTION;
   GpEvent.Elements.Type   = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
@@ -817,6 +818,7 @@ MmioExit (
     // fall through
     //
   case 0xB7:
+    DecodeModRm (Regs, InstructionData);
     Bytes = (Bytes != 0) ? Bytes : 2;
 
     Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -835,7 +837,7 @@ MmioExit (
     }
 
     Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
-    SetMem (Register, InstructionData->DataSize, 0);
+    SetMem (Register, (UINTN) (1 << InstructionData->DataSize), 0);
     CopyMem (Register, Ghcb->SharedBuffer, Bytes);
     break;
 
@@ -848,6 +850,7 @@ MmioExit (
     // fall through
     //
   case 0xBF:
+    DecodeModRm (Regs, InstructionData);
     Bytes = (Bytes != 0) ? Bytes : 2;
 
     Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -878,7 +881,7 @@ MmioExit (
     }
 
     Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
-    SetMem (Register, InstructionData->DataSize, SignByte);
+    SetMem (Register, (UINTN) (1 << InstructionData->DataSize), SignByte);
     CopyMem (Register, Ghcb->SharedBuffer, Bytes);
     break;
 
-- 
2.31.0

[PATCH v2 2/4] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

Enabling TPM support results in guest termination of an SEV-ES guest
because it uses MMIO opcodes that are not currently supported.

Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
use a memory offset directly encoded in the instruction. Also, add a DEBUG
statement to identify an unsupported MMIO opcode being used.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 111 ++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index dd117f971134..4d001406d30f 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -678,6 +678,7 @@ MmioExit (
   UINTN   Bytes;
   UINT64  *Register;
   UINT8   OpCode, SignByte;
+  UINTN   Address;
 
   Bytes = 0;
 
@@ -727,6 +728,57 @@ MmioExit (
     }
     break;
 
+  //
+  // MMIO write (MOV moffsetX, aX)
+  //
+  case 0xA2:
+    Bytes = 1;
+    //
+    // fall through
+    //
+  case 0xA3:
+    Bytes = ((Bytes != 0) ? Bytes :
+             (InstructionData->DataSize == Size16Bits) ? 2 :
+             (InstructionData->DataSize == Size32Bits) ? 4 :
+             (InstructionData->DataSize == Size64Bits) ? 8 :
+             0);
+
+    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+    InstructionData->End += InstructionData->ImmediateSize;
+
+    //
+    // This code is X64 only, so a possible 8-byte copy to a UINTN is ok.
+    // Use a STATIC_ASSERT to be certain the code is being built as X64.
+    //
+    STATIC_ASSERT (
+      sizeof (UINTN) == sizeof (UINT64),
+      "sizeof (UINTN) != sizeof (UINT64), this file must be built as X64"
+      );
+
+    Address = 0;
+    CopyMem (
+      &Address,
+      InstructionData->Immediate,
+      InstructionData->ImmediateSize
+      );
+
+    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+    if (Status != 0) {
+      return Status;
+    }
+
+    ExitInfo1 = Address;
+    ExitInfo2 = Bytes;
+    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
+
+    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+    if (Status != 0) {
+      return Status;
+    }
+    break;
+
   //
   // MMIO write (MOV reg/memX, immX)
   //
@@ -809,6 +861,64 @@ MmioExit (
     CopyMem (Register, Ghcb->SharedBuffer, Bytes);
     break;
 
+  //
+  // MMIO read (MOV aX, moffsetX)
+  //
+  case 0xA0:
+    Bytes = 1;
+    //
+    // fall through
+    //
+  case 0xA1:
+    Bytes = ((Bytes != 0) ? Bytes :
+             (InstructionData->DataSize == Size16Bits) ? 2 :
+             (InstructionData->DataSize == Size32Bits) ? 4 :
+             (InstructionData->DataSize == Size64Bits) ? 8 :
+             0);
+
+    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+    InstructionData->End += InstructionData->ImmediateSize;
+
+    //
+    // This code is X64 only, so a possible 8-byte copy to a UINTN is ok.
+    // Use a STATIC_ASSERT to be certain the code is being built as X64.
+    //
+    STATIC_ASSERT (
+      sizeof (UINTN) == sizeof (UINT64),
+      "sizeof (UINTN) != sizeof (UINT64), this file must be built as X64"
+      );
+
+    Address = 0;
+    CopyMem (
+      &Address,
+      InstructionData->Immediate,
+      InstructionData->ImmediateSize
+      );
+
+    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+    if (Status != 0) {
+      return Status;
+    }
+
+    ExitInfo1 = Address;
+    ExitInfo2 = Bytes;
+
+    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+    if (Status != 0) {
+      return Status;
+    }
+
+    if (Bytes == 4) {
+      //
+      // Zero-extend for 32-bit operation
+      //
+      Regs->Rax = 0;
+    }
+    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
+    break;
+
   //
   // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
   //
@@ -886,6 +996,7 @@ MmioExit (
     break;
 
   default:
+    DEBUG ((DEBUG_ERROR, "Invalid MMIO opcode (%x)\n", OpCode));
     Status = GP_EXCEPTION;
     ASSERT (FALSE);
   }
-- 
2.31.0

[PATCH v2 3/4] OvmfPkg: Define a new PPI GUID to signal TPM MMIO accessability

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

Define a new PPI GUID that is to be used as a signal of when it is safe
to access the TPM MMIO range. This is needed so that, when SEV is active,
the MMIO range can be mapped unencrypted before it is accessed.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/OvmfPkg.dec | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 9629707020ba..6ae733f6e39f 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -128,6 +128,10 @@ [Ppis]
   # has been discovered and recorded
   gOvmfTpmDiscoveredPpiGuid             = {0xb9a61ad0, 0x2802, 0x41f3, {0xb5, 0x13, 0x96, 0x51, 0xce, 0x6b, 0xd5, 0x75}}
 
+  # This PPI signals that accessing the MMIO range of the TPM is possible in
+  # the PEI phase, regardless of memory encryption
+  gOvmfTpmMmioAccessiblePpiGuid         = {0x35c84ff2, 0x7bfe, 0x453d, {0x84, 0x5f, 0x68, 0x3a, 0x49, 0x2c, 0xf7, 0xb7}}
+
 [Protocols]
   gVirtioDeviceProtocolGuid             = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
   gXenBusProtocolGuid                   = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}
-- 
2.31.0

[PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

During PEI, the MMIO range for the TPM is marked as encrypted when running
as an SEV guest. While this isn't an issue for an SEV guest because of
the way the nested page fault is handled, it does result in an SEV-ES
guest terminating because of a mitigation check in the #VC handler to
prevent MMIO to an encrypted address. For an SEV-ES guest, this range
must be marked as unencrypted.

Create a new x86 PEIM for TPM support that will map the TPM MMIO range as
unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI
will be unconditionally installed before exiting. The PEIM will exit with
the EFI_ABORTED status so that the PEIM does not stay resident.

The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a
Depex for IA32 and X64 builds so that the MMIO range is properly mapped
for SEV-ES before the Tcg2Config PEIM is loaded.

Update all OVMF Ia32 and X64 build packages to include this new PEIM.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                              |  1 +
 OvmfPkg/OvmfPkgIa32.dsc                                   |  1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                |  1 +
 OvmfPkg/OvmfPkgX64.dsc                                    |  1 +
 OvmfPkg/AmdSev/AmdSevX64.fdf                              |  1 +
 OvmfPkg/OvmfPkgIa32.fdf                                   |  1 +
 OvmfPkg/OvmfPkgIa32X64.fdf                                |  1 +
 OvmfPkg/OvmfPkgX64.fdf                                    |  1 +
 OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf                  |  2 +-
 OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++++
 OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c  | 76 ++++++++++++++++++++
 11 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index cdb29d53142d..5a5246c64bf7 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -627,6 +627,7 @@ [Components]
 
 !if $(TPM_ENABLE) == TRUE
   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
   SecurityPkg/Tcg/TcgPei/TcgPei.inf
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 1730b6558b5c..a33c14c673a0 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -707,6 +707,7 @@ [Components]
 
 !if $(TPM_ENABLE) == TRUE
   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
   SecurityPkg/Tcg/TcgPei/TcgPei.inf
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 78a559da0d0b..a4ff7ed44705 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -720,6 +720,7 @@ [Components.IA32]
 
 !if $(TPM_ENABLE) == TRUE
   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
   SecurityPkg/Tcg/TcgPei/TcgPei.inf
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a7d747f6b4ab..3fb56b3f9ff9 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -719,6 +719,7 @@ [Components]
 
 !if $(TPM_ENABLE) == TRUE
   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
   SecurityPkg/Tcg/TcgPei/TcgPei.inf
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index c0098502aa90..ab58a9c0b4da 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -148,6 +148,7 @@ [FV.PEIFV]
 
 !if $(TPM_ENABLE) == TRUE
 INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
 INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
 INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index f400c845b9c9..fc0ae1f280df 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -163,6 +163,7 @@ [FV.PEIFV]
 
 !if $(TPM_ENABLE) == TRUE
 INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
 INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
 INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index d055552fd09f..306fc5a9b60d 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -163,6 +163,7 @@ [FV.PEIFV]
 
 !if $(TPM_ENABLE) == TRUE
 INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
 INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
 INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d519f8532822..22c8664427d6 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -175,6 +175,7 @@ [FV.PEIFV]
 
 !if $(TPM_ENABLE) == TRUE
 INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
 INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
 INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
 !endif
diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..39d1deeed16b 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -57,7 +57,7 @@ [Pcd]
   gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid                 ## PRODUCES
 
 [Depex.IA32, Depex.X64]
-  TRUE
+  gOvmfTpmMmioAccessiblePpiGuid
 
 [Depex.ARM, Depex.AARCH64]
   gOvmfTpmDiscoveredPpiGuid
diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
new file mode 100644
index 000000000000..926113b8ffb0
--- /dev/null
+++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
@@ -0,0 +1,40 @@
+## @file
+# Map TPM MMIO range unencrypted when SEV is active
+#
+# Copyright (C) 2021, Advanced Micro Devices, Inc.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = TpmMmioSevDecryptPei
+  FILE_GUID                      = F12F698A-E506-4A1B-B32E-6920E55DA1C4
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = TpmMmioSevDecryptPeimEntryPoint
+
+[Sources]
+  TpmMmioSevDecryptPeim.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  OvmfPkg/OvmfPkg.dec
+  SecurityPkg/SecurityPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  MemEncryptSevLib
+  PeimEntryPoint
+  PeiServicesLib
+
+[Ppis]
+  gOvmfTpmMmioAccessiblePpiGuid                      ## PRODUCES
+
+[FixedPcd]
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress    ## CONSUMES
+
+[Depex]
+  gEfiPeiMemoryDiscoveredPpiGuid
diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
new file mode 100644
index 000000000000..dd1f1a80b5b0
--- /dev/null
+++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
@@ -0,0 +1,76 @@
+/** @file
+  Map TPM MMIO range unencrypted when SEV is active
+
+  Copyright (C) 2021, Advanced Micro Devices, Inc.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+
+#include <PiPei.h>
+
+#include <Library/DebugLib.h>
+#include <Library/MemEncryptSevLib.h>
+#include <Library/PeiServicesLib.h>
+
+STATIC CONST EFI_PEI_PPI_DESCRIPTOR  mTpmMmioRangeAccessible = {
+  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+  &gOvmfTpmMmioAccessiblePpiGuid,
+  NULL
+};
+
+/**
+  The entry point for TPM MMIO range mapping driver.
+
+  @param[in]  FileHandle   Handle of the file being invoked.
+  @param[in]  PeiServices  Describes the list of possible PEI Services.
+
+  @retval  EFI_ABORTED  No need to keep this PEIM resident
+**/
+EFI_STATUS
+EFIAPI
+TpmMmioSevDecryptPeimEntryPoint (
+  IN       EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES     **PeiServices
+  )
+{
+  RETURN_STATUS                   DecryptStatus;
+  EFI_STATUS                      Status;
+
+  DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__));
+
+  //
+  // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical
+  // address because the nested page fault (NPF) that occurs on access does not
+  // include the encryption bit in the guest physical address provided to the
+  // hypervisor.
+  //
+  // However, if SEV-ES is active, before performing the actual MMIO, an
+  // additional MMIO mitigation check is performed in the #VC handler to ensure
+  // that MMIO is being done to an unencrypted address. To prevent guest
+  // termination in this scenario, mark the range unencrypted ahead of access.
+  //
+  if (MemEncryptSevEsIsEnabled ()) {
+    DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", __FUNCTION__));
+
+    DecryptStatus = MemEncryptSevClearPageEncMask (
+                      0,
+                      PcdGet64 (PcdTpmBaseAddress),
+                      EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
+                      FALSE
+                      );
+
+    if (RETURN_ERROR (DecryptStatus)) {
+      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));
+      ASSERT_RETURN_ERROR (DecryptStatus);
+    }
+  }
+
+  //
+  // MMIO range available
+  //
+  Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible);
+  ASSERT_EFI_ERROR (Status);
+
+  return EFI_ABORTED;
+}
-- 
2.31.0

Re: [edk2-devel] [PATCH v2 1/4] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes

[Laszlo Ersek]

On 04/27/21 18:21, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
> 
> The MOVZX and MOVSX instructions use the ModRM byte in the instruction,
> but the instruction decoding support was not decoding it. This resulted
> in invalid decoding and failing of the MMIO operation. Also, when
> performing the zero-extend or sign-extend operation, the memory operation
> should be using the size, and not the size enumeration value.
> 
> Add the ModRM byte decoding for the MOVZX and MOVSX opcodes and use the
> true data size to perform the extend operations. Additionally, add a
> DEBUG statement identifying the MMIO address being flagged as encrypted
> during the MMIO address validation.
> 
> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 24259060fd65..dd117f971134 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -643,6 +643,7 @@ ValidateMmioMemory (
>    //
>    // Any state other than unencrypted is an error, issue a #GP.
>    //
> +  DEBUG ((DEBUG_ERROR, "MMIO using encrypted memory: %lx\n", (UINT64) MemoryAddress));

(1) This line is now too long -- 86 characters. But I'll fix that up on
merge, if I find nothing serious in v2.

Thanks
Laszlo

>    GpEvent.Uint64 = 0;
>    GpEvent.Elements.Vector = GP_EXCEPTION;
>    GpEvent.Elements.Type   = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
> @@ -817,6 +818,7 @@ MmioExit (
>      // fall through
>      //
>    case 0xB7:
> +    DecodeModRm (Regs, InstructionData);
>      Bytes = (Bytes != 0) ? Bytes : 2;
>  
>      Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
> @@ -835,7 +837,7 @@ MmioExit (
>      }
>  
>      Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> -    SetMem (Register, InstructionData->DataSize, 0);
> +    SetMem (Register, (UINTN) (1 << InstructionData->DataSize), 0);
>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>      break;
>  
> @@ -848,6 +850,7 @@ MmioExit (
>      // fall through
>      //
>    case 0xBF:
> +    DecodeModRm (Regs, InstructionData);
>      Bytes = (Bytes != 0) ? Bytes : 2;
>  
>      Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
> @@ -878,7 +881,7 @@ MmioExit (
>      }
>  
>      Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> -    SetMem (Register, InstructionData->DataSize, SignByte);
> +    SetMem (Register, (UINTN) (1 << InstructionData->DataSize), SignByte);
>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>      break;
>  
> 

Re: [edk2-devel] [PATCH v2 2/4] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

[Laszlo Ersek]

On 04/27/21 18:21, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
> 
> Enabling TPM support results in guest termination of an SEV-ES guest
> because it uses MMIO opcodes that are not currently supported.
> 
> Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
> use a memory offset directly encoded in the instruction. Also, add a DEBUG
> statement to identify an unsupported MMIO opcode being used.
> 
> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 111 ++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index dd117f971134..4d001406d30f 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -678,6 +678,7 @@ MmioExit (
>    UINTN   Bytes;
>    UINT64  *Register;
>    UINT8   OpCode, SignByte;
> +  UINTN   Address;
>  
>    Bytes = 0;
>  
> @@ -727,6 +728,57 @@ MmioExit (
>      }
>      break;
>  
> +  //
> +  // MMIO write (MOV moffsetX, aX)
> +  //
> +  case 0xA2:
> +    Bytes = 1;
> +    //
> +    // fall through
> +    //
> +  case 0xA3:
> +    Bytes = ((Bytes != 0) ? Bytes :
> +             (InstructionData->DataSize == Size16Bits) ? 2 :
> +             (InstructionData->DataSize == Size32Bits) ? 4 :
> +             (InstructionData->DataSize == Size64Bits) ? 8 :
> +             0);
> +
> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
> +    InstructionData->End += InstructionData->ImmediateSize;
> +
> +    //
> +    // This code is X64 only, so a possible 8-byte copy to a UINTN is ok.
> +    // Use a STATIC_ASSERT to be certain the code is being built as X64.
> +    //
> +    STATIC_ASSERT (
> +      sizeof (UINTN) == sizeof (UINT64),
> +      "sizeof (UINTN) != sizeof (UINT64), this file must be built as X64"
> +      );
> +
> +    Address = 0;
> +    CopyMem (
> +      &Address,
> +      InstructionData->Immediate,
> +      InstructionData->ImmediateSize
> +      );
> +
> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +
> +    ExitInfo1 = Address;
> +    ExitInfo2 = Bytes;
> +    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
> +
> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +    break;
> +
>    //
>    // MMIO write (MOV reg/memX, immX)
>    //
> @@ -809,6 +861,64 @@ MmioExit (
>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>      break;
>  
> +  //
> +  // MMIO read (MOV aX, moffsetX)
> +  //
> +  case 0xA0:
> +    Bytes = 1;
> +    //
> +    // fall through
> +    //
> +  case 0xA1:
> +    Bytes = ((Bytes != 0) ? Bytes :
> +             (InstructionData->DataSize == Size16Bits) ? 2 :
> +             (InstructionData->DataSize == Size32Bits) ? 4 :
> +             (InstructionData->DataSize == Size64Bits) ? 8 :
> +             0);
> +
> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
> +    InstructionData->End += InstructionData->ImmediateSize;
> +
> +    //
> +    // This code is X64 only, so a possible 8-byte copy to a UINTN is ok.
> +    // Use a STATIC_ASSERT to be certain the code is being built as X64.
> +    //
> +    STATIC_ASSERT (
> +      sizeof (UINTN) == sizeof (UINT64),
> +      "sizeof (UINTN) != sizeof (UINT64), this file must be built as X64"
> +      );
> +
> +    Address = 0;
> +    CopyMem (
> +      &Address,
> +      InstructionData->Immediate,
> +      InstructionData->ImmediateSize
> +      );
> +
> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +
> +    ExitInfo1 = Address;
> +    ExitInfo2 = Bytes;
> +
> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +
> +    if (Bytes == 4) {
> +      //
> +      // Zero-extend for 32-bit operation
> +      //
> +      Regs->Rax = 0;
> +    }
> +    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
> +    break;
> +
>    //
>    // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>    //
> @@ -886,6 +996,7 @@ MmioExit (
>      break;
>  
>    default:
> +    DEBUG ((DEBUG_ERROR, "Invalid MMIO opcode (%x)\n", OpCode));
>      Status = GP_EXCEPTION;
>      ASSERT (FALSE);
>    }
> 

Thanks for the updates!

Acked-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

Re: [edk2-devel] [PATCH v2 3/4] OvmfPkg: Define a new PPI GUID to signal TPM MMIO accessability

[Laszlo Ersek]

On 04/27/21 18:21, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Define a new PPI GUID that is to be used as a signal of when it is safe
> to access the TPM MMIO range. This is needed so that, when SEV is active,
> the MMIO range can be mapped unencrypted before it is accessed.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkg.dec | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 9629707020ba..6ae733f6e39f 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -128,6 +128,10 @@ [Ppis]
>    # has been discovered and recorded
>    gOvmfTpmDiscoveredPpiGuid             = {0xb9a61ad0, 0x2802, 0x41f3, {0xb5, 0x13, 0x96, 0x51, 0xce, 0x6b, 0xd5, 0x75}}
>  
> +  # This PPI signals that accessing the MMIO range of the TPM is possible in
> +  # the PEI phase, regardless of memory encryption
> +  gOvmfTpmMmioAccessiblePpiGuid         = {0x35c84ff2, 0x7bfe, 0x453d, {0x84, 0x5f, 0x68, 0x3a, 0x49, 0x2c, 0xf7, 0xb7}}
> +
>  [Protocols]
>    gVirtioDeviceProtocolGuid             = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
>    gXenBusProtocolGuid                   = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH v2 3/4] OvmfPkg: Define a new PPI GUID to signal TPM MMIO accessability

[Laszlo Ersek]

On 04/28/21 19:12, Laszlo Ersek wrote:
> On 04/27/21 18:21, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> Define a new PPI GUID that is to be used as a signal of when it is safe
>> to access the TPM MMIO range. This is needed so that, when SEV is active,
>> the MMIO range can be mapped unencrypted before it is accessed.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>

(1) Marc-André's name is garbled here, but I can fix it up.

Thanks
Laszlo

>> Cc: Stefan Berger <stefanb@linux.ibm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/OvmfPkg.dec | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 9629707020ba..6ae733f6e39f 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -128,6 +128,10 @@ [Ppis]
>>    # has been discovered and recorded
>>    gOvmfTpmDiscoveredPpiGuid             = {0xb9a61ad0, 0x2802, 0x41f3, {0xb5, 0x13, 0x96, 0x51, 0xce, 0x6b, 0xd5, 0x75}}
>>  
>> +  # This PPI signals that accessing the MMIO range of the TPM is possible in
>> +  # the PEI phase, regardless of memory encryption
>> +  gOvmfTpmMmioAccessiblePpiGuid         = {0x35c84ff2, 0x7bfe, 0x453d, {0x84, 0x5f, 0x68, 0x3a, 0x49, 0x2c, 0xf7, 0xb7}}
>> +
>>  [Protocols]
>>    gVirtioDeviceProtocolGuid             = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
>>    gXenBusProtocolGuid                   = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}
>>
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Re: [edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

[Laszlo Ersek]

I'm going to ask for v3 after all:

On 04/27/21 18:21, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
> 
> During PEI, the MMIO range for the TPM is marked as encrypted when running
> as an SEV guest. While this isn't an issue for an SEV guest because of
> the way the nested page fault is handled, it does result in an SEV-ES
> guest terminating because of a mitigation check in the #VC handler to
> prevent MMIO to an encrypted address. For an SEV-ES guest, this range
> must be marked as unencrypted.
> 
> Create a new x86 PEIM for TPM support that will map the TPM MMIO range as
> unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI
> will be unconditionally installed before exiting. The PEIM will exit with
> the EFI_ABORTED status so that the PEIM does not stay resident.

(1) Please spell out that the new PEIM will depend on the installation
of the permanent PEI RAM, by PlatformPei -- the reason being that, in
case page table splitting proves necessary for clearing the C-bit, the
new page table(s) should be allocated from permanent PEI RAM.


> 
> The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a
> Depex for IA32 and X64 builds so that the MMIO range is properly mapped
> for SEV-ES before the Tcg2Config PEIM is loaded.

(2) The Tcg2Config depex change should be a separate patch -- the last
patch in the series. That covers both the Tcg2Config hunk in the patch
body, and this commit message paragraph right here.


(3) While at it: those parts of this patch that are *not* related to the
Tcg2Config PEIM can be squashed into the previous patch -- if you prefer
that.

I'm certainly happy with three separate patches though: for the DEC
file, for TpmMmioSevDecryptPei + the DSC/FDF files, and finally the
Tcg2Config PEIM. So in total the series should include 4 or 5 patches
(up to you).


> 
> Update all OVMF Ia32 and X64 build packages to include this new PEIM.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>

(4) Marc-André's name is garbled here too.

The following git config options are related:

- For encoding non-ASCII characters in git commits, the
"i18n.commitencoding" knob is relevant. Almost universally, this should
be "UTF-8" (assuming your text editor used for composing commit messages
produces UTF-8-encoded files).

- For formatting commits to patch emails, "i18n.logOutputEncoding"
matters. This should *always* be "UTF-8", when git-format-patch is invoked.


> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc                              |  1 +
>  OvmfPkg/OvmfPkgIa32.dsc                                   |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                                |  1 +
>  OvmfPkg/OvmfPkgX64.dsc                                    |  1 +
>  OvmfPkg/AmdSev/AmdSevX64.fdf                              |  1 +
>  OvmfPkg/OvmfPkgIa32.fdf                                   |  1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf                                |  1 +
>  OvmfPkg/OvmfPkgX64.fdf                                    |  1 +
>  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf                  |  2 +-
>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++++
>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c  | 76 ++++++++++++++++++++
>  11 files changed, 125 insertions(+), 1 deletion(-)

Right, skipping Bhyve is justified, per your previous report /
<https://bugzilla.tianocore.org/show_bug.cgi?id=3354>.

> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index cdb29d53142d..5a5246c64bf7 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -627,6 +627,7 @@ [Components]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>

(5) Functionally correct, but it reads more nicely (from a logical
dependency POV) if we place the new PEIM first.

(Please apply to the rest of the DSC files, and the FDF files too.)


> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 1730b6558b5c..a33c14c673a0 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -707,6 +707,7 @@ [Components]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 78a559da0d0b..a4ff7ed44705 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -720,6 +720,7 @@ [Components.IA32]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index a7d747f6b4ab..3fb56b3f9ff9 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -719,6 +719,7 @@ [Components]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index c0098502aa90..ab58a9c0b4da 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -148,6 +148,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index f400c845b9c9..fc0ae1f280df 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -163,6 +163,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index d055552fd09f..306fc5a9b60d 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -163,6 +163,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index d519f8532822..22c8664427d6 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -175,6 +175,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index 6776ec931ce0..39d1deeed16b 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -57,7 +57,7 @@ [Pcd]
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid                 ## PRODUCES
>  
>  [Depex.IA32, Depex.X64]
> -  TRUE
> +  gOvmfTpmMmioAccessiblePpiGuid
>  
>  [Depex.ARM, Depex.AARCH64]
>    gOvmfTpmDiscoveredPpiGuid
> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
> new file mode 100644
> index 000000000000..926113b8ffb0
> --- /dev/null
> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
> @@ -0,0 +1,40 @@
> +## @file
> +# Map TPM MMIO range unencrypted when SEV is active

(6) Please add another sentence here: "Install
gOvmfTpmMmioAccessiblePpiGuid unconditionally".


> +#
> +# Copyright (C) 2021, Advanced Micro Devices, Inc.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

(7) The latest INF spec version is 1.29:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Draft-Specification
https://tianocore-docs.github.io/edk2-InfSpecification/draft/

plus INF_VERSION no longer requires a binary-only (hex-only) format. So
please just write "1.29".


> +  BASE_NAME                      = TpmMmioSevDecryptPei
> +  FILE_GUID                      = F12F698A-E506-4A1B-B32E-6920E55DA1C4
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = TpmMmioSevDecryptPeimEntryPoint
> +
> +[Sources]
> +  TpmMmioSevDecryptPeim.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  SecurityPkg/SecurityPkg.dec

(8) Is MdeModulePkg necessary?


> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  MemEncryptSevLib
> +  PeimEntryPoint
> +  PeiServicesLib
> +
> +[Ppis]
> +  gOvmfTpmMmioAccessiblePpiGuid                      ## PRODUCES
> +
> +[FixedPcd]
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress    ## CONSUMES
> +
> +[Depex]
> +  gEfiPeiMemoryDiscoveredPpiGuid
> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
> new file mode 100644
> index 000000000000..dd1f1a80b5b0
> --- /dev/null
> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
> @@ -0,0 +1,76 @@
> +/** @file
> +  Map TPM MMIO range unencrypted when SEV is active

(9) Same request as (6) -- please add another sentence: "Install
gOvmfTpmMmioAccessiblePpiGuid unconditionally".


> +
> +  Copyright (C) 2021, Advanced Micro Devices, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +
> +#include <PiPei.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/PeiServicesLib.h>

(10) This Library #include list does not match the [LibraryClasses]
section of the INF file (the PeimEntryPoint class apart, which should
indeed only be in the INF file). In other words, BaseLib appears
superfluous in the INF file.


> +
> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR  mTpmMmioRangeAccessible = {
> +  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> +  &gOvmfTpmMmioAccessiblePpiGuid,
> +  NULL
> +};
> +
> +/**
> +  The entry point for TPM MMIO range mapping driver.
> +
> +  @param[in]  FileHandle   Handle of the file being invoked.
> +  @param[in]  PeiServices  Describes the list of possible PEI Services.
> +
> +  @retval  EFI_ABORTED  No need to keep this PEIM resident
> +**/
> +EFI_STATUS
> +EFIAPI
> +TpmMmioSevDecryptPeimEntryPoint (
> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> +  IN CONST EFI_PEI_SERVICES     **PeiServices
> +  )
> +{
> +  RETURN_STATUS                   DecryptStatus;
> +  EFI_STATUS                      Status;
> +
> +  DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__));
> +
> +  //
> +  // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical
> +  // address because the nested page fault (NPF) that occurs on access does not
> +  // include the encryption bit in the guest physical address provided to the
> +  // hypervisor.
> +  //
> +  // However, if SEV-ES is active, before performing the actual MMIO, an
> +  // additional MMIO mitigation check is performed in the #VC handler to ensure
> +  // that MMIO is being done to an unencrypted address. To prevent guest
> +  // termination in this scenario, mark the range unencrypted ahead of access.
> +  //

Lovely comment, thanks!

> +  if (MemEncryptSevEsIsEnabled ()) {
> +    DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", __FUNCTION__));
> +
> +    DecryptStatus = MemEncryptSevClearPageEncMask (
> +                      0,
> +                      PcdGet64 (PcdTpmBaseAddress),

(11) The INF file says [FixedPcd], so it would be cleanest to say
FixedPcdGet64() here.


(12) PcdLib is missing from both the [LibraryClasses] section and the
#include directives.


> +                      EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
> +                      FALSE
> +                      );
> +
> +    if (RETURN_ERROR (DecryptStatus)) {
> +      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));

(13) Overlong line.


(14) Please report errors with DEBUG_ERROR.


> +      ASSERT_RETURN_ERROR (DecryptStatus);
> +    }
> +  }
> +
> +  //
> +  // MMIO range available
> +  //
> +  Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return EFI_ABORTED;
> +}
> 

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v2 3/4] OvmfPkg: Define a new PPI GUID to signal TPM MMIO accessability

[Lendacky, Thomas]

On 4/28/21 12:15 PM, Laszlo Ersek wrote:
> On 04/28/21 19:12, Laszlo Ersek wrote:
>> On 04/27/21 18:21, Lendacky, Thomas wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> Define a new PPI GUID that is to be used as a signal of when it is safe
>>> to access the TPM MMIO range. This is needed so that, when SEV is active,
>>> the MMIO range can be mapped unencrypted before it is accessed.
>>>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>
> 
> (1) Marc-André's name is garbled here, but I can fix it up.

Sorry about that, looks like my email system didn't like the accent
symbol. I'll look into that and see if I can't fix it (it could also be my
.gitconfig).

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
>>> Cc: Stefan Berger <stefanb@linux.ibm.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>  OvmfPkg/OvmfPkg.dec | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>> index 9629707020ba..6ae733f6e39f 100644
>>> --- a/OvmfPkg/OvmfPkg.dec
>>> +++ b/OvmfPkg/OvmfPkg.dec
>>> @@ -128,6 +128,10 @@ [Ppis]
>>>    # has been discovered and recorded
>>>    gOvmfTpmDiscoveredPpiGuid             = {0xb9a61ad0, 0x2802, 0x41f3, {0xb5, 0x13, 0x96, 0x51, 0xce, 0x6b, 0xd5, 0x75}}
>>>  
>>> +  # This PPI signals that accessing the MMIO range of the TPM is possible in
>>> +  # the PEI phase, regardless of memory encryption
>>> +  gOvmfTpmMmioAccessiblePpiGuid         = {0x35c84ff2, 0x7bfe, 0x453d, {0x84, 0x5f, 0x68, 0x3a, 0x49, 0x2c, 0xf7, 0xb7}}
>>> +
>>>  [Protocols]
>>>    gVirtioDeviceProtocolGuid             = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
>>>    gXenBusProtocolGuid                   = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}}
>>>
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
> 

Re: [edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

[Lendacky, Thomas]

On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote:
> I'm going to ask for v3 after all:
> 
> On 04/27/21 18:21, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=G1GwQc6sZqRuNHWC5vbdb78gCOl4YkAq%2BHi0F0ceucg%3D&amp;reserved=0
>>
>> During PEI, the MMIO range for the TPM is marked as encrypted when running
>> as an SEV guest. While this isn't an issue for an SEV guest because of
>> the way the nested page fault is handled, it does result in an SEV-ES
>> guest terminating because of a mitigation check in the #VC handler to
>> prevent MMIO to an encrypted address. For an SEV-ES guest, this range
>> must be marked as unencrypted.
>>
>> Create a new x86 PEIM for TPM support that will map the TPM MMIO range as
>> unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI
>> will be unconditionally installed before exiting. The PEIM will exit with
>> the EFI_ABORTED status so that the PEIM does not stay resident.
> 
> (1) Please spell out that the new PEIM will depend on the installation
> of the permanent PEI RAM, by PlatformPei -- the reason being that, in
> case page table splitting proves necessary for clearing the C-bit, the
> new page table(s) should be allocated from permanent PEI RAM.

Will do.

> 
> 
>>
>> The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a
>> Depex for IA32 and X64 builds so that the MMIO range is properly mapped
>> for SEV-ES before the Tcg2Config PEIM is loaded.
> 
> (2) The Tcg2Config depex change should be a separate patch -- the last
> patch in the series. That covers both the Tcg2Config hunk in the patch
> body, and this commit message paragraph right here.

Ok, I'll split that out.

> 
> 
> (3) While at it: those parts of this patch that are *not* related to the
> Tcg2Config PEIM can be squashed into the previous patch -- if you prefer
> that.
> 
> I'm certainly happy with three separate patches though: for the DEC
> file, for TpmMmioSevDecryptPei + the DSC/FDF files, and finally the
> Tcg2Config PEIM. So in total the series should include 4 or 5 patches
> (up to you).

I'll do it as 5 patches.

> 
> 
>>
>> Update all OVMF Ia32 and X64 build packages to include this new PEIM.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Marc-Andr?? Lureau <marcandre.lureau@redhat.com>
> 
> (4) Marc-André's name is garbled here too.
> 
> The following git config options are related:
> 
> - For encoding non-ASCII characters in git commits, the
> "i18n.commitencoding" knob is relevant. Almost universally, this should
> be "UTF-8" (assuming your text editor used for composing commit messages
> produces UTF-8-encoded files).
> 
> - For formatting commits to patch emails, "i18n.logOutputEncoding"
> matters. This should *always* be "UTF-8", when git-format-patch is invoked.

We were having problems with sending patches via git-format-patch and
git-send-email and our email system. Likely some left over .gitconfig
entries that are causing the problem. I'll double check everything.

> 
> 
>> Cc: Stefan Berger <stefanb@linux.ibm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/AmdSev/AmdSevX64.dsc                              |  1 +
>>  OvmfPkg/OvmfPkgIa32.dsc                                   |  1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                |  1 +
>>  OvmfPkg/OvmfPkgX64.dsc                                    |  1 +
>>  OvmfPkg/AmdSev/AmdSevX64.fdf                              |  1 +
>>  OvmfPkg/OvmfPkgIa32.fdf                                   |  1 +
>>  OvmfPkg/OvmfPkgIa32X64.fdf                                |  1 +
>>  OvmfPkg/OvmfPkgX64.fdf                                    |  1 +
>>  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf                  |  2 +-
>>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++++
>>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c  | 76 ++++++++++++++++++++
>>  11 files changed, 125 insertions(+), 1 deletion(-)
> 
> Right, skipping Bhyve is justified, per your previous report /
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3354&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=HPF7LTIV17CwZ%2BCFehXpnPb%2BtQCgpFPvLsVqfNj9HBI%3D&amp;reserved=0>.
> 
>>
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> index cdb29d53142d..5a5246c64bf7 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> @@ -627,6 +627,7 @@ [Components]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>      <LibraryClasses>
> 
> (5) Functionally correct, but it reads more nicely (from a logical
> dependency POV) if we place the new PEIM first.
> 
> (Please apply to the rest of the DSC files, and the FDF files too.)

Ok, I was going with the alphabetical placement. I'll switch it up.

> 
> 
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 1730b6558b5c..a33c14c673a0 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -707,6 +707,7 @@ [Components]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>      <LibraryClasses>
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 78a559da0d0b..a4ff7ed44705 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -720,6 +720,7 @@ [Components.IA32]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>      <LibraryClasses>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index a7d747f6b4ab..3fb56b3f9ff9 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -719,6 +719,7 @@ [Components]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>      <LibraryClasses>
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
>> index c0098502aa90..ab58a9c0b4da 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
>> @@ -148,6 +148,7 @@ [FV.PEIFV]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>  !endif
>> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
>> index f400c845b9c9..fc0ae1f280df 100644
>> --- a/OvmfPkg/OvmfPkgIa32.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32.fdf
>> @@ -163,6 +163,7 @@ [FV.PEIFV]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>  !endif
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
>> index d055552fd09f..306fc5a9b60d 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
>> @@ -163,6 +163,7 @@ [FV.PEIFV]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>  !endif
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index d519f8532822..22c8664427d6 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -175,6 +175,7 @@ [FV.PEIFV]
>>
>>  !if $(TPM_ENABLE) == TRUE
>>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>  !endif
>> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> index 6776ec931ce0..39d1deeed16b 100644
>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> @@ -57,7 +57,7 @@ [Pcd]
>>    gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid                 ## PRODUCES
>>
>>  [Depex.IA32, Depex.X64]
>> -  TRUE
>> +  gOvmfTpmMmioAccessiblePpiGuid
>>
>>  [Depex.ARM, Depex.AARCH64]
>>    gOvmfTpmDiscoveredPpiGuid
>> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>> new file mode 100644
>> index 000000000000..926113b8ffb0
>> --- /dev/null
>> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>> @@ -0,0 +1,40 @@
>> +## @file
>> +# Map TPM MMIO range unencrypted when SEV is active
> 
> (6) Please add another sentence here: "Install
> gOvmfTpmMmioAccessiblePpiGuid unconditionally".

Will do.

> 
> 
>> +#
>> +# Copyright (C) 2021, Advanced Micro Devices, Inc.
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
> 
> (7) The latest INF spec version is 1.29:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Draft-Specification&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=AXuQkvUSwLjEyZiwivQQaUwTaY7Mo0wLSHUf8QKNLC8%3D&amp;reserved=0
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftianocore-docs.github.io%2Fedk2-InfSpecification%2Fdraft%2F&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=YUHs6g5aMPWBjCNjcZPKnTSEs2gBazDX094nqj9qpnE%3D&amp;reserved=0
> 
> plus INF_VERSION no longer requires a binary-only (hex-only) format. So
> please just write "1.29".

Will do.

> 
> 
>> +  BASE_NAME                      = TpmMmioSevDecryptPei
>> +  FILE_GUID                      = F12F698A-E506-4A1B-B32E-6920E55DA1C4
>> +  MODULE_TYPE                    = PEIM
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = TpmMmioSevDecryptPeimEntryPoint
>> +
>> +[Sources]
>> +  TpmMmioSevDecryptPeim.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +  SecurityPkg/SecurityPkg.dec
> 
> (8) Is MdeModulePkg necessary?

I don't think so. Let me double check it.

> 
> 
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  DebugLib
>> +  MemEncryptSevLib
>> +  PeimEntryPoint
>> +  PeiServicesLib
>> +
>> +[Ppis]
>> +  gOvmfTpmMmioAccessiblePpiGuid                      ## PRODUCES
>> +
>> +[FixedPcd]
>> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress    ## CONSUMES
>> +
>> +[Depex]
>> +  gEfiPeiMemoryDiscoveredPpiGuid
>> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
>> new file mode 100644
>> index 000000000000..dd1f1a80b5b0
>> --- /dev/null
>> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
>> @@ -0,0 +1,76 @@
>> +/** @file
>> +  Map TPM MMIO range unencrypted when SEV is active
> 
> (9) Same request as (6) -- please add another sentence: "Install
> gOvmfTpmMmioAccessiblePpiGuid unconditionally".
> 

Will do.

> 
>> +
>> +  Copyright (C) 2021, Advanced Micro Devices, Inc.
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +
>> +#include <PiPei.h>
>> +
>> +#include <Library/DebugLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>> +#include <Library/PeiServicesLib.h>
> 
> (10) This Library #include list does not match the [LibraryClasses]
> section of the INF file (the PeimEntryPoint class apart, which should
> indeed only be in the INF file). In other words, BaseLib appears
> superfluous in the INF file.

Ok, let me check on that and fix as appropriate.

> 
> 
>> +
>> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR  mTpmMmioRangeAccessible = {
>> +  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
>> +  &gOvmfTpmMmioAccessiblePpiGuid,
>> +  NULL
>> +};
>> +
>> +/**
>> +  The entry point for TPM MMIO range mapping driver.
>> +
>> +  @param[in]  FileHandle   Handle of the file being invoked.
>> +  @param[in]  PeiServices  Describes the list of possible PEI Services.
>> +
>> +  @retval  EFI_ABORTED  No need to keep this PEIM resident
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +TpmMmioSevDecryptPeimEntryPoint (
>> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
>> +  IN CONST EFI_PEI_SERVICES     **PeiServices
>> +  )
>> +{
>> +  RETURN_STATUS                   DecryptStatus;
>> +  EFI_STATUS                      Status;
>> +
>> +  DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__));
>> +
>> +  //
>> +  // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical
>> +  // address because the nested page fault (NPF) that occurs on access does not
>> +  // include the encryption bit in the guest physical address provided to the
>> +  // hypervisor.
>> +  //
>> +  // However, if SEV-ES is active, before performing the actual MMIO, an
>> +  // additional MMIO mitigation check is performed in the #VC handler to ensure
>> +  // that MMIO is being done to an unencrypted address. To prevent guest
>> +  // termination in this scenario, mark the range unencrypted ahead of access.
>> +  //
> 
> Lovely comment, thanks!
> 
>> +  if (MemEncryptSevEsIsEnabled ()) {
>> +    DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", __FUNCTION__));
>> +
>> +    DecryptStatus = MemEncryptSevClearPageEncMask (
>> +                      0,
>> +                      PcdGet64 (PcdTpmBaseAddress),
> 
> (11) The INF file says [FixedPcd], so it would be cleanest to say
> FixedPcdGet64() here.

Will do.

> 
> 
> (12) PcdLib is missing from both the [LibraryClasses] section and the
> #include directives.

Right, I'll update that.

> 
> 
>> +                      EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
>> +                      FALSE
>> +                      );
>> +
>> +    if (RETURN_ERROR (DecryptStatus)) {
>> +      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));
> 
> (13) Overlong line.

Ok, I'll change that. I though that was ok now since PatchCheck.py didn't
complain.

> 
> 
> (14) Please report errors with DEBUG_ERROR.

Yup, will change.

Thanks,
Tom

> 
> 
>> +      ASSERT_RETURN_ERROR (DecryptStatus);
>> +    }
>> +  }
>> +
>> +  //
>> +  // MMIO range available
>> +  //
>> +  Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  return EFI_ABORTED;
>> +}
>>
> 
> Thanks!
> Laszlo
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

[Lendacky, Thomas]

On 4/28/21 2:43 PM, Tom Lendacky wrote:
> On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote:
>> I'm going to ask for v3 after all:
>>
>> On 04/27/21 18:21, Lendacky, Thomas wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=G1GwQc6sZqRuNHWC5vbdb78gCOl4YkAq%2BHi0F0ceucg%3D&amp;reserved=0
>>>
>>> During PEI, the MMIO range for the TPM is marked as encrypted when running
>>> as an SEV guest. While this isn't an issue for an SEV guest because of
>>> the way the nested page fault is handled, it does result in an SEV-ES
>>> guest terminating because of a mitigation check in the #VC handler to
>>> prevent MMIO to an encrypted address. For an SEV-ES guest, this range
>>> must be marked as unencrypted.
>>>
>>> Create a new x86 PEIM for TPM support that will map the TPM MMIO range as
>>> unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI
>>> will be unconditionally installed before exiting. The PEIM will exit with
>>> the EFI_ABORTED status so that the PEIM does not stay resident.
>>

...

>>> +
>>> +  //
>>> +  // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical
>>> +  // address because the nested page fault (NPF) that occurs on access does not
>>> +  // include the encryption bit in the guest physical address provided to the
>>> +  // hypervisor.
>>> +  //
>>> +  // However, if SEV-ES is active, before performing the actual MMIO, an
>>> +  // additional MMIO mitigation check is performed in the #VC handler to ensure
>>> +  // that MMIO is being done to an unencrypted address. To prevent guest
>>> +  // termination in this scenario, mark the range unencrypted ahead of access.
>>> +  //
>>
>> Lovely comment, thanks!

I'm going to expand on this a bit more to really show the distinction
between SEV and SEV-ES when it comes to MMIO. Look for a bit more info in v3.

Thanks,
Tom

>>
>>> +  if (MemEncryptSevEsIsEnabled ()) {
>>> +    DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", __FUNCTION__));
>>> +
>>> +    DecryptStatus = MemEncryptSevClearPageEncMask (
>>> +                      0,
>>> +                      PcdGet64 (PcdTpmBaseAddress),
>>
>> (11) The INF file says [FixedPcd], so it would be cleanest to say
>> FixedPcdGet64() here.
> 
> Will do.
> 
>>
>>
>> (12) PcdLib is missing from both the [LibraryClasses] section and the
>> #include directives.
> 
> Right, I'll update that.
> 
>>
>>
>>> +                      EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
>>> +                      FALSE
>>> +                      );
>>> +
>>> +    if (RETURN_ERROR (DecryptStatus)) {
>>> +      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));
>>
>> (13) Overlong line.
> 
> Ok, I'll change that. I though that was ok now since PatchCheck.py didn't
> complain.
> 
>>
>>
>> (14) Please report errors with DEBUG_ERROR.
> 
> Yup, will change.
> 
> Thanks,
> Tom
> 
>>
>>
>>> +      ASSERT_RETURN_ERROR (DecryptStatus);
>>> +    }
>>> +  }
>>> +
>>> +  //
>>> +  // MMIO range available
>>> +  //
>>> +  Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible);
>>> +  ASSERT_EFI_ERROR (Status);
>>> +
>>> +  return EFI_ABORTED;
>>> +}
>>>
>>
>> Thanks!
>> Laszlo
>>
>>
>>
>> 
>>
>>

Re: [edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

[Laszlo Ersek]

I need to excuse myself for two items here, where your expectation was
justified:

On 04/28/21 21:43, Tom Lendacky wrote:
> On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote:
>> I'm going to ask for v3 after all:
>>
>> On 04/27/21 18:21, Lendacky, Thomas wrote:

>>> @@ -627,6 +627,7 @@ [Components]
>>>  
>>>  !if $(TPM_ENABLE) == TRUE
>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>>      <LibraryClasses>
>>
>> (5) Functionally correct, but it reads more nicely (from a logical
>> dependency POV) if we place the new PEIM first.
>>
>> (Please apply to the rest of the DSC files, and the FDF files too.)
> 
> Ok, I was going with the alphabetical placement. I'll switch it up.

Well, you are not wrong; what I request for ordering between lib classes
and between FDF/DSC entries is indeed inconsistent. I guess for lib
classes, showing the construction order makes little, as that is
determined with respect to particular library instances, plus we wrangle
lib classes all the time, and keeping consistency is simplest with
alphabetical ordering. Dispatch order is *somewhat* more directly
visible in a FDF file... but yes, keeping INF references in alphabetical
order there too would certainly plausible

>>> +      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));
>>
>> (13) Overlong line.
> 
> Ok, I'll change that. I though that was ok now since PatchCheck.py didn't
> complain.

Sorry about my pickiness; this is a point where I strongly disagree with
the rest of the edk2 maintainers -- I really do insist on 80 chars per
line, as my eyesight isn't the greatest, I totally depend on two code
windows being shown side by side, and I *also* can't work with multiple
monitors (I've tried it, I just can't). So... one monitor, mid-size
fonts, two columns of text --> 80 chars per column.

Thanks & sorry about the trouble,
Laszlo

Re: [edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

[Lendacky, Thomas]

On 4/30/21 10:48 AM, Laszlo Ersek wrote:
> I need to excuse myself for two items here, where your expectation was
> justified:

No worries, I'm flexible!

Thanks,
Tom

> 
> On 04/28/21 21:43, Tom Lendacky wrote:
>> On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote:
>>> I'm going to ask for v3 after all:
>>>
>>> On 04/27/21 18:21, Lendacky, Thomas wrote:
> 
>>>> @@ -627,6 +627,7 @@ [Components]
>>>>  
>>>>  !if $(TPM_ENABLE) == TRUE
>>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>>> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>>>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>>>>      <LibraryClasses>
>>>
>>> (5) Functionally correct, but it reads more nicely (from a logical
>>> dependency POV) if we place the new PEIM first.
>>>
>>> (Please apply to the rest of the DSC files, and the FDF files too.)
>>
>> Ok, I was going with the alphabetical placement. I'll switch it up.
> 
> Well, you are not wrong; what I request for ordering between lib classes
> and between FDF/DSC entries is indeed inconsistent. I guess for lib
> classes, showing the construction order makes little, as that is
> determined with respect to particular library instances, plus we wrangle
> lib classes all the time, and keeping consistency is simplest with
> alphabetical ordering. Dispatch order is *somewhat* more directly
> visible in a FDF file... but yes, keeping INF references in alphabetical
> order there too would certainly plausible
> 
>>>> +      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));
>>>
>>> (13) Overlong line.
>>
>> Ok, I'll change that. I though that was ok now since PatchCheck.py didn't
>> complain.
> 
> Sorry about my pickiness; this is a point where I strongly disagree with
> the rest of the edk2 maintainers -- I really do insist on 80 chars per
> line, as my eyesight isn't the greatest, I totally depend on two code
> windows being shown side by side, and I *also* can't work with multiple
> monitors (I've tried it, I just can't). So... one monitor, mid-size
> fonts, two columns of text --> 80 chars per column.
> 
> Thanks & sorry about the trouble,
> Laszlo
>