The commit message and comments for the #defines
for the retry count and timeouts should state that
the values are required by spec and name the spec.
Thanks,
Mike
toggle quoted message
Show quoted text
-----Original Message-----
From: Zhang, Qi1 <qi1.zhang@...>
Sent: Wednesday, July 27, 2022 5:41 PM
To: Kinney, Michael D <michael.d.kinney@...>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>
Cc: Wang, Jian J <jian.j.wang@...>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: Add retry mechanism for tpm command
Retry count is suggested in the spec.
PtpCrbWaitRegisterBits() already has delay.
Thanks!
Qi Zhang
-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Thursday, July 28, 2022 12:38 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Zhang, Qi1 <qi1.zhang@...>; Kinney, Michael D
<michael.d.kinney@...>
Cc: Wang, Jian J <jian.j.wang@...>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: Add retry mechanism for tpm command
Why is 3 the correct retry count?
Do we need this to be configurable?
Is a delay required between retries?
What specific state is a DTPM getting into that requires this retry mechanism? Can that state be detected?
Thanks,
Mike
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
Jiewen
Sent: Wednesday, July 27, 2022 5:07 AM
To: Zhang, Qi1 <qi1.zhang@...>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>
Subject: Re: [edk2-devel] [PATCH] SecurityPkg: Add retry mechanism for
tpm command
Thanks. Please add Bugzilla ID and add tested-by tag by the people who performed the test.
For the code, reviewed-by: Jiewen Yao <Jiewen.yao@...>
-----Original Message-----
From: Zhang, Qi1 <qi1.zhang@...>
Sent: Wednesday, July 27, 2022 7:36 PM
To: devel@edk2.groups.io
Cc: Zhang, Qi1 <qi1.zhang@...>; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>
Subject: [PATCH] SecurityPkg: Add retry mechanism for tpm command
Signed-off-by: Qi Zhang <qi1.zhang@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
---
.../Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 107 +++++++++++-------
1 file changed, 68 insertions(+), 39 deletions(-)
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
index 1d99beaa10..6b5994fde2 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
@@ -33,6 +33,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent //
#define TPMCMDBUFLENGTH 0x500
+//
+// Max retry count
+//
+#define RETRY_CNT_MAX 3
+
/**
Check whether TPM PTP register exist.
@@ -153,6 +158,7 @@ PtpCrbTpmCommand (
UINT32 TpmOutSize;
UINT16 Data16;
UINT32 Data32;
+ UINT8 RetryCnt;
DEBUG_CODE_BEGIN ();
UINTN DebugSize;
@@ -179,53 +185,76 @@ PtpCrbTpmCommand (
DEBUG_CODE_END ();
TpmOutSize = 0;
- //
- // STEP 0:
- // if CapCRbIdelByPass == 0, enforce Idle state before sending
command
- //
- if ((GetCachedIdleByPass () == 0) && ((MmioRead32
((UINTN)&CrbReg-
CrbControlStatus) & PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE) == 0)) { + RetryCnt = 0;
+ while (TRUE) {
+ //
+ // STEP 0:
+ // if CapCRbIdelByPass == 0, enforce Idle state before sending
+ command
+ //
+ if ((GetCachedIdleByPass () == 0) && ((MmioRead32
+ ((UINTN)&CrbReg-
CrbControlStatus) & PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE) == 0)) { + Status = PtpCrbWaitRegisterBits (
+ &CrbReg->CrbControlStatus,
+ PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE,
+ 0,
+ PTP_TIMEOUT_C
+ );
+ if (EFI_ERROR (Status)) {
+ RetryCnt++;
+ if (RetryCnt < RETRY_CNT_MAX) {
+ MmioWrite32 ((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_GO_IDLE);
+ continue;
+ } else {
+ //
+ // Try to goIdle to recover TPM
+ //
+ Status = EFI_DEVICE_ERROR;
+ goto GoIdle_Exit;
+ }
+ }
+ }
+
+ //
+ // STEP 1:
+ // Ready is any time the TPM is ready to receive a command,
+ following a
write
+ // of 1 by software to Request.cmdReady, as indicated by the
+ Status field
+ // being cleared to 0.
+ //
+ MmioWrite32 ((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
Status = PtpCrbWaitRegisterBits (
- &CrbReg->CrbControlStatus,
- PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE,
+ &CrbReg->CrbControlRequest,
0,
+ PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY,
PTP_TIMEOUT_C
);
if (EFI_ERROR (Status)) {
- //
- // Try to goIdle to recover TPM
- //
- Status = EFI_DEVICE_ERROR;
- goto GoIdle_Exit;
+ RetryCnt++;
+ if (RetryCnt < RETRY_CNT_MAX) {
+ MmioWrite32 ((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_GO_IDLE);
+ continue;
+ } else {
+ Status = EFI_DEVICE_ERROR;
+ goto GoIdle_Exit;
+ }
}
- }
- //
- // STEP 1:
- // Ready is any time the TPM is ready to receive a command,
following a write
- // of 1 by software to Request.cmdReady, as indicated by the
Status field
- // being cleared to 0.
- //
- MmioWrite32 ((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
- Status = PtpCrbWaitRegisterBits (
- &CrbReg->CrbControlRequest,
- 0,
- PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY,
- PTP_TIMEOUT_C
- );
- if (EFI_ERROR (Status)) {
- Status = EFI_DEVICE_ERROR;
- goto GoIdle_Exit;
- }
+ Status = PtpCrbWaitRegisterBits (
+ &CrbReg->CrbControlStatus,
+ 0,
+ PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE,
+ PTP_TIMEOUT_C
+ );
+ if (EFI_ERROR (Status)) {
+ RetryCnt++;
+ if (RetryCnt < RETRY_CNT_MAX) {
+ MmioWrite32 ((UINTN)&CrbReg->CrbControlRequest,
PTP_CRB_CONTROL_AREA_REQUEST_GO_IDLE);
+ continue;
+ } else {
+ Status = EFI_DEVICE_ERROR;
+ goto GoIdle_Exit;
+ }
+ }
- Status = PtpCrbWaitRegisterBits (
- &CrbReg->CrbControlStatus,
- 0,
- PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE,
- PTP_TIMEOUT_C
- );
- if (EFI_ERROR (Status)) {
- Status = EFI_DEVICE_ERROR;
- goto GoIdle_Exit;
+ break;
}
//
--
2.26.2.windows.1
|
