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

[Laszlo Ersek <lersek@redhat.com>]

 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>".

[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
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.

> ---
>  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.

>
> 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?

Thanks,
Laszlo