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.120, mailfrom: mateusz.albecki@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Tue, 25 Jun 2019 02:49:40 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2019 02:49:40 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,415,1557212400"; d="scan'208";a="169731378" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by FMSMGA003.fm.intel.com with ESMTP; 25 Jun 2019 02:49:39 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.159]) by IRSMSX101.ger.corp.intel.com ([169.254.1.80]) with mapi id 14.03.0439.000; Tue, 25 Jun 2019 10:49:38 +0100 From: "Albecki, Mateusz" To: "devel@edk2.groups.io" , "Albecki, Mateusz" CC: "Wu, Hao A" Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Thread-Topic: [edk2-devel] [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Thread-Index: AQHVKzrBeEONUw63uUWnf2VKp0Xc3aasH8ZA Date: Tue, 25 Jun 2019 09:49:37 +0000 Message-ID: <92CF190FF2351747A47C1708F0E09C0875E72F46@IRSMSX102.ger.corp.intel.com> References: <15AB67DFED2AC715.19814@groups.io> In-Reply-To: <15AB67DFED2AC715.19814@groups.io> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWRhZDMxNzUtNzRlNS00MGY0LWJiODgtMmJhYTkzOWM2ZmZkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiK1p3TG5WcHc4NVVWMTB5d3k0Y1dGbUdcL3hTWW5kZ2NTaEczdmtzR1R3TmtZNGF3YVluN0V2alwvK2lsSTJFNXBmIn0= dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.182] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Fixed the review comments from v1. Test info is the same as on v1. UFS enum= eration is passing and I can see LUs enumerated in efi shell. > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Albecki, Mateusz > Sent: Tuesday, June 25, 2019 11:44 AM > To: devel@edk2.groups.io > Cc: Albecki, Mateusz ; Wu, Hao A > > Subject: [edk2-devel] [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 d= ata > 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 dat= a > to fit and transfer was aborted. This change introduces code that alloca= tes > auxiliary DWORD aligned data buffer for unaligned transfer. Device trans= fers > 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 ((BufferSi= ze & > + (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 actu= al > 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) || (TransRe= q- > >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->UfsHostControlle= r, > + Flag, > + DataBuf, > + &MapLength, > + &DataBufPhyAddr, > + &TransReq->DataBufMapping > + ); > + > + if (EFI_ERROR (Status) || (DataLen !=3D MapLength)) { > + if (TransReq->AlignedDataBuf !=3D NULL) { > + 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 >=20 > -------------------------------------------------------------------- >=20 > 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. >=20 > 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 th= e > sole use of the intended recipient(s). If you are not the intended recip= ient, > please contact the sender and delete all copies; any review or distribut= ion by > others is strictly prohibited. >=20 >=20 >=20 -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wyd= zial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-3= 16 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresat= a i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej w= iadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakie= kolwiek 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 recipien= t, please contact the sender and delete all copies; any review or distribut= ion by others is strictly prohibited.