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' <ler...@redhat.com>; devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J > <jian.j.w...@intel.com>; Zhang, Chao B <chao.b.zh...@intel.com>; Justen, > Jordan L <jordan.l.jus...@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Marc-André Lureau > <marcandre.lur...@redhat.com>; Stefan Berger <stef...@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:ler...@redhat.com] > > Sent: Friday, January 3, 2020 10:21 PM > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao....@intel.com> > > Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J > > <jian.j.w...@intel.com>; Zhang, Chao B <chao.b.zh...@intel.com>; > > Justen, Jordan L <jordan.l.jus...@intel.com>; Ard Biesheuvel > > <ard.biesheu...@linaro.org>; Marc-André Lureau > > <marcandre.lur...@redhat.com>; Stefan Berger > <stef...@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....@intel.com> > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > > Cc: Chao Zhang <chao.b.zh...@intel.com> > > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > > > Cc: Marc-André Lureau <marcandre.lur...@redhat.com> > > > Cc: Stefan Berger <stef...@linux.ibm.com> > > > Signed-off-by: Zhichao Gao <zhichao....@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53388): https://edk2.groups.io/g/devel/message/53388 Mute This Topic: https://groups.io/mt/69392333/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
