> -----Original Message----- > From: Albecki, Mateusz > Sent: Wednesday, June 26, 2019 5:55 PM > To: Wu, Hao A; devel@edk2.groups.io > Subject: RE: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data > transfer handling > > 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.
My concern is that the data in 'Packet->OutDataBuffer' (from caller) may contain information. Caller can clean up the content in 'Packet->OutDataBuffer' after the PassThru call but cannot control the auxiliary buffer within UFS driver. Best Regards, Hao Wu > > Thanks, > Mateusz > > > -----Original Message----- > > From: Wu, Hao A > > Sent: Wednesday, June 26, 2019 2:19 AM > > To: Albecki, Mateusz <mateusz.albe...@intel.com>; 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=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 | 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)) != 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,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 != 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); > > > + } > > > + // > > > + // 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 = 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) { > > > > > > 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 <hao.a...@intel.com> > > > > Best Regards, > > Hao Wu > > > > > > > + FreeAlignedPages (TransReq->AlignedDataBuf, EFI_SIZE_TO_PAGES > > > (TransReq->AlignedDataBufSize)); > > > + 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 +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 = 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 +1523,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 +1611,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 +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 clean > > > up > > > resources. > > > @@ -2132,9 +2224,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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42906): https://edk2.groups.io/g/devel/message/42906 Mute This Topic: https://groups.io/mt/32201696/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
