Re: [Qemu-devel] [edk2] [PATCH 1/4] ovmf: add and link with Tcg2PhysicalPresenceLibNull when !TPM2_ENABLE

[Laszlo Ersek]

On 05/15/18 14:30, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> This NULL library will let us call
> Tcg2PhysicalPresenceLibProcessRequest() unconditionally from
> BdsPlatform when building without TPM2_ENABLE.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  .../DxeTcg2PhysicalPresenceLib.c              | 26 ++++++++++++++
>  .../DxeTcg2PhysicalPresenceLib.inf            | 34 +++++++++++++++++++
>  OvmfPkg/OvmfPkgIa32.dsc                       |  2 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |  2 ++
>  OvmfPkg/OvmfPkgX64.dsc                        |  2 ++
>  5 files changed, 66 insertions(+)
>  create mode 100644 
> OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.c
>  create mode 100644 
> OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> 
> diff --git 
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.c 
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.c
> new file mode 100644
> index 000000000000..0b8b98410315
> --- /dev/null
> +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.c
> @@ -0,0 +1,26 @@
> +/** @file
> +  NULL Tcg2PhysicalPresenceLib library instance
> +
> +  Copyright (c) 2018, Red Hat, Inc.
> +  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD 
> License
> +  which accompanies this distribution.  The full text of the license may be 
> found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#include "PiDxe.h"

(1) Can you drop this #include?

> +#include <Library/Tcg2PhysicalPresenceLib.h>
> +
> +VOID
> +EFIAPI
> +Tcg2PhysicalPresenceLibProcessRequest (
> +  IN      TPM2B_AUTH                     *PlatformAuth  OPTIONAL
> +  )
> +{
> +    return;
> +}

(2) Indentation.

Better yet: please replace the "return" statement with a comment:

  //
  // do nothing
  //

> diff --git 
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf 
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> new file mode 100644
> index 000000000000..e6f6239e1e00
> --- /dev/null
> +++ 
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +#  NULL Tcg2PhysicalPresenceLib library instance
> +#
> +#  In SecurityPkg, this library will check and execute TPM 1.2 request
> +#  from OS or BIOS. The request may ask for user confirmation before
> +#  execution. This Library will also lock TPM physical presence at
> +#  last.

(3) The approach on this comment is generally OK, but the specific text
originates from
"SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.inf".
I think we should update the comment from the TPM2 variant, namely
"SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf".

Thus, I suggest the following comment:

"Under SecurityPkg, the corresponding library instance will check and
execute TPM 2.0 request from OS or BIOS; the request may ask for user
confirmation before execution. This Null instance implements a no-op
Tcg2PhysicalPresenceLibProcessRequest(), without user interaction."

> +#
> +# Copyright (C) 2018, Red Hat, Inc.
> +# Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.<BR>

(4) Same comment applies to the Intel copyright notice: from the TCG2
variant, this should come as

"Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>"

> +# This program and the accompanying materials
> +# are licensed and made available under the terms and conditions of the BSD 
> License
> +# which accompanies this distribution. The full text of the license may be 
> found at
> +# http://opensource.org/licenses/bsd-license.php
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = DxeTcg2PhysicalPresenceLibNull
> +  FILE_GUID                      = 2A6BA243-DC22-42D8-9C3D-AE3728DC7AFA
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = Tcg2PhysicalPresenceLib|DXE_DRIVER 
> DXE_RUNTIME_DRIVER DXE_SAL_DRIVER UEFI_APPLICATION UEFI_DRIVER
> +
> +[Sources]
> +  DxeTcg2PhysicalPresenceLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec

(5) I think you can drop "MdeModulePkg/MdeModulePkg.dec". (MdePkg.dec is
needed by all modules, and SecurityPkg.dec below is needed for the lib
class header; so those are OK).

> +  SecurityPkg/SecurityPkg.dec
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 2d6c4c4615b6..6c361b73cd55 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -208,6 +208,8 @@ [LibraryClasses]
>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>    
> Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
>    
> Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> +!else
> +  
> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>  !endif
>  
>  [LibraryClasses.common]
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 43158c5f0627..62a6075a671d 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -213,6 +213,8 @@ [LibraryClasses]
>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>    
> Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
>    
> Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> +!else
> +  
> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>  !endif
>  
>  [LibraryClasses.common]
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index d1fdf7c307c2..cbab1aa328c6 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -213,6 +213,8 @@ [LibraryClasses]
>    Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
>    
> Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
>    
> Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
> +!else
> +  
> Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
>  !endif
>  
>  [LibraryClasses.common]
> 

Thanks!
Laszlo