From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.20; helo=mga02.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0272D2117FD70 for ; Fri, 26 Oct 2018 06:43:45 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Oct 2018 06:43:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,428,1534834800"; d="scan'208";a="244592963" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga004.jf.intel.com with ESMTP; 26 Oct 2018 06:43:45 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Oct 2018 06:43:45 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Oct 2018 06:43:44 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.84]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.214]) with mapi id 14.03.0415.000; Fri, 26 Oct 2018 21:43:42 +0800 From: "Zeng, Star" To: "Wu, Hao A" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , "Yao, Jiewen" , "Zeng, Star" Thread-Topic: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Thread-Index: AQHUbO66PpvKZouju0CcqKBIZUv2gKUwna2AgAAKP4CAAOHIgA== Date: Fri, 26 Oct 2018 13:43:41 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BC5BF16@shsmsx102.ccr.corp.intel.com> References: <1540532541-115032-1-git-send-email-star.zeng@intel.com> <1540532541-115032-4-git-send-email-star.zeng@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZGQxNmY1MTEtMWE4OS00ZDZlLWFlZWUtYjgzYTgyNzA0ODFjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUCtNWXQ1UXNHUURvNVFBODE3SWg0ajJBMHhzc1RleWdwRUJub1NtUGRmY0VwZFhVbEVoRkpvbGNaZmhtdG00QiJ9 dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Oct 2018 13:43:46 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Good catch. We can use XhcFreeUrb directly. I just posted V3 patches to cov= er it. Thanks, Star -----Original Message----- From: Wu, Hao A=20 Sent: Friday, October 26, 2018 4:15 PM To: Wu, Hao A ; Zeng, Star ; edk2-= devel@lists.01.org Cc: Ni, Ruiyu ; Yao, Jiewen ; Zen= g, Star Subject: RE: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer = for AsyncInterruptTransfer > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > Wu, Hao A > Sent: Friday, October 26, 2018 3:38 PM > To: Zeng, Star; edk2-devel@lists.01.org > Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star > Subject: Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common=20 > buffer for AsyncInterruptTransfer >=20 > Reviewed-by: Hao Wu Sorry, missed one question for this patch. Please see below for more detail: >=20 > Best Regards, > Hao Wu >=20 >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 > > Of Star Zeng > > Sent: Friday, October 26, 2018 1:42 PM > > To: edk2-devel@lists.01.org > > Cc: Ni, Ruiyu; Wu, Hao A; Yao, Jiewen; Zeng, Star > > Subject: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common > buffer > > for AsyncInterruptTransfer > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1274 > > > > In current code, XhcMonitorAsyncRequests (timer handler) will do=20 > > unmap and map operations for AsyncIntTransfers to "Flush data from=20 > > 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=20 > > buffer, we can allocate common buffer by PciIo->AllocateBuffer and=20 > > 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=20 > > the processor and a bus master. The buffer is coherent /// from both=20 > > 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 > > Reviewed-by: Ruiyu Ni > > --- > > MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 1 + > > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 141=20 > > +++++++++++----------- > -- > > ------- > > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 27 +++--- > > 3 files changed, 67 insertions(+), 102 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..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 driv= er > > - @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 t= ransfer > > + @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 tran= sferred > > + @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 =3D Type; > > > > Urb->Request =3D Request; > > + if (AllocateCommonBuffer) { > > + ASSERT (Data =3D=3D NULL); > > + Status =3D Xhc->PciIo->AllocateBuffer ( > > + Xhc->PciIo, > > + AllocateAnyPages, > > + EfiBootServicesData, > > + EFI_SIZE_TO_PAGES (DataLen), > > + &Data, > > + 0 > > + ); > > + if (EFI_ERROR (Status) || (Data =3D=3D NULL)) { > > + FreePool (Urb); > > + return NULL; > > + } > > + } > > Urb->Data =3D Data; > > Urb->DataLen =3D DataLen; > > + Urb->AllocateCommonBuffer =3D AllocateCommonBuffer; > > Urb->Callback =3D Callback; > > Urb->Context =3D Context; > > > > @@ -178,6 +196,11 @@ XhcCreateUrb ( > > ASSERT_EFI_ERROR (Status); > > if (EFI_ERROR (Status)) { > > DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb=20 > > Failed, Status =3D %r\n", Status)); > > + Xhc->PciIo->FreeBuffer ( > > + Xhc->PciIo, > > + EFI_SIZE_TO_PAGES (Urb->DataLen), > > + Urb->Data > > + ); Does this "Xhc->PciIo->FreeBuffer()" call here need Urb->AllocateCommonBuff= er check? Best Regards, Hao Wu > > FreePool (Urb); > > Urb =3D NULL; > > } > > @@ -206,6 +229,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 +295,14 @@ XhcCreateTransferTrb ( > > // No need to remap. > > // > > if ((Urb->Data !=3D NULL) && (Urb->DataMap =3D=3D NULL)) { > > - if (((UINT8) (Urb->Ep.Direction)) =3D=3D EfiUsbDataIn) { > > - MapOp =3D EfiPciIoOperationBusMasterWrite; > > + if (Urb->AllocateCommonBuffer) { > > + MapOp =3D EfiPciIoOperationBusMasterCommonBuffer; > > } else { > > - MapOp =3D EfiPciIoOperationBusMasterRead; > > + if (((UINT8) (Urb->Ep.Direction)) =3D=3D EfiUsbDataIn) { > > + MapOp =3D EfiPciIoOperationBusMasterWrite; > > + } else { > > + MapOp =3D EfiPciIoOperationBusMasterRead; > > + } > > } > > > > Len =3D Urb->DataLen; > > @@ -1367,7 +1402,6 @@ XhciDelAsyncIntTransfer ( > > } > > > > RemoveEntryList (&Urb->UrbList); > > - FreePool (Urb->Data); > > XhcFreeUrb (Xhc, Urb); > > return EFI_SUCCESS; > > } > > @@ -1405,7 +1439,6 @@ XhciDelAllAsyncIntTransfers ( > > } > > > > RemoveEntryList (&Urb->UrbList); > > - FreePool (Urb->Data); > > XhcFreeUrb (Xhc, Urb); > > } > > } > > @@ -1438,15 +1471,8 @@ XhciInsertAsyncIntTransfer ( > > IN VOID *Context > > ) > > { > > - VOID *Data; > > URB *Urb; > > > > - Data =3D AllocateZeroPool (DataLen); > > - if (Data =3D=3D NULL) { > > - DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", > > __FUNCTION__)); > > - return NULL; > > - } > > - > > Urb =3D XhcCreateUrb ( > > Xhc, > > BusAddr, > > @@ -1455,14 +1481,14 @@ XhciInsertAsyncIntTransfer ( > > MaxPacket, > > XHC_INT_TRANSFER_ASYNC, > > NULL, > > - Data, > > + TRUE, > > + NULL, > > DataLen, > > Callback, > > Context > > ); > > if (Urb =3D=3D NULL) { > > DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", > __FUNCTION__)); > > - FreePool (Data); > > return NULL; > > } > > > > @@ -1503,61 +1529,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 =3D Xhc->PciIo; > > - Len =3D Urb->DataLen; > > - > > - if (Urb->Ep.Direction =3D=3D EfiUsbDataIn) { > > - MapOp =3D EfiPciIoOperationBusMasterWrite; > > - } else { > > - MapOp =3D EfiPciIoOperationBusMasterRead; > > - } > > - > > - if (Urb->DataMap !=3D NULL) { > > - Status =3D PciIo->Unmap (PciIo, Urb->DataMap); > > - if (EFI_ERROR (Status)) { > > - goto ON_ERROR; > > - } > > - } > > - > > - Urb->DataMap =3D NULL; > > - > > - Status =3D PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr,=20 > > &Map); > > - if (EFI_ERROR (Status) || (Len !=3D Urb->DataLen)) { > > - goto ON_ERROR; > > - } > > - > > - Urb->DataPhy =3D (VOID *) ((UINTN) PhyAddr); > > - Urb->DataMap =3D Map; > > - return EFI_SUCCESS; > > - > > -ON_ERROR: > > - return EFI_DEVICE_ERROR; > > -} > > - > > -/** > > Interrupt transfer periodic check handler. > > > > @param Event Interrupt event. > > @@ -1577,7 +1548,6 @@ XhcMonitorAsyncRequests ( > > UINT8 *ProcBuf; > > URB *Urb; > > UINT8 SlotId; > > - EFI_STATUS Status; > > EFI_TPL OldTpl; > > > > OldTpl =3D gBS->RaiseTPL (XHC_TPL); @@ -1606,15 +1576,6 @@=20 > > XhcMonitorAsyncRequests ( > > } > > > > // > > - // Flush any PCI posted write transactions from a PCI host > > - // bridge to system memory. > > - // > > - Status =3D 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 cd1403f2842a..2b5b95b7fb60 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 t= ransfer > > + @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 tran= sferred > > + @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 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel