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

["Gao, Zhichao" <zhichao.gao@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, January 3, 2020 10:21 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib:
> Use pcd for user response wait time
> 
> 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/DxeTcg2PhysicalPresenc
> eL
> > ib.c
> >
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.c
> > index 00d76ba2c2..13f78cbfac 100644
> > ---
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.c
> > +++
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPrese
> > +++ nceLib.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.

Thanks for the suggestion. It may make sense to adjust the read key logic.
Let me work out the patch and do some test of it. The function is internal, it is fine to change it and won't affect any other interface.

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

Fine. Will do that.

> 
> (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).

Thanks for the suggestion. Would follow that if I work on a big patch.

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

I would separate the patch into simple ones. The user confirm change is independent and it would be a simple patch later.

Thanks,
Zhichao

> 
> Thanks
> Laszlo
> 
> 
> > diff --git
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.inf
> >
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.inf
> > index 85ce0e2b29..701de1836c 100644
> > ---
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.inf
> > +++
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPrese
> > +++ nceLib.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
> >