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

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

Hi Laszlo,

Sorry for misunderstanding the logic in read key function. That makes the incorrect analysis.
The read key function is waiting for "specific" key press not a key press. We should drop all the other pressing-key.
That means we should create a hotkey service like BDS one. It is not simple. I think that is why the original use the busy loop.
I don't think it makes sense to port the function from BDS for such a simple function. If there is a requirement for the hot key service to be public and common, it makes sense.
So I prefer to keep the original busy loop instead of porting hot key service.

Thanks,
Zhichao

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, January 15, 2020 4:03 PM
> To: 'Laszlo Ersek' <lersek@redhat.com>; devel@edk2.groups.io
> 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
> 
> 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