Hi Gary, On 07/03/19 12:22, Gary Lin wrote: > DxeTpmMeasurementLib is only useful when TPM is enabled. > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Marc-André Lureau <marcandre.lur...@redhat.com> > Cc: Stefan Berger <stef...@linux.ibm.com> > Signed-off-by: Gary Lin <g...@suse.com> > --- > OvmfPkg/OvmfPkgIa32.dsc | 10 +++++++--- > OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++++--- > OvmfPkg/OvmfPkgX64.dsc | 10 +++++++--- > 3 files changed, 21 insertions(+), 9 deletions(-)
This is a good patch, thank you for it. I see two opportunities for improvement. (1) There's something weird going on with your newline characters. The view I get (in both my INBOX and in my list folder) is identical to mail-archive.com's view: 20190703102228.25441-1-glin@suse.com">http://mid.mail-archive.com/20190703102228.25441-1-glin@suse.com Can you double check your settings, please? (2) The commit message should be more convincing. How about this: ----------- (a) OvmfPkg first had to resolve the TpmMeasurementLib class -- for SECURE_BOOT_ENABLE only -- when the DxeImageVerificationLib instance became dependent on TpmMeasurementLib. For details, refer to commit 0d28d286bf4d ("OvmfPkg: resolve TpmMeasurementLib dependency introduced in r14687", 2013-09-21). (b) At the time, only one instance of TpmMeasurementLib existed, namely DxeTpmMeasurementLib. This lib instance didn't do anything -- like it was desirable for OVMF --, because OVMF didn't include any Tcg / TrEE protocol implementations. (c) In commit 308521b13354 ("MdeModulePkg: Move TpmMeasurementLib LibraryClass from SecurityPkg", 2015-07-01), TpmMeasurementLibNull was introduced. (d) In commit 285542ebbb03 ("OvmfPkg: Link AuthVariableLib for following merged variable driver deploy", 2015-07-01), a TpmMeasurementLib resolution became necessary regardless of SECURE_BOOT_ENABLE. And so TpmMeasurementLib was resolved to TpmMeasurementLibNull in OVMF, but only in the non-SECURE_BOOT_ENABLE case. This step -- possibly, the larger series containing commit 285542ebbb03 -- missed an opportunity for simplification: given (b), the DxeTpmMeasurementLib instance should have been simply replaced with the TpmMeasurementLibNull instance, regardless of SECURE_BOOT_ENABLE. (e) In commit 1abfa4ce4835 ("Add TPM2 support defined in trusted computing group.", 2015-08-13), the TrEE dependency was replaced with a Tcg2 dependency in DxeTpmMeasurementLib. (f) Starting with commit 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09), OVMF would include a Tcg2 protocol implementation, thereby satisfying DxeTpmMeasurementLib's dependency. With TPM2_ENABLE, it would actually make sense to consume DxeTpmMeasurementLib -- however, DxeTpmMeasurementLib would never be used without SECURE_BOOT_ENABLE. Therefore, we have the following four scenarios: - TPM2_ENABLE + SECURE_BOOT_ENABLE: works as expected. - Neither enabled: works as expected. - Only TPM2_ENABLE: this build is currently incorrect, because Variable/RuntimeDxe consumes TpmMeasurementLib directly, but TpmMeasureAndLogData() will never reach the TPM because we link TpmMeasurementLibNull into the variable driver. This is a problem from the larger series containing (f). - Only SECURE_BOOT_ENABLE: this build works as expected, but it is wasteful -- given that the protocol database will never contain Tcg2 without TPM2_ENABLE, we should simply use TpmMeasurementLibNull. This is a problem from (d). Resolving TpmMeasurementLib to DxeTpmMeasurementLib as a function of *only* TPM2_ENABLE, we can fix / optimize the last two cases. ----------- To reflect the reasoning in the subject line, I suggest: OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE If you agree, please submit a v2 like this. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43226): https://edk2.groups.io/g/devel/message/43226 Mute This Topic: https://groups.io/mt/32295955/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
