From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH V3 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
Date: Sat, 27 Oct 2018 07:47:10 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C846FC0@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <1540561286-112684-4-git-send-email-star.zeng@intel.com>
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 26, 2018 9:41 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star; Ni, Ruiyu; Wu, Hao A; Wang, Jian J; Yao, Jiewen
> Subject: [PATCH V3 3/4] MdeModulePkg XhciDxe: Use common buffer for
> AsyncInterruptTransfer
>
> V3:
> Call XhcFreeUrb after XhcCreateTransferTrb fails in XhcCreateTrb.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
>
> In current code, XhcMonitorAsyncRequests (timer handler) will do
> unmap and map operations for AsyncIntTransfers to "Flush data from
> PCI controller specific address to mapped system memory address".
> XhcMonitorAsyncRequests
> XhcFlushAsyncIntMap
> PciIo->Unmap
> IoMmu->SetAttribute
> PciIo->Map
> IoMmu->SetAttribute
>
> This may impact the boot performance.
>
> Since the data buffer for XhcMonitorAsyncRequests is internal
> buffer, we can allocate common buffer by PciIo->AllocateBuffer
> and map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
> then the unmap and map operations can be removed.
>
> ///
> /// Provides both read and write access to system memory by
> /// both the processor and a bus master. The buffer is coherent
> /// from both the processor's and the bus master's point of view.
> ///
> EfiPciIoOperationBusMasterCommonBuffer,
>
> Test done:
> USB KB works normally.
> USB disk read/write works normally.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
> MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 1 +
> MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 138 +++++++++++-------------
> -------
> MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 27 +++---
> 3 files changed, 63 insertions(+), 103 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 7f64f9c7c982..64855a4c158c 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -769,6 +769,7 @@ XhcTransfer (
> MaximumPacketLength,
> Type,
> Request,
> + FALSE,
> Data,
> *DataLength,
> NULL,
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 75959ae08363..9dd45e93a272 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -118,17 +118,18 @@ ON_EXIT:
> /**
> Create a new URB for a new transaction.
>
> - @param Xhc The XHCI Instance
> - @param BusAddr The logical device address assigned by UsbBus driver
> - @param EpAddr Endpoint addrress
> - @param DevSpeed The device speed
> - @param MaxPacket The max packet length of the endpoint
> - @param Type The transaction type
> - @param Request The standard USB request for control transfer
> - @param Data The user data to transfer
> - @param DataLen The length of data buffer
> - @param Callback The function to call when data is transferred
> - @param Context The context to the callback
> + @param Xhc The XHCI Instance
> + @param BusAddr The logical device address assigned by UsbBus
> driver
> + @param EpAddr Endpoint addrress
> + @param DevSpeed The device speed
> + @param MaxPacket The max packet length of the endpoint
> + @param Type The transaction type
> + @param Request The standard USB request for control transfer
> + @param AllocateCommonBuffer Indicate whether need to allocate
> common buffer for data transfer
> + @param Data The user data to transfer, NULL if
> AllocateCommonBuffer is TRUE
> + @param DataLen The length of data buffer
> + @param Callback The function to call when data is transferred
> + @param Context The context to the callback
>
> @return Created URB or NULL
>
> @@ -142,6 +143,7 @@ XhcCreateUrb (
> IN UINTN MaxPacket,
> IN UINTN Type,
> IN EFI_USB_DEVICE_REQUEST *Request,
> + IN BOOLEAN AllocateCommonBuffer,
> IN VOID *Data,
> IN UINTN DataLen,
> IN EFI_ASYNC_USB_TRANSFER_CALLBACK Callback,
> @@ -169,8 +171,24 @@ XhcCreateUrb (
> Ep->Type = Type;
>
> Urb->Request = Request;
> + if (AllocateCommonBuffer) {
> + ASSERT (Data == NULL);
> + Status = Xhc->PciIo->AllocateBuffer (
> + Xhc->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + EFI_SIZE_TO_PAGES (DataLen),
> + &Data,
> + 0
> + );
> + if (EFI_ERROR (Status) || (Data == NULL)) {
> + FreePool (Urb);
> + return NULL;
> + }
> + }
> Urb->Data = Data;
> Urb->DataLen = DataLen;
> + Urb->AllocateCommonBuffer = AllocateCommonBuffer;
> Urb->Callback = Callback;
> Urb->Context = Context;
>
> @@ -178,7 +196,7 @@ XhcCreateUrb (
> ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb Failed,
> Status = %r\n", Status));
> - FreePool (Urb);
> + XhcFreeUrb (Xhc, Urb);
> Urb = NULL;
> }
>
> @@ -206,6 +224,14 @@ XhcFreeUrb (
> Xhc->PciIo->Unmap (Xhc->PciIo, Urb->DataMap);
> }
>
> + if (Urb->AllocateCommonBuffer) {
> + Xhc->PciIo->FreeBuffer (
> + Xhc->PciIo,
> + EFI_SIZE_TO_PAGES (Urb->DataLen),
> + Urb->Data
> + );
> + }
> +
> FreePool (Urb);
> }
>
> @@ -264,10 +290,14 @@ XhcCreateTransferTrb (
> // No need to remap.
> //
> if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
> - if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> - MapOp = EfiPciIoOperationBusMasterWrite;
> + if (Urb->AllocateCommonBuffer) {
> + MapOp = EfiPciIoOperationBusMasterCommonBuffer;
> } else {
> - MapOp = EfiPciIoOperationBusMasterRead;
> + if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> + MapOp = EfiPciIoOperationBusMasterWrite;
> + } else {
> + MapOp = EfiPciIoOperationBusMasterRead;
> + }
> }
>
> Len = Urb->DataLen;
> @@ -1367,7 +1397,6 @@ XhciDelAsyncIntTransfer (
> }
>
> RemoveEntryList (&Urb->UrbList);
> - FreePool (Urb->Data);
> XhcFreeUrb (Xhc, Urb);
> return EFI_SUCCESS;
> }
> @@ -1405,7 +1434,6 @@ XhciDelAllAsyncIntTransfers (
> }
>
> RemoveEntryList (&Urb->UrbList);
> - FreePool (Urb->Data);
> XhcFreeUrb (Xhc, Urb);
> }
> }
> @@ -1438,15 +1466,8 @@ XhciInsertAsyncIntTransfer (
> IN VOID *Context
> )
> {
> - VOID *Data;
> URB *Urb;
>
> - Data = AllocateZeroPool (DataLen);
> - if (Data == NULL) {
> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> __FUNCTION__));
> - return NULL;
> - }
> -
> Urb = XhcCreateUrb (
> Xhc,
> BusAddr,
> @@ -1455,14 +1476,14 @@ XhciInsertAsyncIntTransfer (
> MaxPacket,
> XHC_INT_TRANSFER_ASYNC,
> NULL,
> - Data,
> + TRUE,
> + NULL,
> DataLen,
> Callback,
> Context
> );
> if (Urb == NULL) {
> DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> - FreePool (Data);
> return NULL;
> }
>
> @@ -1503,61 +1524,6 @@ XhcUpdateAsyncRequest (
> }
>
> /**
> - Flush data from PCI controller specific address to mapped system
> - memory address.
> -
> - @param Xhc The XHCI device.
> - @param Urb The URB to unmap.
> -
> - @retval EFI_SUCCESS Success to flush data to mapped system memory.
> - @retval EFI_DEVICE_ERROR Fail to flush data to mapped system memory.
> -
> -**/
> -EFI_STATUS
> -XhcFlushAsyncIntMap (
> - IN USB_XHCI_INSTANCE *Xhc,
> - IN URB *Urb
> - )
> -{
> - EFI_STATUS Status;
> - EFI_PHYSICAL_ADDRESS PhyAddr;
> - EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
> - EFI_PCI_IO_PROTOCOL *PciIo;
> - UINTN Len;
> - VOID *Map;
> -
> - PciIo = Xhc->PciIo;
> - Len = Urb->DataLen;
> -
> - if (Urb->Ep.Direction == EfiUsbDataIn) {
> - MapOp = EfiPciIoOperationBusMasterWrite;
> - } else {
> - MapOp = EfiPciIoOperationBusMasterRead;
> - }
> -
> - if (Urb->DataMap != NULL) {
> - Status = PciIo->Unmap (PciIo, Urb->DataMap);
> - if (EFI_ERROR (Status)) {
> - goto ON_ERROR;
> - }
> - }
> -
> - Urb->DataMap = NULL;
> -
> - Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
> - if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
> - goto ON_ERROR;
> - }
> -
> - Urb->DataPhy = (VOID *) ((UINTN) PhyAddr);
> - Urb->DataMap = Map;
> - return EFI_SUCCESS;
> -
> -ON_ERROR:
> - return EFI_DEVICE_ERROR;
> -}
> -
> -/**
> Interrupt transfer periodic check handler.
>
> @param Event Interrupt event.
> @@ -1577,7 +1543,6 @@ XhcMonitorAsyncRequests (
> UINT8 *ProcBuf;
> URB *Urb;
> UINT8 SlotId;
> - EFI_STATUS Status;
> EFI_TPL OldTpl;
>
> OldTpl = gBS->RaiseTPL (XHC_TPL);
> @@ -1606,15 +1571,6 @@ XhcMonitorAsyncRequests (
> }
>
> //
> - // Flush any PCI posted write transactions from a PCI host
> - // bridge to system memory.
> - //
> - Status = XhcFlushAsyncIntMap (Xhc, Urb);
> - if (EFI_ERROR (Status)) {
> - DEBUG ((EFI_D_ERROR, "XhcMonitorAsyncRequests: Fail to Flush
> AsyncInt Mapped Memeory\n"));
> - }
> -
> - //
> // Allocate a buffer then copy the transferred data for user.
> // If failed to allocate the buffer, update the URB for next
> // round of transfer. Ignore the data of this round.
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> index b5e192c3b589..1a95d98996c1 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> @@ -172,6 +172,7 @@ typedef struct _URB {
> //
> USB_ENDPOINT Ep;
> EFI_USB_DEVICE_REQUEST *Request;
> + BOOLEAN AllocateCommonBuffer;
> VOID *Data;
> UINTN DataLen;
> VOID *DataPhy;
> @@ -1432,17 +1433,18 @@ XhcSetTrDequeuePointer (
> /**
> Create a new URB for a new transaction.
>
> - @param Xhc The XHCI Instance
> - @param DevAddr The device address
> - @param EpAddr Endpoint addrress
> - @param DevSpeed The device speed
> - @param MaxPacket The max packet length of the endpoint
> - @param Type The transaction type
> - @param Request The standard USB request for control transfer
> - @param Data The user data to transfer
> - @param DataLen The length of data buffer
> - @param Callback The function to call when data is transferred
> - @param Context The context to the callback
> + @param Xhc The XHCI Instance
> + @param BusAddr The logical device address assigned by UsbBus
> driver
> + @param EpAddr Endpoint addrress
> + @param DevSpeed The device speed
> + @param MaxPacket The max packet length of the endpoint
> + @param Type The transaction type
> + @param Request The standard USB request for control transfer
> + @param AllocateCommonBuffer Indicate whether need to allocate
> common buffer for data transfer
> + @param Data The user data to transfer, NULL if
> AllocateCommonBuffer is TRUE
> + @param DataLen The length of data buffer
> + @param Callback The function to call when data is transferred
> + @param Context The context to the callback
>
> @return Created URB or NULL
>
> @@ -1450,12 +1452,13 @@ XhcSetTrDequeuePointer (
> URB*
> XhcCreateUrb (
> IN USB_XHCI_INSTANCE *Xhc,
> - IN UINT8 DevAddr,
> + IN UINT8 BusAddr,
> IN UINT8 EpAddr,
> IN UINT8 DevSpeed,
> IN UINTN MaxPacket,
> IN UINTN Type,
> IN EFI_USB_DEVICE_REQUEST *Request,
> + IN BOOLEAN AllocateCommonBuffer,
> IN VOID *Data,
> IN UINTN DataLen,
> IN EFI_ASYNC_USB_TRANSFER_CALLBACK Callback,
> --
> 2.7.0.windows.1
next prev parent reply other threads:[~2018-10-27 7:47 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-26 13:41 [PATCH V3 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
2018-10-26 13:41 ` [PATCH V3 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
2018-10-26 13:41 ` [PATCH V3 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function Star Zeng
2018-10-26 13:41 ` [PATCH V3 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Star Zeng
2018-10-27 7:47 ` Wu, Hao A [this message]
2018-10-26 13:41 ` [PATCH V3 4/4] MdeModulePkg EhciDxe: " Star Zeng
[not found] ` <CS1PR8401MB1189428C2915107C583C0A42B4CC0@CS1PR8401MB1189.NAMPRD84.PROD.OUTLOOK.COM>
2018-10-30 12:39 ` Ard Biesheuvel
2018-10-30 12:50 ` Leif Lindholm
2018-10-31 4:38 ` Zeng, Star
2018-10-31 12:08 ` Leif Lindholm
2018-11-01 1:12 ` Zeng, Star
2018-11-01 10:32 ` Leif Lindholm
2018-11-06 9:49 ` Ard Biesheuvel
2018-11-06 14:37 ` Zeng, Star
2018-11-07 15:00 ` Zeng, Star
2018-11-07 15:14 ` Ard Biesheuvel
2018-10-28 13:35 ` [PATCH V3 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe " Zeng, Star
2018-10-29 2:19 ` Ni, Ruiyu
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=B80AF82E9BFB8E4FBD8C89DA810C6A093C846FC0@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