public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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