Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time

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

Hello Zhichao,

On 01/03/20 04:04, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> 
> Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the
> wait time of user response.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../DxeTcg2PhysicalPresenceLib.c              | 63 ++++++++++++++-----
>  .../DxeTcg2PhysicalPresenceLib.inf            |  6 +-
>  2 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
> index 00d76ba2c2..13f78cbfac 100644
> --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
> +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
> @@ -9,7 +9,7 @@
>  
>  Copyright (C) 2018, Red Hat, Inc.
>  Copyright (c) 2018, IBM Corporation. All rights reserved.<BR>
> -Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -32,6 +32,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Library/TimerLib.h>
> +#include <Library/PcdLib.h>
>  
>  #include <Library/Tcg2PhysicalPresenceLib.h>
>  
> @@ -365,28 +367,57 @@ Tcg2ReadUserKey (
>  {
>    EFI_STATUS                        Status;
>    EFI_INPUT_KEY                     Key;
> -  UINT16                            InputKey;
> +  UINT16                            ConfirmKey;
> +  UINTN                             Interval;
> +  INT64                             Timeout;
>  
> -  InputKey = 0;
> +  //
> +  // delay 100 milli-second
> +  //
> +  Interval    = 100;
> +  ConfirmKey  = (CautionKey) ? SCAN_F12 : SCAN_F10;
> +  Timeout     = (INT64)PcdGet32 (PcdPhysicalPresenceUserConfirmTimeout);
> +  if (Timeout > 0) {
> +    Timeout   = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
> +  } else {
> +    //
> +    // Wait forever
> +    //
> +    Timeout   = MAX_INT64;
> +  }
> +
> +  //
> +  // Wait for user response within the time-out
> +  //
>    do {
> +    MicroSecondDelay (Interval * 1000);
> +
>      Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
>      if (!EFI_ERROR (Status)) {
>        Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
> -      if (Key.ScanCode == SCAN_ESC) {
> -        InputKey = Key.ScanCode;
> -      }
> -      if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
> -        InputKey = Key.ScanCode;
> -      }
> -      if ((Key.ScanCode == SCAN_F12) && CautionKey) {
> -        InputKey = Key.ScanCode;
> +      if (!EFI_ERROR (Status)) {
> +        if (Key.ScanCode == ConfirmKey) {
> +          //
> +          // User Confirmation
> +          //
> +          return TRUE;
> +        }
> +
> +        if (Key.ScanCode == SCAN_ESC) {
> +          //
> +          // User Rejection
> +          //
> +          return FALSE;
> +        }
> +      } else if (Status == EFI_DEVICE_ERROR) {
> +        //
> +        // If error, assume User Rejection
> +        //
> +        return FALSE;
>        }
>      }
> -  } while (InputKey == 0);
> -
> -  if (InputKey != SCAN_ESC) {
> -    return TRUE;
> -  }
> +    Timeout -= Interval;
> +  } while (Timeout > 0);
>  
>    return FALSE;
>  }

(1) I don't understand why the original (pre-patch) code uses
CheckEvent() in a busy loop. WaitForEvent() looks like a better (more
resource-conservative) option.

Does the original code use CheckEvent() because WaitForEvent() is
restricted to TPL_APPLICATION?

I don't think that being restricted to TPL_APPLICATION should be a
problem. As far as I can tell, the only call path to Tcg2ReadUserKey() is:

  Tcg2PhysicalPresenceLibProcessRequest()
    Tcg2ExecutePendingTpmRequest()
      Tcg2UserConfirm()
        Tcg2ReadUserKey()

In turn, Tcg2PhysicalPresenceLibProcessRequest() is supposed to be
called from PlatformBootManagerLib, which in turn is called from BdsDxe.
Therefore I think we can safely assume TPL_APPLICATION.

I think we should add a separate patch first, for rewriting the present
logic of Tcg2ReadUserKey() with WaitForEvent() -- remove the busy loop.


(2) Once we use WaitForEvent(), it is easier and more elegant to use a
timer event, in addition to "gST->ConIn->WaitForKey", to limit the
waiting for a keypress.

For example, the BdsWaitForSingleEvent() function in
"MdeModulePkg/Universal/BdsDxe/BdsEntry.c" is used for implementing a
timed wait for a hotkey, if I understand correctly.


(3) I think that the Tcg2ReadUserKey() function should take the timeout
parameter, and the PCD should be consumed in the Tcg2UserConfirm()
function instead. Tcg2UserConfirm() should pass the value of the PCD to
Tcg2ReadUserKey().

The PCD is called "PcdPhysicalPresenceUserConfirmTimeout", therefore it
seems more closely tied to the Tcg2UserConfirm() function, in purpose.
The Tcg2ReadUserKey() function seems to be a general utility function,
so it should take a parameter, rather than fetch a particular PCD.


(4) The comment block on the Tcg2ReadUserKey() function should be updated.


(5) Please make sure that you format the patches next time with the
"--stat=1000 --stat-graph-width=20" options. The pathnames of the files
that are modified by this patch set are very long, and they are
truncated in the diffstats. That way, the diffstat is not helpful.

The "BaseTools/Scripts/SetupGit.py" script can automate at least the
"--stat-graph-width=20" option for you.


(6) When posting several patches, or large patches, it is helpful for
reviewers if you also push your topic branch to your local repo, and
expose the corresponding URL / branch name in the cover letter. Some
patches are easier to review locally (after a git-fetch from your repo).


(7) I don't know what happened, but I can see only patch#0 and patch#6
from this series in my list folder. There are multiple lib instances
being modified, and I couldn't compare the changes between those.

Thanks
Laszlo


> diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> index 85ce0e2b29..701de1836c 100644
> --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> @@ -20,7 +20,7 @@
>  #  This external input must be validated carefully to avoid security issue.
>  #
>  # Copyright (C) 2018, Red Hat, Inc.
> -# Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
>  ##
> @@ -62,10 +62,14 @@
>    UefiBootServicesTableLib
>    UefiLib
>    UefiRuntimeServicesTableLib
> +  TimerLib
>  
>  [Protocols]
>    gEfiTcg2ProtocolGuid                 ## SOMETIMES_CONSUMES
>  
> +[Pcd]
> +  gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout
> +
>  [Guids]
>    ## SOMETIMES_CONSUMES ## HII
>    gEfiTcg2PhysicalPresenceGuid
>