* [PATCH 0/3] SEV-ES TPM enablement fixes
@ 2021-04-20 22:54 Lendacky, Thomas
2021-04-20 22:54 ` [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes Lendacky, Thomas
` (3 more replies)
0 siblings, 4 replies; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-20 22:54 UTC (permalink / raw)
To: devel
Cc: Joerg Roedel, Borislav Petkov, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
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.
- Mark TPM MMIO range as un-encrypted during PEI phase.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
---
These patches are based on commit:
d3b0d007a135 ("BaseTools: Add double quote around CLANG_BIN path string")
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: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Tom Lendacky (3):
OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 106 +++++++++++++++++-
OvmfPkg/PlatformPei/AmdSev.c | 19 ++++
3 files changed, 124 insertions(+), 2 deletions(-)
--
2.31.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
2021-04-20 22:54 [PATCH 0/3] SEV-ES TPM enablement fixes Lendacky, Thomas
@ 2021-04-20 22:54 ` Lendacky, Thomas
2021-04-22 5:28 ` [edk2-devel] " Laszlo Ersek
2021-04-20 22:54 ` [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes Lendacky, Thomas
` (2 subsequent siblings)
3 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-20 22:54 UTC (permalink / raw)
To: devel
Cc: Joerg Roedel, Borislav Petkov, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
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: 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 | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd65..273f36499988 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_INFO, "MMIO using encrypted memory: %lx\n", 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
2021-04-20 22:54 [PATCH 0/3] SEV-ES TPM enablement fixes Lendacky, Thomas
2021-04-20 22:54 ` [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes Lendacky, Thomas
@ 2021-04-20 22:54 ` Lendacky, Thomas
2021-04-22 5:50 ` [edk2-devel] " Laszlo Ersek
2021-04-20 22:54 ` [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV Lendacky, Thomas
[not found] ` <1677B2EC90F30786.1355@groups.io>
3 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-20 22:54 UTC (permalink / raw)
To: devel
Cc: Joerg Roedel, Borislav Petkov, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
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: 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 | 99 +++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 273f36499988..f9660b757d8e 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,51 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
+
+ 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 +855,58 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
+
+ if (InstructionData->AddrSize == Size8Bits) {
+ Address = *(UINT8 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size16Bits) {
+ Address = *(UINT16 *) InstructionData->Immediate;
+ } else if (InstructionData->AddrSize == Size32Bits) {
+ Address = *(UINT32 *) InstructionData->Immediate;
+ } else {
+ Address = *(UINTN *) InstructionData->Immediate;
+ }
+
+ 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 +984,7 @@ MmioExit (
break;
default:
+ DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
Status = GP_EXCEPTION;
ASSERT (FALSE);
}
--
2.31.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-20 22:54 [PATCH 0/3] SEV-ES TPM enablement fixes Lendacky, Thomas
2021-04-20 22:54 ` [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes Lendacky, Thomas
2021-04-20 22:54 ` [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes Lendacky, Thomas
@ 2021-04-20 22:54 ` Lendacky, Thomas
2021-04-20 23:17 ` Eric van Tassell
2021-04-23 10:26 ` Laszlo Ersek
[not found] ` <1677B2EC90F30786.1355@groups.io>
3 siblings, 2 replies; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-20 22:54 UTC (permalink / raw)
To: devel
Cc: Joerg Roedel, Borislav Petkov, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
From: Tom Lendacky <thomas.lendacky@amd.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.
Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.
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: 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/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]
[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;
//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}
+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
//
// Check and perform SEV-ES initialization if required.
//
--
2.31.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
[not found] ` <1677B2EC90F30786.1355@groups.io>
@ 2021-04-20 23:13 ` Lendacky, Thomas
2021-04-22 7:34 ` Laszlo Ersek
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-20 23:13 UTC (permalink / raw)
To: devel
Cc: Joerg Roedel, Borislav Petkov, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>
> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
> guest will fail attempting to perform MMIO to an encrypted address.
>
> Read the PcdTpmBaseAddress and mark the specification defined range
> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
> the MMIO requests.
>
> 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: 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/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6ef77ba7bb21..de60332e9390 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -113,6 +113,7 @@ [Pcd]
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index dddffdebda4b..d524929f9e10 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -141,6 +141,7 @@ AmdSevInitialize (
> )
> {
> UINT64 EncryptionMask;
> + UINT64 TpmBaseAddress;
> RETURN_STATUS PcdStatus;
>
> //
> @@ -206,6 +207,24 @@ AmdSevInitialize (
> }
> }
>
> + //
> + // PEI TPM support will perform MMIO accesses, be sure this range is not
> + // marked encrypted.
> + //
> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
> + if (TpmBaseAddress != 0) {
> + RETURN_STATUS DecryptStatus;
> +
> + DecryptStatus = MemEncryptSevClearPageEncMask (
> + 0,
> + TpmBaseAddress,
> + EFI_SIZE_TO_PAGES (0x5000),
> + FALSE
> + );
> +
> + ASSERT_RETURN_ERROR (DecryptStatus);
> + }
> +
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten by
not having the address marked as shared/unencrypted.
I was also thinking of just marking everything above the highest system
memory address below 4GB and up to the PcdOvmfFdBaseAddress as
unencrypted. That also works.
Once DXE starts it takes care of all this and there isn't an issue in DXE.
This is just for PEI.
Thoughts? Other suggestions?
Thanks,
Tom
> //
> // Check and perform SEV-ES initialization if required.
> //
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-20 22:54 ` [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV Lendacky, Thomas
@ 2021-04-20 23:17 ` Eric van Tassell
2021-04-21 14:09 ` [edk2-devel] " Andrew Fish
` (2 more replies)
2021-04-23 10:26 ` Laszlo Ersek
1 sibling, 3 replies; 39+ messages in thread
From: Eric van Tassell @ 2021-04-20 23:17 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Laszlo Ersek, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/20/21 5:54 PM, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>
> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
where are the phases defined and how many other are there?
> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
> guest will fail attempting to perform MMIO to an encrypted address.
>
> Read the PcdTpmBaseAddress and mark the specification defined range
> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
> the MMIO requests.
>
> 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: 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/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6ef77ba7bb21..de60332e9390 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -113,6 +113,7 @@ [Pcd]
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index dddffdebda4b..d524929f9e10 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -141,6 +141,7 @@ AmdSevInitialize (
> )
> {
> UINT64 EncryptionMask;
> + UINT64 TpmBaseAddress;
> RETURN_STATUS PcdStatus;
>
> //
> @@ -206,6 +207,24 @@ AmdSevInitialize (
> }
> }
>
> + //
> + // PEI TPM support will perform MMIO accesses, be sure this range is not
> + // marked encrypted.
> + //
> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
> + if (TpmBaseAddress != 0) {
> + RETURN_STATUS DecryptStatus;
> +
> + DecryptStatus = MemEncryptSevClearPageEncMask (
> + 0,
> + TpmBaseAddress,
> + EFI_SIZE_TO_PAGES (0x5000),
> + FALSE
> + );
> +
> + ASSERT_RETURN_ERROR (DecryptStatus);
> + }
> +
> //
> // Check and perform SEV-ES initialization if required.
> //
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-20 23:17 ` Eric van Tassell
@ 2021-04-21 14:09 ` Andrew Fish
[not found] ` <1677E4DA25FD7265.31957@groups.io>
2021-04-22 6:07 ` Laszlo Ersek
2 siblings, 0 replies; 39+ messages in thread
From: Andrew Fish @ 2021-04-21 14:09 UTC (permalink / raw)
To: devel, evantass
Cc: Tom Lendacky, Joerg Roedel, Borislav Petkov, Laszlo Ersek,
Ard Biesheuvel, Jordan Justen, Brijesh Singh, James Bottomley,
Jiewen Yao, Min Xu
[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]
https://edk2-docs.gitbook.io/edk-ii-build-specification/2_design_discussion/23_boot_sequence
> On Apr 20, 2021, at 11:34 PM, Eric van Tassell <evantass@amd.com> wrote:
>
>
>
>> On 4/20/21 5:54 PM, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>
> where are the phases defined and how many other are there?
>
>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>> guest will fail attempting to perform MMIO to an encrypted address.
>> Read the PcdTpmBaseAddress and mark the specification defined range
>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>> the MMIO requests.
>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>> 2 files changed, 20 insertions(+)
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 6ef77ba7bb21..de60332e9390 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -113,6 +113,7 @@ [Pcd]
>> [FixedPcd]
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index dddffdebda4b..d524929f9e10 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>> )
>> {
>> UINT64 EncryptionMask;
>> + UINT64 TpmBaseAddress;
>> RETURN_STATUS PcdStatus;
>> //
>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>> }
>> }
>> + //
>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>> + // marked encrypted.
>> + //
>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>> + if (TpmBaseAddress != 0) {
>> + RETURN_STATUS DecryptStatus;
>> +
>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>> + 0,
>> + TpmBaseAddress,
>> + EFI_SIZE_TO_PAGES (0x5000),
>> + FALSE
>> + );
>> +
>> + ASSERT_RETURN_ERROR (DecryptStatus);
>> + }
>> +
>> //
>> // Check and perform SEV-ES initialization if required.
>> //
>
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 8205 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
[not found] ` <1677E4DA25FD7265.31957@groups.io>
@ 2021-04-21 17:20 ` Andrew Fish
2021-04-21 17:45 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Andrew Fish @ 2021-04-21 17:20 UTC (permalink / raw)
To: edk2-devel-groups-io, Andrew Fish
Cc: evantass, Tom Lendacky, Joerg Roedel, Borislav Petkov,
Laszlo Ersek, Ard Biesheuvel, Jordan Justen, Brijesh Singh,
James Bottomley, Jiewen Yao, Min Xu
[-- Attachment #1: Type: text/plain, Size: 4054 bytes --]
Tom,
The phases are defined by the UEFI Platform Initialization Specification [1] (PI Spec). Basically the UEFI Specification defines how to write EFI OS Loaders and Option ROMs and EFI is just defined in the context of how EFI services are passed into applications or drivers. The UEFI Platform Initialization Specification is how to write modular bits of the firmware that interoperate. So all PI systems produce UEFI, but not all UEFI systems are built out of PI. There are also some schemes that use the early parts of PI, but not all of it but this is confusing enough without talking about that.
[1] https://uefi.org/specifications
Thanks,
Andrew Fish
> On Apr 21, 2021, at 7:09 AM, Andrew Fish via groups.io <afish=apple.com@groups.io> wrote:
>
> https://edk2-docs.gitbook.io/edk-ii-build-specification/2_design_discussion/23_boot_sequence <https://edk2-docs.gitbook.io/edk-ii-build-specification/2_design_discussion/23_boot_sequence>
>
>
>> On Apr 20, 2021, at 11:34 PM, Eric van Tassell <evantass@amd.com> wrote:
>>
>>
>>
>> On 4/20/21 5:54 PM, Tom Lendacky wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>
>> where are the phases defined and how many other are there?
>>
>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>> guest will fail attempting to perform MMIO to an encrypted address.
>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>> the MMIO requests.
>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>> 2 files changed, 20 insertions(+)
>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> index 6ef77ba7bb21..de60332e9390 100644
>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> @@ -113,6 +113,7 @@ [Pcd]
>>> [FixedPcd]
>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>> index dddffdebda4b..d524929f9e10 100644
>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>> )
>>> {
>>> UINT64 EncryptionMask;
>>> + UINT64 TpmBaseAddress;
>>> RETURN_STATUS PcdStatus;
>>> //
>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>> }
>>> }
>>> + //
>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>> + // marked encrypted.
>>> + //
>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>> + if (TpmBaseAddress != 0) {
>>> + RETURN_STATUS DecryptStatus;
>>> +
>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>> + 0,
>>> + TpmBaseAddress,
>>> + EFI_SIZE_TO_PAGES (0x5000),
>>> + FALSE
>>> + );
>>> +
>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>> + }
>>> +
>>> //
>>> // Check and perform SEV-ES initialization if required.
>>> //
>>
>>
>>
>>
>>
>
[-- Attachment #2: Type: text/html, Size: 14266 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-21 17:20 ` Andrew Fish
@ 2021-04-21 17:45 ` Lendacky, Thomas
2021-04-21 22:24 ` Andrew Fish
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-21 17:45 UTC (permalink / raw)
To: Andrew Fish, edk2-devel-groups-io
Cc: evantass, Joerg Roedel, Borislav Petkov, Laszlo Ersek,
Ard Biesheuvel, Jordan Justen, Brijesh Singh, James Bottomley,
Jiewen Yao, Min Xu
On 4/21/21 12:20 PM, Andrew Fish wrote:
> Tom,
I think you meant this for Eric, who orignally asked the question.
Thanks,
Tom
>
> The phases are defined by the UEFI Platform Initialization Specification
> [1] (PI Spec). Basically the UEFI Specification defines how to write EFI
> OS Loaders and Option ROMs and EFI is just defined in the context of how
> EFI services are passed into applications or drivers. The UEFI Platform
> Initialization Specification is how to write modular bits of the firmware
> that interoperate. So all PI systems produce UEFI, but not all UEFI
> systems are built out of PI. There are also some schemes that use the
> early parts of PI, but not all of it but this is confusing enough without
> talking about that.
>
> [1] https://uefi.org/specifications
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fspecifications&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695823638%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zItDTPHlE2ff245VNo1pf6EmpmWk9Huz5HLLCTFQqA0%3D&reserved=0>
>
> Thanks,
>
> Andrew Fish
>
>
>> On Apr 21, 2021, at 7:09 AM, Andrew Fish via groups.io
>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgroups.io%2F&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695833632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fdsetjCVemD2frKffZYzJcWrhGHsIu%2BtnYQDvHnf5RE%3D&reserved=0>
>> <afish=apple.com@groups.io <mailto:afish=apple.com@groups.io>> wrote:
>>
>> https://edk2-docs.gitbook.io/edk-ii-build-specification/2_design_discussion/23_boot_sequence
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2-docs.gitbook.io%2Fedk-ii-build-specification%2F2_design_discussion%2F23_boot_sequence&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695833632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BhCNrQ503bWtQDO%2FwqvHLd5lJeMm2erXW3ToJy8VTJQ%3D&reserved=0>
>>
>>
>>> On Apr 20, 2021, at 11:34 PM, Eric van Tassell <evantass@amd.com
>>> <mailto:evantass@amd.com>> wrote:
>>>
>>>
>>>
>>> On 4/20/21 5:54 PM, Tom Lendacky wrote:
>>>> From: Tom Lendacky <thomas.lendacky@amd.com
>>>> <mailto:thomas.lendacky@amd.com>>
>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695843628%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=9qC2wXomC0SXSzCnl0NxPWbZBydPoe8447j4Sq8%2BaRo%3D&reserved=0>
>>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>>
>>> where are the phases defined and how many other are there?
>>>
>>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>>> guest will fail attempting to perform MMIO to an encrypted address.
>>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>>> the MMIO requests.
>>>> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org
>>>> <mailto:ardb+tianocore@kernel.org>>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com
>>>> <mailto:jordan.l.justen@intel.com>>
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com <mailto:brijesh.singh@amd.com>>
>>>> Cc: James Bottomley <jejb@linux.ibm.com <mailto:jejb@linux.ibm.com>>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
>>>> Cc: Min Xu <min.m.xu@intel.com <mailto:min.m.xu@intel.com>>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com
>>>> <mailto:thomas.lendacky@amd.com>>
>>>> ---
>>>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
>>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>>> 2 files changed, 20 insertions(+)
>>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> index 6ef77ba7bb21..de60332e9390 100644
>>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> @@ -113,6 +113,7 @@ [Pcd]
>>>> [FixedPcd]
>>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>>> index dddffdebda4b..d524929f9e10 100644
>>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>> )
>>>> {
>>>> UINT64 EncryptionMask;
>>>> + UINT64 TpmBaseAddress;
>>>> RETURN_STATUS PcdStatus;
>>>> //
>>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>> }
>>>> }
>>>> + //
>>>> + // PEI TPM support will perform MMIO accesses, be sure this range
>>>> is not
>>>> + // marked encrypted.
>>>> + //
>>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>>> + if (TpmBaseAddress != 0) {
>>>> + RETURN_STATUS DecryptStatus;
>>>> +
>>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>>> + 0,
>>>> + TpmBaseAddress,
>>>> + EFI_SIZE_TO_PAGES (0x5000),
>>>> + FALSE
>>>> + );
>>>> +
>>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>>> + }
>>>> +
>>>> //
>>>> // Check and perform SEV-ES initialization if required.
>>>> //
>>>
>>>
>>>
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-21 17:45 ` Lendacky, Thomas
@ 2021-04-21 22:24 ` Andrew Fish
0 siblings, 0 replies; 39+ messages in thread
From: Andrew Fish @ 2021-04-21 22:24 UTC (permalink / raw)
To: edk2-devel-groups-io, Tom Lendacky
Cc: evantass, Joerg Roedel, Borislav Petkov, Laszlo Ersek,
Ard Biesheuvel, Jordan Justen, Brijesh Singh, James Bottomley,
Jiewen Yao, Min Xu
[-- Attachment #1: Type: text/plain, Size: 9179 bytes --]
Sorry Tom!
It was hard to follow the mangled threading on my iPhone, especially before my 1st cup of coffee!
Thanks,
Andrew Fish
> On Apr 21, 2021, at 10:45 AM, Lendacky, Thomas <thomas.lendacky@amd.com> wrote:
>
> On 4/21/21 12:20 PM, Andrew Fish wrote:
>> Tom,
>
> I think you meant this for Eric, who orignally asked the question.
>
> Thanks,
> Tom
>
>>
>> The phases are defined by the UEFI Platform Initialization Specification
>> [1] (PI Spec). Basically the UEFI Specification defines how to write EFI
>> OS Loaders and Option ROMs and EFI is just defined in the context of how
>> EFI services are passed into applications or drivers. The UEFI Platform
>> Initialization Specification is how to write modular bits of the firmware
>> that interoperate. So all PI systems produce UEFI, but not all UEFI
>> systems are built out of PI. There are also some schemes that use the
>> early parts of PI, but not all of it but this is confusing enough without
>> talking about that.
>>
>> [1] https://uefi.org/specifications
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fspecifications&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695823638%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zItDTPHlE2ff245VNo1pf6EmpmWk9Huz5HLLCTFQqA0%3D&reserved=0 <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fspecifications&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695823638%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zItDTPHlE2ff245VNo1pf6EmpmWk9Huz5HLLCTFQqA0%3D&reserved=0>>
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>
>>> On Apr 21, 2021, at 7:09 AM, Andrew Fish via groups.io <http://groups.io/>
>>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgroups.io%2F&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695833632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fdsetjCVemD2frKffZYzJcWrhGHsIu%2BtnYQDvHnf5RE%3D&reserved=0 <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgroups.io%2F&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695833632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fdsetjCVemD2frKffZYzJcWrhGHsIu%2BtnYQDvHnf5RE%3D&reserved=0>>
>>> <afish=apple.com@groups.io <mailto:afish=apple.com@groups.io> <mailto:afish=apple.com@groups.io <mailto:afish=apple.com@groups.io>>> wrote:
>>>
>>> https://edk2-docs.gitbook.io/edk-ii-build-specification/2_design_discussion/23_boot_sequence <https://edk2-docs.gitbook.io/edk-ii-build-specification/2_design_discussion/23_boot_sequence>
>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2-docs.gitbook.io%2Fedk-ii-build-specification%2F2_design_discussion%2F23_boot_sequence&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695833632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BhCNrQ503bWtQDO%2FwqvHLd5lJeMm2erXW3ToJy8VTJQ%3D&reserved=0 <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2-docs.gitbook.io%2Fedk-ii-build-specification%2F2_design_discussion%2F23_boot_sequence&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695833632%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BhCNrQ503bWtQDO%2FwqvHLd5lJeMm2erXW3ToJy8VTJQ%3D&reserved=0>>
>>>
>>>
>>>> On Apr 20, 2021, at 11:34 PM, Eric van Tassell <evantass@amd.com <mailto:evantass@amd.com>
>>>> <mailto:evantass@amd.com <mailto:evantass@amd.com>>> wrote:
>>>>
>>>>
>>>>
>>>> On 4/20/21 5:54 PM, Tom Lendacky wrote:
>>>>> From: Tom Lendacky <thomas.lendacky@amd.com <mailto:thomas.lendacky@amd.com>
>>>>> <mailto:thomas.lendacky@amd.com <mailto:thomas.lendacky@amd.com>>>
>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345 <https://bugzilla.tianocore.org/show_bug.cgi?id=3345>
>>>>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695843628%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=9qC2wXomC0SXSzCnl0NxPWbZBydPoe8447j4Sq8%2BaRo%3D&reserved=0 <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&data=04%7C01%7Cthomas.lendacky%40amd.com%7C76eda3b94d3e4f66ab4d08d904e9da40%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546224695843628%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=9qC2wXomC0SXSzCnl0NxPWbZBydPoe8447j4Sq8%2BaRo%3D&reserved=0>>
>>>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>>>
>>>> where are the phases defined and how many other are there?
>>>>
>>>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>>>> guest will fail attempting to perform MMIO to an encrypted address.
>>>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>>>> the MMIO requests.
>>>>> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com> <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>>
>>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>
>>>>> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>>
>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>
>>>>> <mailto:jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>>>
>>>>> Cc: Brijesh Singh <brijesh.singh@amd.com <mailto:brijesh.singh@amd.com> <mailto:brijesh.singh@amd.com <mailto:brijesh.singh@amd.com>>>
>>>>> Cc: James Bottomley <jejb@linux.ibm.com <mailto:jejb@linux.ibm.com> <mailto:jejb@linux.ibm.com <mailto:jejb@linux.ibm.com>>>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com> <mailto:jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>>
>>>>> Cc: Min Xu <min.m.xu@intel.com <mailto:min.m.xu@intel.com> <mailto:min.m.xu@intel.com <mailto:min.m.xu@intel.com>>>
>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com <mailto:thomas.lendacky@amd.com>
>>>>> <mailto:thomas.lendacky@amd.com <mailto:thomas.lendacky@amd.com>>>
>>>>> ---
>>>>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
>>>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>>>> 2 files changed, 20 insertions(+)
>>>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> index 6ef77ba7bb21..de60332e9390 100644
>>>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> @@ -113,6 +113,7 @@ [Pcd]
>>>>> [FixedPcd]
>>>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>>>> index dddffdebda4b..d524929f9e10 100644
>>>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>>> )
>>>>> {
>>>>> UINT64 EncryptionMask;
>>>>> + UINT64 TpmBaseAddress;
>>>>> RETURN_STATUS PcdStatus;
>>>>> //
>>>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>>> }
>>>>> }
>>>>> + //
>>>>> + // PEI TPM support will perform MMIO accesses, be sure this range
>>>>> is not
>>>>> + // marked encrypted.
>>>>> + //
>>>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>>>> + if (TpmBaseAddress != 0) {
>>>>> + RETURN_STATUS DecryptStatus;
>>>>> +
>>>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>>>> + 0,
>>>>> + TpmBaseAddress,
>>>>> + EFI_SIZE_TO_PAGES (0x5000),
>>>>> + FALSE
>>>>> + );
>>>>> +
>>>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>>>> + }
>>>>> +
>>>>> //
>>>>> // Check and perform SEV-ES initialization if required.
>>>>> //
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
>
>
[-- Attachment #2: Type: text/html, Size: 18950 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
2021-04-20 22:54 ` [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes Lendacky, Thomas
@ 2021-04-22 5:28 ` Laszlo Ersek
2021-04-22 13:35 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-22 5:28 UTC (permalink / raw)
To: devel, thomas.lendacky
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/21/21 00:54, 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: 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 | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 24259060fd65..273f36499988 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_INFO, "MMIO using encrypted memory: %lx\n", MemoryAddress));
> GpEvent.Uint64 = 0;
> GpEvent.Elements.Vector = GP_EXCEPTION;
> GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
(1) This can potentially generate a large number of debug messages;
please use the DEBUG_VERBOSE log mask.
(2) "MemoryAddress" has type UINTN, but %lx takes UINT64. Given that
this is X64-only code, functionally there is no bug, but it's still
cleaner to pass "(UINT64)MemoryAddress" to %lx.
With that:
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> @@ -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;
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
2021-04-20 22:54 ` [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes Lendacky, Thomas
@ 2021-04-22 5:50 ` Laszlo Ersek
2021-04-22 14:15 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-22 5:50 UTC (permalink / raw)
To: devel, thomas.lendacky
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/21/21 00:54, 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: 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 | 99 +++++++++++++++++++
> 1 file changed, 99 insertions(+)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 273f36499988..f9660b757d8e 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,51 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
> +
> + if (InstructionData->AddrSize == Size8Bits) {
> + Address = *(UINT8 *) InstructionData->Immediate;
> + } else if (InstructionData->AddrSize == Size16Bits) {
> + Address = *(UINT16 *) InstructionData->Immediate;
> + } else if (InstructionData->AddrSize == Size32Bits) {
> + Address = *(UINT32 *) InstructionData->Immediate;
> + } else {
> + Address = *(UINTN *) InstructionData->Immediate;
> + }
(1) Can we simplify this as follows?
InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
InstructionData->End += InstructionData->ImmediateSize;
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 +855,58 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
> +
> + if (InstructionData->AddrSize == Size8Bits) {
> + Address = *(UINT8 *) InstructionData->Immediate;
> + } else if (InstructionData->AddrSize == Size16Bits) {
> + Address = *(UINT16 *) InstructionData->Immediate;
> + } else if (InstructionData->AddrSize == Size32Bits) {
> + Address = *(UINT32 *) InstructionData->Immediate;
> + } else {
> + Address = *(UINTN *) InstructionData->Immediate;
> + }
(2) Similar question as (1).
> +
> + 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;
> + }
(3) This is also seen with opcode 0x8B, but can you remind me please why
we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
> + CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> //
> // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
> //
> @@ -886,6 +984,7 @@ MmioExit (
> break;
>
> default:
> + DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
> Status = GP_EXCEPTION;
> ASSERT (FALSE);
> }
>
(4) We should use the DEBUG_ERROR log mask here.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-20 23:17 ` Eric van Tassell
2021-04-21 14:09 ` [edk2-devel] " Andrew Fish
[not found] ` <1677E4DA25FD7265.31957@groups.io>
@ 2021-04-22 6:07 ` Laszlo Ersek
2 siblings, 0 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-22 6:07 UTC (permalink / raw)
To: devel, evantass, Tom Lendacky
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/21/21 01:17, Eric van Tassell wrote:
>
>
> On 4/20/21 5:54 PM, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>>
>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>
> where are the phases defined and how many other are there?
See "Figure 1. PI Architecture Firmware Phases" in the PI 1.7 spec,
Volume 1, chapter 2 Overview.
One resource for learning more about the phases is:
https://github.com/tianocore/tianocore.github.io/wiki/UEFI-EDKII-Learning-Dev
Thanks
Laszlo
>
>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>> guest will fail attempting to perform MMIO to an encrypted address.
>>
>> Read the PcdTpmBaseAddress and mark the specification defined range
>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>> the MMIO requests.
>>
>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 6ef77ba7bb21..de60332e9390 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -113,6 +113,7 @@ [Pcd]
>> [FixedPcd]
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index dddffdebda4b..d524929f9e10 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>> )
>> {
>> UINT64 EncryptionMask;
>> + UINT64 TpmBaseAddress;
>> RETURN_STATUS PcdStatus;
>> //
>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>> }
>> }
>> + //
>> + // PEI TPM support will perform MMIO accesses, be sure this range
>> is not
>> + // marked encrypted.
>> + //
>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>> + if (TpmBaseAddress != 0) {
>> + RETURN_STATUS DecryptStatus;
>> +
>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>> + 0,
>> + TpmBaseAddress,
>> + EFI_SIZE_TO_PAGES (0x5000),
>> + FALSE
>> + );
>> +
>> + ASSERT_RETURN_ERROR (DecryptStatus);
>> + }
>> +
>> //
>> // Check and perform SEV-ES initialization if required.
>> //
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-20 23:13 ` Lendacky, Thomas
@ 2021-04-22 7:34 ` Laszlo Ersek
2021-04-22 8:31 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-22 7:34 UTC (permalink / raw)
To: devel, thomas.lendacky
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/21/21 01:13, Lendacky, Thomas wrote:
> On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>>
>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>> guest will fail attempting to perform MMIO to an encrypted address.
(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?
>>
>> Read the PcdTpmBaseAddress and mark the specification defined range
>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>> the MMIO requests.
>>
>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 6ef77ba7bb21..de60332e9390 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -113,6 +113,7 @@ [Pcd]
>>
>> [FixedPcd]
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index dddffdebda4b..d524929f9e10 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>> )
>> {
>> UINT64 EncryptionMask;
>> + UINT64 TpmBaseAddress;
>> RETURN_STATUS PcdStatus;
>>
>> //
>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>> }
>> }
>>
>> + //
>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>> + // marked encrypted.
>> + //
>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>> + if (TpmBaseAddress != 0) {
>> + RETURN_STATUS DecryptStatus;
>> +
>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>> + 0,
>> + TpmBaseAddress,
>> + EFI_SIZE_TO_PAGES (0x5000),
>> + FALSE
>> + );
>> +
>> + ASSERT_RETURN_ERROR (DecryptStatus);
>> + }
>> +
>
> Laszlo, I'm not sure if this is the best way to approach this. It is
> simple and straight forward and the TCG/TPM support is one of the few
> (only?) pieces of code that does actual MMIO during PEI that is bitten
> by not having the address marked as shared/unencrypted.
In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?
Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.
Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
The driver uses the following library instance:
SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
This library instance is what depends on "PcdTpmBaseAddress".
And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.
I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)
(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index 6776ec931ce0..0d0572b83599 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -20,13 +20,16 @@ [Defines]
> ENTRY_POINT = Tcg2ConfigPeimEntryPoint
>
> [Sources]
> + MemEncrypt.h
> Tcg2ConfigPeim.c
> Tpm12Support.h
>
> [Sources.IA32, Sources.X64]
> + MemEncryptSev.c
> Tpm12Support.c
>
> [Sources.ARM, Sources.AARCH64]
> + MemEncryptNull.c
> Tpm12SupportNull.c
>
> [Packages]
> @@ -43,6 +46,7 @@ [LibraryClasses]
>
> [LibraryClasses.IA32, LibraryClasses.X64]
> BaseLib
> + MemEncryptSevLib
> Tpm12DeviceLib
>
> [Guids]
> @@ -56,6 +60,9 @@ [Ppis]
> [Pcd]
> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
>
> +[Pcd.IA32, Pcd.X64]
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
> +
> [Depex.IA32, Depex.X64]
> TRUE
>
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
should check MemEncryptSevIsEnabled(), and then make the above-seen
MemEncryptSevClearPageEncMask() call.
The new InternalTpmDecryptAddressRange() function should be called from
Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().
(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the library
*class* carries SEV in the name, which is inherently X64-specific, thus
I wouldn't even like the lib *class* to leak into ArmVirtPkg.)
(3) If the approach in (2) works, then please don't forget to update the
patch subject (it currently refers to PlatformPei).
(4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
it to UINTN.
(In fact I would prefer a new macro for 0x5000, somewhere in the
"MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
SecurityPkg already open-codes the 0x5000 constant in
"Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-22 7:34 ` Laszlo Ersek
@ 2021-04-22 8:31 ` Laszlo Ersek
2021-04-22 8:39 ` Laszlo Ersek
2021-04-22 14:51 ` Lendacky, Thomas
2 siblings, 0 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-22 8:31 UTC (permalink / raw)
To: devel, thomas.lendacky
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/22/21 09:34, Laszlo Ersek wrote:
> Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
>
> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>
> The driver uses the following library instance:
>
> SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>
> This library instance is what depends on "PcdTpmBaseAddress".
>
> And it's not just that decrypting the TPM MMIO range in PlatformPei
> "looks awkward", but I don't even see it immediately why PlatformPei
> is guaranteed to be dispatched before Tcg2ConfigPei. The effective
> depex of Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according
> to the build report file. If Tcg2ConfigPei runs first, whatever we do
> in PlatformPei is too late.
>
> I also don't like that, with this patch, we'd decrypt the TPM range
> even if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
> "PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
> default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
> set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
> too.)
>
>
> (2) So, can you please try the following, in the
> "OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
>
>> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> index 6776ec931ce0..0d0572b83599 100644
>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> @@ -20,13 +20,16 @@ [Defines]
>> ENTRY_POINT = Tcg2ConfigPeimEntryPoint
>>
>> [Sources]
>> + MemEncrypt.h
>> Tcg2ConfigPeim.c
>> Tpm12Support.h
>>
>> [Sources.IA32, Sources.X64]
>> + MemEncryptSev.c
>> Tpm12Support.c
>>
>> [Sources.ARM, Sources.AARCH64]
>> + MemEncryptNull.c
>> Tpm12SupportNull.c
>>
>> [Packages]
>> @@ -43,6 +46,7 @@ [LibraryClasses]
>>
>> [LibraryClasses.IA32, LibraryClasses.X64]
>> BaseLib
>> + MemEncryptSevLib
>> Tpm12DeviceLib
>>
>> [Guids]
>> @@ -56,6 +60,9 @@ [Ppis]
>> [Pcd]
>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
>>
>> +[Pcd.IA32, Pcd.X64]
>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
>> +
>> [Depex.IA32, Depex.X64]
>> TRUE
>>
>
> In the "MemEncrypt.h" file, declare a function called
> InternalTpmDecryptAddressRange(). The function definition in
> "MemEncryptNull.c" should do nothing, while the one in
> "MemEncryptSev.c" should check MemEncryptSevIsEnabled(), and then make
> the above-seen MemEncryptSevClearPageEncMask() call.
>
> The new InternalTpmDecryptAddressRange() function should be called
> from Tcg2ConfigPeimEntryPoint(), before the latter calls
> InternalTpm12Detect(). Regarding error checking... if
> InternalTpmDecryptAddressRange() fails, I think we can log an error
> message, and hang with CpuDeadLoop().
>
> (An alternative approach would be to call MemEncryptSevIsEnabled() and
> MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
> on ARM / AARCH64. In addition to that, we'd have to implement a Null
> instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
> instance in the ArmVirtPkg DSC files. But I don't like that: the
> library *class* carries SEV in the name, which is inherently
> X64-specific, thus I wouldn't even like the lib *class* to leak into
> ArmVirtPkg.)
Here's another thing. Above, I mention that nothing guarantees that
Tcg2ConfigPei runs before PlatformPei. That raises a problem even if we
use approach (2).
In approach (2), we massage page table entries, and ultimately use
OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
for that. But that library instance can allocate full pages, in case
page table splitting is needed (from 1GB to 2MB to 4KB).
I can't tell off-hand if such page table splitting will actually occur
when we decrypt the TPM MMIO address range -- but even if it does not,
for whatever reason, I wouldn't like to rely on that particular reason.
And I definitely don't want such page allocations to be satisfied from
the temporary SEC/PEI heap, before we migrate to permanent PEI RAM. See
how "PcdOvmfSecPeiTempRamBase" and "PcdOvmfSecPeiTempRamSize" are set in
the FDF files, and see the OVMF log too:
> Temp Stack : BaseAddress=0x818000 Length=0x8000
> Temp Heap : BaseAddress=0x810000 Length=0x8000
> Total temporary memory: 65536 bytes.
> temporary memory stack ever used: 30128 bytes.
> temporary memory heap used for HobList: 7208 bytes.
> temporary memory heap occupied by memory pages: 0 bytes.
What I'm saying is that we've probably been missing the following hunk
for a long time now:
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> index 03a78c32df28..1b3808305415 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> @@ -55,3 +55,6 @@ [FeaturePcd]
>
> [FixedPcd]
> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
> +
> +[Depex]
> + gEfiPeiMemoryDiscoveredPpiGuid
In other words, whatever PEIM uses the PeiMemEncryptSevLib instance,
should be delayed until PlatformPei installs the permanent PEI RAM, by
inheriting a gEfiPeiMemoryDiscoveredPpiGuid depex from
PeiMemEncryptSevLib.
... Unfortunately, this wouldn't work, because PlatformPei itself uses
PeiMemEncryptSevLib [*], so we'd create a circular dependency.
[*] first from commit 13b5d743c87a ("OvmfPkg/PlatformPei: Set memory
encryption PCD when SEV is enabled", 2017-07-10), which called
MemEncryptSevIsEnabled(),
and then from commit 449a6e493418 ("OvmfPkg: Create GHCB pages for
use during Pei and Dxe phase", 2020-08-17), which even called
MemEncryptSevClearPageEncMask() -- but note that AmdSevInitialize()
is called *after* PublishPeiMemory(), in PlatformPei!
So, we can't add this "permanent PEI RAM" dependency to
"PeiMemEncryptSevLib.inf" directly. Instead, as a work-around, we should
add the dependency to "Tcg2ConfigPei".
(5a) So ultimately, please update the "Tcg2ConfigPei.inf" file like
this:
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index 6776ec931ce0..6605b9bbaf91 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -20,13 +20,16 @@ [Defines]
> ENTRY_POINT = Tcg2ConfigPeimEntryPoint
>
> [Sources]
> + MemEncrypt.h
> Tcg2ConfigPeim.c
> Tpm12Support.h
>
> [Sources.IA32, Sources.X64]
> + MemEncryptSev.c
> Tpm12Support.c
>
> [Sources.ARM, Sources.AARCH64]
> + MemEncryptNull.c
> Tpm12SupportNull.c
>
> [Packages]
> @@ -43,6 +46,7 @@ [LibraryClasses]
>
> [LibraryClasses.IA32, LibraryClasses.X64]
> BaseLib
> + MemEncryptSevLib
> Tpm12DeviceLib
>
> [Guids]
> @@ -56,8 +60,11 @@ [Ppis]
> [Pcd]
> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
>
> +[Pcd.IA32, Pcd.X64]
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
> +
> [Depex.IA32, Depex.X64]
> - TRUE
> + gEfiPeiMemoryDiscoveredPpiGuid
>
> [Depex.ARM, Depex.AARCH64]
> gOvmfTpmDiscoveredPpiGuid
(5b) And in the commit message, please state that:
We don't want PeiMemEncryptSevLib to allocate any pages, for
potential page table splitting, from the temporary SEC/PEI heap. But
we can't make PeiMemEncryptSevLib itself depend on
gEfiPeiMemoryDiscoveredPpiGuid, because PlatformPei, which installs
the permanent PEI RAM, consumes PeiMemEncryptSevLib too. Thus,
restrict the DEPEX of Tcg2ConfigPei directly.
--*--
(
Note that, in OVMF, the other PEIM that (indirectly) uses
PeiMemEncryptSevLib is "UefiCpuPkg/CpuMpPei/CpuMpPei.inf". But, the
effective initialization of that PEIM is already delayed until after the
permanent PEI RAM is installed -- not with a depex, but with a notify
callback.
Also note that the library instance
OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
doesn't support manipulating the page tables at all, and so it doesn't
need to allocate memory for page table splitting either. So that's good.
)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-22 7:34 ` Laszlo Ersek
2021-04-22 8:31 ` Laszlo Ersek
@ 2021-04-22 8:39 ` Laszlo Ersek
2021-04-22 19:10 ` Lendacky, Thomas
2021-04-22 14:51 ` Lendacky, Thomas
2 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-22 8:39 UTC (permalink / raw)
To: devel, thomas.lendacky
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/22/21 09:34, Laszlo Ersek wrote:
> The new InternalTpmDecryptAddressRange() function should be called
> from Tcg2ConfigPeimEntryPoint(), before the latter calls
> InternalTpm12Detect(). Regarding error checking... if
> InternalTpmDecryptAddressRange() fails, I think we can log an error
> message, and hang with CpuDeadLoop().
Sorry, another point:
(6) where we determine that no TPM is available:
//
// If no TPM2 was detected, we still need to install
// TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon seeing
// the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus we have
// to install the PPI in its place, in order to unblock any dependent
// PEIMs.
//
Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);
we should re-encrypt the address range, as if nothing had happened.
For this, we'll likely need a similarly polymorphic function called
InternalTpmEncryptAddressRange().
(
For some background on this particular branch of the code, please refer
to commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
2018-03-09):
- Check the QEMU hardware for TPM2 availability only
- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
&gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
the firmware about the TPM type.
- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
and the consumption of the "TPM type" PCD.
- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
(Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)
)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
2021-04-22 5:28 ` [edk2-devel] " Laszlo Ersek
@ 2021-04-22 13:35 ` Lendacky, Thomas
2021-04-23 9:07 ` Laszlo Ersek
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-22 13:35 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/22/21 12:28 AM, Laszlo Ersek wrote:
> On 04/21/21 00:54, 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C22bf3a3ae9cb4421e93208d9054f79c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546661229697941%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1EmUDf%2FfuCuu%2BkXPZijzatfliplMhKEQH8kiZ9Z8ZF0%3D&reserved=0
>>
>> 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: 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 | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> index 24259060fd65..273f36499988 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_INFO, "MMIO using encrypted memory: %lx\n", MemoryAddress));
>> GpEvent.Uint64 = 0;
>> GpEvent.Elements.Vector = GP_EXCEPTION;
>> GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
>
> (1) This can potentially generate a large number of debug messages;
> please use the DEBUG_VERBOSE log mask.
Actually, you will see this only once since the code will propagate a GP
and the guest will terminate in this situation.
>
> (2) "MemoryAddress" has type UINTN, but %lx takes UINT64. Given that
> this is X64-only code, functionally there is no bug, but it's still
> cleaner to pass "(UINT64)MemoryAddress" to %lx.
Will do.
Thanks,
Tom
>
> With that:
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks
> Laszlo
>
>
>> @@ -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;
>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
2021-04-22 5:50 ` [edk2-devel] " Laszlo Ersek
@ 2021-04-22 14:15 ` Lendacky, Thomas
2021-04-22 15:42 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-22 14:15 UTC (permalink / raw)
To: devel, lersek
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
> On 04/21/21 00:54, 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&reserved=0
>>
>> 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: 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 | 99 +++++++++++++++++++
>> 1 file changed, 99 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> index 273f36499988..f9660b757d8e 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,51 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>> +
>> + if (InstructionData->AddrSize == Size8Bits) {
>> + Address = *(UINT8 *) InstructionData->Immediate;
>> + } else if (InstructionData->AddrSize == Size16Bits) {
>> + Address = *(UINT16 *) InstructionData->Immediate;
>> + } else if (InstructionData->AddrSize == Size32Bits) {
>> + Address = *(UINT32 *) InstructionData->Immediate;
>> + } else {
>> + Address = *(UINTN *) InstructionData->Immediate;
>> + }
>
> (1) Can we simplify this as follows?
>
> InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
> InstructionData->End += InstructionData->ImmediateSize;
> Address = 0;
> CopyMem (&Address, InstructionData->Immediate,
> InstructionData->ImmediateSize);
Yup, that can be done.
>
>> +
>> + 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 +855,58 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>> +
>> + if (InstructionData->AddrSize == Size8Bits) {
>> + Address = *(UINT8 *) InstructionData->Immediate;
>> + } else if (InstructionData->AddrSize == Size16Bits) {
>> + Address = *(UINT16 *) InstructionData->Immediate;
>> + } else if (InstructionData->AddrSize == Size32Bits) {
>> + Address = *(UINT32 *) InstructionData->Immediate;
>> + } else {
>> + Address = *(UINTN *) InstructionData->Immediate;
>> + }
>
> (2) Similar question as (1).
Will do.
>
>> +
>> + 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;
>> + }
>
> (3) This is also seen with opcode 0x8B, but can you remind me please why
> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
32-bit operand size the 32-bit register results are zero-extended to 64-bits.
>
>> + CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> //
>> // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>> //
>> @@ -886,6 +984,7 @@ MmioExit (
>> break;
>>
>> default:
>> + DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>> Status = GP_EXCEPTION;
>> ASSERT (FALSE);
>> }
>>
>
> (4) We should use the DEBUG_ERROR log mask here.
Will change.
Thanks,
Tom
>
> Thanks
> Laszlo
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-22 7:34 ` Laszlo Ersek
2021-04-22 8:31 ` Laszlo Ersek
2021-04-22 8:39 ` Laszlo Ersek
@ 2021-04-22 14:51 ` Lendacky, Thomas
2021-04-22 16:04 ` Lendacky, Thomas
2 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-22 14:51 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/22/21 2:34 AM, Laszlo Ersek wrote:
> On 04/21/21 01:13, Lendacky, Thomas wrote:
>> On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&reserved=0
>>>
>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>> guest will fail attempting to perform MMIO to an encrypted address.
>
> (1) The subject says SEV, not SEV-ES, and the code in the patch too
> suggests SEV, not SEV-ES. If that's correct, can you please update the
> commit message?
Yes, I'll update the commit message. The action is correct for all SEV
guests in general, but it is only with SEV-ES, where the tighter MMIO
checks can be performed, that an actual issue shows up.
>
>>>
>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>> the MMIO requests.
>>>
>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>> 2 files changed, 20 insertions(+)
>>>
>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> index 6ef77ba7bb21..de60332e9390 100644
>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> @@ -113,6 +113,7 @@ [Pcd]
>>>
>>> [FixedPcd]
>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>> index dddffdebda4b..d524929f9e10 100644
>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>> )
>>> {
>>> UINT64 EncryptionMask;
>>> + UINT64 TpmBaseAddress;
>>> RETURN_STATUS PcdStatus;
>>>
>>> //
>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>> }
>>> }
>>>
>>> + //
>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>> + // marked encrypted.
>>> + //
>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>> + if (TpmBaseAddress != 0) {
>>> + RETURN_STATUS DecryptStatus;
>>> +
>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>> + 0,
>>> + TpmBaseAddress,
>>> + EFI_SIZE_TO_PAGES (0x5000),
>>> + FALSE
>>> + );
>>> +
>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>> + }
>>> +
>>
>> Laszlo, I'm not sure if this is the best way to approach this. It is
>> simple and straight forward and the TCG/TPM support is one of the few
>> (only?) pieces of code that does actual MMIO during PEI that is bitten
>> by not having the address marked as shared/unencrypted.
>
> In SEC, I think we have MMIO access too (LAPIC --
> InitializeApicTimer()); why does that work?
>
> Hmm... Is that because we're immediately in x2apic mode, and that means
> CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
> decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
> support", 2015-11-30).) And, we have #VC handling in SEC too.
>
> Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
>
> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>
> The driver uses the following library instance:
>
> SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>
> This library instance is what depends on "PcdTpmBaseAddress".
>
> And it's not just that decrypting the TPM MMIO range in PlatformPei
> "looks awkward", but I don't even see it immediately why PlatformPei is
> guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
> Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
> build report file. If Tcg2ConfigPei runs first, whatever we do in
> PlatformPei is too late.
>
> I also don't like that, with this patch, we'd decrypt the TPM range even
> if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
> "PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
> default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
> set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
> too.)
>
>
> (2) So, can you please try the following, in the
> "OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
I'll take the input from each of your emails on this and see how that all
works. Thanks for the insight and knowledge!
Tom
>
>> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> index 6776ec931ce0..0d0572b83599 100644
>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> @@ -20,13 +20,16 @@ [Defines]
>> ENTRY_POINT = Tcg2ConfigPeimEntryPoint
>>
>> [Sources]
>> + MemEncrypt.h
>> Tcg2ConfigPeim.c
>> Tpm12Support.h
>>
>> [Sources.IA32, Sources.X64]
>> + MemEncryptSev.c
>> Tpm12Support.c
>>
>> [Sources.ARM, Sources.AARCH64]
>> + MemEncryptNull.c
>> Tpm12SupportNull.c
>>
>> [Packages]
>> @@ -43,6 +46,7 @@ [LibraryClasses]
>>
>> [LibraryClasses.IA32, LibraryClasses.X64]
>> BaseLib
>> + MemEncryptSevLib
>> Tpm12DeviceLib
>>
>> [Guids]
>> @@ -56,6 +60,9 @@ [Ppis]
>> [Pcd]
>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
>>
>> +[Pcd.IA32, Pcd.X64]
>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
>> +
>> [Depex.IA32, Depex.X64]
>> TRUE
>>
>
> In the "MemEncrypt.h" file, declare a function called
> InternalTpmDecryptAddressRange(). The function definition in
> "MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
> should check MemEncryptSevIsEnabled(), and then make the above-seen
> MemEncryptSevClearPageEncMask() call.
>
> The new InternalTpmDecryptAddressRange() function should be called from
> Tcg2ConfigPeimEntryPoint(), before the latter calls
> InternalTpm12Detect(). Regarding error checking... if
> InternalTpmDecryptAddressRange() fails, I think we can log an error
> message, and hang with CpuDeadLoop().
>
> (An alternative approach would be to call MemEncryptSevIsEnabled() and
> MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
> on ARM / AARCH64. In addition to that, we'd have to implement a Null
> instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
> instance in the ArmVirtPkg DSC files. But I don't like that: the library
> *class* carries SEV in the name, which is inherently X64-specific, thus
> I wouldn't even like the lib *class* to leak into ArmVirtPkg.)
>
>
> (3) If the approach in (2) works, then please don't forget to update the
> patch subject (it currently refers to PlatformPei).
>
>
> (4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
> have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
> it to UINTN.
>
> (In fact I would prefer a new macro for 0x5000, somewhere in the
> "MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
> SecurityPkg already open-codes the 0x5000 constant in
> "Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
2021-04-22 14:15 ` Lendacky, Thomas
@ 2021-04-22 15:42 ` Lendacky, Thomas
2021-04-23 9:10 ` Laszlo Ersek
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-22 15:42 UTC (permalink / raw)
To: devel, lersek
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/22/21 9:15 AM, Tom Lendacky wrote:
> On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
>> On 04/21/21 00:54, 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&reserved=0
>>>
>>> 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: 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 | 99 +++++++++++++++++++
>>> 1 file changed, 99 insertions(+)
>>>
>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>> index 273f36499988..f9660b757d8e 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,51 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>>> +
>>> + if (InstructionData->AddrSize == Size8Bits) {
>>> + Address = *(UINT8 *) InstructionData->Immediate;
>>> + } else if (InstructionData->AddrSize == Size16Bits) {
>>> + Address = *(UINT16 *) InstructionData->Immediate;
>>> + } else if (InstructionData->AddrSize == Size32Bits) {
>>> + Address = *(UINT32 *) InstructionData->Immediate;
>>> + } else {
>>> + Address = *(UINTN *) InstructionData->Immediate;
>>> + }
>>
>> (1) Can we simplify this as follows?
>>
>> InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>> InstructionData->End += InstructionData->ImmediateSize;
>> Address = 0;
>> CopyMem (&Address, InstructionData->Immediate,
>> InstructionData->ImmediateSize);
>
> Yup, that can be done.
"Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
isn't an issue. Should I add a comment about that above the setting of
"Address"? Or should I convert "Address" to a UINT64 - although
ValidateMmioMemory expects a UINTN... Thoughts?
Thanks,
Tom
>
>>
>>> +
>>> + 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 +855,58 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>>> +
>>> + if (InstructionData->AddrSize == Size8Bits) {
>>> + Address = *(UINT8 *) InstructionData->Immediate;
>>> + } else if (InstructionData->AddrSize == Size16Bits) {
>>> + Address = *(UINT16 *) InstructionData->Immediate;
>>> + } else if (InstructionData->AddrSize == Size32Bits) {
>>> + Address = *(UINT32 *) InstructionData->Immediate;
>>> + } else {
>>> + Address = *(UINTN *) InstructionData->Immediate;
>>> + }
>>
>> (2) Similar question as (1).
>
> Will do.
>
>>
>>> +
>>> + 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;
>>> + }
>>
>> (3) This is also seen with opcode 0x8B, but can you remind me please why
>> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
>
> That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
> 32-bit operand size the 32-bit register results are zero-extended to 64-bits.
>
>>
>>> + CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>>> + break;
>>> +
>>> //
>>> // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>> //
>>> @@ -886,6 +984,7 @@ MmioExit (
>>> break;
>>>
>>> default:
>>> + DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>> Status = GP_EXCEPTION;
>>> ASSERT (FALSE);
>>> }
>>>
>>
>> (4) We should use the DEBUG_ERROR log mask here.
>
> Will change.
>
> Thanks,
> Tom
>
>>
>> Thanks
>> Laszlo
>>
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-22 14:51 ` Lendacky, Thomas
@ 2021-04-22 16:04 ` Lendacky, Thomas
0 siblings, 0 replies; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-22 16:04 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/22/21 9:51 AM, Tom Lendacky wrote:
> On 4/22/21 2:34 AM, Laszlo Ersek wrote:
>> On 04/21/21 01:13, Lendacky, Thomas wrote:
>>> On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&reserved=0
>>>>
>>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>>> guest will fail attempting to perform MMIO to an encrypted address.
>>
>> (1) The subject says SEV, not SEV-ES, and the code in the patch too
>> suggests SEV, not SEV-ES. If that's correct, can you please update the
>> commit message?
>
> Yes, I'll update the commit message. The action is correct for all SEV
> guests in general, but it is only with SEV-ES, where the tighter MMIO
> checks can be performed, that an actual issue shows up.
>
>>
>>>>
>>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>>> the MMIO requests.
>>>>
>>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>>> 2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> index 6ef77ba7bb21..de60332e9390 100644
>>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> @@ -113,6 +113,7 @@ [Pcd]
>>>>
>>>> [FixedPcd]
>>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>>> index dddffdebda4b..d524929f9e10 100644
>>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>> )
>>>> {
>>>> UINT64 EncryptionMask;
>>>> + UINT64 TpmBaseAddress;
>>>> RETURN_STATUS PcdStatus;
>>>>
>>>> //
>>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>> }
>>>> }
>>>>
>>>> + //
>>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>>> + // marked encrypted.
>>>> + //
>>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>>> + if (TpmBaseAddress != 0) {
>>>> + RETURN_STATUS DecryptStatus;
>>>> +
>>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>>> + 0,
>>>> + TpmBaseAddress,
>>>> + EFI_SIZE_TO_PAGES (0x5000),
>>>> + FALSE
>>>> + );
>>>> +
>>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>>> + }
>>>> +
>>>
>>> Laszlo, I'm not sure if this is the best way to approach this. It is
>>> simple and straight forward and the TCG/TPM support is one of the few
>>> (only?) pieces of code that does actual MMIO during PEI that is bitten
>>> by not having the address marked as shared/unencrypted.
>>
>> In SEC, I think we have MMIO access too (LAPIC --
>> InitializeApicTimer()); why does that work?
>>
>> Hmm... Is that because we're immediately in x2apic mode, and that means
>> CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
>> decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
>> support", 2015-11-30).) And, we have #VC handling in SEC too.
Missed this question in my earlier reply... LAPIC access has a dedicated
check in ValidateMmioMemory() to allow access in this case.
Thanks,
Tom
>>
>> Anyway: I think the TPM (MMIO) access you see comes from this PEIM:
>>
>> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>
>> The driver uses the following library instance:
>>
>> SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>>
>> This library instance is what depends on "PcdTpmBaseAddress".
>>
>> And it's not just that decrypting the TPM MMIO range in PlatformPei
>> "looks awkward", but I don't even see it immediately why PlatformPei is
>> guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
>> Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
>> build report file. If Tcg2ConfigPei runs first, whatever we do in
>> PlatformPei is too late.
>>
>> I also don't like that, with this patch, we'd decrypt the TPM range even
>> if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
>> "PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
>> default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
>> set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
>> too.)
>>
>>
>> (2) So, can you please try the following, in the
>> "OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
>
> I'll take the input from each of your emails on this and see how that all
> works. Thanks for the insight and knowledge!
>
> Tom
>
>>
>>> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> index 6776ec931ce0..0d0572b83599 100644
>>> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> @@ -20,13 +20,16 @@ [Defines]
>>> ENTRY_POINT = Tcg2ConfigPeimEntryPoint
>>>
>>> [Sources]
>>> + MemEncrypt.h
>>> Tcg2ConfigPeim.c
>>> Tpm12Support.h
>>>
>>> [Sources.IA32, Sources.X64]
>>> + MemEncryptSev.c
>>> Tpm12Support.c
>>>
>>> [Sources.ARM, Sources.AARCH64]
>>> + MemEncryptNull.c
>>> Tpm12SupportNull.c
>>>
>>> [Packages]
>>> @@ -43,6 +46,7 @@ [LibraryClasses]
>>>
>>> [LibraryClasses.IA32, LibraryClasses.X64]
>>> BaseLib
>>> + MemEncryptSevLib
>>> Tpm12DeviceLib
>>>
>>> [Guids]
>>> @@ -56,6 +60,9 @@ [Ppis]
>>> [Pcd]
>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES
>>>
>>> +[Pcd.IA32, Pcd.X64]
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
>>> +
>>> [Depex.IA32, Depex.X64]
>>> TRUE
>>>
>>
>> In the "MemEncrypt.h" file, declare a function called
>> InternalTpmDecryptAddressRange(). The function definition in
>> "MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
>> should check MemEncryptSevIsEnabled(), and then make the above-seen
>> MemEncryptSevClearPageEncMask() call.
>>
>> The new InternalTpmDecryptAddressRange() function should be called from
>> Tcg2ConfigPeimEntryPoint(), before the latter calls
>> InternalTpm12Detect(). Regarding error checking... if
>> InternalTpmDecryptAddressRange() fails, I think we can log an error
>> message, and hang with CpuDeadLoop().
>>
>> (An alternative approach would be to call MemEncryptSevIsEnabled() and
>> MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
>> on ARM / AARCH64. In addition to that, we'd have to implement a Null
>> instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
>> instance in the ArmVirtPkg DSC files. But I don't like that: the library
>> *class* carries SEV in the name, which is inherently X64-specific, thus
>> I wouldn't even like the lib *class* to leak into ArmVirtPkg.)
>>
>>
>> (3) If the approach in (2) works, then please don't forget to update the
>> patch subject (it currently refers to PlatformPei).
>>
>>
>> (4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
>> have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
>> it to UINTN.
>>
>> (In fact I would prefer a new macro for 0x5000, somewhere in the
>> "MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
>> SecurityPkg already open-codes the 0x5000 constant in
>> "Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)
>>
>> Thanks
>> Laszlo
>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-22 8:39 ` Laszlo Ersek
@ 2021-04-22 19:10 ` Lendacky, Thomas
2021-04-23 9:28 ` Laszlo Ersek
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-22 19:10 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/22/21 3:39 AM, Laszlo Ersek wrote:
> On 04/22/21 09:34, Laszlo Ersek wrote:
>
>> The new InternalTpmDecryptAddressRange() function should be called
>> from Tcg2ConfigPeimEntryPoint(), before the latter calls
>> InternalTpm12Detect(). Regarding error checking... if
>> InternalTpmDecryptAddressRange() fails, I think we can log an error
>> message, and hang with CpuDeadLoop().
>
Unfortunately, this method doesn't work. The OVMF Tcg2ConfigPei.inf file
uses the SecurityPkg Tpm2DeviceLib library. The SecurityPkg Tpm2DeviceLib
library's constructor is called before the OVMF Tcg2ConfigPei constructor.
The Tpm2DeviceLib constructor performs MMIO to the TPM base address and
fails because the pages haven't been marked unencrypted yet by OVMF
Tcg2ConfigPei. Some debug output:
Loading PEIM at 0x0007F793000 EntryPoint=0x0007F794E4F Tcg2ConfigPei.efi
*** DEBUG: InternalTpm2DeviceLibDTpmCommonConstructor:55
*** DEBUG: Tpm2GetPtpInterface:425
*** DEBUG: Tpm2IsPtpPresence:51
MMIO using encrypted memory: FED40000
!!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!!
Thanks,
Tom
> Sorry, another point:
>
> (6) where we determine that no TPM is available:
>
> //
> // If no TPM2 was detected, we still need to install
> // TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon seeing
> // the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus we have
> // to install the PPI in its place, in order to unblock any dependent
> // PEIMs.
> //
> Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);
>
> we should re-encrypt the address range, as if nothing had happened.
>
> For this, we'll likely need a similarly polymorphic function called
> InternalTpmEncryptAddressRange().
>
> (
>
> For some background on this particular branch of the code, please refer
> to commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
> 2018-03-09):
>
> - Check the QEMU hardware for TPM2 availability only
>
> - If found, set the dynamic PCD "PcdTpmInstanceGuid" to
> &gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
> the firmware about the TPM type.
>
> - Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
> PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
> In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
> and the consumption of the "TPM type" PCD.
>
> - If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
> (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
> no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)
>
> )
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
2021-04-22 13:35 ` Lendacky, Thomas
@ 2021-04-23 9:07 ` Laszlo Ersek
0 siblings, 0 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-23 9:07 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/22/21 15:35, Tom Lendacky wrote:
> On 4/22/21 12:28 AM, Laszlo Ersek wrote:
>> On 04/21/21 00:54, 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C22bf3a3ae9cb4421e93208d9054f79c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546661229697941%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1EmUDf%2FfuCuu%2BkXPZijzatfliplMhKEQH8kiZ9Z8ZF0%3D&reserved=0
>>>
>>> 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: 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 | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>> index 24259060fd65..273f36499988 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_INFO, "MMIO using encrypted memory: %lx\n", MemoryAddress));
>>> GpEvent.Uint64 = 0;
>>> GpEvent.Elements.Vector = GP_EXCEPTION;
>>> GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
>>
>> (1) This can potentially generate a large number of debug messages;
>> please use the DEBUG_VERBOSE log mask.
>
> Actually, you will see this only once since the code will propagate a GP
> and the guest will terminate in this situation.
Ugh, sorry, I must have completely lost track of the context here. I
apologize.
In that case however, it should be DEBUG_ERROR.
Thanks,
Laszlo
>
>>
>> (2) "MemoryAddress" has type UINTN, but %lx takes UINT64. Given that
>> this is X64-only code, functionally there is no bug, but it's still
>> cleaner to pass "(UINT64)MemoryAddress" to %lx.
>
> Will do.
>
> Thanks,
> Tom
>
>>
>> With that:
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>>
>>> @@ -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;
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
2021-04-22 15:42 ` Lendacky, Thomas
@ 2021-04-23 9:10 ` Laszlo Ersek
2021-04-23 13:24 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-23 9:10 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/22/21 17:42, Tom Lendacky wrote:
> On 4/22/21 9:15 AM, Tom Lendacky wrote:
>> On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
>>> On 04/21/21 00:54, 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&reserved=0
>>>>
>>>> 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: 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 | 99 +++++++++++++++++++
>>>> 1 file changed, 99 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>> index 273f36499988..f9660b757d8e 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,51 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>>>> +
>>>> + if (InstructionData->AddrSize == Size8Bits) {
>>>> + Address = *(UINT8 *) InstructionData->Immediate;
>>>> + } else if (InstructionData->AddrSize == Size16Bits) {
>>>> + Address = *(UINT16 *) InstructionData->Immediate;
>>>> + } else if (InstructionData->AddrSize == Size32Bits) {
>>>> + Address = *(UINT32 *) InstructionData->Immediate;
>>>> + } else {
>>>> + Address = *(UINTN *) InstructionData->Immediate;
>>>> + }
>>>
>>> (1) Can we simplify this as follows?
>>>
>>> InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>>> InstructionData->End += InstructionData->ImmediateSize;
>>> Address = 0;
>>> CopyMem (&Address, InstructionData->Immediate,
>>> InstructionData->ImmediateSize);
>>
>> Yup, that can be done.
>
> "Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
> isn't an issue. Should I add a comment about that above the setting of
> "Address"? Or should I convert "Address" to a UINT64 - although
> ValidateMmioMemory expects a UINTN... Thoughts?
Yes, I had the exact same thought process :)
The comment looks good, but how about expressing it as a STATIC_ASSERT,
with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
about MAX_UINT64 being equal to MAX_UINTN.)
If you find that too verbose, a comment is good enough too, of course.
Thanks!
Laszlo
>
> Thanks,
> Tom
>
>>
>>>
>>>> +
>>>> + 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 +855,58 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>>>> +
>>>> + if (InstructionData->AddrSize == Size8Bits) {
>>>> + Address = *(UINT8 *) InstructionData->Immediate;
>>>> + } else if (InstructionData->AddrSize == Size16Bits) {
>>>> + Address = *(UINT16 *) InstructionData->Immediate;
>>>> + } else if (InstructionData->AddrSize == Size32Bits) {
>>>> + Address = *(UINT32 *) InstructionData->Immediate;
>>>> + } else {
>>>> + Address = *(UINTN *) InstructionData->Immediate;
>>>> + }
>>>
>>> (2) Similar question as (1).
>>
>> Will do.
>>
>>>
>>>> +
>>>> + 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;
>>>> + }
>>>
>>> (3) This is also seen with opcode 0x8B, but can you remind me please why
>>> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
>>
>> That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
>> 32-bit operand size the 32-bit register results are zero-extended to 64-bits.
>>
>>>
>>>> + CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>>>> + break;
>>>> +
>>>> //
>>>> // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>>> //
>>>> @@ -886,6 +984,7 @@ MmioExit (
>>>> break;
>>>>
>>>> default:
>>>> + DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>>> Status = GP_EXCEPTION;
>>>> ASSERT (FALSE);
>>>> }
>>>>
>>>
>>> (4) We should use the DEBUG_ERROR log mask here.
>>
>> Will change.
>>
>> Thanks,
>> Tom
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>>
>>>
>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-22 19:10 ` Lendacky, Thomas
@ 2021-04-23 9:28 ` Laszlo Ersek
0 siblings, 0 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-23 9:28 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/22/21 21:10, Tom Lendacky wrote:
> On 4/22/21 3:39 AM, Laszlo Ersek wrote:
>> On 04/22/21 09:34, Laszlo Ersek wrote:
>>
>>> The new InternalTpmDecryptAddressRange() function should be called
>>> from Tcg2ConfigPeimEntryPoint(), before the latter calls
>>> InternalTpm12Detect(). Regarding error checking... if
>>> InternalTpmDecryptAddressRange() fails, I think we can log an error
>>> message, and hang with CpuDeadLoop().
>>
>
> Unfortunately, this method doesn't work. The OVMF Tcg2ConfigPei.inf file
> uses the SecurityPkg Tpm2DeviceLib library. The SecurityPkg Tpm2DeviceLib
> library's constructor is called before the OVMF Tcg2ConfigPei constructor.
> The Tpm2DeviceLib constructor performs MMIO to the TPM base address and
> fails because the pages haven't been marked unencrypted yet by OVMF
> Tcg2ConfigPei. Some debug output:
>
> Loading PEIM at 0x0007F793000 EntryPoint=0x0007F794E4F Tcg2ConfigPei.efi
> *** DEBUG: InternalTpm2DeviceLibDTpmCommonConstructor:55
> *** DEBUG: Tpm2GetPtpInterface:425
> *** DEBUG: Tpm2IsPtpPresence:51
> MMIO using encrypted memory: FED40000
> !!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!!
Thank you for checking this approach.
Let me re-review this patch from scratch.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-20 22:54 ` [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV Lendacky, Thomas
2021-04-20 23:17 ` Eric van Tassell
@ 2021-04-23 10:26 ` Laszlo Ersek
2021-04-23 13:04 ` [edk2-devel] " Laszlo Ersek
1 sibling, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-23 10:26 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, Joerg Roedel, Borislav Petkov, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
review#2 from scratch:
On 04/21/21 00:54, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>
> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
> guest will fail attempting to perform MMIO to an encrypted address.
(1) As discussed, please update the commit message, for more clarify
about SEV vs. SEV-ES.
>
> Read the PcdTpmBaseAddress and mark the specification defined range
> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
> the MMIO requests.
>
> 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: 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/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6ef77ba7bb21..de60332e9390 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -113,6 +113,7 @@ [Pcd]
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index dddffdebda4b..d524929f9e10 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -141,6 +141,7 @@ AmdSevInitialize (
> )
> {
> UINT64 EncryptionMask;
> + UINT64 TpmBaseAddress;
> RETURN_STATUS PcdStatus;
>
> //
> @@ -206,6 +207,24 @@ AmdSevInitialize (
> }
> }
>
> + //
> + // PEI TPM support will perform MMIO accesses, be sure this range is not
> + // marked encrypted.
> + //
> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
> + if (TpmBaseAddress != 0) {
It's OK to keep this as a sanity check, yes.
> + RETURN_STATUS DecryptStatus;
> +
> + DecryptStatus = MemEncryptSevClearPageEncMask (
> + 0,
> + TpmBaseAddress,
> + EFI_SIZE_TO_PAGES (0x5000),
(2) Should be (UINTN)0x5000, as discussed earlier.
> + FALSE
> + );
> +
> + ASSERT_RETURN_ERROR (DecryptStatus);
(3) So this is where the mess begins.
The idea is to delay the dispatch of Tcg2ConfigPei until after
PlatformPei determines if SEV is active, and (in case SEV is active)
PlatformPei decrypts the MMIO range of the TPM.
For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
the current TRUE, to some PPI GUID.
There are two choices for that PPI:
(a) gEfiPeiMemoryDiscoveredPpiGuid
Advantages:
- no new PPI definition needed,
- no new PPI installation needed,
- OvmfPkg/Bhyve/PlatformPei needs no separate change
Disadvantages:
- total abuse of gEfiPeiMemoryDiscoveredPpiGuid
(b) gOvmfTpmMmioAccessiblePpiGuid
Disadvantages:
- this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
in a separate patch; its comment should say "this PPI signals that
accessing the MMIO range of the TPM is possible in the PEI phase,
regardless of memory encryption". The PPI definitions should be kept
alphabetically ordered.
- OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
(See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
install this new PPI either when the SEV check at the top of
AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
- OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
"Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
"OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
stricter-than-before depex, so something on the bhyve platform too must
produce the new PPI.
Advantages:
- more or less palatable as a concept, with the new PPI precisely
expressing the dependency we have.
In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
to the Bhyve reviewers. If the Bhyve reviewers determine that such an
update is actually unnecessary, because on Bhyve, there is no TPM
support and/or no SEV support in fact, then *first* we have to create an
independent Bhyve cleanup series, that rips out the TPM and/or SEV
remnants from the OvmfPkg/Bhyve sub-tree.
I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
but I strongly believe in keeping all platforms in the tree, and that
means we need to spend time on such changes.
I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
patch review thread, and they'd have no useful context. I suggest simply
including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
this series, with a proper explanation in the blurb (patch#0) and on the
"OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
to evaluate whether the change is necessary, or whether we should purge
the TPM and/or the SEV bits from Bhyve. You could also ask them just
this question in advance, in a separate email on the list (with
distilled context). Personally I'm unsure if the TPM and SEV bits
survived into Bhyve because those bits are actually put to use there, or
because the initial platform creation / cloning wasn't as minimal as it
could have been.
Note that in case TPM makes sense on bhyve but SEV doesn't, then
"OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
unconditionally.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-23 10:26 ` Laszlo Ersek
@ 2021-04-23 13:04 ` Laszlo Ersek
2021-04-23 13:09 ` Laszlo Ersek
2021-04-23 17:41 ` Lendacky, Thomas
0 siblings, 2 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-23 13:04 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, Joerg Roedel, Borislav Petkov, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/23/21 12:26, Laszlo Ersek wrote:
> review#2 from scratch:
>
> On 04/21/21 00:54, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>>
>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>> guest will fail attempting to perform MMIO to an encrypted address.
>
> (1) As discussed, please update the commit message, for more clarify
> about SEV vs. SEV-ES.
>
>>
>> Read the PcdTpmBaseAddress and mark the specification defined range
>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>> the MMIO requests.
>>
>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 6ef77ba7bb21..de60332e9390 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -113,6 +113,7 @@ [Pcd]
>>
>> [FixedPcd]
>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index dddffdebda4b..d524929f9e10 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>> )
>> {
>> UINT64 EncryptionMask;
>> + UINT64 TpmBaseAddress;
>> RETURN_STATUS PcdStatus;
>>
>> //
>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>> }
>> }
>>
>> + //
>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>> + // marked encrypted.
>> + //
>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>> + if (TpmBaseAddress != 0) {
>
> It's OK to keep this as a sanity check, yes.
>
>> + RETURN_STATUS DecryptStatus;
>> +
>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>> + 0,
>> + TpmBaseAddress,
>> + EFI_SIZE_TO_PAGES (0x5000),
>
> (2) Should be (UINTN)0x5000, as discussed earlier.
>
>> + FALSE
>> + );
>> +
>> + ASSERT_RETURN_ERROR (DecryptStatus);
>
> (3) So this is where the mess begins.
>
> The idea is to delay the dispatch of Tcg2ConfigPei until after
> PlatformPei determines if SEV is active, and (in case SEV is active)
> PlatformPei decrypts the MMIO range of the TPM.
>
> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
> the current TRUE, to some PPI GUID.
>
> There are two choices for that PPI:
>
> (a) gEfiPeiMemoryDiscoveredPpiGuid
>
> Advantages:
>
> - no new PPI definition needed,
>
> - no new PPI installation needed,
>
> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>
> Disadvantages:
>
> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>
>
> (b) gOvmfTpmMmioAccessiblePpiGuid
>
> Disadvantages:
>
> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
> in a separate patch; its comment should say "this PPI signals that
> accessing the MMIO range of the TPM is possible in the PEI phase,
> regardless of memory encryption". The PPI definitions should be kept
> alphabetically ordered.
>
> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
> install this new PPI either when the SEV check at the top of
> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>
> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
> stricter-than-before depex, so something on the bhyve platform too must
> produce the new PPI.
>
> Advantages:
>
> - more or less palatable as a concept, with the new PPI precisely
> expressing the dependency we have.
>
>
> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
> update is actually unnecessary, because on Bhyve, there is no TPM
> support and/or no SEV support in fact, then *first* we have to create an
> independent Bhyve cleanup series, that rips out the TPM and/or SEV
> remnants from the OvmfPkg/Bhyve sub-tree.
>
>
> I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
> but I strongly believe in keeping all platforms in the tree, and that
> means we need to spend time on such changes.
>
> I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
> patch review thread, and they'd have no useful context. I suggest simply
> including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
> this series, with a proper explanation in the blurb (patch#0) and on the
> "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
> to evaluate whether the change is necessary, or whether we should purge
> the TPM and/or the SEV bits from Bhyve. You could also ask them just
> this question in advance, in a separate email on the list (with
> distilled context). Personally I'm unsure if the TPM and SEV bits
> survived into Bhyve because those bits are actually put to use there, or
> because the initial platform creation / cloning wasn't as minimal as it
> could have been.
>
> Note that in case TPM makes sense on bhyve but SEV doesn't, then
> "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
> unconditionally.
I've had a further idea on this.
You could add an entirely new PEIM just for this. The entry point
function of the PEIM would check for SEV, decrypt the TPM range if SEV
were active, and then install gOvmfTpmMmioAccessiblePpiGuid
(unconditionally). The exit status of the PEIM would always be
EFI_ABORTED, because there would be no need to keep the PEIM resident.
The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
make sure that potential page table splitting for the potential MMIO
range decryption could be satisfied from permanent PEI RAM.
The new PEIM would be included in the DSC and FDF files of the usual
three OVMF platforms, and in the Bhyve platform -- dependent on the
TPM_ENABLE build flag.
There are several advantages to such a separate PEIM:
- For Bhyve, the update is minimal. Just include one line in each of the
FDF and the DSC files. No need to customize an existent
platform-specific PEIM, no code duplication between two PlatformPei modules.
- The new PEIM would depend on the TPM_ENABLE build flag, so it would
only be included in the firmware binaries if and only if Tcg2ConfigPei
were. No useless PPI installation would occur in the absence of TPM_ENABLE.
- No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
already has the right value.
- The new logic would be properly ordered between PlatformPei and
Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
via "memory discovered" (needed for potential page table splitting), TPM
MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
You could place the new PEIM at:
OvmfPkg/Tcg/TpmMmioSevDecryptPei
If you haven't lost your patience with me yet, I'd really appreciate if
you could investigate this!
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-23 13:04 ` [edk2-devel] " Laszlo Ersek
@ 2021-04-23 13:09 ` Laszlo Ersek
2021-04-23 17:41 ` Lendacky, Thomas
1 sibling, 0 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-23 13:09 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, Joerg Roedel, Borislav Petkov, Ard Biesheuvel,
Jordan Justen, Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/23/21 15:04, Laszlo Ersek wrote:
> There are several advantages to such a separate PEIM:
>
> - For Bhyve, the update is minimal. Just include one line in each of the
> FDF and the DSC files. No need to customize an existent
> platform-specific PEIM, no code duplication between two PlatformPei modules.
OTOH, you'd have to update OvmfPkg/AmdSev/AmdSevX64.* too.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
2021-04-23 9:10 ` Laszlo Ersek
@ 2021-04-23 13:24 ` Lendacky, Thomas
0 siblings, 0 replies; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-23 13:24 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/23/21 4:10 AM, Laszlo Ersek wrote:
> On 04/22/21 17:42, Tom Lendacky wrote:
>> On 4/22/21 9:15 AM, Tom Lendacky wrote:
>>> On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
>>>> On 04/21/21 00:54, 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ca0ff778cdbb146a6186508d90637ba60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547658699539157%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=vMmmGGXXT8MtM4WOB4gkwtkr0cwwkx98LoOXYg6fclQ%3D&reserved=0
>>>>>
>>>>> 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: 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 | 99 +++++++++++++++++++
>>>>> 1 file changed, 99 insertions(+)
>>>>>
>>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>>> index 273f36499988..f9660b757d8e 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,51 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>>>>> +
>>>>> + if (InstructionData->AddrSize == Size8Bits) {
>>>>> + Address = *(UINT8 *) InstructionData->Immediate;
>>>>> + } else if (InstructionData->AddrSize == Size16Bits) {
>>>>> + Address = *(UINT16 *) InstructionData->Immediate;
>>>>> + } else if (InstructionData->AddrSize == Size32Bits) {
>>>>> + Address = *(UINT32 *) InstructionData->Immediate;
>>>>> + } else {
>>>>> + Address = *(UINTN *) InstructionData->Immediate;
>>>>> + }
>>>>
>>>> (1) Can we simplify this as follows?
>>>>
>>>> InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>>>> InstructionData->End += InstructionData->ImmediateSize;
>>>> Address = 0;
>>>> CopyMem (&Address, InstructionData->Immediate,
>>>> InstructionData->ImmediateSize);
>>>
>>> Yup, that can be done.
>>
>> "Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
>> isn't an issue. Should I add a comment about that above the setting of
>> "Address"? Or should I convert "Address" to a UINT64 - although
>> ValidateMmioMemory expects a UINTN... Thoughts?
>
> Yes, I had the exact same thought process :)
>
> The comment looks good, but how about expressing it as a STATIC_ASSERT,
> with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
> about MAX_UINT64 being equal to MAX_UINTN.)
I like the STATIC_ASSERT idea, I'll do that.
Thanks,
Tom
>
> If you find that too verbose, a comment is good enough too, of course.
>
> Thanks!
> Laszlo
>
>>
>> Thanks,
>> Tom
>>
>>>
>>>>
>>>>> +
>>>>> + 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 +855,58 @@ 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 += (UINTN) (1 << InstructionData->AddrSize);
>>>>> +
>>>>> + if (InstructionData->AddrSize == Size8Bits) {
>>>>> + Address = *(UINT8 *) InstructionData->Immediate;
>>>>> + } else if (InstructionData->AddrSize == Size16Bits) {
>>>>> + Address = *(UINT16 *) InstructionData->Immediate;
>>>>> + } else if (InstructionData->AddrSize == Size32Bits) {
>>>>> + Address = *(UINT32 *) InstructionData->Immediate;
>>>>> + } else {
>>>>> + Address = *(UINTN *) InstructionData->Immediate;
>>>>> + }
>>>>
>>>> (2) Similar question as (1).
>>>
>>> Will do.
>>>
>>>>
>>>>> +
>>>>> + 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;
>>>>> + }
>>>>
>>>> (3) This is also seen with opcode 0x8B, but can you remind me please why
>>>> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
>>>
>>> That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
>>> 32-bit operand size the 32-bit register results are zero-extended to 64-bits.
>>>
>>>>
>>>>> + CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>>>>> + break;
>>>>> +
>>>>> //
>>>>> // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>>>> //
>>>>> @@ -886,6 +984,7 @@ MmioExit (
>>>>> break;
>>>>>
>>>>> default:
>>>>> + DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>>>> Status = GP_EXCEPTION;
>>>>> ASSERT (FALSE);
>>>>> }
>>>>>
>>>>
>>>> (4) We should use the DEBUG_ERROR log mask here.
>>>
>>> Will change.
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-23 13:04 ` [edk2-devel] " Laszlo Ersek
2021-04-23 13:09 ` Laszlo Ersek
@ 2021-04-23 17:41 ` Lendacky, Thomas
2021-04-23 20:02 ` Lendacky, Thomas
2021-04-26 11:08 ` Laszlo Ersek
1 sibling, 2 replies; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-23 17:41 UTC (permalink / raw)
To: devel, lersek
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/23/21 8:04 AM, Laszlo Ersek wrote:
> On 04/23/21 12:26, Laszlo Ersek wrote:
>> review#2 from scratch:
>>
>> On 04/21/21 00:54, Tom Lendacky 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ca24cbb3f52404e466fe808d906585034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547798659640707%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o2rAydWG7jenRhqbZx3oDffZhlCUimJL9f9Tpmy4etk%3D&reserved=0
>>>
>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>> guest will fail attempting to perform MMIO to an encrypted address.
>>
>> (1) As discussed, please update the commit message, for more clarify
>> about SEV vs. SEV-ES.
>>
>>>
>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>> the MMIO requests.
>>>
>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>> 2 files changed, 20 insertions(+)
>>>
>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> index 6ef77ba7bb21..de60332e9390 100644
>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> @@ -113,6 +113,7 @@ [Pcd]
>>>
>>> [FixedPcd]
>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>> index dddffdebda4b..d524929f9e10 100644
>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>> )
>>> {
>>> UINT64 EncryptionMask;
>>> + UINT64 TpmBaseAddress;
>>> RETURN_STATUS PcdStatus;
>>>
>>> //
>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>> }
>>> }
>>>
>>> + //
>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>> + // marked encrypted.
>>> + //
>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>> + if (TpmBaseAddress != 0) {
>>
>> It's OK to keep this as a sanity check, yes.
>>
>>> + RETURN_STATUS DecryptStatus;
>>> +
>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>> + 0,
>>> + TpmBaseAddress,
>>> + EFI_SIZE_TO_PAGES (0x5000),
>>
>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>
>>> + FALSE
>>> + );
>>> +
>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>
>> (3) So this is where the mess begins.
>>
>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>> PlatformPei determines if SEV is active, and (in case SEV is active)
>> PlatformPei decrypts the MMIO range of the TPM.
>>
>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>> the current TRUE, to some PPI GUID.
>>
>> There are two choices for that PPI:
>>
>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>
>> Advantages:
>>
>> - no new PPI definition needed,
>>
>> - no new PPI installation needed,
>>
>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>
>> Disadvantages:
>>
>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>
>>
>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>
>> Disadvantages:
>>
>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>> in a separate patch; its comment should say "this PPI signals that
>> accessing the MMIO range of the TPM is possible in the PEI phase,
>> regardless of memory encryption". The PPI definitions should be kept
>> alphabetically ordered.
>>
>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>> install this new PPI either when the SEV check at the top of
>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>
>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
>> stricter-than-before depex, so something on the bhyve platform too must
>> produce the new PPI.
>>
>> Advantages:
>>
>> - more or less palatable as a concept, with the new PPI precisely
>> expressing the dependency we have.
>>
>>
>> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
>> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
>> update is actually unnecessary, because on Bhyve, there is no TPM
>> support and/or no SEV support in fact, then *first* we have to create an
>> independent Bhyve cleanup series, that rips out the TPM and/or SEV
>> remnants from the OvmfPkg/Bhyve sub-tree.
>>
>>
>> I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
>> but I strongly believe in keeping all platforms in the tree, and that
>> means we need to spend time on such changes.
>>
>> I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
>> patch review thread, and they'd have no useful context. I suggest simply
>> including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
>> this series, with a proper explanation in the blurb (patch#0) and on the
>> "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
>> to evaluate whether the change is necessary, or whether we should purge
>> the TPM and/or the SEV bits from Bhyve. You could also ask them just
>> this question in advance, in a separate email on the list (with
>> distilled context). Personally I'm unsure if the TPM and SEV bits
>> survived into Bhyve because those bits are actually put to use there, or
>> because the initial platform creation / cloning wasn't as minimal as it
>> could have been.
>>
>> Note that in case TPM makes sense on bhyve but SEV doesn't, then
>> "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
>> unconditionally.
>
> I've had a further idea on this.
>
> You could add an entirely new PEIM just for this. The entry point
> function of the PEIM would check for SEV, decrypt the TPM range if SEV
> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
> (unconditionally). The exit status of the PEIM would always be
> EFI_ABORTED, because there would be no need to keep the PEIM resident.
>
> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
> make sure that potential page table splitting for the potential MMIO
> range decryption could be satisfied from permanent PEI RAM.
>
> The new PEIM would be included in the DSC and FDF files of the usual
> three OVMF platforms, and in the Bhyve platform -- dependent on the
> TPM_ENABLE build flag.
>
> There are several advantages to such a separate PEIM:
>
> - For Bhyve, the update is minimal. Just include one line in each of the
> FDF and the DSC files. No need to customize an existent
> platform-specific PEIM, no code duplication between two PlatformPei modules.
>
> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
> only be included in the firmware binaries if and only if Tcg2ConfigPei
> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
>
> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
> already has the right value.
>
> - The new logic would be properly ordered between PlatformPei and
> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
> via "memory discovered" (needed for potential page table splitting), TPM
> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
>
> You could place the new PEIM at:
>
> OvmfPkg/Tcg/TpmMmioSevDecryptPei
>
> If you haven't lost your patience with me yet, I'd really appreciate if
> you could investigate this!
>
So far, this appears to be working nicely. I'm new at the whole PEIM
thing, so hopefully I haven't missed anything. I should be submitting the
patches soon for review.
One thing I found is that the Bhyve package makes reference to the
OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
think that TPM enablement has been tested. I didn't update the Bhyve
support for that reason.
Thanks,
Tom
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-23 17:41 ` Lendacky, Thomas
@ 2021-04-23 20:02 ` Lendacky, Thomas
2021-04-26 12:07 ` Laszlo Ersek
2021-04-26 11:08 ` Laszlo Ersek
1 sibling, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-23 20:02 UTC (permalink / raw)
To: devel, lersek
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/23/21 12:41 PM, Tom Lendacky wrote:
> On 4/23/21 8:04 AM, Laszlo Ersek wrote:
>> On 04/23/21 12:26, Laszlo Ersek wrote:
>>> review#2 from scratch:
>>>
>>> On 04/21/21 00:54, Tom Lendacky 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ca24cbb3f52404e466fe808d906585034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547798659640707%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o2rAydWG7jenRhqbZx3oDffZhlCUimJL9f9Tpmy4etk%3D&reserved=0
>>>>
>>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>>> guest will fail attempting to perform MMIO to an encrypted address.
>>>
>>> (1) As discussed, please update the commit message, for more clarify
>>> about SEV vs. SEV-ES.
>>>
>>>>
>>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>>> the MMIO requests.
>>>>
>>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>>> 2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> index 6ef77ba7bb21..de60332e9390 100644
>>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>> @@ -113,6 +113,7 @@ [Pcd]
>>>>
>>>> [FixedPcd]
>>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>>> index dddffdebda4b..d524929f9e10 100644
>>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>> )
>>>> {
>>>> UINT64 EncryptionMask;
>>>> + UINT64 TpmBaseAddress;
>>>> RETURN_STATUS PcdStatus;
>>>>
>>>> //
>>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>> }
>>>> }
>>>>
>>>> + //
>>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>>> + // marked encrypted.
>>>> + //
>>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>>> + if (TpmBaseAddress != 0) {
>>>
>>> It's OK to keep this as a sanity check, yes.
>>>
>>>> + RETURN_STATUS DecryptStatus;
>>>> +
>>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>>> + 0,
>>>> + TpmBaseAddress,
>>>> + EFI_SIZE_TO_PAGES (0x5000),
>>>
>>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>>
>>>> + FALSE
>>>> + );
>>>> +
>>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>>
>>> (3) So this is where the mess begins.
>>>
>>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>>> PlatformPei determines if SEV is active, and (in case SEV is active)
>>> PlatformPei decrypts the MMIO range of the TPM.
>>>
>>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>>> the current TRUE, to some PPI GUID.
>>>
>>> There are two choices for that PPI:
>>>
>>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>>
>>> Advantages:
>>>
>>> - no new PPI definition needed,
>>>
>>> - no new PPI installation needed,
>>>
>>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>>
>>> Disadvantages:
>>>
>>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>>
>>>
>>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>>
>>> Disadvantages:
>>>
>>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>>> in a separate patch; its comment should say "this PPI signals that
>>> accessing the MMIO range of the TPM is possible in the PEI phase,
>>> regardless of memory encryption". The PPI definitions should be kept
>>> alphabetically ordered.
>>>
>>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>>> install this new PPI either when the SEV check at the top of
>>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>>
>>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>>> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
>>> stricter-than-before depex, so something on the bhyve platform too must
>>> produce the new PPI.
>>>
>>> Advantages:
>>>
>>> - more or less palatable as a concept, with the new PPI precisely
>>> expressing the dependency we have.
>>>
>>>
>>> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
>>> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
>>> update is actually unnecessary, because on Bhyve, there is no TPM
>>> support and/or no SEV support in fact, then *first* we have to create an
>>> independent Bhyve cleanup series, that rips out the TPM and/or SEV
>>> remnants from the OvmfPkg/Bhyve sub-tree.
>>>
>>>
>>> I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
>>> but I strongly believe in keeping all platforms in the tree, and that
>>> means we need to spend time on such changes.
>>>
>>> I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
>>> patch review thread, and they'd have no useful context. I suggest simply
>>> including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
>>> this series, with a proper explanation in the blurb (patch#0) and on the
>>> "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
>>> to evaluate whether the change is necessary, or whether we should purge
>>> the TPM and/or the SEV bits from Bhyve. You could also ask them just
>>> this question in advance, in a separate email on the list (with
>>> distilled context). Personally I'm unsure if the TPM and SEV bits
>>> survived into Bhyve because those bits are actually put to use there, or
>>> because the initial platform creation / cloning wasn't as minimal as it
>>> could have been.
>>>
>>> Note that in case TPM makes sense on bhyve but SEV doesn't, then
>>> "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
>>> unconditionally.
>>
>> I've had a further idea on this.
>>
>> You could add an entirely new PEIM just for this. The entry point
>> function of the PEIM would check for SEV, decrypt the TPM range if SEV
>> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
>> (unconditionally). The exit status of the PEIM would always be
>> EFI_ABORTED, because there would be no need to keep the PEIM resident.
>>
>> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
>> make sure that potential page table splitting for the potential MMIO
>> range decryption could be satisfied from permanent PEI RAM.
>>
>> The new PEIM would be included in the DSC and FDF files of the usual
>> three OVMF platforms, and in the Bhyve platform -- dependent on the
>> TPM_ENABLE build flag.
>>
>> There are several advantages to such a separate PEIM:
>>
>> - For Bhyve, the update is minimal. Just include one line in each of the
>> FDF and the DSC files. No need to customize an existent
>> platform-specific PEIM, no code duplication between two PlatformPei modules.
>>
>> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
>> only be included in the firmware binaries if and only if Tcg2ConfigPei
>> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
>>
>> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
>> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
>> already has the right value.
>>
>> - The new logic would be properly ordered between PlatformPei and
>> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
>> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
>> via "memory discovered" (needed for potential page table splitting), TPM
>> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
>>
>> You could place the new PEIM at:
>>
>> OvmfPkg/Tcg/TpmMmioSevDecryptPei
>>
>> If you haven't lost your patience with me yet, I'd really appreciate if
>> you could investigate this!
>>
>
> So far, this appears to be working nicely. I'm new at the whole PEIM
> thing, so hopefully I haven't missed anything. I should be submitting the
> patches soon for review.
So one thing I failed to do before submitting my previous patch was to
complete my testing against the IA32 and X64 combination build. In this
build, PEI is built as Ia32, and MemEncryptSevClearPageEncMask() will
return UNSUPPORTED causing an ASSERT (since I check the return code). So
there are a few options:
1. SEV works with the current encrypted mapping, it is only the SEV-ES
support that fails because of the ValidateMmioMemory() check. I can do
the mapping change just for SEV-ES since it is X64 only. This works,
because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
when running in 64-bit.
2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
the return status.
3. Create Ia32 and X64 versions of internal functions, where the Ia32
version simply returns SUCCESS because it can't do anything and the X64
version calls MemEncryptSevClearPageEncMask(), allowing the main code
to ASSERT on any errors.
I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
Thanks,
Tom
>
> One thing I found is that the Bhyve package makes reference to the
> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
> think that TPM enablement has been tested. I didn't update the Bhyve
> support for that reason.
>
> Thanks,
> Tom
>
>> Thanks!
>> Laszlo
>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-23 17:41 ` Lendacky, Thomas
2021-04-23 20:02 ` Lendacky, Thomas
@ 2021-04-26 11:08 ` Laszlo Ersek
1 sibling, 0 replies; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-26 11:08 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/23/21 19:41, Tom Lendacky wrote:
> On 4/23/21 8:04 AM, Laszlo Ersek wrote:
>> I've had a further idea on this.
>>
>> You could add an entirely new PEIM just for this. The entry point
>> function of the PEIM would check for SEV, decrypt the TPM range if SEV
>> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
>> (unconditionally). The exit status of the PEIM would always be
>> EFI_ABORTED, because there would be no need to keep the PEIM resident.
>>
>> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
>> make sure that potential page table splitting for the potential MMIO
>> range decryption could be satisfied from permanent PEI RAM.
>>
>> The new PEIM would be included in the DSC and FDF files of the usual
>> three OVMF platforms, and in the Bhyve platform -- dependent on the
>> TPM_ENABLE build flag.
>>
>> There are several advantages to such a separate PEIM:
>>
>> - For Bhyve, the update is minimal. Just include one line in each of the
>> FDF and the DSC files. No need to customize an existent
>> platform-specific PEIM, no code duplication between two PlatformPei modules.
>>
>> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
>> only be included in the firmware binaries if and only if Tcg2ConfigPei
>> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
>>
>> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
>> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
>> already has the right value.
>>
>> - The new logic would be properly ordered between PlatformPei and
>> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
>> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
>> via "memory discovered" (needed for potential page table splitting), TPM
>> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
>>
>> You could place the new PEIM at:
>>
>> OvmfPkg/Tcg/TpmMmioSevDecryptPei
>>
>> If you haven't lost your patience with me yet, I'd really appreciate if
>> you could investigate this!
>>
>
> So far, this appears to be working nicely. I'm new at the whole PEIM
> thing, so hopefully I haven't missed anything. I should be submitting the
> patches soon for review.
Thanks!
>
> One thing I found is that the Bhyve package makes reference to the
> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
> think that TPM enablement has been tested. I didn't update the Bhyve
> support for that reason.
That's good to know; thanks for reporting this. I've turned it now into
a BZ ticket (assigned to Rebecca):
https://bugzilla.tianocore.org/show_bug.cgi?id=3354
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-23 20:02 ` Lendacky, Thomas
@ 2021-04-26 12:07 ` Laszlo Ersek
2021-04-26 14:21 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-26 12:07 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/23/21 22:02, Tom Lendacky wrote:
> On 4/23/21 12:41 PM, Tom Lendacky wrote:
>> On 4/23/21 8:04 AM, Laszlo Ersek wrote:
>>> On 04/23/21 12:26, Laszlo Ersek wrote:
>>>> review#2 from scratch:
>>>>
>>>> On 04/21/21 00:54, Tom Lendacky 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ca24cbb3f52404e466fe808d906585034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547798659640707%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o2rAydWG7jenRhqbZx3oDffZhlCUimJL9f9Tpmy4etk%3D&reserved=0
>>>>>
>>>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>>>> guest will fail attempting to perform MMIO to an encrypted address.
>>>>
>>>> (1) As discussed, please update the commit message, for more clarify
>>>> about SEV vs. SEV-ES.
>>>>
>>>>>
>>>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>>>> the MMIO requests.
>>>>>
>>>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>>>> 2 files changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> index 6ef77ba7bb21..de60332e9390 100644
>>>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>> @@ -113,6 +113,7 @@ [Pcd]
>>>>>
>>>>> [FixedPcd]
>>>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>>>> index dddffdebda4b..d524929f9e10 100644
>>>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>>> )
>>>>> {
>>>>> UINT64 EncryptionMask;
>>>>> + UINT64 TpmBaseAddress;
>>>>> RETURN_STATUS PcdStatus;
>>>>>
>>>>> //
>>>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>>> }
>>>>> }
>>>>>
>>>>> + //
>>>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>>>> + // marked encrypted.
>>>>> + //
>>>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>>>> + if (TpmBaseAddress != 0) {
>>>>
>>>> It's OK to keep this as a sanity check, yes.
>>>>
>>>>> + RETURN_STATUS DecryptStatus;
>>>>> +
>>>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>>>> + 0,
>>>>> + TpmBaseAddress,
>>>>> + EFI_SIZE_TO_PAGES (0x5000),
>>>>
>>>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>>>
>>>>> + FALSE
>>>>> + );
>>>>> +
>>>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>>>
>>>> (3) So this is where the mess begins.
>>>>
>>>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>>>> PlatformPei determines if SEV is active, and (in case SEV is active)
>>>> PlatformPei decrypts the MMIO range of the TPM.
>>>>
>>>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>>>> the current TRUE, to some PPI GUID.
>>>>
>>>> There are two choices for that PPI:
>>>>
>>>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>>>
>>>> Advantages:
>>>>
>>>> - no new PPI definition needed,
>>>>
>>>> - no new PPI installation needed,
>>>>
>>>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>>>
>>>> Disadvantages:
>>>>
>>>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>>>
>>>>
>>>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>>>
>>>> Disadvantages:
>>>>
>>>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>>>> in a separate patch; its comment should say "this PPI signals that
>>>> accessing the MMIO range of the TPM is possible in the PEI phase,
>>>> regardless of memory encryption". The PPI definitions should be kept
>>>> alphabetically ordered.
>>>>
>>>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>>>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>>>> install this new PPI either when the SEV check at the top of
>>>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>>>
>>>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>>>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>>>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>>>> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
>>>> stricter-than-before depex, so something on the bhyve platform too must
>>>> produce the new PPI.
>>>>
>>>> Advantages:
>>>>
>>>> - more or less palatable as a concept, with the new PPI precisely
>>>> expressing the dependency we have.
>>>>
>>>>
>>>> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
>>>> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
>>>> update is actually unnecessary, because on Bhyve, there is no TPM
>>>> support and/or no SEV support in fact, then *first* we have to create an
>>>> independent Bhyve cleanup series, that rips out the TPM and/or SEV
>>>> remnants from the OvmfPkg/Bhyve sub-tree.
>>>>
>>>>
>>>> I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
>>>> but I strongly believe in keeping all platforms in the tree, and that
>>>> means we need to spend time on such changes.
>>>>
>>>> I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
>>>> patch review thread, and they'd have no useful context. I suggest simply
>>>> including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
>>>> this series, with a proper explanation in the blurb (patch#0) and on the
>>>> "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
>>>> to evaluate whether the change is necessary, or whether we should purge
>>>> the TPM and/or the SEV bits from Bhyve. You could also ask them just
>>>> this question in advance, in a separate email on the list (with
>>>> distilled context). Personally I'm unsure if the TPM and SEV bits
>>>> survived into Bhyve because those bits are actually put to use there, or
>>>> because the initial platform creation / cloning wasn't as minimal as it
>>>> could have been.
>>>>
>>>> Note that in case TPM makes sense on bhyve but SEV doesn't, then
>>>> "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
>>>> unconditionally.
>>>
>>> I've had a further idea on this.
>>>
>>> You could add an entirely new PEIM just for this. The entry point
>>> function of the PEIM would check for SEV, decrypt the TPM range if SEV
>>> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
>>> (unconditionally). The exit status of the PEIM would always be
>>> EFI_ABORTED, because there would be no need to keep the PEIM resident.
>>>
>>> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
>>> make sure that potential page table splitting for the potential MMIO
>>> range decryption could be satisfied from permanent PEI RAM.
>>>
>>> The new PEIM would be included in the DSC and FDF files of the usual
>>> three OVMF platforms, and in the Bhyve platform -- dependent on the
>>> TPM_ENABLE build flag.
>>>
>>> There are several advantages to such a separate PEIM:
>>>
>>> - For Bhyve, the update is minimal. Just include one line in each of the
>>> FDF and the DSC files. No need to customize an existent
>>> platform-specific PEIM, no code duplication between two PlatformPei modules.
>>>
>>> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
>>> only be included in the firmware binaries if and only if Tcg2ConfigPei
>>> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
>>>
>>> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
>>> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
>>> already has the right value.
>>>
>>> - The new logic would be properly ordered between PlatformPei and
>>> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
>>> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
>>> via "memory discovered" (needed for potential page table splitting), TPM
>>> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
>>>
>>> You could place the new PEIM at:
>>>
>>> OvmfPkg/Tcg/TpmMmioSevDecryptPei
>>>
>>> If you haven't lost your patience with me yet, I'd really appreciate if
>>> you could investigate this!
>>>
>>
>> So far, this appears to be working nicely. I'm new at the whole PEIM
>> thing, so hopefully I haven't missed anything. I should be submitting the
>> patches soon for review.
>
> So one thing I failed to do before submitting my previous patch was to
> complete my testing against the IA32 and X64 combination build. In this
> build, PEI is built as Ia32, and MemEncryptSevClearPageEncMask() will
> return UNSUPPORTED causing an ASSERT (since I check the return code). So
> there are a few options:
>
> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
> support that fails because of the ValidateMmioMemory() check. I can do
> the mapping change just for SEV-ES since it is X64 only. This works,
> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
> when running in 64-bit.
Can we really say "SEV works" though? Because, even using an X64 PEI
phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
the PEI phase. Is my understanding correct?
I think the behavior you currently see is actually what we want, we
should double down on it -- if MemEncryptSevClearPageEncMask() fails,
report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
simply unusable. Silently pretending that the TPM is not there, even
though it may have been configured on the QEMU command line, we just
failed to communicate with it, is not a good idea, IMO.
This is somewhat similar IMO to the S3Verification() function in
"OvmfPkg/PlatformPei/Platform.c".
TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
Thanks,
Laszlo
>
> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
> the return status.
>
> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
> version simply returns SUCCESS because it can't do anything and the X64
> version calls MemEncryptSevClearPageEncMask(), allowing the main code
> to ASSERT on any errors.
>
> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>
> Thanks,
> Tom
>
>>
>> One thing I found is that the Bhyve package makes reference to the
>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>> think that TPM enablement has been tested. I didn't update the Bhyve
>> support for that reason.
>>
>> Thanks,
>> Tom
>>
>>> Thanks!
>>> Laszlo
>>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-26 12:07 ` Laszlo Ersek
@ 2021-04-26 14:21 ` Lendacky, Thomas
2021-04-27 14:58 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-26 14:21 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/26/21 7:07 AM, Laszlo Ersek wrote:
> On 04/23/21 22:02, Tom Lendacky wrote:
>> On 4/23/21 12:41 PM, Tom Lendacky wrote:
>>> On 4/23/21 8:04 AM, Laszlo Ersek wrote:
>>>> On 04/23/21 12:26, Laszlo Ersek wrote:
>>>>> review#2 from scratch:
>>>>>
>>>>> On 04/21/21 00:54, Tom Lendacky 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cc8a12e7c1e6a4282963508d908abe333%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637550356650588901%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3SeLVc3SscAozGX62BSAyWseujfwxzm6qIB%2Fh8ALyq0%3D&reserved=0
>>>>>>
>>>>>> The TPM support in OVMF performs MMIO accesses during the PEI phase. At
>>>>>> this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
>>>>>> guest will fail attempting to perform MMIO to an encrypted address.
>>>>>
>>>>> (1) As discussed, please update the commit message, for more clarify
>>>>> about SEV vs. SEV-ES.
>>>>>
>>>>>>
>>>>>> Read the PcdTpmBaseAddress and mark the specification defined range
>>>>>> (0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
>>>>>> the MMIO requests.
>>>>>>
>>>>>> 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: 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/PlatformPei/PlatformPei.inf | 1 +
>>>>>> OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
>>>>>> 2 files changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>>> index 6ef77ba7bb21..de60332e9390 100644
>>>>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>>>>> @@ -113,6 +113,7 @@ [Pcd]
>>>>>>
>>>>>> [FixedPcd]
>>>>>> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>>>> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
>>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>>>>>> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>>>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>>>>> index dddffdebda4b..d524929f9e10 100644
>>>>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>>>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>>>>> @@ -141,6 +141,7 @@ AmdSevInitialize (
>>>>>> )
>>>>>> {
>>>>>> UINT64 EncryptionMask;
>>>>>> + UINT64 TpmBaseAddress;
>>>>>> RETURN_STATUS PcdStatus;
>>>>>>
>>>>>> //
>>>>>> @@ -206,6 +207,24 @@ AmdSevInitialize (
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> + //
>>>>>> + // PEI TPM support will perform MMIO accesses, be sure this range is not
>>>>>> + // marked encrypted.
>>>>>> + //
>>>>>> + TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
>>>>>> + if (TpmBaseAddress != 0) {
>>>>>
>>>>> It's OK to keep this as a sanity check, yes.
>>>>>
>>>>>> + RETURN_STATUS DecryptStatus;
>>>>>> +
>>>>>> + DecryptStatus = MemEncryptSevClearPageEncMask (
>>>>>> + 0,
>>>>>> + TpmBaseAddress,
>>>>>> + EFI_SIZE_TO_PAGES (0x5000),
>>>>>
>>>>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>>>>
>>>>>> + FALSE
>>>>>> + );
>>>>>> +
>>>>>> + ASSERT_RETURN_ERROR (DecryptStatus);
>>>>>
>>>>> (3) So this is where the mess begins.
>>>>>
>>>>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>>>>> PlatformPei determines if SEV is active, and (in case SEV is active)
>>>>> PlatformPei decrypts the MMIO range of the TPM.
>>>>>
>>>>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>>>>> the current TRUE, to some PPI GUID.
>>>>>
>>>>> There are two choices for that PPI:
>>>>>
>>>>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>>>>
>>>>> Advantages:
>>>>>
>>>>> - no new PPI definition needed,
>>>>>
>>>>> - no new PPI installation needed,
>>>>>
>>>>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>>>>
>>>>> Disadvantages:
>>>>>
>>>>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>>>>
>>>>>
>>>>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>>>>
>>>>> Disadvantages:
>>>>>
>>>>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>>>>> in a separate patch; its comment should say "this PPI signals that
>>>>> accessing the MMIO range of the TPM is possible in the PEI phase,
>>>>> regardless of memory encryption". The PPI definitions should be kept
>>>>> alphabetically ordered.
>>>>>
>>>>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>>>>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>>>>> install this new PPI either when the SEV check at the top of
>>>>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>>>>
>>>>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>>>>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>>>>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>>>>> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
>>>>> stricter-than-before depex, so something on the bhyve platform too must
>>>>> produce the new PPI.
>>>>>
>>>>> Advantages:
>>>>>
>>>>> - more or less palatable as a concept, with the new PPI precisely
>>>>> expressing the dependency we have.
>>>>>
>>>>>
>>>>> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
>>>>> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
>>>>> update is actually unnecessary, because on Bhyve, there is no TPM
>>>>> support and/or no SEV support in fact, then *first* we have to create an
>>>>> independent Bhyve cleanup series, that rips out the TPM and/or SEV
>>>>> remnants from the OvmfPkg/Bhyve sub-tree.
>>>>>
>>>>>
>>>>> I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
>>>>> but I strongly believe in keeping all platforms in the tree, and that
>>>>> means we need to spend time on such changes.
>>>>>
>>>>> I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
>>>>> patch review thread, and they'd have no useful context. I suggest simply
>>>>> including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
>>>>> this series, with a proper explanation in the blurb (patch#0) and on the
>>>>> "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
>>>>> to evaluate whether the change is necessary, or whether we should purge
>>>>> the TPM and/or the SEV bits from Bhyve. You could also ask them just
>>>>> this question in advance, in a separate email on the list (with
>>>>> distilled context). Personally I'm unsure if the TPM and SEV bits
>>>>> survived into Bhyve because those bits are actually put to use there, or
>>>>> because the initial platform creation / cloning wasn't as minimal as it
>>>>> could have been.
>>>>>
>>>>> Note that in case TPM makes sense on bhyve but SEV doesn't, then
>>>>> "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
>>>>> unconditionally.
>>>>
>>>> I've had a further idea on this.
>>>>
>>>> You could add an entirely new PEIM just for this. The entry point
>>>> function of the PEIM would check for SEV, decrypt the TPM range if SEV
>>>> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
>>>> (unconditionally). The exit status of the PEIM would always be
>>>> EFI_ABORTED, because there would be no need to keep the PEIM resident.
>>>>
>>>> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
>>>> make sure that potential page table splitting for the potential MMIO
>>>> range decryption could be satisfied from permanent PEI RAM.
>>>>
>>>> The new PEIM would be included in the DSC and FDF files of the usual
>>>> three OVMF platforms, and in the Bhyve platform -- dependent on the
>>>> TPM_ENABLE build flag.
>>>>
>>>> There are several advantages to such a separate PEIM:
>>>>
>>>> - For Bhyve, the update is minimal. Just include one line in each of the
>>>> FDF and the DSC files. No need to customize an existent
>>>> platform-specific PEIM, no code duplication between two PlatformPei modules.
>>>>
>>>> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
>>>> only be included in the firmware binaries if and only if Tcg2ConfigPei
>>>> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
>>>>
>>>> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
>>>> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
>>>> already has the right value.
>>>>
>>>> - The new logic would be properly ordered between PlatformPei and
>>>> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
>>>> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
>>>> via "memory discovered" (needed for potential page table splitting), TPM
>>>> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
>>>>
>>>> You could place the new PEIM at:
>>>>
>>>> OvmfPkg/Tcg/TpmMmioSevDecryptPei
>>>>
>>>> If you haven't lost your patience with me yet, I'd really appreciate if
>>>> you could investigate this!
>>>>
>>>
>>> So far, this appears to be working nicely. I'm new at the whole PEIM
>>> thing, so hopefully I haven't missed anything. I should be submitting the
>>> patches soon for review.
>>
>> So one thing I failed to do before submitting my previous patch was to
>> complete my testing against the IA32 and X64 combination build. In this
>> build, PEI is built as Ia32, and MemEncryptSevClearPageEncMask() will
>> return UNSUPPORTED causing an ASSERT (since I check the return code). So
>> there are a few options:
>>
>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>> support that fails because of the ValidateMmioMemory() check. I can do
>> the mapping change just for SEV-ES since it is X64 only. This works,
>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>> when running in 64-bit.
>
> Can we really say "SEV works" though? Because, even using an X64 PEI
> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
> the PEI phase. Is my understanding correct?
Because the memory range is marked as MMIO, we'll take a nested page fault
(NPF). The GPA passed as part of the NPF does not include the c-bit. So we
do in fact work properly with a TPM in SEV. SEV-ES would also work
properly if the mitigation for accessing an encrypted address was removed
from the #VC handler. It is only this added mitigation to protect MMIO
that results in an issue with the TPM in PEI.
>
> I think the behavior you currently see is actually what we want, we
> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
> simply unusable. Silently pretending that the TPM is not there, even
> though it may have been configured on the QEMU command line, we just
> failed to communicate with it, is not a good idea, IMO.
However, because the c-bit is not part of the NPF, we do communicate
successfully with the TPM.
So we could actually do following:
- For IA32:
- Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
- Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
- For X64:
- Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
- Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
That might be confusing, though. So we could just do option #3 below.
Thanks,
Tom
>
> This is somewhat similar IMO to the S3Verification() function in
> "OvmfPkg/PlatformPei/Platform.c".
>
> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
>
> Thanks,
> Laszlo
>
>>
>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>> the return status.
>>
>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>> version simply returns SUCCESS because it can't do anything and the X64
>> version calls MemEncryptSevClearPageEncMask(), allowing the main code
>> to ASSERT on any errors.
>>
>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>
>> Thanks,
>> Tom
>>
>>>
>>> One thing I found is that the Bhyve package makes reference to the
>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>>> think that TPM enablement has been tested. I didn't update the Bhyve
>>> support for that reason.
>>>
>>> Thanks,
>>> Tom
>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-26 14:21 ` Lendacky, Thomas
@ 2021-04-27 14:58 ` Lendacky, Thomas
2021-04-28 16:12 ` Laszlo Ersek
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-27 14:58 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/26/21 9:21 AM, Tom Lendacky wrote:
> On 4/26/21 7:07 AM, Laszlo Ersek wrote:
>> On 04/23/21 22:02, Tom Lendacky wrote:
>>> On 4/23/21 12:41 PM, Tom Lendacky wrote:
>>>> On 4/23/21 8:04 AM, Laszlo Ersek wrote:
>>>>> On 04/23/21 12:26, Laszlo Ersek wrote:
>>>>>> review#2 from scratch:
>>>>>>
>>>>>> On 04/21/21 00:54, Tom Lendacky wrote:
>>>>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
...
>>>>>
>>>>> I've had a further idea on this.
>>>>>
>>>>> You could add an entirely new PEIM just for this. The entry point
>>>>> function of the PEIM would check for SEV, decrypt the TPM range if SEV
>>>>> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
>>>>> (unconditionally). The exit status of the PEIM would always be
>>>>> EFI_ABORTED, because there would be no need to keep the PEIM resident.
>>>>>
>>>>> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
>>>>> make sure that potential page table splitting for the potential MMIO
>>>>> range decryption could be satisfied from permanent PEI RAM.
>>>>>
>>>>> The new PEIM would be included in the DSC and FDF files of the usual
>>>>> three OVMF platforms, and in the Bhyve platform -- dependent on the
>>>>> TPM_ENABLE build flag.
>>>>>
>>>>> There are several advantages to such a separate PEIM:
>>>>>
>>>>> - For Bhyve, the update is minimal. Just include one line in each of the
>>>>> FDF and the DSC files. No need to customize an existent
>>>>> platform-specific PEIM, no code duplication between two PlatformPei modules.
>>>>>
>>>>> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
>>>>> only be included in the firmware binaries if and only if Tcg2ConfigPei
>>>>> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
>>>>>
>>>>> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
>>>>> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
>>>>> already has the right value.
>>>>>
>>>>> - The new logic would be properly ordered between PlatformPei and
>>>>> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
>>>>> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
>>>>> via "memory discovered" (needed for potential page table splitting), TPM
>>>>> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
>>>>>
>>>>> You could place the new PEIM at:
>>>>>
>>>>> OvmfPkg/Tcg/TpmMmioSevDecryptPei
>>>>>
>>>>> If you haven't lost your patience with me yet, I'd really appreciate if
>>>>> you could investigate this!
>>>>>
>>>>
>>>> So far, this appears to be working nicely. I'm new at the whole PEIM
>>>> thing, so hopefully I haven't missed anything. I should be submitting the
>>>> patches soon for review.
>>>
>>> So one thing I failed to do before submitting my previous patch was to
>>> complete my testing against the IA32 and X64 combination build. In this
>>> build, PEI is built as Ia32, and MemEncryptSevClearPageEncMask() will
>>> return UNSUPPORTED causing an ASSERT (since I check the return code). So
>>> there are a few options:
>>>
>>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>>> support that fails because of the ValidateMmioMemory() check. I can do
>>> the mapping change just for SEV-ES since it is X64 only. This works,
>>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>>> when running in 64-bit.
>>
>> Can we really say "SEV works" though? Because, even using an X64 PEI
>> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
>> the PEI phase. Is my understanding correct?
>
> Because the memory range is marked as MMIO, we'll take a nested page fault
> (NPF). The GPA passed as part of the NPF does not include the c-bit. So we
> do in fact work properly with a TPM in SEV. SEV-ES would also work
> properly if the mitigation for accessing an encrypted address was removed
> from the #VC handler. It is only this added mitigation to protect MMIO
> that results in an issue with the TPM in PEI.
So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this:
//
// 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 ()) {
// Do MemEncryptSevClearPageEncMask() ...
}
Let me submit the next version with this and see what you think.
Thanks,
Tom
>
>>
>> I think the behavior you currently see is actually what we want, we
>> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
>> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
>> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
>> simply unusable. Silently pretending that the TPM is not there, even
>> though it may have been configured on the QEMU command line, we just
>> failed to communicate with it, is not a good idea, IMO.
>
> However, because the c-bit is not part of the NPF, we do communicate
> successfully with the TPM.
>
> So we could actually do following:
> - For IA32:
> - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
> - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>
> - For X64:
> - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
> - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>
> That might be confusing, though. So we could just do option #3 below.
>
> Thanks,
> Tom
>
>>
>> This is somewhat similar IMO to the S3Verification() function in
>> "OvmfPkg/PlatformPei/Platform.c".
>>
>> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>>> the return status.
>>>
>>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>>> version simply returns SUCCESS because it can't do anything and the X64
>>> version calls MemEncryptSevClearPageEncMask(), allowing the main code
>>> to ASSERT on any errors.
>>>
>>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>> One thing I found is that the Bhyve package makes reference to the
>>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>>>> think that TPM enablement has been tested. I didn't update the Bhyve
>>>> support for that reason.
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>> Thanks!
>>>>> Laszlo
>>>>>
>>>
>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-27 14:58 ` Lendacky, Thomas
@ 2021-04-28 16:12 ` Laszlo Ersek
2021-04-28 19:09 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-28 16:12 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/27/21 16:58, Tom Lendacky wrote:
> On 4/26/21 9:21 AM, Tom Lendacky wrote:
>> On 4/26/21 7:07 AM, Laszlo Ersek wrote:
>>> On 04/23/21 22:02, Tom Lendacky wrote:
>>>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>>>> support that fails because of the ValidateMmioMemory() check. I can do
>>>> the mapping change just for SEV-ES since it is X64 only. This works,
>>>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>>>> when running in 64-bit.
>>>
>>> Can we really say "SEV works" though? Because, even using an X64 PEI
>>> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
>>> the PEI phase. Is my understanding correct?
>>
>> Because the memory range is marked as MMIO, we'll take a nested page fault
>> (NPF). The GPA passed as part of the NPF does not include the c-bit. So we
>> do in fact work properly with a TPM in SEV.
Thanks for the explanation.
Here's what bothers me about it:
In AmdSevDxe, we clear the C-bit from MMIO and NonExistent areas in the
GCD memory space map. This occurs early in the DXE phase (see "APRIORI
DXE" in the FDF files). The justification is that we want the flash and
(for example) the PCI MMIO apertures decrypted.
Now, I realize there is a difference between flash and TPM. TPM is
purely MMIO (no KVM memslot), but flash (when it is not in programming
mode) is backed by a read-only KVM memslot. IOW, flash is "actual
memory", and so it is affected by SEV. TPM is never "actual memory", so
(according to your explanation, AIUI) it always traps to QEMU, per
access, and the C-bit doesn't interfere with that.
This is consistent with two facts about OVMF's PEI phase:
- We use IO Port-based fw_cfg (never DMA), if SEV is enabled (see
"QemuFwCfgPei.c").
- We access PCI config space via IO Ports (0xCF8, 0xCFC), never ECAM.
(This was not motivated by SEV, see commit 7523788faa51, but it does
play nice with SEV, in the PEI phase -- I think?)
What I'm confused about, now, in retrospect, is the reference to the PCI
MMIO aperture, in AmdSevDxe. If that area isn't backed by a KVM memslot
*either* -- similarly to the TPM area --, then decrypting *that* in
AmdSevDxe (via "nonexistent") is not strictly necessary. Is that correct?
I'm not asking for any code changes, just trying to form a consistent view.
Another question (still for "base SEV"): when OVMF is built with
SMM_REQUIRE, PlatformPei performs a (read-only) variable access. See the
ReadOnlyVariable2->GetVariable() call in the RefreshMemTypeInfo()
function. When SEV is active, does control reach RefreshMemTypeInfo() on
your end? And does ReadOnlyVariable2->GetVariable() succeed for you?
(There is a DEBUG_VERBOSE message in OnReadOnlyVariable2Available(), and
a DEBUG_ERROR in RefreshMemTypeInfo(), so the above questions can be
answered just from the log; no need to modify the code for that.)
Basically, with SEV enabled, I expect ReadOnlyVariable2->GetVariable()
to fail -- or even to remain unreached, as FaultTolerantWritePei and
VariablePei could bail out earlier (before installing the Variable PPI),
due to failing flash accesses. In case I'm *not* wrong -- it's not the
end of the world, I'm only asking this question too for the sake of
clarifying "C-bit vs. MMIO".
More or less it seems to boil down to whether there is a KVM memslot or
not -- which is *not* equivalent to OVMF considering the area MMIO or not.
>> SEV-ES would also work
>> properly if the mitigation for accessing an encrypted address was removed
>> from the #VC handler. It is only this added mitigation to protect MMIO
>> that results in an issue with the TPM in PEI.
>
> So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this:
>
> //
> // 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 ()) {
> // Do MemEncryptSevClearPageEncMask() ...
> }
>
> Let me submit the next version with this and see what you think.
Yep, I'll review that now.
Thanks
Laszlo
>
> Thanks,
> Tom
>
>>
>>>
>>> I think the behavior you currently see is actually what we want, we
>>> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
>>> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
>>> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
>>> simply unusable. Silently pretending that the TPM is not there, even
>>> though it may have been configured on the QEMU command line, we just
>>> failed to communicate with it, is not a good idea, IMO.
>>
>> However, because the c-bit is not part of the NPF, we do communicate
>> successfully with the TPM.
>>
>> So we could actually do following:
>> - For IA32:
>> - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
>> - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>
>> - For X64:
>> - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
>> - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>
>> That might be confusing, though. So we could just do option #3 below.
>>
>> Thanks,
>> Tom
>>
>>>
>>> This is somewhat similar IMO to the S3Verification() function in
>>> "OvmfPkg/PlatformPei/Platform.c".
>>>
>>> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>>
>>>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>>>> the return status.
>>>>
>>>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>>>> version simply returns SUCCESS because it can't do anything and the X64
>>>> version calls MemEncryptSevClearPageEncMask(), allowing the main code
>>>> to ASSERT on any errors.
>>>>
>>>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>>
>>>>> One thing I found is that the Bhyve package makes reference to the
>>>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>>>>> think that TPM enablement has been tested. I didn't update the Bhyve
>>>>> support for that reason.
>>>>>
>>>>> Thanks,
>>>>> Tom
>>>>>
>>>>>> Thanks!
>>>>>> Laszlo
>>>>>>
>>>>
>>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-28 16:12 ` Laszlo Ersek
@ 2021-04-28 19:09 ` Lendacky, Thomas
2021-04-30 15:39 ` Laszlo Ersek
0 siblings, 1 reply; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-28 19:09 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/28/21 11:12 AM, Laszlo Ersek wrote:
> On 04/27/21 16:58, Tom Lendacky wrote:
>> On 4/26/21 9:21 AM, Tom Lendacky wrote:
>>> On 4/26/21 7:07 AM, Laszlo Ersek wrote:
>>>> On 04/23/21 22:02, Tom Lendacky wrote:
>
>>>>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>>>>> support that fails because of the ValidateMmioMemory() check. I can do
>>>>> the mapping change just for SEV-ES since it is X64 only. This works,
>>>>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>>>>> when running in 64-bit.
>>>>
>>>> Can we really say "SEV works" though? Because, even using an X64 PEI
>>>> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
>>>> the PEI phase. Is my understanding correct?
>>>
>>> Because the memory range is marked as MMIO, we'll take a nested page fault
>>> (NPF). The GPA passed as part of the NPF does not include the c-bit. So we
>>> do in fact work properly with a TPM in SEV.
>
> Thanks for the explanation.
>
> Here's what bothers me about it:
>
> In AmdSevDxe, we clear the C-bit from MMIO and NonExistent areas in the
> GCD memory space map. This occurs early in the DXE phase (see "APRIORI
> DXE" in the FDF files). The justification is that we want the flash and
> (for example) the PCI MMIO apertures decrypted.
>
> Now, I realize there is a difference between flash and TPM. TPM is
> purely MMIO (no KVM memslot), but flash (when it is not in programming
> mode) is backed by a read-only KVM memslot. IOW, flash is "actual
> memory", and so it is affected by SEV. TPM is never "actual memory", so
> (according to your explanation, AIUI) it always traps to QEMU, per
> access, and the C-bit doesn't interfere with that.
>
> This is consistent with two facts about OVMF's PEI phase:
>
> - We use IO Port-based fw_cfg (never DMA), if SEV is enabled (see
> "QemuFwCfgPei.c").
>
> - We access PCI config space via IO Ports (0xCF8, 0xCFC), never ECAM.
> (This was not motivated by SEV, see commit 7523788faa51, but it does
> play nice with SEV, in the PEI phase -- I think?)
>
> What I'm confused about, now, in retrospect, is the reference to the PCI
> MMIO aperture, in AmdSevDxe. If that area isn't backed by a KVM memslot
> *either* -- similarly to the TPM area --, then decrypting *that* in
> AmdSevDxe (via "nonexistent") is not strictly necessary. Is that correct?
It is necessary for (and was added for) SEV-ES. The explicit mapping of
the PCI MMCONFIG range as unencrypted was added for SEV-ES so that the
SEV-ES mitigation would not terminate the guest because MMIO was being
performed against an encrypted address.
There's a nice comment in OvmfPkg/PlatformPei/Platform.c that talks about
how the MMCONFIG range is not added as MMIO, but instead as reserved
memory. Because of that, the loop through the memory space map in
AmdSevDxe.c does not mark the MMCONFIG range as unencrypted.
>
> I'm not asking for any code changes, just trying to form a consistent view.
>
> Another question (still for "base SEV"): when OVMF is built with
> SMM_REQUIRE, PlatformPei performs a (read-only) variable access. See the
> ReadOnlyVariable2->GetVariable() call in the RefreshMemTypeInfo()
> function. When SEV is active, does control reach RefreshMemTypeInfo() on
> your end? And does ReadOnlyVariable2->GetVariable() succeed for you?
>
> (There is a DEBUG_VERBOSE message in OnReadOnlyVariable2Available(), and
> a DEBUG_ERROR in RefreshMemTypeInfo(), so the above questions can be
> answered just from the log; no need to modify the code for that.)
Yes, control reaches that point. But, notably, with a legacy VM I see the
following messages:
RefreshMemTypeInfo: GetVariable(): Not Found
But with an SEV VM I see:
Firmware Volume for Variable Store is corrupted
RefreshMemTypeInfo: GetVariable(): Not Found
So I get the "Not Found" in both cases. But with SEV, I see the
"corrupted" message from InitRealNonVolatileVariableStore() in
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c. I
imagine that since the range is marked encrypted, it reads the header
incorrectly and fails.
Thanks,
Tom
>
> Basically, with SEV enabled, I expect ReadOnlyVariable2->GetVariable()
> to fail -- or even to remain unreached, as FaultTolerantWritePei and
> VariablePei could bail out earlier (before installing the Variable PPI),
> due to failing flash accesses. In case I'm *not* wrong -- it's not the
> end of the world, I'm only asking this question too for the sake of
> clarifying "C-bit vs. MMIO".
>
> More or less it seems to boil down to whether there is a KVM memslot or
> not -- which is *not* equivalent to OVMF considering the area MMIO or not.
>
>
>>> SEV-ES would also work
>>> properly if the mitigation for accessing an encrypted address was removed
>>> from the #VC handler. It is only this added mitigation to protect MMIO
>>> that results in an issue with the TPM in PEI.
>>
>> So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this:
>>
>> //
>> // 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 ()) {
>> // Do MemEncryptSevClearPageEncMask() ...
>> }
>>
>> Let me submit the next version with this and see what you think.
>
> Yep, I'll review that now.
>
> Thanks
> Laszlo
>
>>
>> Thanks,
>> Tom
>>
>>>
>>>>
>>>> I think the behavior you currently see is actually what we want, we
>>>> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
>>>> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
>>>> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
>>>> simply unusable. Silently pretending that the TPM is not there, even
>>>> though it may have been configured on the QEMU command line, we just
>>>> failed to communicate with it, is not a good idea, IMO.
>>>
>>> However, because the c-bit is not part of the NPF, we do communicate
>>> successfully with the TPM.
>>>
>>> So we could actually do following:
>>> - For IA32:
>>> - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
>>> - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>
>>> - For X64:
>>> - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
>>> - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>
>>> That might be confusing, though. So we could just do option #3 below.
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>> This is somewhat similar IMO to the S3Verification() function in
>>>> "OvmfPkg/PlatformPei/Platform.c".
>>>>
>>>> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>>>
>>>>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>>>>> the return status.
>>>>>
>>>>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>>>>> version simply returns SUCCESS because it can't do anything and the X64
>>>>> version calls MemEncryptSevClearPageEncMask(), allowing the main code
>>>>> to ASSERT on any errors.
>>>>>
>>>>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>>>>
>>>>> Thanks,
>>>>> Tom
>>>>>
>>>>>>
>>>>>> One thing I found is that the Bhyve package makes reference to the
>>>>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>>>>>> think that TPM enablement has been tested. I didn't update the Bhyve
>>>>>> support for that reason.
>>>>>>
>>>>>> Thanks,
>>>>>> Tom
>>>>>>
>>>>>>> Thanks!
>>>>>>> Laszlo
>>>>>>>
>>>>>
>>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-28 19:09 ` Lendacky, Thomas
@ 2021-04-30 15:39 ` Laszlo Ersek
2021-04-30 17:37 ` Lendacky, Thomas
0 siblings, 1 reply; 39+ messages in thread
From: Laszlo Ersek @ 2021-04-30 15:39 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 04/28/21 21:09, Tom Lendacky wrote:
> On 4/28/21 11:12 AM, Laszlo Ersek wrote:
>> On 04/27/21 16:58, Tom Lendacky wrote:
>>> On 4/26/21 9:21 AM, Tom Lendacky wrote:
>>>> On 4/26/21 7:07 AM, Laszlo Ersek wrote:
>>>>> On 04/23/21 22:02, Tom Lendacky wrote:
>>
>>>>>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>>>>>> support that fails because of the ValidateMmioMemory() check. I can do
>>>>>> the mapping change just for SEV-ES since it is X64 only. This works,
>>>>>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>>>>>> when running in 64-bit.
>>>>>
>>>>> Can we really say "SEV works" though? Because, even using an X64 PEI
>>>>> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
>>>>> the PEI phase. Is my understanding correct?
>>>>
>>>> Because the memory range is marked as MMIO, we'll take a nested page fault
>>>> (NPF). The GPA passed as part of the NPF does not include the c-bit. So we
>>>> do in fact work properly with a TPM in SEV.
>>
>> Thanks for the explanation.
>>
>> Here's what bothers me about it:
>>
>> In AmdSevDxe, we clear the C-bit from MMIO and NonExistent areas in the
>> GCD memory space map. This occurs early in the DXE phase (see "APRIORI
>> DXE" in the FDF files). The justification is that we want the flash and
>> (for example) the PCI MMIO apertures decrypted.
>>
>> Now, I realize there is a difference between flash and TPM. TPM is
>> purely MMIO (no KVM memslot), but flash (when it is not in programming
>> mode) is backed by a read-only KVM memslot. IOW, flash is "actual
>> memory", and so it is affected by SEV. TPM is never "actual memory", so
>> (according to your explanation, AIUI) it always traps to QEMU, per
>> access, and the C-bit doesn't interfere with that.
>>
>> This is consistent with two facts about OVMF's PEI phase:
>>
>> - We use IO Port-based fw_cfg (never DMA), if SEV is enabled (see
>> "QemuFwCfgPei.c").
>>
>> - We access PCI config space via IO Ports (0xCF8, 0xCFC), never ECAM.
>> (This was not motivated by SEV, see commit 7523788faa51, but it does
>> play nice with SEV, in the PEI phase -- I think?)
>>
>> What I'm confused about, now, in retrospect, is the reference to the PCI
>> MMIO aperture, in AmdSevDxe. If that area isn't backed by a KVM memslot
>> *either* -- similarly to the TPM area --, then decrypting *that* in
>> AmdSevDxe (via "nonexistent") is not strictly necessary. Is that correct?
>
> It is necessary for (and was added for) SEV-ES. The explicit mapping of
> the PCI MMCONFIG range as unencrypted was added for SEV-ES so that the
> SEV-ES mitigation would not terminate the guest because MMIO was being
> performed against an encrypted address.
>
> There's a nice comment in OvmfPkg/PlatformPei/Platform.c that talks about
> how the MMCONFIG range is not added as MMIO, but instead as reserved
> memory. Because of that, the loop through the memory space map in
> AmdSevDxe.c does not mark the MMCONFIG range as unencrypted.
I didn't mean commit 84cddd70820f ("OvmfPkg/AmdSevDxe: Clear encryption
bit on PCIe MMCONFIG range", 2021-01-07) -- I didn't mean PCI config space.
I meant commit 24e4ad75546b ("OvmfPkg: Add AmdSevDxe driver",
2017-07-10) -- I wrote "PCI MMIO aperture".
So, to repeat my question: when 24e4ad75546b3 was implemented (which
happened just for SEV), then (apparently) clearing the C-bit on the PCI
MMIO aperture (where PCI MMIO BARs were going to be allocated from)
wasn't strictly necessary *at the time*; correct? Because that area
isn't backed by RAM, accesses trap to QEMU directly, and the C bit does
not make a difference there. (IIUC). Does this make sense or am I wrong?
>
>>
>> I'm not asking for any code changes, just trying to form a consistent view.
>>
>> Another question (still for "base SEV"): when OVMF is built with
>> SMM_REQUIRE, PlatformPei performs a (read-only) variable access. See the
>> ReadOnlyVariable2->GetVariable() call in the RefreshMemTypeInfo()
>> function. When SEV is active, does control reach RefreshMemTypeInfo() on
>> your end? And does ReadOnlyVariable2->GetVariable() succeed for you?
>>
>> (There is a DEBUG_VERBOSE message in OnReadOnlyVariable2Available(), and
>> a DEBUG_ERROR in RefreshMemTypeInfo(), so the above questions can be
>> answered just from the log; no need to modify the code for that.)
>
> Yes, control reaches that point. But, notably, with a legacy VM I see the
> following messages:
> RefreshMemTypeInfo: GetVariable(): Not Found
>
> But with an SEV VM I see:
> Firmware Volume for Variable Store is corrupted
> RefreshMemTypeInfo: GetVariable(): Not Found
>
> So I get the "Not Found" in both cases. But with SEV, I see the
> "corrupted" message from InitRealNonVolatileVariableStore() in
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c. I
> imagine that since the range is marked encrypted, it reads the header
> incorrectly and fails.
Thank you for confirming! I don't think we need to sweat this symptom.
I'm just glad my hunch wasn't wrong.
Thanks,
Laszlo
>
> Thanks,
> Tom
>
>>
>> Basically, with SEV enabled, I expect ReadOnlyVariable2->GetVariable()
>> to fail -- or even to remain unreached, as FaultTolerantWritePei and
>> VariablePei could bail out earlier (before installing the Variable PPI),
>> due to failing flash accesses. In case I'm *not* wrong -- it's not the
>> end of the world, I'm only asking this question too for the sake of
>> clarifying "C-bit vs. MMIO".
>>
>> More or less it seems to boil down to whether there is a KVM memslot or
>> not -- which is *not* equivalent to OVMF considering the area MMIO or not.
>>
>>
>>>> SEV-ES would also work
>>>> properly if the mitigation for accessing an encrypted address was removed
>>>> from the #VC handler. It is only this added mitigation to protect MMIO
>>>> that results in an issue with the TPM in PEI.
>>>
>>> So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this:
>>>
>>> //
>>> // 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 ()) {
>>> // Do MemEncryptSevClearPageEncMask() ...
>>> }
>>>
>>> Let me submit the next version with this and see what you think.
>>
>> Yep, I'll review that now.
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>>>
>>>>> I think the behavior you currently see is actually what we want, we
>>>>> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
>>>>> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
>>>>> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
>>>>> simply unusable. Silently pretending that the TPM is not there, even
>>>>> though it may have been configured on the QEMU command line, we just
>>>>> failed to communicate with it, is not a good idea, IMO.
>>>>
>>>> However, because the c-bit is not part of the NPF, we do communicate
>>>> successfully with the TPM.
>>>>
>>>> So we could actually do following:
>>>> - For IA32:
>>>> - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
>>>> - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>>
>>>> - For X64:
>>>> - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
>>>> - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>>
>>>> That might be confusing, though. So we could just do option #3 below.
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>>
>>>>> This is somewhat similar IMO to the S3Verification() function in
>>>>> "OvmfPkg/PlatformPei/Platform.c".
>>>>>
>>>>> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
>>>>>
>>>>> Thanks,
>>>>> Laszlo
>>>>>
>>>>>>
>>>>>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>>>>>> the return status.
>>>>>>
>>>>>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>>>>>> version simply returns SUCCESS because it can't do anything and the X64
>>>>>> version calls MemEncryptSevClearPageEncMask(), allowing the main code
>>>>>> to ASSERT on any errors.
>>>>>>
>>>>>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>>>>>
>>>>>> Thanks,
>>>>>> Tom
>>>>>>
>>>>>>>
>>>>>>> One thing I found is that the Bhyve package makes reference to the
>>>>>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>>>>>>> think that TPM enablement has been tested. I didn't update the Bhyve
>>>>>>> support for that reason.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Tom
>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> Laszlo
>>>>>>>>
>>>>>>
>>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
2021-04-30 15:39 ` Laszlo Ersek
@ 2021-04-30 17:37 ` Lendacky, Thomas
0 siblings, 0 replies; 39+ messages in thread
From: Lendacky, Thomas @ 2021-04-30 17:37 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Joerg Roedel, Borislav Petkov, Ard Biesheuvel, Jordan Justen,
Brijesh Singh, James Bottomley, Jiewen Yao, Min Xu
On 4/30/21 10:39 AM, Laszlo Ersek wrote:
> On 04/28/21 21:09, Tom Lendacky wrote:
>> On 4/28/21 11:12 AM, Laszlo Ersek wrote:
>>> On 04/27/21 16:58, Tom Lendacky wrote:
>>>> On 4/26/21 9:21 AM, Tom Lendacky wrote:
>>>>> On 4/26/21 7:07 AM, Laszlo Ersek wrote:
>>>>>> On 04/23/21 22:02, Tom Lendacky wrote:
>>>
>>>>>>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>>>>>>> support that fails because of the ValidateMmioMemory() check. I can do
>>>>>>> the mapping change just for SEV-ES since it is X64 only. This works,
>>>>>>> because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>>>>>>> when running in 64-bit.
>>>>>>
>>>>>> Can we really say "SEV works" though? Because, even using an X64 PEI
>>>>>> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
>>>>>> the PEI phase. Is my understanding correct?
>>>>>
>>>>> Because the memory range is marked as MMIO, we'll take a nested page fault
>>>>> (NPF). The GPA passed as part of the NPF does not include the c-bit. So we
>>>>> do in fact work properly with a TPM in SEV.
>>>
>>> Thanks for the explanation.
>>>
>>> Here's what bothers me about it:
>>>
>>> In AmdSevDxe, we clear the C-bit from MMIO and NonExistent areas in the
>>> GCD memory space map. This occurs early in the DXE phase (see "APRIORI
>>> DXE" in the FDF files). The justification is that we want the flash and
>>> (for example) the PCI MMIO apertures decrypted.
>>>
>>> Now, I realize there is a difference between flash and TPM. TPM is
>>> purely MMIO (no KVM memslot), but flash (when it is not in programming
>>> mode) is backed by a read-only KVM memslot. IOW, flash is "actual
>>> memory", and so it is affected by SEV. TPM is never "actual memory", so
>>> (according to your explanation, AIUI) it always traps to QEMU, per
>>> access, and the C-bit doesn't interfere with that.
>>>
>>> This is consistent with two facts about OVMF's PEI phase:
>>>
>>> - We use IO Port-based fw_cfg (never DMA), if SEV is enabled (see
>>> "QemuFwCfgPei.c").
>>>
>>> - We access PCI config space via IO Ports (0xCF8, 0xCFC), never ECAM.
>>> (This was not motivated by SEV, see commit 7523788faa51, but it does
>>> play nice with SEV, in the PEI phase -- I think?)
>>>
>>> What I'm confused about, now, in retrospect, is the reference to the PCI
>>> MMIO aperture, in AmdSevDxe. If that area isn't backed by a KVM memslot
>>> *either* -- similarly to the TPM area --, then decrypting *that* in
>>> AmdSevDxe (via "nonexistent") is not strictly necessary. Is that correct?
>>
>> It is necessary for (and was added for) SEV-ES. The explicit mapping of
>> the PCI MMCONFIG range as unencrypted was added for SEV-ES so that the
>> SEV-ES mitigation would not terminate the guest because MMIO was being
>> performed against an encrypted address.
>>
>> There's a nice comment in OvmfPkg/PlatformPei/Platform.c that talks about
>> how the MMCONFIG range is not added as MMIO, but instead as reserved
>> memory. Because of that, the loop through the memory space map in
>> AmdSevDxe.c does not mark the MMCONFIG range as unencrypted.
>
> I didn't mean commit 84cddd70820f ("OvmfPkg/AmdSevDxe: Clear encryption
> bit on PCIe MMCONFIG range", 2021-01-07) -- I didn't mean PCI config space.
>
> I meant commit 24e4ad75546b ("OvmfPkg: Add AmdSevDxe driver",
> 2017-07-10) -- I wrote "PCI MMIO aperture".
>
> So, to repeat my question: when 24e4ad75546b3 was implemented (which
> happened just for SEV), then (apparently) clearing the C-bit on the PCI
> MMIO aperture (where PCI MMIO BARs were going to be allocated from)
> wasn't strictly necessary *at the time*; correct? Because that area
> isn't backed by RAM, accesses trap to QEMU directly, and the C bit does
> not make a difference there. (IIUC). Does this make sense or am I wrong?
Ah, sorry, I misunderstood. For the most part, that is true. Not clearing
the c-bit from an area where the PCI MMIO BARs are going to be allocated
would not necessarily cause a problem.
However, clearing the c-bit for non-existent areas was needed for other
reasons. It would result in NVRAM variable corruption because of the way
that area is represented in OVMF and how it is mapped by the hypervisor.
For example, if the clearing of the c-bit for the address at 0xfef00000,
which is marked as EfiGcdMemoryTypeNonExistent, isn't done, then using the
same nvram FD file in an SEV guest results in the following:
Variable FV header is not valid. It will be reinitialized.
because a different encryption key is used on each launch of the guest.
And attempting to use the same nvram FD file for a non-SEV guest, after
running as an SEV guest, causes an ASSERT:
ASSERT [VariableRuntimeDxe] /root/kernels/ovmf-build-X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c(218): VariableStore->Size == VariableStoreLength
because the contents have been encrypted.
The video buffer suffers from a similar problem in that the video buffer
contents end up containing data that appears encrypted to the hypervisor
and results in corruption of the screen contents when attaching, e.g.,
via VNC. Although, that area is marked as EfiGcdMemoryTypeMemoryMappedIo.
Thanks,
Tom
>
>
>>
>>>
>>> I'm not asking for any code changes, just trying to form a consistent view.
>>>
>>> Another question (still for "base SEV"): when OVMF is built with
>>> SMM_REQUIRE, PlatformPei performs a (read-only) variable access. See the
>>> ReadOnlyVariable2->GetVariable() call in the RefreshMemTypeInfo()
>>> function. When SEV is active, does control reach RefreshMemTypeInfo() on
>>> your end? And does ReadOnlyVariable2->GetVariable() succeed for you?
>>>
>>> (There is a DEBUG_VERBOSE message in OnReadOnlyVariable2Available(), and
>>> a DEBUG_ERROR in RefreshMemTypeInfo(), so the above questions can be
>>> answered just from the log; no need to modify the code for that.)
>>
>> Yes, control reaches that point. But, notably, with a legacy VM I see the
>> following messages:
>> RefreshMemTypeInfo: GetVariable(): Not Found
>>
>> But with an SEV VM I see:
>> Firmware Volume for Variable Store is corrupted
>> RefreshMemTypeInfo: GetVariable(): Not Found
>>
>> So I get the "Not Found" in both cases. But with SEV, I see the
>> "corrupted" message from InitRealNonVolatileVariableStore() in
>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c. I
>> imagine that since the range is marked encrypted, it reads the header
>> incorrectly and fails.
>
> Thank you for confirming! I don't think we need to sweat this symptom.
> I'm just glad my hunch wasn't wrong.
>
> Thanks,
> Laszlo
>
>>
>> Thanks,
>> Tom
>>
>>>
>>> Basically, with SEV enabled, I expect ReadOnlyVariable2->GetVariable()
>>> to fail -- or even to remain unreached, as FaultTolerantWritePei and
>>> VariablePei could bail out earlier (before installing the Variable PPI),
>>> due to failing flash accesses. In case I'm *not* wrong -- it's not the
>>> end of the world, I'm only asking this question too for the sake of
>>> clarifying "C-bit vs. MMIO".
>>>
>>> More or less it seems to boil down to whether there is a KVM memslot or
>>> not -- which is *not* equivalent to OVMF considering the area MMIO or not.
>>>
>>>
>>>>> SEV-ES would also work
>>>>> properly if the mitigation for accessing an encrypted address was removed
>>>>> from the #VC handler. It is only this added mitigation to protect MMIO
>>>>> that results in an issue with the TPM in PEI.
>>>>
>>>> So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this:
>>>>
>>>> //
>>>> // 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 ()) {
>>>> // Do MemEncryptSevClearPageEncMask() ...
>>>> }
>>>>
>>>> Let me submit the next version with this and see what you think.
>>>
>>> Yep, I'll review that now.
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>>
>>>>>>
>>>>>> I think the behavior you currently see is actually what we want, we
>>>>>> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
>>>>>> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
>>>>>> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
>>>>>> simply unusable. Silently pretending that the TPM is not there, even
>>>>>> though it may have been configured on the QEMU command line, we just
>>>>>> failed to communicate with it, is not a good idea, IMO.
>>>>>
>>>>> However, because the c-bit is not part of the NPF, we do communicate
>>>>> successfully with the TPM.
>>>>>
>>>>> So we could actually do following:
>>>>> - For IA32:
>>>>> - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
>>>>> - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>>>
>>>>> - For X64:
>>>>> - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
>>>>> - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>>>>
>>>>> That might be confusing, though. So we could just do option #3 below.
>>>>>
>>>>> Thanks,
>>>>> Tom
>>>>>
>>>>>>
>>>>>> This is somewhat similar IMO to the S3Verification() function in
>>>>>> "OvmfPkg/PlatformPei/Platform.c".
>>>>>>
>>>>>> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
>>>>>>
>>>>>> Thanks,
>>>>>> Laszlo
>>>>>>
>>>>>>>
>>>>>>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>>>>>>> the return status.
>>>>>>>
>>>>>>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>>>>>>> version simply returns SUCCESS because it can't do anything and the X64
>>>>>>> version calls MemEncryptSevClearPageEncMask(), allowing the main code
>>>>>>> to ASSERT on any errors.
>>>>>>>
>>>>>>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Tom
>>>>>>>
>>>>>>>>
>>>>>>>> One thing I found is that the Bhyve package makes reference to the
>>>>>>>> OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
>>>>>>>> think that TPM enablement has been tested. I didn't update the Bhyve
>>>>>>>> support for that reason.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Tom
>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>> Laszlo
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2021-04-30 17:37 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-20 22:54 [PATCH 0/3] SEV-ES TPM enablement fixes Lendacky, Thomas
2021-04-20 22:54 ` [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes Lendacky, Thomas
2021-04-22 5:28 ` [edk2-devel] " Laszlo Ersek
2021-04-22 13:35 ` Lendacky, Thomas
2021-04-23 9:07 ` Laszlo Ersek
2021-04-20 22:54 ` [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes Lendacky, Thomas
2021-04-22 5:50 ` [edk2-devel] " Laszlo Ersek
2021-04-22 14:15 ` Lendacky, Thomas
2021-04-22 15:42 ` Lendacky, Thomas
2021-04-23 9:10 ` Laszlo Ersek
2021-04-23 13:24 ` Lendacky, Thomas
2021-04-20 22:54 ` [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV Lendacky, Thomas
2021-04-20 23:17 ` Eric van Tassell
2021-04-21 14:09 ` [edk2-devel] " Andrew Fish
[not found] ` <1677E4DA25FD7265.31957@groups.io>
2021-04-21 17:20 ` Andrew Fish
2021-04-21 17:45 ` Lendacky, Thomas
2021-04-21 22:24 ` Andrew Fish
2021-04-22 6:07 ` Laszlo Ersek
2021-04-23 10:26 ` Laszlo Ersek
2021-04-23 13:04 ` [edk2-devel] " Laszlo Ersek
2021-04-23 13:09 ` Laszlo Ersek
2021-04-23 17:41 ` Lendacky, Thomas
2021-04-23 20:02 ` Lendacky, Thomas
2021-04-26 12:07 ` Laszlo Ersek
2021-04-26 14:21 ` Lendacky, Thomas
2021-04-27 14:58 ` Lendacky, Thomas
2021-04-28 16:12 ` Laszlo Ersek
2021-04-28 19:09 ` Lendacky, Thomas
2021-04-30 15:39 ` Laszlo Ersek
2021-04-30 17:37 ` Lendacky, Thomas
2021-04-26 11:08 ` Laszlo Ersek
[not found] ` <1677B2EC90F30786.1355@groups.io>
2021-04-20 23:13 ` Lendacky, Thomas
2021-04-22 7:34 ` Laszlo Ersek
2021-04-22 8:31 ` Laszlo Ersek
2021-04-22 8:39 ` Laszlo Ersek
2021-04-22 19:10 ` Lendacky, Thomas
2021-04-23 9:28 ` Laszlo Ersek
2021-04-22 14:51 ` Lendacky, Thomas
2021-04-22 16:04 ` Lendacky, Thomas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox