[edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
Wu, Hao A
hao.a.wu at intel.com
Tue Jun 25 01:44:46 UTC 2019
> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Monday, June 24, 2019 7:39 PM
> To: devel at 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.wu at intel.com>
>
> > -----Original Message-----
> > From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> > Albecki, Mateusz
> > Sent: Friday, June 21, 2019 5:20 PM
> > To: devel at edk2.groups.io
> > Cc: Albecki, Mateusz <mateusz.albecki at intel.com>; Wu, Hao A
> > <hao.a.wu at 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.wu at intel.com>
> > Signed-off-by: Mateusz Albecki <mateusz.albecki at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list