Re: [edk2] [PATCH 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer

2018-10-25 Thread Ni, Ruiyu

On 10/25/2018 6:58 PM, Star Zeng wrote:

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 
Cc: Hao Wu 
Cc: Jian J Wang 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c  |   1 +
  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 134 +++
  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 ---
  3 files changed, 64 insertions(+), 98 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 2d7c08dc5bfa..d03a6681ce0d 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  EpAddrEndpoint 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  EpAddrEndpoint 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 BOOLEANAllocateCommonBuffer,
IN VOID   *Data,
IN UINTN  DataLen,
IN EFI_ASYNC_USB_TRANSFER_CALLBACKCallback,
@@ -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),
+   ,
+   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,6 +196,11 @@ XhcCreateUrb (

ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
  DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb Failed, Status = 
%r\n", Status));
+Xhc->PciIo->FreeBuffer (
+  Xhc->PciIo,
+

[edk2] [PATCH 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer

2018-10-25 Thread Star Zeng
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 
Cc: Hao Wu 
Cc: Jian J Wang 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c  |   1 +
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 134 +++
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 ---
 3 files changed, 64 insertions(+), 98 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 2d7c08dc5bfa..d03a6681ce0d 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  EpAddrEndpoint 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  EpAddrEndpoint 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 BOOLEANAllocateCommonBuffer,
   IN VOID   *Data,
   IN UINTN  DataLen,
   IN EFI_ASYNC_USB_TRANSFER_CALLBACKCallback,
@@ -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),
+   ,
+   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,6 +196,11 @@ XhcCreateUrb (
   ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
 DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb Failed, Status = 
%r\n", Status));
+Xhc->PciIo->FreeBuffer (
+  Xhc->PciIo,
+  EFI_SIZE_TO_PAGES (Urb->DataLen),
+  Urb->Data
+