On 06/07/21 15:37, Brijesh Singh wrote:
Also, I was trying to avoid the cases where the malicious hypervisorIf there is *feasible* security problem (attack), then my "information
flow purity" criteria are irrelevant. Security trumps all.
My understanding is though (per your explanation above) that there is no
real security problem here.
Furthermore, we're not going to query the feature set in every phase.
We're going to set the PCDs in the PEI phase; there shouldn't be
hardware querying in the DXE phase. That's quite standard business in OVMF.
As far as I remember, a circular dependency is only a problem if bothInstead, we should have a new MemEncryptSevLib function that outputs the FEATURES bitmask. It should be similar to MemEncryptSevGetEncryptionMask(), but it should return a RETURN_STATUS, and produce the FEATURES bitmask through an output parameter.This is one of thing which I noticed last week, we are actually creating
library instances in question have constructors. If a library instance
needs no construction (has no constructor), then it does not partake in
the topological sorting (for initialization) performed by BaseTools.
In this case, at the end of your RFCv3 series (minus patch#21), no INF
file in either "OvmfPkg/Library/BaseMemEncryptSevLib" or
"OvmfPkg/Library/VmgExitLib" seems to specify a CONSTRUCTOR, so purely
from the build perspective, there should be no issue.
But, I have another idea here:
the above basically tells us that this library API would be a single-useThe SEC instance of the function should return RETURN_UNSUPPORTED.
interface. It wouldn't work in SEC, it would do actual work in PEI
(namely, in PlatformPei), and it wouldn't do any "real work" in DXE.
To me, the boundary is not crystal clear, but the above struggles
*suggest* that we might not want this API to be a MemEncryptSevLib
function (or any library function) at all. If abstracting out the API
causes more pain than it does good, then let's just not abstract it.
Meaning, you could open-code the fetching of features (using VmgExitLib)
in PlatormPei, set the PCD, and be done with it. The only place where
the PCD (PcdGhcbHypervisorFeatures) is actually used (as far as I can
see) is patch#21, in UefiCpuPkg. We could reasonably say that the "real
API", namely the declaration of the PCD, *already exists*, namely in the
"UefiCpuPkg/UefiCpuPkg.dec" file, from your commit e6994b1d5226
("UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs", 2021-06-04).
There are other examples where a cross-package PCD is set once and for
all in OvmfPkg/PlatformPei (random example:
"PcdCpuBootLogicalProcessorNumber"). We don't have to turn everything
into a lib class API, especially if it causes more pain than help.
... But maybe I just need to accept that we have to repurpose
SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts.
Same as the PEI phase is considered the "HOB producer phase", outputting
a bunch of disparate bits of info, we could consider the SEV-ES parts of
the Reset Vector such an "early info bits" producer phase. I think this
is a very big conceptual step away from the original purpose of
SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"!
HOBs are not "work areas", they are effectively read-only, once
produced). But perhaps this is what we need (and then with proper
NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types
are specified in PI, and GUIDs are expressly declared to stand for
various purposes at least in edk2 DEC files. All that helps with
discerning the information flow. So... I'd still prefer keeping
SEC_SEV_ES_WORK_AREA as minimal as possible.
Tom, any comments?
Thank you Brijesh for raising great points!