> -----Original Message----- > From: Albecki, Mateusz > Sent: Monday, June 24, 2019 7:39 PM > To: devel@edk2.groups.io; Albecki, Mateusz > Cc: Wu, Hao A > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Fix > unaligned data transfer handling > > Test info: > > Tested UFS enumeration in EFI shell and it is passing on real HW with > Samsung device. > Tested read/write to UFS device in EFI shell.
Thanks for the information. Two comments below with regard to the cleanup of the auxiliary buffer. With these handled, Reviewed-by: Hao A Wu <hao.a...@intel.com> > > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Albecki, Mateusz > > Sent: Friday, June 21, 2019 5:20 PM > > To: devel@edk2.groups.io > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Wu, Hao A > > <hao.a...@intel.com> > > Subject: [edk2-devel] [PATCH] 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 | 194 > +++++++++++++++- > > ----- > > 2 files changed, 142 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..6eb50f09ac 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,135 @@ 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); > > + } Please help to wipe out the content in the 'TransReq->AlignedDataBuf' before freeing. It may contain information that is not intended to remain in memory. > > + 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) { > > + FreePages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES > > (TransReq->AlignedDataBufSize)); I think FreeAlignedPages() should be used here. Also, please help to wipe out the content in 'TransReq->AlignedDataBuf'. Best Regards, Hao Wu > > + 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 +1478,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 +1519,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 +1607,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 +1622,6 @@ Exit1: > > return Status; > > } > > > > - > > /** > > Sent UIC DME_LINKSTARTUP command to start the link startup procedure. > > > > @@ -2100,7 +2189,6 @@ UfsControllerStop ( > > return EFI_SUCCESS; > > } > > > > - > > /** > > Internal helper function which will signal the caller event and clean up > > resources. > > @@ -2132,9 +2220,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 > > > > -------------------------------------------------------------------- > > > > 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. > > > > 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 the > > sole use of the intended recipient(s). If you are not the intended > > recipient, > > please contact the sender and delete all copies; any review or distribution > by > > others is strictly prohibited. > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42771): https://edk2.groups.io/g/devel/message/42771 Mute This Topic: https://groups.io/mt/32159485/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
