On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote: > I'm going to ask for v3 after all: > > On 04/27/21 18:21, Lendacky, Thomas wrote: >> From: Tom Lendacky <thomas.lenda...@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%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=G1GwQc6sZqRuNHWC5vbdb78gCOl4YkAq%2BHi0F0ceucg%3D&reserved=0 >> >> During PEI, the MMIO range for the TPM is marked as encrypted when running >> as an SEV guest. While this isn't an issue for an SEV guest because of >> the way the nested page fault is handled, it does result in an SEV-ES >> guest terminating because of a mitigation check in the #VC handler to >> prevent MMIO to an encrypted address. For an SEV-ES guest, this range >> must be marked as unencrypted. >> >> Create a new x86 PEIM for TPM support that will map the TPM MMIO range as >> unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI >> will be unconditionally installed before exiting. The PEIM will exit with >> the EFI_ABORTED status so that the PEIM does not stay resident. > > (1) Please spell out that the new PEIM will depend on the installation > of the permanent PEI RAM, by PlatformPei -- the reason being that, in > case page table splitting proves necessary for clearing the C-bit, the > new page table(s) should be allocated from permanent PEI RAM.
Will do. > > >> >> The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a >> Depex for IA32 and X64 builds so that the MMIO range is properly mapped >> for SEV-ES before the Tcg2Config PEIM is loaded. > > (2) The Tcg2Config depex change should be a separate patch -- the last > patch in the series. That covers both the Tcg2Config hunk in the patch > body, and this commit message paragraph right here. Ok, I'll split that out. > > > (3) While at it: those parts of this patch that are *not* related to the > Tcg2Config PEIM can be squashed into the previous patch -- if you prefer > that. > > I'm certainly happy with three separate patches though: for the DEC > file, for TpmMmioSevDecryptPei + the DSC/FDF files, and finally the > Tcg2Config PEIM. So in total the series should include 4 or 5 patches > (up to you). I'll do it as 5 patches. > > >> >> Update all OVMF Ia32 and X64 build packages to include this new PEIM. >> >> Cc: Laszlo Ersek <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. We were having problems with sending patches via git-format-patch and git-send-email and our email system. Likely some left over .gitconfig entries that are causing the problem. I'll double check everything. > > >> Cc: Stefan Berger <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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3354&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HPF7LTIV17CwZ%2BCFehXpnPb%2BtQCgpFPvLsVqfNj9HBI%3D&reserved=0>. > >> >> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc >> index cdb29d53142d..5a5246c64bf7 100644 >> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc >> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc >> @@ -627,6 +627,7 @@ [Components] >> >> !if $(TPM_ENABLE) == TRUE >> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> SecurityPkg/Tcg/TcgPei/TcgPei.inf >> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { >> <LibraryClasses> > > (5) Functionally correct, but it reads more nicely (from a logical > dependency POV) if we place the new PEIM first. > > (Please apply to the rest of the DSC files, and the FDF files too.) Ok, I was going with the alphabetical placement. I'll switch it up. > > >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index 1730b6558b5c..a33c14c673a0 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -707,6 +707,7 @@ [Components] >> >> !if $(TPM_ENABLE) == TRUE >> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> SecurityPkg/Tcg/TcgPei/TcgPei.inf >> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { >> <LibraryClasses> >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 78a559da0d0b..a4ff7ed44705 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -720,6 +720,7 @@ [Components.IA32] >> >> !if $(TPM_ENABLE) == TRUE >> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> SecurityPkg/Tcg/TcgPei/TcgPei.inf >> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { >> <LibraryClasses> >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index a7d747f6b4ab..3fb56b3f9ff9 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -719,6 +719,7 @@ [Components] >> >> !if $(TPM_ENABLE) == TRUE >> OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> SecurityPkg/Tcg/TcgPei/TcgPei.inf >> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { >> <LibraryClasses> >> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf >> index c0098502aa90..ab58a9c0b4da 100644 >> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf >> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf >> @@ -148,6 +148,7 @@ [FV.PEIFV] >> >> !if $(TPM_ENABLE) == TRUE >> INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >> !endif >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf >> index f400c845b9c9..fc0ae1f280df 100644 >> --- a/OvmfPkg/OvmfPkgIa32.fdf >> +++ b/OvmfPkg/OvmfPkgIa32.fdf >> @@ -163,6 +163,7 @@ [FV.PEIFV] >> >> !if $(TPM_ENABLE) == TRUE >> INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >> !endif >> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf >> index d055552fd09f..306fc5a9b60d 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.fdf >> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf >> @@ -163,6 +163,7 @@ [FV.PEIFV] >> >> !if $(TPM_ENABLE) == TRUE >> INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >> !endif >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >> index d519f8532822..22c8664427d6 100644 >> --- a/OvmfPkg/OvmfPkgX64.fdf >> +++ b/OvmfPkg/OvmfPkgX64.fdf >> @@ -175,6 +175,7 @@ [FV.PEIFV] >> >> !if $(TPM_ENABLE) == TRUE >> INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >> !endif >> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> index 6776ec931ce0..39d1deeed16b 100644 >> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> @@ -57,7 +57,7 @@ [Pcd] >> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## >> PRODUCES >> >> [Depex.IA32, Depex.X64] >> - TRUE >> + gOvmfTpmMmioAccessiblePpiGuid >> >> [Depex.ARM, Depex.AARCH64] >> gOvmfTpmDiscoveredPpiGuid >> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> new file mode 100644 >> index 000000000000..926113b8ffb0 >> --- /dev/null >> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> @@ -0,0 +1,40 @@ >> +## @file >> +# Map TPM MMIO range unencrypted when SEV is active > > (6) Please add another sentence here: "Install > gOvmfTpmMmioAccessiblePpiGuid unconditionally". Will do. > > >> +# >> +# Copyright (C) 2021, Advanced Micro Devices, Inc. >> +# >> +# SPDX-License-Identifier: BSD-2-Clause-Patent >> +## >> + >> +[Defines] >> + INF_VERSION = 0x00010005 > > (7) The latest INF spec version is 1.29: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Draft-Specification&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AXuQkvUSwLjEyZiwivQQaUwTaY7Mo0wLSHUf8QKNLC8%3D&reserved=0 > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftianocore-docs.github.io%2Fedk2-InfSpecification%2Fdraft%2F&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YUHs6g5aMPWBjCNjcZPKnTSEs2gBazDX094nqj9qpnE%3D&reserved=0 > > plus INF_VERSION no longer requires a binary-only (hex-only) format. So > please just write "1.29". Will do. > > >> + BASE_NAME = TpmMmioSevDecryptPei >> + FILE_GUID = F12F698A-E506-4A1B-B32E-6920E55DA1C4 >> + MODULE_TYPE = PEIM >> + VERSION_STRING = 1.0 >> + ENTRY_POINT = TpmMmioSevDecryptPeimEntryPoint >> + >> +[Sources] >> + TpmMmioSevDecryptPeim.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + OvmfPkg/OvmfPkg.dec >> + SecurityPkg/SecurityPkg.dec > > (8) Is MdeModulePkg necessary? I don't think so. Let me double check it. > > >> + >> +[LibraryClasses] >> + BaseLib >> + DebugLib >> + MemEncryptSevLib >> + PeimEntryPoint >> + PeiServicesLib >> + >> +[Ppis] >> + gOvmfTpmMmioAccessiblePpiGuid ## PRODUCES >> + >> +[FixedPcd] >> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## CONSUMES >> + >> +[Depex] >> + gEfiPeiMemoryDiscoveredPpiGuid >> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c >> b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c >> new file mode 100644 >> index 000000000000..dd1f1a80b5b0 >> --- /dev/null >> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c >> @@ -0,0 +1,76 @@ >> +/** @file >> + Map TPM MMIO range unencrypted when SEV is active > > (9) Same request as (6) -- please add another sentence: "Install > gOvmfTpmMmioAccessiblePpiGuid unconditionally". > Will do. > >> + >> + Copyright (C) 2021, Advanced Micro Devices, Inc. >> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> +**/ >> + >> + >> +#include <PiPei.h> >> + >> +#include <Library/DebugLib.h> >> +#include <Library/MemEncryptSevLib.h> >> +#include <Library/PeiServicesLib.h> > > (10) This Library #include list does not match the [LibraryClasses] > section of the INF file (the PeimEntryPoint class apart, which should > indeed only be in the INF file). In other words, BaseLib appears > superfluous in the INF file. Ok, let me check on that and fix as appropriate. > > >> + >> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmMmioRangeAccessible = { >> + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, >> + &gOvmfTpmMmioAccessiblePpiGuid, >> + NULL >> +}; >> + >> +/** >> + The entry point for TPM MMIO range mapping driver. >> + >> + @param[in] FileHandle Handle of the file being invoked. >> + @param[in] PeiServices Describes the list of possible PEI Services. >> + >> + @retval EFI_ABORTED No need to keep this PEIM resident >> +**/ >> +EFI_STATUS >> +EFIAPI >> +TpmMmioSevDecryptPeimEntryPoint ( >> + IN EFI_PEI_FILE_HANDLE FileHandle, >> + IN CONST EFI_PEI_SERVICES **PeiServices >> + ) >> +{ >> + RETURN_STATUS DecryptStatus; >> + EFI_STATUS Status; >> + >> + DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__)); >> + >> + // >> + // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical >> + // address because the nested page fault (NPF) that occurs on access does >> not >> + // include the encryption bit in the guest physical address provided to >> the >> + // hypervisor. >> + // >> + // However, if SEV-ES is active, before performing the actual MMIO, an >> + // additional MMIO mitigation check is performed in the #VC handler to >> ensure >> + // that MMIO is being done to an unencrypted address. To prevent guest >> + // termination in this scenario, mark the range unencrypted ahead of >> access. >> + // > > Lovely comment, thanks! > >> + if (MemEncryptSevEsIsEnabled ()) { >> + DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", >> __FUNCTION__)); >> + >> + DecryptStatus = MemEncryptSevClearPageEncMask ( >> + 0, >> + PcdGet64 (PcdTpmBaseAddress), > > (11) The INF file says [FixedPcd], so it would be cleanest to say > FixedPcdGet64() here. Will do. > > > (12) PcdLib is missing from both the [LibraryClasses] section and the > #include directives. Right, I'll update that. > > >> + EFI_SIZE_TO_PAGES ((UINTN) 0x5000), >> + FALSE >> + ); >> + >> + if (RETURN_ERROR (DecryptStatus)) { >> + DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range >> unencrypted\n", __FUNCTION__)); > > (13) Overlong line. Ok, I'll change that. I though that was ok now since PatchCheck.py didn't complain. > > > (14) Please report errors with DEBUG_ERROR. Yup, will change. Thanks, Tom > > >> + ASSERT_RETURN_ERROR (DecryptStatus); >> + } >> + } >> + >> + // >> + // MMIO range available >> + // >> + Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible); >> + ASSERT_EFI_ERROR (Status); >> + >> + return EFI_ABORTED; >> +} >> > > Thanks! > Laszlo > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74576): https://edk2.groups.io/g/devel/message/74576 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] -=-=-=-=-=-=-=-=-=-=-=-
