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.93, mailfrom: mateusz.albecki@intel.com) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by groups.io with SMTP; Wed, 26 Jun 2019 02:55:02 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jun 2019 02:55:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,419,1557212400"; d="scan'208";a="184816300" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga004.fm.intel.com with ESMTP; 26 Jun 2019 02:55:00 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.159]) by IRSMSX106.ger.corp.intel.com ([169.254.8.222]) with mapi id 14.03.0439.000; Wed, 26 Jun 2019 10:54:59 +0100 From: "Albecki, Mateusz" To: "Wu, Hao A" , "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: AQHVKzqvuk+24/gQLUG84HJKBh0jx6atAl6AgACxMHA= Date: Wed, 26 Jun 2019 09:54:59 +0000 Message-ID: <92CF190FF2351747A47C1708F0E09C0875E74292@IRSMSX102.ger.corp.intel.com> References: <20190625094428.600-1-mateusz.albecki@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTAyMGUxOTItZDQ0NC00OThjLWE0MTgtOTcwNWEzODJhZjNhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoielRINXRNdklpMWtwRGtxbG1BaldkVmhJbUZveVhSb1lMSUZNUXljQm5IOG1ud2szd281Nk03eE5ZNzZWZGIzMyJ9 dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; 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=3D1341 > > > > 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 > > 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(-) > > > > 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)) !=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 ((BufferS= ize > > + & (BIT1 | BIT0)) =3D=3D 0); > > > > if (BufferSize =3D=3D 0) { > > return EFI_SUCCESS; > > } > > > > - 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; > > } > > > > +/** > > + 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 > > + 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 (((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->UfsHostControll= er, > > + 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. > > > > @@ -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 =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; > > } > > > > 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 ( > > > > TransReq->CmdDescSize =3D TransReq->Trd->PrdtO * sizeof (UINT32) + > > TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD); > > > > - 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); > > > > // > > // 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: > > > > UfsStopExecCmd (Private, TransReq->Slot); > > > > - if (TransReq->DataBufMapping !=3D NULL) { > > - UfsHc->Unmap (UfsHc, TransReq->DataBufMapping); > > - } > > + UfsReconcileDataTransferBuffer (Private, TransReq); > > > > Exit1: > > if (TransReq->CmdDescMapping !=3D 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 clea= n up > > resources. > > @@ -2132,9 +2224,7 @@ SignalCallerEvent ( > > > > UfsStopExecCmd (Private, TransReq->Slot); > > > > - if (TransReq->DataBufMapping !=3D NULL) { > > - UfsHc->Unmap (UfsHc, TransReq->DataBufMapping); > > - } > > + UfsReconcileDataTransferBuffer (Private, TransReq); > > > > if (TransReq->CmdDescMapping !=3D 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 Wydz= ial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-31= 6 | 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 wi= adomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiek= olwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the s= ole use of the intended recipient(s). If you are not the intended recipient= , please contact the sender and delete all copies; any review or distributi= on by others is strictly prohibited.