Re: [edk2-devel] [PATCH v3 6/9] ArmVirtPkg/ArmVirtQemu: enable TPM2 support in the PEI phase

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

On 02/26/20 20:05, Ard Biesheuvel wrote:
> Incorporate the PEI components and the associated library class
> resolutions and PCD declarations to enable TPM2 support in the
> PEI phase.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc | 20 ++++++++++++++++++++
>  ArmVirtPkg/ArmVirtQemu.fdf |  2 ++
>  2 files changed, 22 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index d1757cdba671..8950116dacab 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -75,11 +75,17 @@ [LibraryClasses.common]
>    PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>    PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>
> +!if $(TPM2_ENABLE) == TRUE
> +  Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
> +!endif
> +
>  [LibraryClasses.common.PEIM]
>    ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
>
>  !if $(TPM2_ENABLE) == TRUE
> +  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>    ResetSystemLib|MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
> +  Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>  !endif
>
>  [LibraryClasses.common.DXE_DRIVER]
> @@ -248,6 +254,10 @@ [PcdsDynamicDefault.common]
>    # TPM2 support
>    #
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
> +!if $(TPM2_ENABLE) == TRUE
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|0
> +!endif
>
>  [PcdsDynamicHii]
>    gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
> @@ -278,6 +288,16 @@ [Components.common]
>      <LibraryClasses>
>        ResetSystemLib|ArmVirtPkg/Library/ArmVirtPsciResetSystemPeiLib/ArmVirtPsciResetSystemPeiLib.inf
>    }
> +  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> +    <LibraryClasses>
> +      HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> +      NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> +  }
>  !endif
>
>    MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> index f55918d26b06..b5e2253295fe 100644
> --- a/ArmVirtPkg/ArmVirtQemu.fdf
> +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> @@ -115,6 +115,8 @@ [FV.FVMAIN_COMPACT]
>
>  !if $(TPM2_ENABLE) == TRUE
>    INF MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
> +  INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
>
>    FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>

I'm OK with this patch; I have two requests:

(1) Please add the following to the commit message:

-----
This patch ports (parts of) the following OvmfPkg commits to ArmVirtQemu:
- 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
                2018-03-09)
- 4672a4892867 ("OvmfPkg: include Tcg2Pei module", 2018-03-09)
- b9130c866dc0 ("OvmfPkg: link Sha384 and Sha512 support into Tcg2Pei
                and Tcg2Dxe", 2018-08-16)
- 5d3ef15da7c3 ("OvmfPkg: link SM3 support into Tcg2Pei and Tcg2Dxe",
                2019-07-19)
-----

(2) I don't understand why PcdTpm2HashMask is necessary, especially that
it diverges from the "SecurityPkg.dec" default. Please either drop the
PCD, or (more likely) add a sentence to the commit message explaining
why we're setting it to zero (unlike in OVMF).

With those addressed:

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

Thanks!
Laszlo