public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Albecki, Mateusz" <mateusz.albecki@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Subject: Re: [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
Date: Thu, 27 Jun 2019 01:08:16 +0000	[thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C8F48F5@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <92CF190FF2351747A47C1708F0E09C0875E74292@IRSMSX102.ger.corp.intel.com>

> -----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.albecki@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.wu@intel.com>
> > > Signed-off-by: Mateusz Albecki <mateusz.albecki@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.wu@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


  reply	other threads:[~2019-06-27  1:08 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-25  9:44 [PATCH v2] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz
2019-06-26  0:19 ` Wu, Hao A
2019-06-26  9:54   ` Albecki, Mateusz
2019-06-27  1:08     ` Wu, Hao A [this message]
2019-07-01  1:09       ` [edk2-devel] " Wu, Hao A

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C8F48F5@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox