I'm going to ask for v3 after all: On 04/27/21 18:21, Lendacky, Thomas wrote: > From: Tom Lendacky <thomas.lenda...@amd.com> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345 > > During PEI, the MMIO range for the TPM is marked as encrypted when running > as an SEV guest. While this isn't an issue for an SEV guest because of > the way the nested page fault is handled, it does result in an SEV-ES > guest terminating because of a mitigation check in the #VC handler to > prevent MMIO to an encrypted address. For an SEV-ES guest, this range > must be marked as unencrypted. > > Create a new x86 PEIM for TPM support that will map the TPM MMIO range as > unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI > will be unconditionally installed before exiting. The PEIM will exit with > the EFI_ABORTED status so that the PEIM does not stay resident.
(1) Please spell out that the new PEIM will depend on the installation of the permanent PEI RAM, by PlatformPei -- the reason being that, in case page table splitting proves necessary for clearing the C-bit, the new page table(s) should be allocated from permanent PEI RAM. > > The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a > Depex for IA32 and X64 builds so that the MMIO range is properly mapped > for SEV-ES before the Tcg2Config PEIM is loaded. (2) The Tcg2Config depex change should be a separate patch -- the last patch in the series. That covers both the Tcg2Config hunk in the patch body, and this commit message paragraph right here. (3) While at it: those parts of this patch that are *not* related to the Tcg2Config PEIM can be squashed into the previous patch -- if you prefer that. I'm certainly happy with three separate patches though: for the DEC file, for TpmMmioSevDecryptPei + the DSC/FDF files, and finally the Tcg2Config PEIM. So in total the series should include 4 or 5 patches (up to you). > > Update all OVMF Ia32 and X64 build packages to include this new PEIM. > > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Brijesh Singh <brijesh.si...@amd.com> > Cc: Erdem Aktas <erdemak...@google.com> > Cc: James Bottomley <j...@linux.ibm.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Min Xu <min.m...@intel.com> > Cc: Marc-Andr?? Lureau <marcandre.lur...@redhat.com> (4) Marc-André's name is garbled here too. The following git config options are related: - For encoding non-ASCII characters in git commits, the "i18n.commitencoding" knob is relevant. Almost universally, this should be "UTF-8" (assuming your text editor used for composing commit messages produces UTF-8-encoded files). - For formatting commits to patch emails, "i18n.logOutputEncoding" matters. This should *always* be "UTF-8", when git-format-patch is invoked. > Cc: Stefan Berger <stef...@linux.ibm.com> > Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> > --- > OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/AmdSev/AmdSevX64.fdf | 1 + > OvmfPkg/OvmfPkgIa32.fdf | 1 + > OvmfPkg/OvmfPkgIa32X64.fdf | 1 + > OvmfPkg/OvmfPkgX64.fdf | 1 + > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 2 +- > OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++++ > OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c | 76 > ++++++++++++++++++++ > 11 files changed, 125 insertions(+), 1 deletion(-) Right, skipping Bhyve is justified, per your previous report / <https://bugzilla.tianocore.org/show_bug.cgi?id=3354>. > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc > index cdb29d53142d..5a5246c64bf7 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.dsc > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc > @@ -627,6 +627,7 @@ [Components] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > <LibraryClasses> (5) Functionally correct, but it reads more nicely (from a logical dependency POV) if we place the new PEIM first. (Please apply to the rest of the DSC files, and the FDF files too.) > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 1730b6558b5c..a33c14c673a0 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -707,6 +707,7 @@ [Components] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > <LibraryClasses> > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 78a559da0d0b..a4ff7ed44705 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -720,6 +720,7 @@ [Components.IA32] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > <LibraryClasses> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index a7d747f6b4ab..3fb56b3f9ff9 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -719,6 +719,7 @@ [Components] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > <LibraryClasses> > diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf > index c0098502aa90..ab58a9c0b4da 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.fdf > +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf > @@ -148,6 +148,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index f400c845b9c9..fc0ae1f280df 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -163,6 +163,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index d055552fd09f..306fc5a9b60d 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -163,6 +163,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index d519f8532822..22c8664427d6 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -175,6 +175,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > index 6776ec931ce0..39d1deeed16b 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -57,7 +57,7 @@ [Pcd] > gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## > PRODUCES > > [Depex.IA32, Depex.X64] > - TRUE > + gOvmfTpmMmioAccessiblePpiGuid > > [Depex.ARM, Depex.AARCH64] > gOvmfTpmDiscoveredPpiGuid > diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > new file mode 100644 > index 000000000000..926113b8ffb0 > --- /dev/null > +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > @@ -0,0 +1,40 @@ > +## @file > +# Map TPM MMIO range unencrypted when SEV is active (6) Please add another sentence here: "Install gOvmfTpmMmioAccessiblePpiGuid unconditionally". > +# > +# Copyright (C) 2021, Advanced Micro Devices, Inc. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION = 0x00010005 (7) The latest INF spec version is 1.29: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Draft-Specification https://tianocore-docs.github.io/edk2-InfSpecification/draft/ plus INF_VERSION no longer requires a binary-only (hex-only) format. So please just write "1.29". > + BASE_NAME = TpmMmioSevDecryptPei > + FILE_GUID = F12F698A-E506-4A1B-B32E-6920E55DA1C4 > + MODULE_TYPE = PEIM > + VERSION_STRING = 1.0 > + ENTRY_POINT = TpmMmioSevDecryptPeimEntryPoint > + > +[Sources] > + TpmMmioSevDecryptPeim.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + SecurityPkg/SecurityPkg.dec (8) Is MdeModulePkg necessary? > + > +[LibraryClasses] > + BaseLib > + DebugLib > + MemEncryptSevLib > + PeimEntryPoint > + PeiServicesLib > + > +[Ppis] > + gOvmfTpmMmioAccessiblePpiGuid ## PRODUCES > + > +[FixedPcd] > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## CONSUMES > + > +[Depex] > + gEfiPeiMemoryDiscoveredPpiGuid > diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c > b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c > new file mode 100644 > index 000000000000..dd1f1a80b5b0 > --- /dev/null > +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c > @@ -0,0 +1,76 @@ > +/** @file > + Map TPM MMIO range unencrypted when SEV is active (9) Same request as (6) -- please add another sentence: "Install gOvmfTpmMmioAccessiblePpiGuid unconditionally". > + > + Copyright (C) 2021, Advanced Micro Devices, Inc. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > + > +#include <PiPei.h> > + > +#include <Library/DebugLib.h> > +#include <Library/MemEncryptSevLib.h> > +#include <Library/PeiServicesLib.h> (10) This Library #include list does not match the [LibraryClasses] section of the INF file (the PeimEntryPoint class apart, which should indeed only be in the INF file). In other words, BaseLib appears superfluous in the INF file. > + > +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmMmioRangeAccessible = { > + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gOvmfTpmMmioAccessiblePpiGuid, > + NULL > +}; > + > +/** > + The entry point for TPM MMIO range mapping driver. > + > + @param[in] FileHandle Handle of the file being invoked. > + @param[in] PeiServices Describes the list of possible PEI Services. > + > + @retval EFI_ABORTED No need to keep this PEIM resident > +**/ > +EFI_STATUS > +EFIAPI > +TpmMmioSevDecryptPeimEntryPoint ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + RETURN_STATUS DecryptStatus; > + EFI_STATUS Status; > + > + DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__)); > + > + // > + // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical > + // address because the nested page fault (NPF) that occurs on access does > not > + // include the encryption bit in the guest physical address provided to the > + // hypervisor. > + // > + // However, if SEV-ES is active, before performing the actual MMIO, an > + // additional MMIO mitigation check is performed in the #VC handler to > ensure > + // that MMIO is being done to an unencrypted address. To prevent guest > + // termination in this scenario, mark the range unencrypted ahead of > access. > + // Lovely comment, thanks! > + if (MemEncryptSevEsIsEnabled ()) { > + DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", > __FUNCTION__)); > + > + DecryptStatus = MemEncryptSevClearPageEncMask ( > + 0, > + PcdGet64 (PcdTpmBaseAddress), (11) The INF file says [FixedPcd], so it would be cleanest to say FixedPcdGet64() here. (12) PcdLib is missing from both the [LibraryClasses] section and the #include directives. > + EFI_SIZE_TO_PAGES ((UINTN) 0x5000), > + FALSE > + ); > + > + if (RETURN_ERROR (DecryptStatus)) { > + DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range > unencrypted\n", __FUNCTION__)); (13) Overlong line. (14) Please report errors with DEBUG_ERROR. > + ASSERT_RETURN_ERROR (DecryptStatus); > + } > + } > + > + // > + // MMIO range available > + // > + Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible); > + ASSERT_EFI_ERROR (Status); > + > + return EFI_ABORTED; > +} > Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74572): https://edk2.groups.io/g/devel/message/74572 Mute This Topic: https://groups.io/mt/82407869/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
