> -----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...@intel.com>
> Signed-off-by: Mateusz Albecki <mateusz.albe...@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...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42840): https://edk2.groups.io/g/devel/message/42840
Mute This Topic: https://groups.io/mt/32201696/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
