From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: hao.a.wu@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Tue, 25 Jun 2019 17:19:22 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2019 17:19:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,417,1557212400"; d="scan'208";a="360565921" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 25 Jun 2019 17:19:22 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 25 Jun 2019 17:19:21 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.185]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.156]) with mapi id 14.03.0439.000; Wed, 26 Jun 2019 08:19:19 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" Subject: Re: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Thread-Topic: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Thread-Index: AQHVKzquduOtylR5mUy+79XOf9PBoaatElpg Date: Wed, 26 Jun 2019 00:19:19 +0000 Message-ID: References: <20190625094428.600-1-mateusz.albecki@intel.com> In-Reply-To: <20190625094428.600-1-mateusz.albecki@intel.com> 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 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1341 >=20 > 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. >=20 > Cc: Hao A Wu > Signed-off-by: Mateusz Albecki > --- > MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h | 2 + > .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 198 +++++++++++++++= - > ----- > 2 files changed, 146 insertions(+), 54 deletions(-) >=20 > 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; >=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 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; >=20 > - if ((BufferSize & (BIT0 | BIT1)) !=3D 0) { > - BufferSize &=3D ~(BIT0 | BIT1); > - DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not > dword-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); > @@ -1317,6 +1313,139 @@ 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 auxiliary 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); > + } > + // > + // 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 =3D 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 =3D (EFI_PHYSICAL_ADDRESS) NULL; > + DataBuf =3D NULL; > + > + // > + // For unaligned data transfers we allocate auxiliary DWORD aligned > memory pool. > + // When command is finished auxiliary memory pool is copied into actua= l > 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 (((UINTN)TransReq->Packet->InDataBuffer % 4 !=3D 0) || (TransReq- > >Packet->InTransferLength % 4 !=3D 0)) { > + DataLen =3D TransReq->Packet->InTransferLength + (4 - (TransReq- > >Packet->InTransferLength % 4)); > + DataBuf =3D AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4); > + if (DataBuf =3D=3D NULL) { > + return EFI_DEVICE_ERROR; > + } > + ZeroMem (DataBuf, DataLen); > + TransReq->AlignedDataBuf =3D DataBuf; > + TransReq->AlignedDataBufSize =3D DataLen; > + } else { > + DataLen =3D TransReq->Packet->InTransferLength; > + DataBuf =3D TransReq->Packet->InDataBuffer; > + } > + Flag =3D EdkiiUfsHcOperationBusMasterWrite; > + } else { > + if (((UINTN)TransReq->Packet->OutDataBuffer % 4 !=3D 0) || (TransReq= - > >Packet->OutTransferLength % 4 !=3D 0)) { > + DataLen =3D TransReq->Packet->OutTransferLength + (4 - (TransReq- > >Packet->OutTransferLength % 4)); > + DataBuf =3D AllocateAlignedPages (EFI_SIZE_TO_PAGES (DataLen), 4); > + if (DataBuf =3D=3D NULL) { > + return EFI_DEVICE_ERROR; > + } > + CopyMem (DataBuf, TransReq->Packet->OutDataBuffer, TransReq- > >Packet->OutTransferLength); > + TransReq->AlignedDataBuf =3D DataBuf; > + TransReq->AlignedDataBufSize =3D DataLen; > + } 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)) { > + if (TransReq->AlignedDataBuf !=3D 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 Best Regards, Hao Wu > + FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES > (TransReq->AlignedDataBufSize)); > + TransReq->AlignedDataBuf =3D NULL; > + } > + return EFI_DEVICE_ERROR; > + } > + } > + > + // > + // 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 > attached to the UFS host controller. >=20 > @@ -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; >=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. > @@ -1399,44 +1523,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); > @@ -1515,9 +1611,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) { > @@ -1532,7 +1626,6 @@ Exit1: > return Status; > } >=20 > - > /** > Sent UIC DME_LINKSTARTUP command to start the link startup procedure. >=20 > @@ -2100,7 +2193,6 @@ UfsControllerStop ( > return EFI_SUCCESS; > } >=20 > - > /** > Internal helper function which will signal the caller event and clean = up > resources. > @@ -2132,9 +2224,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