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.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%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 <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: James Bottomley <j...@linux.ibm.com> >>> Cc: Jiewen Yao <jiewen....@intel.com> >>> Cc: Min Xu <min.m...@intel.com> >>> Signed-off-by: Tom Lendacky <thomas.lenda...@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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74396): https://edk2.groups.io/g/devel/message/74396 Mute This Topic: https://groups.io/mt/82247968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
