Re: [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
review#2 from scratch:
On 04/21/21 00:54, Tom Lendacky wrote:
From: Tom Lendacky <firstname.lastname@example.org>(1) As discussed, please update the commit message, for more clarify
about SEV vs. SEV-ES.
It's OK to keep this as a sanity check, yes.
+ RETURN_STATUS DecryptStatus;(2) Should be (UINTN)0x5000, as discussed earlier.
+ FALSE(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:
- no new PPI definition needed,
- no new PPI installation needed,
- OvmfPkg/Bhyve/PlatformPei needs no separate change
- total abuse of gEfiPeiMemoryDiscoveredPpiGuid
- 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
- 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.
- 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