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

["Marc-André Lureau" <mlureau@redhat.com>]

Hi

On Wed, Feb 26, 2020 at 11:46 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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.

I agree, I'll drop it for now. It can be added later.

Thanks

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