From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 58499211EA9EC for ; Sun, 31 Mar 2019 23:53:14 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Mar 2019 23:53:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,295,1549958400"; d="scan'208";a="219372714" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 31 Mar 2019 23:53:12 -0700 Received: from fmsmsx161.amr.corp.intel.com (10.18.125.9) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 31 Mar 2019 23:53:13 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX161.amr.corp.intel.com (10.18.125.9) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 31 Mar 2019 23:53:13 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.164]) with mapi id 14.03.0415.000; Mon, 1 Apr 2019 14:53:10 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Thread-Index: AQHU5W4L+Xa0BbGi/k6+8D67mPtmLaYiFlHwgAAzbEA= Date: Mon, 1 Apr 2019 06:53:10 +0000 Message-ID: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Apr 2019 06:53:15 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hello Mateusz, There has been a Bugzilla tracker for the issue you addressed in this patch: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1341 Could you help to add this information in the commit log message? Thanks. > -----Original Message----- > From: Albecki, Mateusz > Sent: Thursday, March 28, 2019 9:56 PM > To: edk2-devel@lists.01.org > Cc: Albecki, Mateusz; Wu, Hao A > Subject: [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data > transfer handling >=20 > Since UFS spcification requiers the data buffer specified specification -> specification requiers -> requires > 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 > auxilary DWORD aligned data buffer for unaligned transfer. Device auxilary -> auxiliary There are some 'auxilary' typos within the update in file UfsPassThruHci.c as well, please help to update them. > transfers data to aligned buffer and when data transfer is over driver co= pies The above line seems too long. Please help to refine it under 76 chars. > data from aligned buffer to data buffer passed by user. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Cc: Hao Wu > Signed-off-by: Albecki Mateusz > --- > MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h | 1 + > .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 190 +++++++++++++++= ----- > - > 2 files changed, 135 insertions(+), 56 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > index b8937842b8..e591e78661 100644 > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > @@ -98,6 +98,7 @@ typedef struct { > UINT32 CmdDescSize; > VOID *CmdDescHost; > VOID *CmdDescMapping; > + VOID *AlignedDataBuf; > VOID *DataBufMapping; >=20 > 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 0238264878..9d0793c6be 100644 > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > @@ -400,17 +400,13 @@ UfsInitUtpPrdt ( > UINT8 *Remaining; > UINTN PrdtNumber; >=20 > - if ((BufferSize & (BIT0 | BIT1)) !=3D 0) { > - BufferSize &=3D ~(BIT0 | BIT1); > - DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not dwor= d- > aligned!\n", BufferSize)); > - } > + ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) =3D=3D 0); > + ASSERT ((BufferSize & (BIT1 | BIT0)) =3D=3D 0); >=20 > if (BufferSize =3D=3D 0) { > return EFI_SUCCESS; > } >=20 > - ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) =3D=3D 0); > - > RemainingLen =3D BufferSize; > Remaining =3D Buffer; > PrdtNumber =3D (UINTN)DivU64x32 ((UINT64)BufferSize + > UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD); > @@ -1212,8 +1208,6 @@ UfsSetFlag ( > return Status; > } >=20 > - > - > /** > Read specified flag from a UFS device. >=20 > @@ -1323,6 +1317,129 @@ Exit: > return Status; > } >=20 > +/** > + 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 !=3D 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 auxilary aligned data buffer is not NULL then > + // unaligned transfer has been performed. > + // > + if (TransReq->AlignedDataBuf !=3D NULL) { > + if (TransReq->Packet->DataDirection =3D=3D > EFI_EXT_SCSI_DATA_DIRECTION_READ) { > + CopyMem (TransReq->Packet->InDataBuffer, TransReq->AlignedDataBuf, > TransReq->Packet->InTransferLength); > + } > + FreePool (TransReq->AlignedDataBuf); > + TransReq->AlignedDataBuf =3D NULL; > + } > +} > + > +/** > + Prepare data buffer for transfer > + > + @param[in] Private Pointer to the UFS_PASS_THRU_PRIVATE_DATA > + @param[in] TransReq Pointer to the transfer request Minor comment: @param[in, out] TransReq ... > + > + @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; > + VOID *AlignedDataBuf; 'AlignedDataBuf' seems not used here. Please help to remove it. > + UINT32 DataLen; > + UINTN MapLength; > + EFI_PHYSICAL_ADDRESS DataBufPhyAddr; > + EDKII_UFS_HOST_CONTROLLER_OPERATION Flag; > + UTP_TR_PRD *PrdtBase; > + > + DataBufPhyAddr =3D (EFI_PHYSICAL_ADDRESS) NULL; > + AlignedDataBuf =3D NULL; > + DataBuf =3D NULL; > + > + // > + // For unaligned data transfers we allocate auxilary DWORD aligned > memory pool. > + // When command is finished auxilary 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 =3D=3D > EFI_EXT_SCSI_DATA_DIRECTION_READ) { > + if (TransReq->Packet->InTransferLength % 4 !=3D 0) { > + DataLen =3D TransReq->Packet->InTransferLength + (4 - (TransReq->P= acket- > >InTransferLength % 4)); > + DataBuf =3D AllocateZeroPool (DataLen); The above allocated buffer may not meet the alignment requirement. It is possible to lead to the check failure within function UfsInitUtpPrdt(). Please consider using AllocateAlignedPages() and aligns to the 'IoAlign' field within EFI_EXT_SCSI_PASS_THRU_MODE. > + if (DataBuf =3D=3D NULL) { > + return EFI_DEVICE_ERROR; > + } > + TransReq->AlignedDataBuf =3D DataBuf; > + } else { > + DataLen =3D TransReq->Packet->InTransferLength; > + DataBuf =3D TransReq->Packet->InDataBuffer; > + } > + Flag =3D EdkiiUfsHcOperationBusMasterWrite; > + } else { > + if (TransReq->Packet->OutTransferLength % 4 !=3D 0) { > + DataLen =3D TransReq->Packet->OutTransferLength + (4 - (TransReq- > >Packet->OutTransferLength % 4)); > + DataBuf =3D AllocateCopyPool (TransReq->Packet->OutTransferLength, > TransReq->Packet->OutDataBuffer); It seems to me that the size of 'DataBuf' is not dword aligned. Also, similarly, the above allocated buffer may not meet the alignment requirement. > + if (DataBuf =3D=3D NULL) { > + return EFI_DEVICE_ERROR; > + } > + TransReq->AlignedDataBuf =3D DataBuf; > + } else { > + DataLen =3D TransReq->Packet->OutTransferLength; > + DataBuf =3D TransReq->Packet->OutDataBuffer; > + } > + Flag =3D EdkiiUfsHcOperationBusMasterRead; > + } > + > + if (DataLen !=3D 0) { > + MapLength =3D DataLen; > + Status =3D Private->UfsHostController->Map ( > + Private->UfsHostController= , > + Flag, > + DataBuf, > + &MapLength, > + &DataBufPhyAddr, > + &TransReq->DataBufMapping > + ); > + > + if (EFI_ERROR (Status) || (DataLen !=3D MapLength)) { > + return EFI_DEVICE_ERROR; Need to handle the free of the auxiliary buffer for this case, since it will not be covered by function UfsReconcileDataTransferBuffer(). Best Regards, Hao Wu > + } > + } > + > + // > + // Fill PRDT table of Command UPIU for executed SCSI cmd. > + // > + PrdtBase =3D (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8 > (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU))); > + ASSERT (PrdtBase !=3D NULL); > + UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen); > + > + return EFI_SUCCESS; > +} > + > /** > Sends a UFS-supported SCSI Request Packet to a UFS device that is atta= ched to > the UFS host controller. >=20 > @@ -1359,24 +1476,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; >=20 > - TransReq =3D AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ))= ; > + TransReq =3D AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ)); > if (TransReq =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } >=20 > TransReq->Signature =3D UFS_PASS_THRU_TRANS_REQ_SIG; > TransReq->TimeoutRemain =3D Packet->Timeout; > - DataBufPhyAddr =3D 0; > + TransReq->Packet =3D Packet; > + > UfsHc =3D Private->UfsHostController; > // > // Find out which slot of transfer request list is available. > @@ -1405,44 +1517,16 @@ UfsExecScsiCmds ( >=20 > TransReq->CmdDescSize =3D TransReq->Trd->PrdtO * sizeof (UINT32) + > TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD); >=20 > - if (Packet->DataDirection =3D=3D EFI_EXT_SCSI_DATA_DIRECTION_READ) { > - DataBuf =3D Packet->InDataBuffer; > - DataLen =3D Packet->InTransferLength; > - Flag =3D EdkiiUfsHcOperationBusMasterWrite; > - } else { > - DataBuf =3D Packet->OutDataBuffer; > - DataLen =3D Packet->OutTransferLength; > - Flag =3D EdkiiUfsHcOperationBusMasterRead; > - } > - > - if (DataLen !=3D 0) { > - MapLength =3D DataLen; > - Status =3D UfsHc->Map ( > - UfsHc, > - Flag, > - DataBuf, > - &MapLength, > - &DataBufPhyAddr, > - &TransReq->DataBufMapping > - ); > - > - if (EFI_ERROR (Status) || (DataLen !=3D MapLength)) { > - goto Exit1; > - } > + Status =3D UfsPrepareDataTransferBuffer (Private, TransReq); > + if (EFI_ERROR (Status)) { > + goto Exit1; > } > - // > - // Fill PRDT table of Command UPIU for executed SCSI cmd. > - // > - PrdtBase =3D (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8 > (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU))); > - ASSERT (PrdtBase !=3D NULL); > - UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen); >=20 > // > // Insert the async SCSI cmd to the Async I/O list > // > if (Event !=3D NULL) { > OldTpl =3D gBS->RaiseTPL (TPL_NOTIFY); > - TransReq->Packet =3D Packet; > TransReq->CallerEvent =3D Event; > InsertTailList (&Private->Queue, &TransReq->TransferList); > gBS->RestoreTPL (OldTpl); > @@ -1521,9 +1605,7 @@ Exit: >=20 > UfsStopExecCmd (Private, TransReq->Slot); >=20 > - if (TransReq->DataBufMapping !=3D NULL) { > - UfsHc->Unmap (UfsHc, TransReq->DataBufMapping); > - } > + UfsReconcileDataTransferBuffer (Private, TransReq); >=20 > Exit1: > if (TransReq->CmdDescMapping !=3D NULL) { > @@ -1538,7 +1620,6 @@ Exit1: > return Status; > } >=20 > - > /** > Sent UIC DME_LINKSTARTUP command to start the link startup procedure. >=20 > @@ -2106,7 +2187,6 @@ UfsControllerStop ( > return EFI_SUCCESS; > } >=20 > - > /** > Internal helper function which will signal the caller event and clean = up > resources. > @@ -2138,9 +2218,7 @@ SignalCallerEvent ( >=20 > UfsStopExecCmd (Private, TransReq->Slot); >=20 > - if (TransReq->DataBufMapping !=3D NULL) { > - UfsHc->Unmap (UfsHc, TransReq->DataBufMapping); > - } > + UfsReconcileDataTransferBuffer (Private, TransReq); >=20 > if (TransReq->CmdDescMapping !=3D NULL) { > UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping); > -- > 2.14.1.windows.1