Re: [PATCH v3 6/6] OvmfPkg: add TCG Configuration menu to the Device Manager menu

["Laszlo Ersek" <lersek@redhat.com>]

Hi,

On 02/26/20 10:34, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Matches TPM 2.0 commit 3103389043bd ("OvmfPkg: Add TCG2 Configuration
> menu to the Device Manager menu", 2019-02-11).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 6 ++++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>  OvmfPkg/OvmfPkgX64.dsc     | 6 ++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 2fc10d2393e3..02300886563e 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -936,4 +936,10 @@
>      <LibraryClasses>
>        Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>    }
> +!if $(TPM_CONFIG_ENABLE) == TRUE
> +  SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDxe.inf {
> +    <LibraryClasses>
> +      PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> +  }
> +!endif
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index cd9d2ac724ca..3adc75223d05 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -950,4 +950,10 @@
>      <LibraryClasses>
>        Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>    }
> +!if $(TPM_CONFIG_ENABLE) == TRUE
> +  SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDxe.inf {
> +    <LibraryClasses>
> +      PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> +  }
> +!endif
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 317a23b994b8..5f3740ae890a 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -948,4 +948,10 @@
>      <LibraryClasses>
>        Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>    }
> +!if $(TPM_CONFIG_ENABLE) == TRUE
> +  SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDxe.inf {
> +    <LibraryClasses>
> +      PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> +  }
> +!endif
>  !endif
> 

I've got two comments on this:

(1) I'm unsure why we need to explicitly specify the PcdLib class
resolution here. The module in question is of type DXE_DRIVER, and we
already have the following in the DSC files:

[LibraryClasses.common.DXE_DRIVER]
  PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

So I'd think the bracketed { <LibraryClasses> ... }  parts should be
possible to omit.

(2) The driver is not being added to the FDF files, so they will be
complied, but not included in the firmware executable.

TBH I'd suggest simply dropping this patch.

If Stefan insists on including this patch in the series, then please fix
(1) and (2) above. Like that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo