Re: [PATCH V3 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

(add back the list)

On 30 October 2018 at 09:07, Cohen, Eugene <eugene@hp.com> wrote:
> Has this patch been tested on a system that does not have coherent DMA?
>
>
>
> It's not clear that this change would actually be faster on a system of that
> type since using common buffers imply access to uncached memory.  Depending
> on the access patterns the uncached memory access could be more time
> consuming than cache maintenance operations.
>

I haven't had time to look at these patches yet.

I agree with Eugene's concern: the directional DMA routines are much
more performant on implementations with non-coherent DMA, and so
common buffers should be avoided unless we are dealing with data
structures that are truly shared between the CPU and the device.

Since this is obviously not the case here, could we please have some
numbers about the performance improvement we are talking about here?
Would it be possible to improve the IOMMU handling code instead?


>
>
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Star Zeng
> Sent: Friday, October 26, 2018 7:41 AM
> To: edk2-devel@lists.01.org
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Hao Wu <hao.a.wu@intel.com>; Jiewen Yao
> <jiewen.yao@intel.com>; Star Zeng <star.zeng@intel.com>
> Subject: [edk2] [PATCH V3 4/4] MdeModulePkg EhciDxe: Use common buffer for
> AsyncInterruptTransfer
>
>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
>
> In current code, EhcMonitorAsyncRequests (timer handler) will do
> unmap and map operations for AsyncIntTransfers to "Flush data from
> PCI controller specific address to mapped system memory address".
> EhcMonitorAsyncRequests
> EhcFlushAsyncIntMap
> PciIo->Unmap
> IoMmu->SetAttribute
> PciIo->Map
> IoMmu->SetAttribute
>
> This may impact the boot performance.
>
> Since the data buffer for EhcMonitorAsyncRequests 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>
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
> ---
> MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 3 ++
> MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 78
> +-------------------------------
> MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c | 38 ++++++++++++++--
> MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h | 33 ++++++++------
> 4 files changed, 57 insertions(+), 95 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> index 5569f4f9618b..764eeda58ba1 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> @@ -763,6 +763,7 @@ EhcControlTransfer (
> Translator,
> EHC_CTRL_TRANSFER,
> Request,
> + FALSE,
> Data,
> *DataLength,
> NULL,
> @@ -906,6 +907,7 @@ EhcBulkTransfer (
> Translator,
> EHC_BULK_TRANSFER,
> NULL,
> + FALSE,
> Data[0],
> *DataLength,
> NULL,
> @@ -1163,6 +1165,7 @@ EhcSyncInterruptTransfer (
> Translator,
> EHC_INT_TRANSFER_SYNC,
> NULL,
> + FALSE,
> Data,
> *DataLength,
> NULL,
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> index ec8d796fab11..b067fd02d1ce 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> @@ -778,7 +778,6 @@ EhciDelAsyncIntTransfer (
> EhcUnlinkQhFromPeriod (Ehc, Urb->Qh);
> RemoveEntryList (&Urb->UrbList);
>
> - gBS->FreePool (Urb->Data);
> EhcFreeUrb (Ehc, Urb);
> return EFI_SUCCESS;
> }
> @@ -809,7 +808,6 @@ EhciDelAllAsyncIntTransfers (
> EhcUnlinkQhFromPeriod (Ehc, Urb->Qh);
> RemoveEntryList (&Urb->UrbList);
>
> - gBS->FreePool (Urb->Data);
> EhcFreeUrb (Ehc, Urb);
> }
> }
> @@ -848,16 +846,8 @@ EhciInsertAsyncIntTransfer (
> IN UINTN Interval
> )
> {
> - VOID *Data;
> URB *Urb;
>
> - Data = AllocatePool (DataLen);
> -
> - if (Data == NULL) {
> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", __FUNCTION__));
> - return NULL;
> - }
> -
> Urb = EhcCreateUrb (
> Ehc,
> DevAddr,
> @@ -868,7 +858,8 @@ EhciInsertAsyncIntTransfer (
> Hub,
> EHC_INT_TRANSFER_ASYNC,
> NULL,
> - Data,
> + TRUE,
> + NULL,
> DataLen,
> Callback,
> Context,
> @@ -877,7 +868,6 @@ EhciInsertAsyncIntTransfer (
>
> if (Urb == NULL) {
> DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> - gBS->FreePool (Data);
> return NULL;
> }
>
> @@ -892,60 +882,6 @@ EhciInsertAsyncIntTransfer (
> }
>
> /**
> - Flush data from PCI controller specific address to mapped system
> - memory address.
> -
> - @param Ehc The EHCI 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
> -EhcFlushAsyncIntMap (
> - IN USB2_HC_DEV *Ehc,
> - 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 = Ehc->PciIo;
> - Len = Urb->DataLen;
> -
> - if (Urb->Ep.Direction == EfiUsbDataIn) {
> - MapOp = EfiPciIoOperationBusMasterWrite;
> - } else {
> - MapOp = EfiPciIoOperationBusMasterRead;
> - }
> -
> - 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;
> -}
> -
> -
> -/**
> Update the queue head for next round of asynchronous transfer.
>
> @param Ehc The EHCI device.
> @@ -1050,7 +986,6 @@ EhcMonitorAsyncRequests (
> BOOLEAN Finished;
> UINT8 *ProcBuf;
> URB *Urb;
> - EFI_STATUS Status;
>
> OldTpl = gBS->RaiseTPL (EHC_TPL);
> Ehc = (USB2_HC_DEV *) Context;
> @@ -1069,15 +1004,6 @@ EhcMonitorAsyncRequests (
> }
>
> //
> - // Flush any PCI posted write transactions from a PCI host
> - // bridge to system memory.
> - //
> - Status = EhcFlushAsyncIntMap (Ehc, Urb);
> - if (EFI_ERROR (Status)) {
> - DEBUG ((EFI_D_ERROR, "EhcMonitorAsyncRequests: 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/EhciDxe/EhciUrb.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> index 6afb327df165..09c12776ecdb 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> @@ -339,6 +339,14 @@ EhcFreeUrb (
> PciIo->Unmap (PciIo, Urb->DataMap);
> }
>
> + if (Urb->AllocateCommonBuffer) {
> + PciIo->FreeBuffer (
> + PciIo,
> + EFI_SIZE_TO_PAGES (Urb->DataLen),
> + Urb->Data
> + );
> + }
> +
> if (Urb->Qh != NULL) {
> //
> // Ensure that this queue head has been unlinked from the
> @@ -529,7 +537,8 @@ ON_ERROR:
> @param Hub The transaction translator to use.
> @param Type The transaction type.
> @param Request The standard USB request for control transfer.
> - @param Data The user data to 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.
> @@ -549,6 +558,7 @@ EhcCreateUrb (
> IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
> 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,
> @@ -596,8 +606,24 @@ EhcCreateUrb (
> Ep->PollRate = EhcConvertPollRate (Interval);
>
> Urb->Request = Request;
> + if (AllocateCommonBuffer) {
> + ASSERT (Data == NULL);
> + Status = Ehc->PciIo->AllocateBuffer (
> + Ehc->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;
>
> @@ -627,10 +653,14 @@ EhcCreateUrb (
> if (Data != NULL) {
> Len = DataLen;
>
> - if (Ep->Direction == EfiUsbDataIn) {
> - MapOp = EfiPciIoOperationBusMasterWrite;
> + if (Urb->AllocateCommonBuffer) {
> + MapOp = EfiPciIoOperationBusMasterCommonBuffer;
> } else {
> - MapOp = EfiPciIoOperationBusMasterRead;
> + if (Ep->Direction == EfiUsbDataIn) {
> + MapOp = EfiPciIoOperationBusMasterWrite;
> + } else {
> + MapOp = EfiPciIoOperationBusMasterRead;
> + }
> }
>
> Status = PciIo->Map (PciIo, MapOp, Data, &Len, &PhyAddr, &Map);
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> index 02e9af81be39..cb3599f9d361 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> @@ -3,7 +3,7 @@
> This file contains URB request, each request is warpped in a
> URB (Usb Request Block).
>
> -Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -216,6 +216,7 @@ struct _URB {
> EFI_USB_DEVICE_REQUEST *Request; // Control transfer only
> VOID *RequestPhy; // Address of the mapped request
> VOID *RequestMap;
> + BOOLEAN AllocateCommonBuffer;
> VOID *Data;
> UINTN DataLen;
> VOID *DataPhy; // Address of the mapped user data
> @@ -298,20 +299,21 @@ EhcFreeUrb (
> /**
> Create a new URB and its associated QTD.
>
> - @param Ehc The EHCI device.
> - @param DevAddr The device address.
> - @param EpAddr Endpoint addrress & its direction.
> - @param DevSpeed The device speed.
> - @param Toggle Initial data toggle to use.
> - @param MaxPacket The max packet length of the endpoint.
> - @param Hub The transaction translator to use.
> - @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 Interval The interval for interrupt transfer.
> + @param Ehc The EHCI device.
> + @param DevAddr The device address.
> + @param EpAddr Endpoint addrress & its direction.
> + @param DevSpeed The device speed.
> + @param Toggle Initial data toggle to use.
> + @param MaxPacket The max packet length of the endpoint.
> + @param Hub The transaction translator to use.
> + @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.
> + @param Interval The interval for interrupt transfer.
>
> @return Created URB or NULL.
>
> @@ -327,6 +329,7 @@ EhcCreateUrb (
> IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
> 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel