Re: [PATCH] OvmfPkg: Add TCG2 Configuration menu to the Device Manager menu

[Stefan Berger <stefanb@linux.ibm.com>]

On 1/25/19 2:30 PM, Laszlo Ersek wrote:
>   Hi,
>
> (1) when posting a patch, please CC all people marked with "M" for the
> subject package in Maintainers.txt. And, if in doubt, please CC the "R"
> people too.
>
> Adding the others now.
>
> (2) In [a] [b], you mentioned a need to repost, due to your email
> address having changed. However, that posting, and this posting, seem to
> come from the same email address, and to use the same S-o-b: "Stefan
> Berger <stefanb@linux.ibm.com>".

I re-subscribed to the ml. Old address was stefanb@linux.vnet.ibm.com.


>
> [a] http://mid.mail-archive.com/d0222c94-4578-74bd-94ad-90d10911b534@linux.ibm.com
> [b] https://lists.01.org/pipermail/edk2-devel/2019-January/035701.html
>
> On 01/25/19 18:12, Stefan Berger wrote:
>> This patch adds the TCG2 Configuration menu to the Device Manager
>> menu.
> OK.
>
>> We can reuse all the code in the SecurityPkg and need to
>> only add it to the build and flash config files.
> (3) I disagree with this formulation.
>
> Instead, we can apparently include the *sample* Tcg2ConfigDxe from
> SecurityPkg as-is, without obvious adverse effects. I consider this an


I will reuse some of this formulation.


> experimental feature that's being enabled (not by default) in order to
> support further R&D. The Tcg2ConfigDxe has not been trimmed / modified
> for OVMF / QEMU specifics (unlike Tcg2ConfigPei in commit 6cf1880fb5b6).
> That's why we are gating Tcg2ConfigDxe with TPM2_CONFIG_ENABLE, and not
> enabling it simply as part of TPM2_ENABLE.
>
>> The code is added to Ovfm by building with -DTPM2_ENABLE and
>> -DTPM2_CONFIG_ENABLE.
> (4) Typo, s/Ovfm/OVMF/. "OVMF" stands for Open Virtual Machine Firmware.


:-)


>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> (5) Before your S-o-b, please add:
>
> """
> Contributed-under: TianoCore Contribution Agreement 1.1
> """
>
> after reviewing "Contributions.txt" in the project root dir.


Done.


>
>> ---
>>   OvmfPkg/OvmfPkgIa32.dsc    | 3 +++
>>   OvmfPkg/OvmfPkgIa32.fdf    | 4 ++++
>>   OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++
>>   OvmfPkg/OvmfPkgIa32X64.fdf | 4 ++++
>>   OvmfPkg/OvmfPkgX64.dsc     | 3 +++
>>   OvmfPkg/OvmfPkgX64.fdf     | 4 ++++
>>   6 files changed, 21 insertions(+)
> (6) In all three DSC files, please DEFINE TPM2_CONFIG_ENABLE = FALSE as
> well, just below TPM2_ENABLE.


Done.


>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index aee19b75d7..68e707e7e5 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -632,6 +632,9 @@
>>         NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>>         NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
>>     }
>> +!if $(TPM2_CONFIG_ENABLE) == TRUE
>> +  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>> +!endif
>>   !endif
>>
>>     #
> This looks good.
>
>> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
>> index e013099136..a46598f2c3 100644
>> --- a/OvmfPkg/OvmfPkgIa32.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32.fdf
>> @@ -295,6 +295,10 @@ INF  ShellPkg/Application/Shell/Shell.inf
>>
>>   INF MdeModulePkg/Logo/LogoDxe.inf
>>
>> +!if ($(TPM2_ENABLE) == TRUE) && ($(TPM2_CONFIG_ENABLE) == TRUE)
>> +INF  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>> +!endif
>> +
> (7) In this file, we already have
>
> """
> !if $(TPM2_ENABLE) == TRUE
> INF  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
> !endif
> """
>
> elsewhere. Can you please nest the addition in that scope?

Done.


>
> Thanks,
> Laszlo
>