Re: [PATCH 2/3] OvmfPkg/Tcg2ConfigPei: factor out InternalTpm12Detect()

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/21/20 12:58 AM, Laszlo Ersek wrote:
> Move the calls to the Tpm12RequestUseTpm() and Tpm12SubmitCommand()
> Tpm12DeviceLib functions to a separate C file, so that we can override
> these actions in a subsequent patch.
> 
> This code movement requires moving the TPM_RSP_GET_TICKS / TestTpm12()
> helper structure / function too.
> 
> While at it, give the TestTpm12() function @retval / @return
> documentation, plus wrap an overlong line in it.
> 
> Functionally, this patch is a no-op.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Simon Hardy <simon.hardy@itdev.co.uk>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2728
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf |  2 +
>   OvmfPkg/Tcg/Tcg2Config/Tpm12Support.h    | 30 ++++++++
>   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c  | 46 +-----------
>   OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c    | 79 ++++++++++++++++++++
>   4 files changed, 115 insertions(+), 42 deletions(-)
> 
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index b79d0a3fb912..aa996b7da778 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -21,6 +21,8 @@ [Defines]
>   
>   [Sources]
>     Tcg2ConfigPeim.c
> +  Tpm12Support.c
> +  Tpm12Support.h
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.h b/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.h
> new file mode 100644
> index 000000000000..c739775d2353
> --- /dev/null
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.h
> @@ -0,0 +1,30 @@
> +/** @file
> +  Declare the InternalTpm12Detect() function, hiding the TPM-1.2 detection
> +  internals.
> +
> +  Copyright (C) 2020, Red Hat, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef TPM12_SUPPORT_H_
> +#define TPM12_SUPPORT_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +/**
> +  Detect the presence of a TPM with interface version 1.2.
> +
> +  @retval EFI_SUCCESS      TPM-1.2 available. The Tpm12RequestUseTpm() and
> +                           Tpm12SubmitCommand(TPM_ORD_GetTicks) operations
> +                           (from the Tpm12DeviceLib class) have succeeded.
> +
> +  @return                  Error codes propagated from Tpm12RequestUseTpm() and
> +                           Tpm12SubmitCommand().
> +**/
> +EFI_STATUS
> +InternalTpm12Detect (
> +  VOID
> +  );
> +
> +#endif // TPM12_SUPPORT_H_
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
> index 44abd6c541f9..cc54d95cad19 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
> @@ -15,13 +15,13 @@
>   #include <PiPei.h>
>   
>   #include <Guid/TpmInstance.h>
> -#include <Library/BaseLib.h>
>   #include <Library/DebugLib.h>
>   #include <Library/PeiServicesLib.h>
>   #include <Library/Tpm2DeviceLib.h>
> -#include <Library/Tpm12DeviceLib.h>
>   #include <Ppi/TpmInitialized.h>
>   
> +#include "Tpm12Support.h"
> +
>   STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmSelectedPpi = {
>     (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>     &gEfiTpmDeviceSelectedGuid,
> @@ -34,44 +34,6 @@ STATIC CONST EFI_PEI_PPI_DESCRIPTOR  mTpmInitializationDonePpiList = {
>     NULL
>   };
>   
> -#pragma pack (1)
> -
> -typedef struct {
> -  TPM_RSP_COMMAND_HDR   Hdr;
> -  TPM_CURRENT_TICKS     CurrentTicks;
> -} TPM_RSP_GET_TICKS;
> -
> -#pragma pack ()
> -
> -/**
> -  Probe for the TPM for 1.2 version, by sending TPM1.2 GetTicks
> -
> -  Sending a TPM1.2 command to a TPM2 should return a TPM1.2
> -  header (tag = 0xc4) and error code (TPM_BADTAG = 0x1e)
> -**/
> -static
> -EFI_STATUS
> -TestTpm12 (
> -  )
> -{
> -  EFI_STATUS           Status;
> -  TPM_RQU_COMMAND_HDR  Command;
> -  TPM_RSP_GET_TICKS    Response;
> -  UINT32               Length;
> -
> -  Command.tag       = SwapBytes16 (TPM_TAG_RQU_COMMAND);
> -  Command.paramSize = SwapBytes32 (sizeof (Command));
> -  Command.ordinal   = SwapBytes32 (TPM_ORD_GetTicks);
> -
> -  Length = sizeof (Response);
> -  Status = Tpm12SubmitCommand (sizeof (Command), (UINT8 *)&Command, &Length, (UINT8 *)&Response);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  return EFI_SUCCESS;
> -}
> -
>   /**
>     The entry point for Tcg2 configuration driver.
>   
> @@ -90,8 +52,8 @@ Tcg2ConfigPeimEntryPoint (
>   
>     DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__));
>   
> -  Status = Tpm12RequestUseTpm ();
> -  if (!EFI_ERROR (Status) && !EFI_ERROR (TestTpm12 ())) {
> +  Status = InternalTpm12Detect ();
> +  if (!EFI_ERROR (Status)) {
>       DEBUG ((DEBUG_INFO, "%a: TPM1.2 detected\n", __FUNCTION__));
>       Size = sizeof (gEfiTpmDeviceInstanceTpm12Guid);
>       Status = PcdSetPtrS (
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c b/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c
> new file mode 100644
> index 000000000000..4f5a775c7a03
> --- /dev/null
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tpm12Support.c
> @@ -0,0 +1,79 @@
> +/** @file
> +  Implement the InternalTpm12Detect() function on top of the Tpm12DeviceLib
> +  class.
> +
> +  Copyright (C) 2020, Red Hat, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/Tpm12DeviceLib.h>
> +
> +#include "Tpm12Support.h"
> +
> +#pragma pack (1)
> +typedef struct {
> +  TPM_RSP_COMMAND_HDR   Hdr;
> +  TPM_CURRENT_TICKS     CurrentTicks;
> +} TPM_RSP_GET_TICKS;
> +#pragma pack ()
> +
> +/**
> +  Probe for the TPM for 1.2 version, by sending TPM1.2 GetTicks
> +
> +  Sending a TPM1.2 command to a TPM2 should return a TPM1.2
> +  header (tag = 0xc4) and error code (TPM_BADTAG = 0x1e)
> +
> +  @retval EFI_SUCCESS  TPM version 1.2 probing successful.
> +
> +  @return              Error codes propagated from Tpm12SubmitCommand().
> +**/
> +STATIC
> +EFI_STATUS
> +TestTpm12 (
> +  )
> +{
> +  EFI_STATUS           Status;
> +  TPM_RQU_COMMAND_HDR  Command;
> +  TPM_RSP_GET_TICKS    Response;
> +  UINT32               Length;
> +
> +  Command.tag       = SwapBytes16 (TPM_TAG_RQU_COMMAND);
> +  Command.paramSize = SwapBytes32 (sizeof (Command));
> +  Command.ordinal   = SwapBytes32 (TPM_ORD_GetTicks);
> +
> +  Length = sizeof (Response);
> +  Status = Tpm12SubmitCommand (sizeof (Command), (UINT8 *)&Command, &Length,
> +             (UINT8 *)&Response);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Detect the presence of a TPM with interface version 1.2.
> +
> +  @retval EFI_SUCCESS      TPM-1.2 available. The Tpm12RequestUseTpm() and
> +                           Tpm12SubmitCommand(TPM_ORD_GetTicks) operations
> +                           (from the Tpm12DeviceLib class) have succeeded.
> +
> +  @return                  Error codes propagated from Tpm12RequestUseTpm() and
> +                           Tpm12SubmitCommand().
> +**/
> +EFI_STATUS
> +InternalTpm12Detect (
> +  VOID
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  Status = Tpm12RequestUseTpm ();
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  return TestTpm12 ();
> +}
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>