public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
@ 2019-06-25  9:44 Albecki, Mateusz
  2019-06-26  0:19 ` Wu, Hao A
  0 siblings, 1 reply; 5+ messages in thread
From: Albecki, Mateusz @ 2019-06-25  9:44 UTC (permalink / raw)
  To: devel; +Cc: Mateusz Albecki, Hao A Wu

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1341

Since UFS specification requirs the data buffer specified
in PRDT to be DWORD aligned in size we had a code in
UfsInitUtpPrdt that aligned the data buffer by rounding down
the buffer size to DWORD boundary. This meant that for SCSI
commands that wanted to perform unaligned data transfer(such as
SENSE command) we specified to small buffer for the data to fit
and transfer was aborted. This change introduces code that allocates
auxiliary DWORD aligned data buffer for unaligned transfer. Device
transfers data to aligned buffer and when data transfer is over driver
copies data from aligned buffer to data buffer passed by user.

Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   2 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 198 +++++++++++++++------
 2 files changed, 146 insertions(+), 54 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
index 6c98b3a76a..9b68db5ffe 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
@@ -92,6 +92,8 @@ typedef struct {
   UINT32                                        CmdDescSize;
   VOID                                          *CmdDescHost;
   VOID                                          *CmdDescMapping;
+  VOID                                          *AlignedDataBuf;
+  UINTN                                         AlignedDataBufSize;
   VOID                                          *DataBufMapping;
 
   EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index 4b93821f38..4f41183504 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -394,17 +394,13 @@ UfsInitUtpPrdt (
   UINT8      *Remaining;
   UINTN      PrdtNumber;
 
-  if ((BufferSize & (BIT0 | BIT1)) != 0) {
-    BufferSize &= ~(BIT0 | BIT1);
-    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not dword-aligned!\n", BufferSize));
-  }
+  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
+  ASSERT ((BufferSize & (BIT1 | BIT0)) == 0);
 
   if (BufferSize == 0) {
     return EFI_SUCCESS;
   }
 
-  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
-
   RemainingLen = BufferSize;
   Remaining    = Buffer;
   PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize + UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
@@ -1317,6 +1313,139 @@ Exit:
   return Status;
 }
 
+/**
+  Cleanup data buffers after data transfer. This function
+  also takes care to copy all data to user memory pool for
+  unaligned data transfers.
+
+  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
+  @param[in] TransReq  Pointer to the transfer request
+**/
+VOID
+UfsReconcileDataTransferBuffer (
+  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
+  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
+  )
+{
+  if (TransReq->DataBufMapping != NULL) {
+    Private->UfsHostController->Unmap (
+                                  Private->UfsHostController,
+                                  TransReq->DataBufMapping
+                                  );
+  }
+
+  //
+  // Check if unaligned transfer was performed. If it was and we read
+  // data from device copy memory to user data buffers before cleanup.
+  // The assumption is if auxiliary aligned data buffer is not NULL then
+  // unaligned transfer has been performed.
+  //
+  if (TransReq->AlignedDataBuf != NULL) {
+    if (TransReq->Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+      CopyMem (TransReq->Packet->InDataBuffer, TransReq->AlignedDataBuf, TransReq->Packet->InTransferLength);
+    }
+    //
+    // Wipe out the transfer buffer in case it contains sensitive data.
+    //
+    ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);
+    FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES (TransReq->AlignedDataBufSize));
+    TransReq->AlignedDataBuf = NULL;
+  }
+}
+
+/**
+  Prepare data buffer for transfer
+
+  @param[in]      Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
+  @param[in, out] TransReq  Pointer to the transfer request
+
+  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
+  @retval EFI_SUCCESS       Buffer ready for transfer
+**/
+EFI_STATUS
+UfsPrepareDataTransferBuffer (
+  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
+  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
+  )
+{
+  EFI_STATUS                           Status;
+  VOID                                 *DataBuf;
+  UINT32                               DataLen;
+  UINTN                                MapLength;
+  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
+  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
+  UTP_TR_PRD                           *PrdtBase;
+
+  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;
+  DataBuf        = NULL;
+
+  //
+  // For unaligned data transfers we allocate auxiliary DWORD aligned memory pool.
+  // When command is finished auxiliary memory pool is copied into actual user memory.
+  // This is requiered to assure data transfer safety(DWORD alignment required by UFS spec.)
+  //
+  if (TransReq->Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+    if (((UINTN)TransReq->Packet->InDataBuffer % 4 != 0) || (TransReq->Packet->InTransferLength % 4 != 0)) {
+      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq->Packet->InTransferLength % 4));
+      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
+      if (DataBuf == NULL) {
+        return EFI_DEVICE_ERROR;
+      }
+      ZeroMem (DataBuf, DataLen);
+      TransReq->AlignedDataBuf = DataBuf;
+      TransReq->AlignedDataBufSize = DataLen;
+    } else {
+      DataLen       = TransReq->Packet->InTransferLength;
+      DataBuf       = TransReq->Packet->InDataBuffer;
+    }
+    Flag          = EdkiiUfsHcOperationBusMasterWrite;
+  } else {
+    if (((UINTN)TransReq->Packet->OutDataBuffer % 4 != 0) || (TransReq->Packet->OutTransferLength % 4 != 0)) {
+      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq->Packet->OutTransferLength % 4));
+      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
+      if (DataBuf == NULL) {
+        return EFI_DEVICE_ERROR;
+      }
+      CopyMem (DataBuf, TransReq->Packet->OutDataBuffer, TransReq->Packet->OutTransferLength);
+      TransReq->AlignedDataBuf = DataBuf;
+      TransReq->AlignedDataBufSize = DataLen;
+    } else {
+      DataLen       = TransReq->Packet->OutTransferLength;
+      DataBuf       = TransReq->Packet->OutDataBuffer;
+    }
+    Flag          = EdkiiUfsHcOperationBusMasterRead;
+  }
+
+  if (DataLen != 0) {
+    MapLength = DataLen;
+    Status    = Private->UfsHostController->Map (
+                                              Private->UfsHostController,
+                                              Flag,
+                                              DataBuf,
+                                              &MapLength,
+                                              &DataBufPhyAddr,
+                                              &TransReq->DataBufMapping
+                                              );
+
+    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
+      if (TransReq->AlignedDataBuf != NULL) {
+        FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES (TransReq->AlignedDataBufSize));
+        TransReq->AlignedDataBuf = NULL;
+      }
+      return EFI_DEVICE_ERROR;
+    }
+  }
+
+  //
+  // Fill PRDT table of Command UPIU for executed SCSI cmd.
+  //
+  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU)));
+  ASSERT (PrdtBase != NULL);
+  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
+
+  return EFI_SUCCESS;
+}
+
 /**
   Sends a UFS-supported SCSI Request Packet to a UFS device that is attached to the UFS host controller.
 
@@ -1353,24 +1482,19 @@ UfsExecScsiCmds (
   UTP_RESPONSE_UPIU                    *Response;
   UINT16                               SenseDataLen;
   UINT32                               ResTranCount;
-  VOID                                 *DataBuf;
-  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
-  UINT32                               DataLen;
-  UINTN                                MapLength;
-  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
-  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
-  UTP_TR_PRD                           *PrdtBase;
   EFI_TPL                              OldTpl;
   UFS_PASS_THRU_TRANS_REQ              *TransReq;
+  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
 
-  TransReq       = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
+  TransReq = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
   if (TransReq == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
 
   TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
   TransReq->TimeoutRemain = Packet->Timeout;
-  DataBufPhyAddr = 0;
+  TransReq->Packet        = Packet;
+
   UfsHc          = Private->UfsHostController;
   //
   // Find out which slot of transfer request list is available.
@@ -1399,44 +1523,16 @@ UfsExecScsiCmds (
 
   TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) + TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
 
-  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
-    DataBuf       = Packet->InDataBuffer;
-    DataLen       = Packet->InTransferLength;
-    Flag          = EdkiiUfsHcOperationBusMasterWrite;
-  } else {
-    DataBuf       = Packet->OutDataBuffer;
-    DataLen       = Packet->OutTransferLength;
-    Flag          = EdkiiUfsHcOperationBusMasterRead;
-  }
-
-  if (DataLen != 0) {
-    MapLength = DataLen;
-    Status    = UfsHc->Map (
-                         UfsHc,
-                         Flag,
-                         DataBuf,
-                         &MapLength,
-                         &DataBufPhyAddr,
-                         &TransReq->DataBufMapping
-                         );
-
-    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
-      goto Exit1;
-    }
+  Status = UfsPrepareDataTransferBuffer (Private, TransReq);
+  if (EFI_ERROR (Status)) {
+    goto Exit1;
   }
-  //
-  // Fill PRDT table of Command UPIU for executed SCSI cmd.
-  //
-  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU)));
-  ASSERT (PrdtBase != NULL);
-  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
 
   //
   // Insert the async SCSI cmd to the Async I/O list
   //
   if (Event != NULL) {
     OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
-    TransReq->Packet      = Packet;
     TransReq->CallerEvent = Event;
     InsertTailList (&Private->Queue, &TransReq->TransferList);
     gBS->RestoreTPL (OldTpl);
@@ -1515,9 +1611,7 @@ Exit:
 
   UfsStopExecCmd (Private, TransReq->Slot);
 
-  if (TransReq->DataBufMapping != NULL) {
-    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
-  }
+  UfsReconcileDataTransferBuffer (Private, TransReq);
 
 Exit1:
   if (TransReq->CmdDescMapping != NULL) {
@@ -1532,7 +1626,6 @@ Exit1:
   return Status;
 }
 
-
 /**
   Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
 
@@ -2100,7 +2193,6 @@ UfsControllerStop (
   return EFI_SUCCESS;
 }
 
-
 /**
   Internal helper function which will signal the caller event and clean up
   resources.
@@ -2132,9 +2224,7 @@ SignalCallerEvent (
 
   UfsStopExecCmd (Private, TransReq->Slot);
 
-  if (TransReq->DataBufMapping != NULL) {
-    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
-  }
+  UfsReconcileDataTransferBuffer (Private, TransReq);
 
   if (TransReq->CmdDescMapping != NULL) {
     UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
-- 
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
  2019-06-25  9:44 [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz
@ 2019-06-26  0:19 ` Wu, Hao A
  2019-06-26  9:54   ` Albecki, Mateusz
  0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2019-06-26  0:19 UTC (permalink / raw)
  To: Albecki, Mateusz, devel@edk2.groups.io

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Tuesday, June 25, 2019 5:44 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A
> Subject: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data
> transfer handling
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1341
> 
> Since UFS specification requirs the data buffer specified
> in PRDT to be DWORD aligned in size we had a code in
> UfsInitUtpPrdt that aligned the data buffer by rounding down
> the buffer size to DWORD boundary. This meant that for SCSI
> commands that wanted to perform unaligned data transfer(such as
> SENSE command) we specified to small buffer for the data to fit
> and transfer was aborted. This change introduces code that allocates
> auxiliary DWORD aligned data buffer for unaligned transfer. Device
> transfers data to aligned buffer and when data transfer is over driver
> copies data from aligned buffer to data buffer passed by user.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   2 +
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 198 +++++++++++++++-
> -----
>  2 files changed, 146 insertions(+), 54 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index 6c98b3a76a..9b68db5ffe 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -92,6 +92,8 @@ typedef struct {
>    UINT32                                        CmdDescSize;
>    VOID                                          *CmdDescHost;
>    VOID                                          *CmdDescMapping;
> +  VOID                                          *AlignedDataBuf;
> +  UINTN                                         AlignedDataBufSize;
>    VOID                                          *DataBufMapping;
> 
>    EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 4b93821f38..4f41183504 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -394,17 +394,13 @@ UfsInitUtpPrdt (
>    UINT8      *Remaining;
>    UINTN      PrdtNumber;
> 
> -  if ((BufferSize & (BIT0 | BIT1)) != 0) {
> -    BufferSize &= ~(BIT0 | BIT1);
> -    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not
> dword-aligned!\n", BufferSize));
> -  }
> +  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> +  ASSERT ((BufferSize & (BIT1 | BIT0)) == 0);
> 
>    if (BufferSize == 0) {
>      return EFI_SUCCESS;
>    }
> 
> -  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> -
>    RemainingLen = BufferSize;
>    Remaining    = Buffer;
>    PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize +
> UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
> @@ -1317,6 +1313,139 @@ Exit:
>    return Status;
>  }
> 
> +/**
> +  Cleanup data buffers after data transfer. This function
> +  also takes care to copy all data to user memory pool for
> +  unaligned data transfers.
> +
> +  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> +  @param[in] TransReq  Pointer to the transfer request
> +**/
> +VOID
> +UfsReconcileDataTransferBuffer (
> +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
> +  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
> +  )
> +{
> +  if (TransReq->DataBufMapping != NULL) {
> +    Private->UfsHostController->Unmap (
> +                                  Private->UfsHostController,
> +                                  TransReq->DataBufMapping
> +                                  );
> +  }
> +
> +  //
> +  // Check if unaligned transfer was performed. If it was and we read
> +  // data from device copy memory to user data buffers before cleanup.
> +  // The assumption is if auxiliary aligned data buffer is not NULL then
> +  // unaligned transfer has been performed.
> +  //
> +  if (TransReq->AlignedDataBuf != NULL) {
> +    if (TransReq->Packet->DataDirection ==
> EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +      CopyMem (TransReq->Packet->InDataBuffer, TransReq-
> >AlignedDataBuf, TransReq->Packet->InTransferLength);
> +    }
> +    //
> +    // Wipe out the transfer buffer in case it contains sensitive data.
> +    //
> +    ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);
> +    FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> (TransReq->AlignedDataBufSize));
> +    TransReq->AlignedDataBuf = NULL;
> +  }
> +}
> +
> +/**
> +  Prepare data buffer for transfer
> +
> +  @param[in]      Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> +  @param[in, out] TransReq  Pointer to the transfer request
> +
> +  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
> +  @retval EFI_SUCCESS       Buffer ready for transfer
> +**/
> +EFI_STATUS
> +UfsPrepareDataTransferBuffer (
> +  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
> +  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
> +  )
> +{
> +  EFI_STATUS                           Status;
> +  VOID                                 *DataBuf;
> +  UINT32                               DataLen;
> +  UINTN                                MapLength;
> +  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> +  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> +  UTP_TR_PRD                           *PrdtBase;
> +
> +  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;
> +  DataBuf        = NULL;
> +
> +  //
> +  // For unaligned data transfers we allocate auxiliary DWORD aligned
> memory pool.
> +  // When command is finished auxiliary memory pool is copied into actual
> user memory.
> +  // This is requiered to assure data transfer safety(DWORD alignment
> required by UFS spec.)
> +  //
> +  if (TransReq->Packet->DataDirection ==
> EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    if (((UINTN)TransReq->Packet->InDataBuffer % 4 != 0) || (TransReq-
> >Packet->InTransferLength % 4 != 0)) {
> +      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq-
> >Packet->InTransferLength % 4));
> +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> +      if (DataBuf == NULL) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +      ZeroMem (DataBuf, DataLen);
> +      TransReq->AlignedDataBuf = DataBuf;
> +      TransReq->AlignedDataBufSize = DataLen;
> +    } else {
> +      DataLen       = TransReq->Packet->InTransferLength;
> +      DataBuf       = TransReq->Packet->InDataBuffer;
> +    }
> +    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> +  } else {
> +    if (((UINTN)TransReq->Packet->OutDataBuffer % 4 != 0) || (TransReq-
> >Packet->OutTransferLength % 4 != 0)) {
> +      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq-
> >Packet->OutTransferLength % 4));
> +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> +      if (DataBuf == NULL) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +      CopyMem (DataBuf, TransReq->Packet->OutDataBuffer, TransReq-
> >Packet->OutTransferLength);
> +      TransReq->AlignedDataBuf = DataBuf;
> +      TransReq->AlignedDataBufSize = DataLen;
> +    } else {
> +      DataLen       = TransReq->Packet->OutTransferLength;
> +      DataBuf       = TransReq->Packet->OutDataBuffer;
> +    }
> +    Flag          = EdkiiUfsHcOperationBusMasterRead;
> +  }
> +
> +  if (DataLen != 0) {
> +    MapLength = DataLen;
> +    Status    = Private->UfsHostController->Map (
> +                                              Private->UfsHostController,
> +                                              Flag,
> +                                              DataBuf,
> +                                              &MapLength,
> +                                              &DataBufPhyAddr,
> +                                              &TransReq->DataBufMapping
> +                                              );
> +
> +    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> +      if (TransReq->AlignedDataBuf != NULL) {


Hello,

I will also add

  ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);

here before freeing the auxiliary buffer when pushing the patch.
Please help to raise if there is any concern.

With that,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> +        FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> (TransReq->AlignedDataBufSize));
> +        TransReq->AlignedDataBuf = NULL;
> +      }
> +      return EFI_DEVICE_ERROR;
> +    }
> +  }
> +
> +  //
> +  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> +  //
> +  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> (UTP_RESPONSE_UPIU)));
> +  ASSERT (PrdtBase != NULL);
> +  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Sends a UFS-supported SCSI Request Packet to a UFS device that is
> attached to the UFS host controller.
> 
> @@ -1353,24 +1482,19 @@ UfsExecScsiCmds (
>    UTP_RESPONSE_UPIU                    *Response;
>    UINT16                               SenseDataLen;
>    UINT32                               ResTranCount;
> -  VOID                                 *DataBuf;
> -  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> -  UINT32                               DataLen;
> -  UINTN                                MapLength;
> -  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> -  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> -  UTP_TR_PRD                           *PrdtBase;
>    EFI_TPL                              OldTpl;
>    UFS_PASS_THRU_TRANS_REQ              *TransReq;
> +  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> 
> -  TransReq       = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
> +  TransReq = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
>    if (TransReq == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
>    TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
>    TransReq->TimeoutRemain = Packet->Timeout;
> -  DataBufPhyAddr = 0;
> +  TransReq->Packet        = Packet;
> +
>    UfsHc          = Private->UfsHostController;
>    //
>    // Find out which slot of transfer request list is available.
> @@ -1399,44 +1523,16 @@ UfsExecScsiCmds (
> 
>    TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) +
> TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
> 
> -  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> -    DataBuf       = Packet->InDataBuffer;
> -    DataLen       = Packet->InTransferLength;
> -    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> -  } else {
> -    DataBuf       = Packet->OutDataBuffer;
> -    DataLen       = Packet->OutTransferLength;
> -    Flag          = EdkiiUfsHcOperationBusMasterRead;
> -  }
> -
> -  if (DataLen != 0) {
> -    MapLength = DataLen;
> -    Status    = UfsHc->Map (
> -                         UfsHc,
> -                         Flag,
> -                         DataBuf,
> -                         &MapLength,
> -                         &DataBufPhyAddr,
> -                         &TransReq->DataBufMapping
> -                         );
> -
> -    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> -      goto Exit1;
> -    }
> +  Status = UfsPrepareDataTransferBuffer (Private, TransReq);
> +  if (EFI_ERROR (Status)) {
> +    goto Exit1;
>    }
> -  //
> -  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> -  //
> -  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> (UTP_RESPONSE_UPIU)));
> -  ASSERT (PrdtBase != NULL);
> -  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> 
>    //
>    // Insert the async SCSI cmd to the Async I/O list
>    //
>    if (Event != NULL) {
>      OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> -    TransReq->Packet      = Packet;
>      TransReq->CallerEvent = Event;
>      InsertTailList (&Private->Queue, &TransReq->TransferList);
>      gBS->RestoreTPL (OldTpl);
> @@ -1515,9 +1611,7 @@ Exit:
> 
>    UfsStopExecCmd (Private, TransReq->Slot);
> 
> -  if (TransReq->DataBufMapping != NULL) {
> -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> -  }
> +  UfsReconcileDataTransferBuffer (Private, TransReq);
> 
>  Exit1:
>    if (TransReq->CmdDescMapping != NULL) {
> @@ -1532,7 +1626,6 @@ Exit1:
>    return Status;
>  }
> 
> -
>  /**
>    Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
> 
> @@ -2100,7 +2193,6 @@ UfsControllerStop (
>    return EFI_SUCCESS;
>  }
> 
> -
>  /**
>    Internal helper function which will signal the caller event and clean up
>    resources.
> @@ -2132,9 +2224,7 @@ SignalCallerEvent (
> 
>    UfsStopExecCmd (Private, TransReq->Slot);
> 
> -  if (TransReq->DataBufMapping != NULL) {
> -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> -  }
> +  UfsReconcileDataTransferBuffer (Private, TransReq);
> 
>    if (TransReq->CmdDescMapping != NULL) {
>      UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
> --
> 2.14.1.windows.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
  2019-06-26  0:19 ` Wu, Hao A
@ 2019-06-26  9:54   ` Albecki, Mateusz
  2019-06-27  1:08     ` Wu, Hao A
  0 siblings, 1 reply; 5+ messages in thread
From: Albecki, Mateusz @ 2019-06-26  9:54 UTC (permalink / raw)
  To: Wu, Hao A, devel@edk2.groups.io

Hi,

No concerns from my side. I just didn't think ZeroMem would be useful there since buffer doesn't contain any data from device at that point so I didn't add it.

Thanks,
Mateusz

> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, June 26, 2019 2:19 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Subject: RE: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data
> transfer handling
> 
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Tuesday, June 25, 2019 5:44 PM
> > To: devel@edk2.groups.io
> > Cc: Albecki, Mateusz; Wu, Hao A
> > Subject: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data
> > transfer handling
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1341
> >
> > Since UFS specification requirs the data buffer specified in PRDT to
> > be DWORD aligned in size we had a code in UfsInitUtpPrdt that aligned
> > the data buffer by rounding down the buffer size to DWORD boundary.
> > This meant that for SCSI commands that wanted to perform unaligned
> > data transfer(such as SENSE command) we specified to small buffer for
> > the data to fit and transfer was aborted. This change introduces code
> > that allocates auxiliary DWORD aligned data buffer for unaligned
> > transfer. Device transfers data to aligned buffer and when data
> > transfer is over driver copies data from aligned buffer to data buffer
> > passed by user.
> >
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> > ---
> >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   2 +
> >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 198
> +++++++++++++++-
> > -----
> >  2 files changed, 146 insertions(+), 54 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > index 6c98b3a76a..9b68db5ffe 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > @@ -92,6 +92,8 @@ typedef struct {
> >    UINT32                                        CmdDescSize;
> >    VOID                                          *CmdDescHost;
> >    VOID                                          *CmdDescMapping;
> > +  VOID                                          *AlignedDataBuf;
> > +  UINTN                                         AlignedDataBufSize;
> >    VOID                                          *DataBufMapping;
> >
> >    EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > index 4b93821f38..4f41183504 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > @@ -394,17 +394,13 @@ UfsInitUtpPrdt (
> >    UINT8      *Remaining;
> >    UINTN      PrdtNumber;
> >
> > -  if ((BufferSize & (BIT0 | BIT1)) != 0) {
> > -    BufferSize &= ~(BIT0 | BIT1);
> > -    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not
> > dword-aligned!\n", BufferSize));
> > -  }
> > +  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);  ASSERT ((BufferSize
> > + & (BIT1 | BIT0)) == 0);
> >
> >    if (BufferSize == 0) {
> >      return EFI_SUCCESS;
> >    }
> >
> > -  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> > -
> >    RemainingLen = BufferSize;
> >    Remaining    = Buffer;
> >    PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize +
> > UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
> @@ -1317,6
> > +1313,139 @@ Exit:
> >    return Status;
> >  }
> >
> > +/**
> > +  Cleanup data buffers after data transfer. This function
> > +  also takes care to copy all data to user memory pool for
> > +  unaligned data transfers.
> > +
> > +  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> > +  @param[in] TransReq  Pointer to the transfer request **/ VOID
> > +UfsReconcileDataTransferBuffer (
> > +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
> > +  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
> > +  )
> > +{
> > +  if (TransReq->DataBufMapping != NULL) {
> > +    Private->UfsHostController->Unmap (
> > +                                  Private->UfsHostController,
> > +                                  TransReq->DataBufMapping
> > +                                  );
> > +  }
> > +
> > +  //
> > +  // Check if unaligned transfer was performed. If it was and we read
> > + // data from device copy memory to user data buffers before cleanup.
> > +  // The assumption is if auxiliary aligned data buffer is not NULL
> > + then  // unaligned transfer has been performed.
> > +  //
> > +  if (TransReq->AlignedDataBuf != NULL) {
> > +    if (TransReq->Packet->DataDirection ==
> > EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > +      CopyMem (TransReq->Packet->InDataBuffer, TransReq-
> > >AlignedDataBuf, TransReq->Packet->InTransferLength);
> > +    }
> > +    //
> > +    // Wipe out the transfer buffer in case it contains sensitive data.
> > +    //
> > +    ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);
> > +    FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> > (TransReq->AlignedDataBufSize));
> > +    TransReq->AlignedDataBuf = NULL;
> > +  }
> > +}
> > +
> > +/**
> > +  Prepare data buffer for transfer
> > +
> > +  @param[in]      Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> > +  @param[in, out] TransReq  Pointer to the transfer request
> > +
> > +  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
> > +  @retval EFI_SUCCESS       Buffer ready for transfer
> > +**/
> > +EFI_STATUS
> > +UfsPrepareDataTransferBuffer (
> > +  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
> > +  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
> > +  )
> > +{
> > +  EFI_STATUS                           Status;
> > +  VOID                                 *DataBuf;
> > +  UINT32                               DataLen;
> > +  UINTN                                MapLength;
> > +  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> > +  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> > +  UTP_TR_PRD                           *PrdtBase;
> > +
> > +  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;
> > +  DataBuf        = NULL;
> > +
> > +  //
> > +  // For unaligned data transfers we allocate auxiliary DWORD aligned
> > memory pool.
> > +  // When command is finished auxiliary memory pool is copied into
> > + actual
> > user memory.
> > +  // This is requiered to assure data transfer safety(DWORD alignment
> > required by UFS spec.)
> > +  //
> > +  if (TransReq->Packet->DataDirection ==
> > EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > +    if (((UINTN)TransReq->Packet->InDataBuffer % 4 != 0) ||
> > + (TransReq-
> > >Packet->InTransferLength % 4 != 0)) {
> > +      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq-
> > >Packet->InTransferLength % 4));
> > +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> > +      if (DataBuf == NULL) {
> > +        return EFI_DEVICE_ERROR;
> > +      }
> > +      ZeroMem (DataBuf, DataLen);
> > +      TransReq->AlignedDataBuf = DataBuf;
> > +      TransReq->AlignedDataBufSize = DataLen;
> > +    } else {
> > +      DataLen       = TransReq->Packet->InTransferLength;
> > +      DataBuf       = TransReq->Packet->InDataBuffer;
> > +    }
> > +    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> > +  } else {
> > +    if (((UINTN)TransReq->Packet->OutDataBuffer % 4 != 0) ||
> > + (TransReq-
> > >Packet->OutTransferLength % 4 != 0)) {
> > +      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq-
> > >Packet->OutTransferLength % 4));
> > +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> > +      if (DataBuf == NULL) {
> > +        return EFI_DEVICE_ERROR;
> > +      }
> > +      CopyMem (DataBuf, TransReq->Packet->OutDataBuffer, TransReq-
> > >Packet->OutTransferLength);
> > +      TransReq->AlignedDataBuf = DataBuf;
> > +      TransReq->AlignedDataBufSize = DataLen;
> > +    } else {
> > +      DataLen       = TransReq->Packet->OutTransferLength;
> > +      DataBuf       = TransReq->Packet->OutDataBuffer;
> > +    }
> > +    Flag          = EdkiiUfsHcOperationBusMasterRead;
> > +  }
> > +
> > +  if (DataLen != 0) {
> > +    MapLength = DataLen;
> > +    Status    = Private->UfsHostController->Map (
> > +                                              Private->UfsHostController,
> > +                                              Flag,
> > +                                              DataBuf,
> > +                                              &MapLength,
> > +                                              &DataBufPhyAddr,
> > +                                              &TransReq->DataBufMapping
> > +                                              );
> > +
> > +    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> > +      if (TransReq->AlignedDataBuf != NULL) {
> 
> 
> Hello,
> 
> I will also add
> 
>   ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);
> 
> here before freeing the auxiliary buffer when pushing the patch.
> Please help to raise if there is any concern.
> 
> With that,
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> Best Regards,
> Hao Wu
> 
> 
> > +        FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> > (TransReq->AlignedDataBufSize));
> > +        TransReq->AlignedDataBuf = NULL;
> > +      }
> > +      return EFI_DEVICE_ERROR;
> > +    }
> > +  }
> > +
> > +  //
> > +  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> > +  //
> > +  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> > ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> > (UTP_RESPONSE_UPIU)));
> > +  ASSERT (PrdtBase != NULL);
> > +  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> >  /**
> >    Sends a UFS-supported SCSI Request Packet to a UFS device that is
> > attached to the UFS host controller.
> >
> > @@ -1353,24 +1482,19 @@ UfsExecScsiCmds (
> >    UTP_RESPONSE_UPIU                    *Response;
> >    UINT16                               SenseDataLen;
> >    UINT32                               ResTranCount;
> > -  VOID                                 *DataBuf;
> > -  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> > -  UINT32                               DataLen;
> > -  UINTN                                MapLength;
> > -  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> > -  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> > -  UTP_TR_PRD                           *PrdtBase;
> >    EFI_TPL                              OldTpl;
> >    UFS_PASS_THRU_TRANS_REQ              *TransReq;
> > +  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> >
> > -  TransReq       = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
> > +  TransReq = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
> >    if (TransReq == NULL) {
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> >
> >    TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
> >    TransReq->TimeoutRemain = Packet->Timeout;
> > -  DataBufPhyAddr = 0;
> > +  TransReq->Packet        = Packet;
> > +
> >    UfsHc          = Private->UfsHostController;
> >    //
> >    // Find out which slot of transfer request list is available.
> > @@ -1399,44 +1523,16 @@ UfsExecScsiCmds (
> >
> >    TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) +
> > TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
> >
> > -  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > -    DataBuf       = Packet->InDataBuffer;
> > -    DataLen       = Packet->InTransferLength;
> > -    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> > -  } else {
> > -    DataBuf       = Packet->OutDataBuffer;
> > -    DataLen       = Packet->OutTransferLength;
> > -    Flag          = EdkiiUfsHcOperationBusMasterRead;
> > -  }
> > -
> > -  if (DataLen != 0) {
> > -    MapLength = DataLen;
> > -    Status    = UfsHc->Map (
> > -                         UfsHc,
> > -                         Flag,
> > -                         DataBuf,
> > -                         &MapLength,
> > -                         &DataBufPhyAddr,
> > -                         &TransReq->DataBufMapping
> > -                         );
> > -
> > -    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> > -      goto Exit1;
> > -    }
> > +  Status = UfsPrepareDataTransferBuffer (Private, TransReq);  if
> > + (EFI_ERROR (Status)) {
> > +    goto Exit1;
> >    }
> > -  //
> > -  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> > -  //
> > -  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> > ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> > (UTP_RESPONSE_UPIU)));
> > -  ASSERT (PrdtBase != NULL);
> > -  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> >
> >    //
> >    // Insert the async SCSI cmd to the Async I/O list
> >    //
> >    if (Event != NULL) {
> >      OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> > -    TransReq->Packet      = Packet;
> >      TransReq->CallerEvent = Event;
> >      InsertTailList (&Private->Queue, &TransReq->TransferList);
> >      gBS->RestoreTPL (OldTpl);
> > @@ -1515,9 +1611,7 @@ Exit:
> >
> >    UfsStopExecCmd (Private, TransReq->Slot);
> >
> > -  if (TransReq->DataBufMapping != NULL) {
> > -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> > -  }
> > +  UfsReconcileDataTransferBuffer (Private, TransReq);
> >
> >  Exit1:
> >    if (TransReq->CmdDescMapping != NULL) { @@ -1532,7 +1626,6 @@
> > Exit1:
> >    return Status;
> >  }
> >
> > -
> >  /**
> >    Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
> >
> > @@ -2100,7 +2193,6 @@ UfsControllerStop (
> >    return EFI_SUCCESS;
> >  }
> >
> > -
> >  /**
> >    Internal helper function which will signal the caller event and clean up
> >    resources.
> > @@ -2132,9 +2224,7 @@ SignalCallerEvent (
> >
> >    UfsStopExecCmd (Private, TransReq->Slot);
> >
> > -  if (TransReq->DataBufMapping != NULL) {
> > -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> > -  }
> > +  UfsReconcileDataTransferBuffer (Private, TransReq);
> >
> >    if (TransReq->CmdDescMapping != NULL) {
> >      UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
> > --
> > 2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
  2019-06-26  9:54   ` Albecki, Mateusz
@ 2019-06-27  1:08     ` Wu, Hao A
  2019-07-01  1:09       ` [edk2-devel] " Wu, Hao A
  0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2019-06-27  1:08 UTC (permalink / raw)
  To: Albecki, Mateusz, devel@edk2.groups.io

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Wednesday, June 26, 2019 5:55 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Subject: RE: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data
> transfer handling
> 
> Hi,
> 
> No concerns from my side. I just didn't think ZeroMem would be useful
> there since buffer doesn't contain any data from device at that point so I
> didn't add it.


My concern is that the data in 'Packet->OutDataBuffer' (from caller) may
contain information.

Caller can clean up the content in 'Packet->OutDataBuffer' after the
PassThru call but cannot control the auxiliary buffer within UFS driver.

Best Regards,
Hao Wu


> 
> Thanks,
> Mateusz
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Wednesday, June 26, 2019 2:19 AM
> > To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> > Subject: RE: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned
> data
> > transfer handling
> >
> > > -----Original Message-----
> > > From: Albecki, Mateusz
> > > Sent: Tuesday, June 25, 2019 5:44 PM
> > > To: devel@edk2.groups.io
> > > Cc: Albecki, Mateusz; Wu, Hao A
> > > Subject: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data
> > > transfer handling
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1341
> > >
> > > Since UFS specification requirs the data buffer specified in PRDT to
> > > be DWORD aligned in size we had a code in UfsInitUtpPrdt that aligned
> > > the data buffer by rounding down the buffer size to DWORD boundary.
> > > This meant that for SCSI commands that wanted to perform unaligned
> > > data transfer(such as SENSE command) we specified to small buffer for
> > > the data to fit and transfer was aborted. This change introduces code
> > > that allocates auxiliary DWORD aligned data buffer for unaligned
> > > transfer. Device transfers data to aligned buffer and when data
> > > transfer is over driver copies data from aligned buffer to data buffer
> > > passed by user.
> > >
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> > > ---
> > >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   2 +
> > >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 198
> > +++++++++++++++-
> > > -----
> > >  2 files changed, 146 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > index 6c98b3a76a..9b68db5ffe 100644
> > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > @@ -92,6 +92,8 @@ typedef struct {
> > >    UINT32                                        CmdDescSize;
> > >    VOID                                          *CmdDescHost;
> > >    VOID                                          *CmdDescMapping;
> > > +  VOID                                          *AlignedDataBuf;
> > > +  UINTN                                         AlignedDataBufSize;
> > >    VOID                                          *DataBufMapping;
> > >
> > >    EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
> > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > index 4b93821f38..4f41183504 100644
> > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > @@ -394,17 +394,13 @@ UfsInitUtpPrdt (
> > >    UINT8      *Remaining;
> > >    UINTN      PrdtNumber;
> > >
> > > -  if ((BufferSize & (BIT0 | BIT1)) != 0) {
> > > -    BufferSize &= ~(BIT0 | BIT1);
> > > -    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not
> > > dword-aligned!\n", BufferSize));
> > > -  }
> > > +  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);  ASSERT ((BufferSize
> > > + & (BIT1 | BIT0)) == 0);
> > >
> > >    if (BufferSize == 0) {
> > >      return EFI_SUCCESS;
> > >    }
> > >
> > > -  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> > > -
> > >    RemainingLen = BufferSize;
> > >    Remaining    = Buffer;
> > >    PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize +
> > > UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
> > @@ -1317,6
> > > +1313,139 @@ Exit:
> > >    return Status;
> > >  }
> > >
> > > +/**
> > > +  Cleanup data buffers after data transfer. This function
> > > +  also takes care to copy all data to user memory pool for
> > > +  unaligned data transfers.
> > > +
> > > +  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> > > +  @param[in] TransReq  Pointer to the transfer request **/ VOID
> > > +UfsReconcileDataTransferBuffer (
> > > +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
> > > +  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
> > > +  )
> > > +{
> > > +  if (TransReq->DataBufMapping != NULL) {
> > > +    Private->UfsHostController->Unmap (
> > > +                                  Private->UfsHostController,
> > > +                                  TransReq->DataBufMapping
> > > +                                  );
> > > +  }
> > > +
> > > +  //
> > > +  // Check if unaligned transfer was performed. If it was and we read
> > > + // data from device copy memory to user data buffers before cleanup.
> > > +  // The assumption is if auxiliary aligned data buffer is not NULL
> > > + then  // unaligned transfer has been performed.
> > > +  //
> > > +  if (TransReq->AlignedDataBuf != NULL) {
> > > +    if (TransReq->Packet->DataDirection ==
> > > EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > > +      CopyMem (TransReq->Packet->InDataBuffer, TransReq-
> > > >AlignedDataBuf, TransReq->Packet->InTransferLength);
> > > +    }
> > > +    //
> > > +    // Wipe out the transfer buffer in case it contains sensitive data.
> > > +    //
> > > +    ZeroMem (TransReq->AlignedDataBuf, TransReq-
> >AlignedDataBufSize);
> > > +    FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> > > (TransReq->AlignedDataBufSize));
> > > +    TransReq->AlignedDataBuf = NULL;
> > > +  }
> > > +}
> > > +
> > > +/**
> > > +  Prepare data buffer for transfer
> > > +
> > > +  @param[in]      Private   Pointer to the
> UFS_PASS_THRU_PRIVATE_DATA
> > > +  @param[in, out] TransReq  Pointer to the transfer request
> > > +
> > > +  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
> > > +  @retval EFI_SUCCESS       Buffer ready for transfer
> > > +**/
> > > +EFI_STATUS
> > > +UfsPrepareDataTransferBuffer (
> > > +  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
> > > +  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
> > > +  )
> > > +{
> > > +  EFI_STATUS                           Status;
> > > +  VOID                                 *DataBuf;
> > > +  UINT32                               DataLen;
> > > +  UINTN                                MapLength;
> > > +  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> > > +  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> > > +  UTP_TR_PRD                           *PrdtBase;
> > > +
> > > +  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;
> > > +  DataBuf        = NULL;
> > > +
> > > +  //
> > > +  // For unaligned data transfers we allocate auxiliary DWORD aligned
> > > memory pool.
> > > +  // When command is finished auxiliary memory pool is copied into
> > > + actual
> > > user memory.
> > > +  // This is requiered to assure data transfer safety(DWORD alignment
> > > required by UFS spec.)
> > > +  //
> > > +  if (TransReq->Packet->DataDirection ==
> > > EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > > +    if (((UINTN)TransReq->Packet->InDataBuffer % 4 != 0) ||
> > > + (TransReq-
> > > >Packet->InTransferLength % 4 != 0)) {
> > > +      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq-
> > > >Packet->InTransferLength % 4));
> > > +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> > > +      if (DataBuf == NULL) {
> > > +        return EFI_DEVICE_ERROR;
> > > +      }
> > > +      ZeroMem (DataBuf, DataLen);
> > > +      TransReq->AlignedDataBuf = DataBuf;
> > > +      TransReq->AlignedDataBufSize = DataLen;
> > > +    } else {
> > > +      DataLen       = TransReq->Packet->InTransferLength;
> > > +      DataBuf       = TransReq->Packet->InDataBuffer;
> > > +    }
> > > +    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> > > +  } else {
> > > +    if (((UINTN)TransReq->Packet->OutDataBuffer % 4 != 0) ||
> > > + (TransReq-
> > > >Packet->OutTransferLength % 4 != 0)) {
> > > +      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq-
> > > >Packet->OutTransferLength % 4));
> > > +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> > > +      if (DataBuf == NULL) {
> > > +        return EFI_DEVICE_ERROR;
> > > +      }
> > > +      CopyMem (DataBuf, TransReq->Packet->OutDataBuffer, TransReq-
> > > >Packet->OutTransferLength);
> > > +      TransReq->AlignedDataBuf = DataBuf;
> > > +      TransReq->AlignedDataBufSize = DataLen;
> > > +    } else {
> > > +      DataLen       = TransReq->Packet->OutTransferLength;
> > > +      DataBuf       = TransReq->Packet->OutDataBuffer;
> > > +    }
> > > +    Flag          = EdkiiUfsHcOperationBusMasterRead;
> > > +  }
> > > +
> > > +  if (DataLen != 0) {
> > > +    MapLength = DataLen;
> > > +    Status    = Private->UfsHostController->Map (
> > > +                                              Private->UfsHostController,
> > > +                                              Flag,
> > > +                                              DataBuf,
> > > +                                              &MapLength,
> > > +                                              &DataBufPhyAddr,
> > > +                                              &TransReq->DataBufMapping
> > > +                                              );
> > > +
> > > +    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> > > +      if (TransReq->AlignedDataBuf != NULL) {
> >
> >
> > Hello,
> >
> > I will also add
> >
> >   ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);
> >
> > here before freeing the auxiliary buffer when pushing the patch.
> > Please help to raise if there is any concern.
> >
> > With that,
> > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > > +        FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> > > (TransReq->AlignedDataBufSize));
> > > +        TransReq->AlignedDataBuf = NULL;
> > > +      }
> > > +      return EFI_DEVICE_ERROR;
> > > +    }
> > > +  }
> > > +
> > > +  //
> > > +  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> > > +  //
> > > +  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> > > ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> > > (UTP_RESPONSE_UPIU)));
> > > +  ASSERT (PrdtBase != NULL);
> > > +  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > >  /**
> > >    Sends a UFS-supported SCSI Request Packet to a UFS device that is
> > > attached to the UFS host controller.
> > >
> > > @@ -1353,24 +1482,19 @@ UfsExecScsiCmds (
> > >    UTP_RESPONSE_UPIU                    *Response;
> > >    UINT16                               SenseDataLen;
> > >    UINT32                               ResTranCount;
> > > -  VOID                                 *DataBuf;
> > > -  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> > > -  UINT32                               DataLen;
> > > -  UINTN                                MapLength;
> > > -  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> > > -  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> > > -  UTP_TR_PRD                           *PrdtBase;
> > >    EFI_TPL                              OldTpl;
> > >    UFS_PASS_THRU_TRANS_REQ              *TransReq;
> > > +  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> > >
> > > -  TransReq       = AllocateZeroPool (sizeof
> (UFS_PASS_THRU_TRANS_REQ));
> > > +  TransReq = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
> > >    if (TransReq == NULL) {
> > >      return EFI_OUT_OF_RESOURCES;
> > >    }
> > >
> > >    TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
> > >    TransReq->TimeoutRemain = Packet->Timeout;
> > > -  DataBufPhyAddr = 0;
> > > +  TransReq->Packet        = Packet;
> > > +
> > >    UfsHc          = Private->UfsHostController;
> > >    //
> > >    // Find out which slot of transfer request list is available.
> > > @@ -1399,44 +1523,16 @@ UfsExecScsiCmds (
> > >
> > >    TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) +
> > > TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
> > >
> > > -  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > > -    DataBuf       = Packet->InDataBuffer;
> > > -    DataLen       = Packet->InTransferLength;
> > > -    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> > > -  } else {
> > > -    DataBuf       = Packet->OutDataBuffer;
> > > -    DataLen       = Packet->OutTransferLength;
> > > -    Flag          = EdkiiUfsHcOperationBusMasterRead;
> > > -  }
> > > -
> > > -  if (DataLen != 0) {
> > > -    MapLength = DataLen;
> > > -    Status    = UfsHc->Map (
> > > -                         UfsHc,
> > > -                         Flag,
> > > -                         DataBuf,
> > > -                         &MapLength,
> > > -                         &DataBufPhyAddr,
> > > -                         &TransReq->DataBufMapping
> > > -                         );
> > > -
> > > -    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> > > -      goto Exit1;
> > > -    }
> > > +  Status = UfsPrepareDataTransferBuffer (Private, TransReq);  if
> > > + (EFI_ERROR (Status)) {
> > > +    goto Exit1;
> > >    }
> > > -  //
> > > -  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> > > -  //
> > > -  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> > > ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> > > (UTP_RESPONSE_UPIU)));
> > > -  ASSERT (PrdtBase != NULL);
> > > -  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> > >
> > >    //
> > >    // Insert the async SCSI cmd to the Async I/O list
> > >    //
> > >    if (Event != NULL) {
> > >      OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > -    TransReq->Packet      = Packet;
> > >      TransReq->CallerEvent = Event;
> > >      InsertTailList (&Private->Queue, &TransReq->TransferList);
> > >      gBS->RestoreTPL (OldTpl);
> > > @@ -1515,9 +1611,7 @@ Exit:
> > >
> > >    UfsStopExecCmd (Private, TransReq->Slot);
> > >
> > > -  if (TransReq->DataBufMapping != NULL) {
> > > -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> > > -  }
> > > +  UfsReconcileDataTransferBuffer (Private, TransReq);
> > >
> > >  Exit1:
> > >    if (TransReq->CmdDescMapping != NULL) { @@ -1532,7 +1626,6 @@
> > > Exit1:
> > >    return Status;
> > >  }
> > >
> > > -
> > >  /**
> > >    Sent UIC DME_LINKSTARTUP command to start the link startup
> procedure.
> > >
> > > @@ -2100,7 +2193,6 @@ UfsControllerStop (
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > -
> > >  /**
> > >    Internal helper function which will signal the caller event and clean up
> > >    resources.
> > > @@ -2132,9 +2224,7 @@ SignalCallerEvent (
> > >
> > >    UfsStopExecCmd (Private, TransReq->Slot);
> > >
> > > -  if (TransReq->DataBufMapping != NULL) {
> > > -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> > > -  }
> > > +  UfsReconcileDataTransferBuffer (Private, TransReq);
> > >
> > >    if (TransReq->CmdDescMapping != NULL) {
> > >      UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
> > > --
> > > 2.14.1.windows.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
  2019-06-27  1:08     ` Wu, Hao A
@ 2019-07-01  1:09       ` Wu, Hao A
  0 siblings, 0 replies; 5+ messages in thread
From: Wu, Hao A @ 2019-07-01  1:09 UTC (permalink / raw)
  To: devel@edk2.groups.io, Wu, Hao A, Albecki, Mateusz

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wu, Hao A
> Sent: Thursday, June 27, 2019 9:08 AM
> To: Albecki, Mateusz; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix
> unaligned data transfer handling
> 
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Wednesday, June 26, 2019 5:55 PM
> > To: Wu, Hao A; devel@edk2.groups.io
> > Subject: RE: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned
> data
> > transfer handling
> >
> > Hi,
> >
> > No concerns from my side. I just didn't think ZeroMem would be useful
> > there since buffer doesn't contain any data from device at that point so I
> > didn't add it.
> 
> 
> My concern is that the data in 'Packet->OutDataBuffer' (from caller) may
> contain information.
> 
> Caller can clean up the content in 'Packet->OutDataBuffer' after the
> PassThru call but cannot control the auxiliary buffer within UFS driver.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Thanks,
> > Mateusz
> >
> > > -----Original Message-----
> > > From: Wu, Hao A
> > > Sent: Wednesday, June 26, 2019 2:19 AM
> > > To: Albecki, Mateusz <mateusz.albecki@intel.com>;
> devel@edk2.groups.io
> > > Subject: RE: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned
> > data
> > > transfer handling
> > >
> > > > -----Original Message-----
> > > > From: Albecki, Mateusz
> > > > Sent: Tuesday, June 25, 2019 5:44 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Albecki, Mateusz; Wu, Hao A
> > > > Subject: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned
> data
> > > > transfer handling
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1341
> > > >
> > > > Since UFS specification requirs the data buffer specified in PRDT to
> > > > be DWORD aligned in size we had a code in UfsInitUtpPrdt that aligned
> > > > the data buffer by rounding down the buffer size to DWORD boundary.
> > > > This meant that for SCSI commands that wanted to perform unaligned
> > > > data transfer(such as SENSE command) we specified to small buffer for
> > > > the data to fit and transfer was aborted. This change introduces code
> > > > that allocates auxiliary DWORD aligned data buffer for unaligned
> > > > transfer. Device transfers data to aligned buffer and when data
> > > > transfer is over driver copies data from aligned buffer to data buffer
> > > > passed by user.
> > > >
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> > > > ---
> > > >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   2 +
> > > >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 198
> > > +++++++++++++++-
> > > > -----
> > > >  2 files changed, 146 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > > index 6c98b3a76a..9b68db5ffe 100644
> > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > > @@ -92,6 +92,8 @@ typedef struct {
> > > >    UINT32                                        CmdDescSize;
> > > >    VOID                                          *CmdDescHost;
> > > >    VOID                                          *CmdDescMapping;
> > > > +  VOID                                          *AlignedDataBuf;
> > > > +  UINTN                                         AlignedDataBufSize;
> > > >    VOID                                          *DataBufMapping;
> > > >
> > > >    EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
> > > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > > index 4b93821f38..4f41183504 100644
> > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > > @@ -394,17 +394,13 @@ UfsInitUtpPrdt (
> > > >    UINT8      *Remaining;
> > > >    UINTN      PrdtNumber;
> > > >
> > > > -  if ((BufferSize & (BIT0 | BIT1)) != 0) {
> > > > -    BufferSize &= ~(BIT0 | BIT1);
> > > > -    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not
> > > > dword-aligned!\n", BufferSize));
> > > > -  }
> > > > +  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);  ASSERT ((BufferSize
> > > > + & (BIT1 | BIT0)) == 0);
> > > >
> > > >    if (BufferSize == 0) {
> > > >      return EFI_SUCCESS;
> > > >    }
> > > >
> > > > -  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
> > > > -
> > > >    RemainingLen = BufferSize;
> > > >    Remaining    = Buffer;
> > > >    PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize +
> > > > UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
> > > @@ -1317,6
> > > > +1313,139 @@ Exit:
> > > >    return Status;
> > > >  }
> > > >
> > > > +/**
> > > > +  Cleanup data buffers after data transfer. This function
> > > > +  also takes care to copy all data to user memory pool for
> > > > +  unaligned data transfers.
> > > > +
> > > > +  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
> > > > +  @param[in] TransReq  Pointer to the transfer request **/ VOID
> > > > +UfsReconcileDataTransferBuffer (
> > > > +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
> > > > +  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
> > > > +  )
> > > > +{
> > > > +  if (TransReq->DataBufMapping != NULL) {
> > > > +    Private->UfsHostController->Unmap (
> > > > +                                  Private->UfsHostController,
> > > > +                                  TransReq->DataBufMapping
> > > > +                                  );
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Check if unaligned transfer was performed. If it was and we read
> > > > + // data from device copy memory to user data buffers before cleanup.
> > > > +  // The assumption is if auxiliary aligned data buffer is not NULL
> > > > + then  // unaligned transfer has been performed.
> > > > +  //
> > > > +  if (TransReq->AlignedDataBuf != NULL) {
> > > > +    if (TransReq->Packet->DataDirection ==
> > > > EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > > > +      CopyMem (TransReq->Packet->InDataBuffer, TransReq-
> > > > >AlignedDataBuf, TransReq->Packet->InTransferLength);
> > > > +    }
> > > > +    //
> > > > +    // Wipe out the transfer buffer in case it contains sensitive data.
> > > > +    //
> > > > +    ZeroMem (TransReq->AlignedDataBuf, TransReq-
> > >AlignedDataBufSize);
> > > > +    FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES
> > > > (TransReq->AlignedDataBufSize));
> > > > +    TransReq->AlignedDataBuf = NULL;
> > > > +  }
> > > > +}
> > > > +
> > > > +/**
> > > > +  Prepare data buffer for transfer
> > > > +
> > > > +  @param[in]      Private   Pointer to the
> > UFS_PASS_THRU_PRIVATE_DATA
> > > > +  @param[in, out] TransReq  Pointer to the transfer request
> > > > +
> > > > +  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
> > > > +  @retval EFI_SUCCESS       Buffer ready for transfer
> > > > +**/
> > > > +EFI_STATUS
> > > > +UfsPrepareDataTransferBuffer (
> > > > +  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
> > > > +  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
> > > > +  )
> > > > +{
> > > > +  EFI_STATUS                           Status;
> > > > +  VOID                                 *DataBuf;
> > > > +  UINT32                               DataLen;
> > > > +  UINTN                                MapLength;
> > > > +  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> > > > +  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> > > > +  UTP_TR_PRD                           *PrdtBase;
> > > > +
> > > > +  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;


Changed the above line from:
DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;

to:
DataBufPhyAddr = 0;

to address a build failure for GCC.

With this change and my previous 'Reviewed-by' tag, pushed via commit a37e18f6fc.

Best Regards,
Hao Wu


> > > > +  DataBuf        = NULL;
> > > > +
> > > > +  //
> > > > +  // For unaligned data transfers we allocate auxiliary DWORD aligned
> > > > memory pool.
> > > > +  // When command is finished auxiliary memory pool is copied into
> > > > + actual
> > > > user memory.
> > > > +  // This is requiered to assure data transfer safety(DWORD alignment
> > > > required by UFS spec.)
> > > > +  //
> > > > +  if (TransReq->Packet->DataDirection ==
> > > > EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> > > > +    if (((UINTN)TransReq->Packet->InDataBuffer % 4 != 0) ||
> > > > + (TransReq-
> > > > >Packet->InTransferLength % 4 != 0)) {
> > > > +      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq-
> > > > >Packet->InTransferLength % 4));
> > > > +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> > > > +      if (DataBuf == NULL) {
> > > > +        return EFI_DEVICE_ERROR;
> > > > +      }
> > > > +      ZeroMem (DataBuf, DataLen);
> > > > +      TransReq->AlignedDataBuf = DataBuf;
> > > > +      TransReq->AlignedDataBufSize = DataLen;
> > > > +    } else {
> > > > +      DataLen       = TransReq->Packet->InTransferLength;
> > > > +      DataBuf       = TransReq->Packet->InDataBuffer;
> > > > +    }
> > > > +    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> > > > +  } else {
> > > > +    if (((UINTN)TransReq->Packet->OutDataBuffer % 4 != 0) ||
> > > > + (TransReq-
> > > > >Packet->OutTransferLength % 4 != 0)) {
> > > > +      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq-
> > > > >Packet->OutTransferLength % 4));
> > > > +      DataBuf = AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4);
> > > > +      if (DataBuf == NULL) {
> > > > +        return EFI_DEVICE_ERROR;
> > > > +      }
> > > > +      CopyMem (DataBuf, TransReq->Packet->OutDataBuffer, TransReq-
> > > > >Packet->OutTransferLength);
> > > > +      TransReq->AlignedDataBuf = DataBuf;
> > > > +      TransReq->AlignedDataBufSize = DataLen;
> > > > +    } else {
> > > > +      DataLen       = TransReq->Packet->OutTransferLength;
> > > > +      DataBuf       = TransReq->Packet->OutDataBuffer;
> > > > +    }
> > > > +    Flag          = EdkiiUfsHcOperationBusMasterRead;
> > > > +  }
> > > > +
> > > > +  if (DataLen != 0) {
> > > > +    MapLength = DataLen;
> > > > +    Status    = Private->UfsHostController->Map (
> > > > +                                              Private->UfsHostController,
> > > > +                                              Flag,
> > > > +                                              DataBuf,
> > > > +                                              &MapLength,
> > > > +                                              &DataBufPhyAddr,
> > > > +                                              &TransReq->DataBufMapping
> > > > +                                              );
> > > > +
> > > > +    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> > > > +      if (TransReq->AlignedDataBuf != NULL) {
> > >
> > >
> > > Hello,
> > >
> > > I will also add
> > >
> > >   ZeroMem (TransReq->AlignedDataBuf, TransReq->AlignedDataBufSize);
> > >
> > > here before freeing the auxiliary buffer when pushing the patch.
> > > Please help to raise if there is any concern.
> > >
> > > With that,
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > > +        FreeAlignedPages (TransReq->AlignedDataBuf,
> EFI_SIZE_TO_PAGES
> > > > (TransReq->AlignedDataBufSize));
> > > > +        TransReq->AlignedDataBuf = NULL;
> > > > +      }
> > > > +      return EFI_DEVICE_ERROR;
> > > > +    }
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> > > > +  //
> > > > +  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> > > > ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> > > > (UTP_RESPONSE_UPIU)));
> > > > +  ASSERT (PrdtBase != NULL);
> > > > +  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr,
> DataLen);
> > > > +
> > > > +  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > >  /**
> > > >    Sends a UFS-supported SCSI Request Packet to a UFS device that is
> > > > attached to the UFS host controller.
> > > >
> > > > @@ -1353,24 +1482,19 @@ UfsExecScsiCmds (
> > > >    UTP_RESPONSE_UPIU                    *Response;
> > > >    UINT16                               SenseDataLen;
> > > >    UINT32                               ResTranCount;
> > > > -  VOID                                 *DataBuf;
> > > > -  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
> > > > -  UINT32                               DataLen;
> > > > -  UINTN                                MapLength;
> > > > -  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> > > > -  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
> > > > -  UTP_TR_PRD                           *PrdtBase;
> > > >    EFI_TPL                              OldTpl;
> > > >    UFS_PASS_THRU_TRANS_REQ              *TransReq;
> > > > +  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> > > >
> > > > -  TransReq       = AllocateZeroPool (sizeof
> > (UFS_PASS_THRU_TRANS_REQ));
> > > > +  TransReq = AllocateZeroPool (sizeof
> (UFS_PASS_THRU_TRANS_REQ));
> > > >    if (TransReq == NULL) {
> > > >      return EFI_OUT_OF_RESOURCES;
> > > >    }
> > > >
> > > >    TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
> > > >    TransReq->TimeoutRemain = Packet->Timeout;
> > > > -  DataBufPhyAddr = 0;
> > > > +  TransReq->Packet        = Packet;
> > > > +
> > > >    UfsHc          = Private->UfsHostController;
> > > >    //
> > > >    // Find out which slot of transfer request list is available.
> > > > @@ -1399,44 +1523,16 @@ UfsExecScsiCmds (
> > > >
> > > >    TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) +
> > > > TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
> > > >
> > > > -  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ)
> {
> > > > -    DataBuf       = Packet->InDataBuffer;
> > > > -    DataLen       = Packet->InTransferLength;
> > > > -    Flag          = EdkiiUfsHcOperationBusMasterWrite;
> > > > -  } else {
> > > > -    DataBuf       = Packet->OutDataBuffer;
> > > > -    DataLen       = Packet->OutTransferLength;
> > > > -    Flag          = EdkiiUfsHcOperationBusMasterRead;
> > > > -  }
> > > > -
> > > > -  if (DataLen != 0) {
> > > > -    MapLength = DataLen;
> > > > -    Status    = UfsHc->Map (
> > > > -                         UfsHc,
> > > > -                         Flag,
> > > > -                         DataBuf,
> > > > -                         &MapLength,
> > > > -                         &DataBufPhyAddr,
> > > > -                         &TransReq->DataBufMapping
> > > > -                         );
> > > > -
> > > > -    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
> > > > -      goto Exit1;
> > > > -    }
> > > > +  Status = UfsPrepareDataTransferBuffer (Private, TransReq);  if
> > > > + (EFI_ERROR (Status)) {
> > > > +    goto Exit1;
> > > >    }
> > > > -  //
> > > > -  // Fill PRDT table of Command UPIU for executed SCSI cmd.
> > > > -  //
> > > > -  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost +
> > > > ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof
> > > > (UTP_RESPONSE_UPIU)));
> > > > -  ASSERT (PrdtBase != NULL);
> > > > -  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
> > > >
> > > >    //
> > > >    // Insert the async SCSI cmd to the Async I/O list
> > > >    //
> > > >    if (Event != NULL) {
> > > >      OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > -    TransReq->Packet      = Packet;
> > > >      TransReq->CallerEvent = Event;
> > > >      InsertTailList (&Private->Queue, &TransReq->TransferList);
> > > >      gBS->RestoreTPL (OldTpl);
> > > > @@ -1515,9 +1611,7 @@ Exit:
> > > >
> > > >    UfsStopExecCmd (Private, TransReq->Slot);
> > > >
> > > > -  if (TransReq->DataBufMapping != NULL) {
> > > > -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> > > > -  }
> > > > +  UfsReconcileDataTransferBuffer (Private, TransReq);
> > > >
> > > >  Exit1:
> > > >    if (TransReq->CmdDescMapping != NULL) { @@ -1532,7 +1626,6 @@
> > > > Exit1:
> > > >    return Status;
> > > >  }
> > > >
> > > > -
> > > >  /**
> > > >    Sent UIC DME_LINKSTARTUP command to start the link startup
> > procedure.
> > > >
> > > > @@ -2100,7 +2193,6 @@ UfsControllerStop (
> > > >    return EFI_SUCCESS;
> > > >  }
> > > >
> > > > -
> > > >  /**
> > > >    Internal helper function which will signal the caller event and clean up
> > > >    resources.
> > > > @@ -2132,9 +2224,7 @@ SignalCallerEvent (
> > > >
> > > >    UfsStopExecCmd (Private, TransReq->Slot);
> > > >
> > > > -  if (TransReq->DataBufMapping != NULL) {
> > > > -    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
> > > > -  }
> > > > +  UfsReconcileDataTransferBuffer (Private, TransReq);
> > > >
> > > >    if (TransReq->CmdDescMapping != NULL) {
> > > >      UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
> > > > --
> > > > 2.14.1.windows.1
> 
> 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-07-01  1:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-25  9:44 [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz
2019-06-26  0:19 ` Wu, Hao A
2019-06-26  9:54   ` Albecki, Mateusz
2019-06-27  1:08     ` Wu, Hao A
2019-07-01  1:09       ` [edk2-devel] " Wu, Hao A

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox