* [PATCH] SecurityPkg: Add retry mechanism for tpm command @ 2022-07-27 11:35 Qi Zhang 2022-07-27 12:06 ` Yao, Jiewen 0 siblings, 1 reply; 5+ messages in thread From: Qi Zhang @ 2022-07-27 11:35 UTC (permalink / raw) To: devel; +Cc: Qi Zhang, Jiewen Yao, Jian J Wang Signed-off-by: Qi Zhang <qi1.zhang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> --- .../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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] SecurityPkg: Add retry mechanism for tpm command 2022-07-27 11:35 [PATCH] SecurityPkg: Add retry mechanism for tpm command Qi Zhang @ 2022-07-27 12:06 ` Yao, Jiewen 2022-07-27 16:37 ` [edk2-devel] " Michael D Kinney 0 siblings, 1 reply; 5+ messages in thread From: Yao, Jiewen @ 2022-07-27 12:06 UTC (permalink / raw) To: Zhang, Qi1, devel@edk2.groups.io; +Cc: Wang, Jian J 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@intel.com> > -----Original Message----- > From: Zhang, Qi1 <qi1.zhang@intel.com> > Sent: Wednesday, July 27, 2022 7:36 PM > To: devel@edk2.groups.io > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Wang, Jian J <jian.j.wang@intel.com> > Subject: [PATCH] SecurityPkg: Add retry mechanism for tpm command > > Signed-off-by: Qi Zhang <qi1.zhang@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > --- > .../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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] SecurityPkg: Add retry mechanism for tpm command 2022-07-27 12:06 ` Yao, Jiewen @ 2022-07-27 16:37 ` Michael D Kinney 2022-07-28 0:40 ` Qi Zhang 0 siblings, 1 reply; 5+ messages in thread From: Michael D Kinney @ 2022-07-27 16:37 UTC (permalink / raw) To: devel@edk2.groups.io, Yao, Jiewen, Zhang, Qi1, Kinney, Michael D Cc: Wang, Jian J 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@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com> > 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@intel.com> > > > -----Original Message----- > > From: Zhang, Qi1 <qi1.zhang@intel.com> > > Sent: Wednesday, July 27, 2022 7:36 PM > > To: devel@edk2.groups.io > > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > > Wang, Jian J <jian.j.wang@intel.com> > > Subject: [PATCH] SecurityPkg: Add retry mechanism for tpm command > > > > Signed-off-by: Qi Zhang <qi1.zhang@intel.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > --- > > .../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 > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] SecurityPkg: Add retry mechanism for tpm command 2022-07-27 16:37 ` [edk2-devel] " Michael D Kinney @ 2022-07-28 0:40 ` Qi Zhang 2022-07-28 1:40 ` Michael D Kinney 0 siblings, 1 reply; 5+ messages in thread From: Qi Zhang @ 2022-07-28 0:40 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, Yao, Jiewen; +Cc: Wang, Jian J Retry count is suggested in the spec. PtpCrbWaitRegisterBits() already has delay. Thanks! Qi Zhang -----Original Message----- From: Kinney, Michael D <michael.d.kinney@intel.com> Sent: Thursday, July 28, 2022 12:38 AM To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Cc: Wang, Jian J <jian.j.wang@intel.com> 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@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com> > 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@intel.com> > > > -----Original Message----- > > From: Zhang, Qi1 <qi1.zhang@intel.com> > > Sent: Wednesday, July 27, 2022 7:36 PM > > To: devel@edk2.groups.io > > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Yao, Jiewen > > <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com> > > Subject: [PATCH] SecurityPkg: Add retry mechanism for tpm command > > > > Signed-off-by: Qi Zhang <qi1.zhang@intel.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > --- > > .../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 > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] SecurityPkg: Add retry mechanism for tpm command 2022-07-28 0:40 ` Qi Zhang @ 2022-07-28 1:40 ` Michael D Kinney 0 siblings, 0 replies; 5+ messages in thread From: Michael D Kinney @ 2022-07-28 1:40 UTC (permalink / raw) To: Zhang, Qi1, devel@edk2.groups.io, Yao, Jiewen, Kinney, Michael D Cc: Wang, Jian J 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 > -----Original Message----- > From: Zhang, Qi1 <qi1.zhang@intel.com> > Sent: Wednesday, July 27, 2022 5:41 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> > Cc: Wang, Jian J <jian.j.wang@intel.com> > 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@intel.com> > Sent: Thursday, July 28, 2022 12:38 AM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: Wang, Jian J <jian.j.wang@intel.com> > 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@intel.com>; devel@edk2.groups.io > > Cc: Wang, Jian J <jian.j.wang@intel.com> > > 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@intel.com> > > > > > -----Original Message----- > > > From: Zhang, Qi1 <qi1.zhang@intel.com> > > > Sent: Wednesday, July 27, 2022 7:36 PM > > > To: devel@edk2.groups.io > > > Cc: Zhang, Qi1 <qi1.zhang@intel.com>; Yao, Jiewen > > > <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com> > > > Subject: [PATCH] SecurityPkg: Add retry mechanism for tpm command > > > > > > Signed-off-by: Qi Zhang <qi1.zhang@intel.com> > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > --- > > > .../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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-28 1:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-27 11:35 [PATCH] SecurityPkg: Add retry mechanism for tpm command Qi Zhang 2022-07-27 12:06 ` Yao, Jiewen 2022-07-27 16:37 ` [edk2-devel] " Michael D Kinney 2022-07-28 0:40 ` Qi Zhang 2022-07-28 1:40 ` Michael D Kinney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox