[edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
Laszlo Ersek
lersek at redhat.com
Fri Jan 3 14:21:02 UTC 2020
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 at intel.com>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Chao Zhang <chao.b.zhang at intel.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> Cc: Stefan Berger <stefanb at linux.ibm.com>
> Signed-off-by: Zhichao Gao <zhichao.gao at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52834): https://edk2.groups.io/g/devel/message/52834
Mute This Topic: https://groups.io/mt/69392333/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list