Laszlo, After I sent the mail to propose changing the CpuIo driver to improve IO performance, I now saw your patch sent earlier than my last mail. A very interesting feeling.
Reviewed-by: Ruiyu Ni <ruiyu...@intel.com> Regards, Ray >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo >Ersek >Sent: Friday, April 8, 2016 5:52 AM >To: edk2-devel-01 <edk2-de...@ml01.01.org> >Cc: Ni, Ruiyu <ruiyu...@intel.com>; Justen, Jordan L ><jordan.l.jus...@intel.com>; Fan, Jeff <jeff....@intel.com>; Mark ><kram...@gmail.com> >Subject: [edk2] [PATCH] UefiCpuPkg: CpuIo2Dxe: optimize FIFO reads and writes >of IO ports > >* Short description: > > The CpuIoServiceRead() and CpuIoServiceWrite() functions transfer data > between memory and IO ports with individual Io(Read|Write)(8|16|32) > function calls, each in an appropriately set up loop. > > On the Ia32 and X64 platforms however, FIFO reads and writes can be > optimized, by coding them in assembly, and delegating the loop to the > CPU, with the REP prefix. > > On KVM virtualization hosts, this difference has a huge performance > impact: if the loop is open-coded, then the virtual machine traps to the > hypervisor on every single UINT8 / UINT16 / UINT32 transfer, whereas > with the REP prefix, KVM can transfer up to a page of data per VM trap. > This is especially noticeable with IDE PIO transfers, where all the data > are squeezed through IO ports. > >* Long description: > > The RootBridgeIoIoRW() function in > > PcAtChipsetPkg/PciHostBridgeDxe/PciRootBridgeIo.c > > used to have the exact same IO port acces optimization, dating back > verbatim to commit 1fd376d9792: > > PcAtChipsetPkg/PciHostBridgeDxe: Improve KVM FIFO I/O read/write > performance > > OvmfPkg cloned the "PcAtChipsetPkg/PciHostBridgeDxe" driver (for > unrelated reasons), and inherited the optimization from PcAtChipsetPkg. > > The "PcAtChipsetPkg/PciHostBridgeDxe" driver was ultimately removed in > commit 111d79db47: > > PcAtChipsetPkg/PciHostBridge: Remove PciHostBridge driver > > and OvmfPkg too was rebased to the new core Pci Host Bridge Driver, in > commit 4014885ffd: > > OvmfPkg: switch to MdeModulePkg/Bus/Pci/PciHostBridgeDxe > > This caused the optimization to go lost. Namely, the > RootBridgeIoIoRead() and RootBridgeIoIoWrite() functions in the new core > Pci Host Bridge Driver delegate IO port accesses to > EFI_CPU_IO2_PROTOCOL. And, in OvmfPkg (and likely most other Ia32 / X64 > edk2 platforms), this protocol is provided by "UefiCpuPkg/CpuIo2Dxe", > which lacks the optimization. > > Therefore, this patch ports the C source code logic from commit > 1fd376d9792 (see above) to "UefiCpuPkg/CpuIo2Dxe", plus it ports the > NASM-converted assembly helper functions from OvmfPkg commits > 6026bf460037 and ace1d0517b65: > > OvmfPkg PciHostBridgeDxe: Convert Ia32/IoFifo.asm to NASM > > OvmfPkg PciHostBridgeDxe: Convert X64/IoFifo.asm to NASM > >* Notes about the port: > > - The write and read branches from commit 1fd376d9792 are split to the > separate functions CpuIoServiceWrite() and CpuIoServiceRead(). > > - The EfiPciWidthUintXX constants are replaced with EfiCpuIoWidthUintXX. > > - The cast expression "(UINTN) Address" is replaced with > "(UINTN)Address" (i.e., no space), because that's how the receiving > functions spell it as well. > > - The labels in the switch statements are unindented by one level, to > match the edk2 coding style (and the rest of UefiCpuPkg) better. > >* The first signoff belongs to Jordan, because he authored all of > 1fd376d9792, 6026bf460037 and ace1d0517b65. > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Laszlo Ersek <ler...@redhat.com> >Ref: https://www.redhat.com/archives/vfio-users/2016-April/msg00029.html >Reported-by: Mark <kram...@gmail.com> >Ref: http://thread.gmane.org/gmane.comp.bios.edk2.devel/10424/focus=10432 >Reported-by: Jordan Justen <jordan.l.jus...@intel.com> >Cc: Jordan Justen <jordan.l.jus...@intel.com> >Cc: Ruiyu Ni <ruiyu...@intel.com> >Cc: Jeff Fan <jeff....@intel.com> >Cc: Mark <kram...@gmail.com> >--- > UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf | 7 + > UefiCpuPkg/CpuIo2Dxe/IoFifo.h | 176 ++++++++++++++++++++ > UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c | 49 ++++++ > UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.nasm | 136 +++++++++++++++ > UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.nasm | 125 ++++++++++++++ > 5 files changed, 493 insertions(+) > >diff --git a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf >b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf >index 8ef8b3d31cff..be79b1b3b992 100644 >--- a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf >+++ b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf >@@ -30,7 +30,14 @@ [Defines] > [Sources] > CpuIo2Dxe.c > CpuIo2Dxe.h >+ IoFifo.h > >+[Sources.IA32] >+ Ia32/IoFifo.nasm >+ >+[Sources.X64] >+ X64/IoFifo.nasm >+ > [Packages] > MdePkg/MdePkg.dec > >diff --git a/UefiCpuPkg/CpuIo2Dxe/IoFifo.h b/UefiCpuPkg/CpuIo2Dxe/IoFifo.h >new file mode 100644 >index 000000000000..9978f8bfc39a >--- /dev/null >+++ b/UefiCpuPkg/CpuIo2Dxe/IoFifo.h >@@ -0,0 +1,176 @@ >+/** @file >+ I/O FIFO routines >+ >+ Copyright (c) 2008 - 2012, 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 >+ http://opensource.org/licenses/bsd-license.php >+ >+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >IMPLIED. >+ >+**/ >+ >+#ifndef _IO_FIFO_H_INCLUDED_ >+#define _IO_FIFO_H_INCLUDED_ >+ >+/** >+ Reads an 8-bit I/O port fifo into a block of memory. >+ >+ Reads the 8-bit I/O fifo port specified by Port. >+ >+ The port is read Count times, and the read data is >+ stored in the provided Buffer. >+ >+ This function must guarantee that all I/O read and write operations are >+ serialized. >+ >+ If 8-bit I/O port operations are not supported, then ASSERT(). >+ >+ @param Port The I/O port to read. >+ @param Count The number of times to read I/O port. >+ @param Buffer The buffer to store the read data into. >+ >+**/ >+VOID >+EFIAPI >+IoReadFifo8 ( >+ IN UINTN Port, >+ IN UINTN Count, >+ OUT VOID *Buffer >+ ); >+ >+/** >+ Reads a 16-bit I/O port fifo into a block of memory. >+ >+ Reads the 16-bit I/O fifo port specified by Port. >+ >+ The port is read Count times, and the read data is >+ stored in the provided Buffer. >+ >+ This function must guarantee that all I/O read and write operations are >+ serialized. >+ >+ If 16-bit I/O port operations are not supported, then ASSERT(). >+ >+ @param Port The I/O port to read. >+ @param Count The number of times to read I/O port. >+ @param Buffer The buffer to store the read data into. >+ >+**/ >+VOID >+EFIAPI >+IoReadFifo16 ( >+ IN UINTN Port, >+ IN UINTN Count, >+ OUT VOID *Buffer >+ ); >+ >+/** >+ Reads a 32-bit I/O port fifo into a block of memory. >+ >+ Reads the 32-bit I/O fifo port specified by Port. >+ >+ The port is read Count times, and the read data is >+ stored in the provided Buffer. >+ >+ This function must guarantee that all I/O read and write operations are >+ serialized. >+ >+ If 32-bit I/O port operations are not supported, then ASSERT(). >+ >+ @param Port The I/O port to read. >+ @param Count The number of times to read I/O port. >+ @param Buffer The buffer to store the read data into. >+ >+**/ >+VOID >+EFIAPI >+IoReadFifo32 ( >+ IN UINTN Port, >+ IN UINTN Count, >+ OUT VOID *Buffer >+ ); >+ >+/** >+ Writes a block of memory into an 8-bit I/O port fifo. >+ >+ Writes the 8-bit I/O fifo port specified by Port. >+ >+ The port is written Count times, and the write data is >+ retrieved from the provided Buffer. >+ >+ This function must guarantee that all I/O write and write operations are >+ serialized. >+ >+ If 8-bit I/O port operations are not supported, then ASSERT(). >+ >+ @param Port The I/O port to write. >+ @param Count The number of times to write I/O port. >+ @param Buffer The buffer to store the write data into. >+ >+**/ >+VOID >+EFIAPI >+IoWriteFifo8 ( >+ IN UINTN Port, >+ IN UINTN Count, >+ OUT VOID *Buffer >+ ); >+ >+/** >+ Writes a block of memory into a 16-bit I/O port fifo. >+ >+ Writes the 16-bit I/O fifo port specified by Port. >+ >+ The port is written Count times, and the write data is >+ retrieved from the provided Buffer. >+ >+ This function must guarantee that all I/O write and write operations are >+ serialized. >+ >+ If 16-bit I/O port operations are not supported, then ASSERT(). >+ >+ @param Port The I/O port to write. >+ @param Count The number of times to write I/O port. >+ @param Buffer The buffer to store the write data into. >+ >+**/ >+VOID >+EFIAPI >+IoWriteFifo16 ( >+ IN UINTN Port, >+ IN UINTN Count, >+ OUT VOID *Buffer >+ ); >+ >+/** >+ Writes a block of memory into a 32-bit I/O port fifo. >+ >+ Writes the 32-bit I/O fifo port specified by Port. >+ >+ The port is written Count times, and the write data is >+ retrieved from the provided Buffer. >+ >+ This function must guarantee that all I/O write and write operations are >+ serialized. >+ >+ If 32-bit I/O port operations are not supported, then ASSERT(). >+ >+ @param Port The I/O port to write. >+ @param Count The number of times to write I/O port. >+ @param Buffer The buffer to store the write data into. >+ >+**/ >+VOID >+EFIAPI >+IoWriteFifo32 ( >+ IN UINTN Port, >+ IN UINTN Count, >+ OUT VOID *Buffer >+ ); >+ >+#endif >+ >diff --git a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c >b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c >index 3d8a79923025..6ccfc40e10f8 100644 >--- a/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c >+++ b/UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c >@@ -13,6 +13,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >EXPRESS OR IMPLIED. > **/ > > #include "CpuIo2Dxe.h" >+#include "IoFifo.h" > > // > // Handle for the CPU I/O 2 Protocol >@@ -410,6 +411,30 @@ CpuIoServiceRead ( > InStride = mInStride[Width]; > OutStride = mOutStride[Width]; > OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH) (Width & 0x03); >+ >+#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) >+ if (InStride == 0) { >+ switch (OperationWidth) { >+ case EfiCpuIoWidthUint8: >+ IoReadFifo8 ((UINTN)Address, Count, Buffer); >+ return EFI_SUCCESS; >+ case EfiCpuIoWidthUint16: >+ IoReadFifo16 ((UINTN)Address, Count, Buffer); >+ return EFI_SUCCESS; >+ case EfiCpuIoWidthUint32: >+ IoReadFifo32 ((UINTN)Address, Count, Buffer); >+ return EFI_SUCCESS; >+ default: >+ // >+ // The CpuIoCheckParameter call above will ensure that this >+ // path is not taken. >+ // >+ ASSERT (FALSE); >+ break; >+ } >+ } >+#endif >+ > for (Uint8Buffer = Buffer; Count > 0; Address += InStride, Uint8Buffer += > OutStride, Count--) { > if (OperationWidth == EfiCpuIoWidthUint8) { > *Uint8Buffer = IoRead8 ((UINTN)Address); >@@ -492,6 +517,30 @@ CpuIoServiceWrite ( > InStride = mInStride[Width]; > OutStride = mOutStride[Width]; > OperationWidth = (EFI_CPU_IO_PROTOCOL_WIDTH) (Width & 0x03); >+ >+#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) >+ if (InStride == 0) { >+ switch (OperationWidth) { >+ case EfiCpuIoWidthUint8: >+ IoWriteFifo8 ((UINTN)Address, Count, Buffer); >+ return EFI_SUCCESS; >+ case EfiCpuIoWidthUint16: >+ IoWriteFifo16 ((UINTN)Address, Count, Buffer); >+ return EFI_SUCCESS; >+ case EfiCpuIoWidthUint32: >+ IoWriteFifo32 ((UINTN)Address, Count, Buffer); >+ return EFI_SUCCESS; >+ default: >+ // >+ // The CpuIoCheckParameter call above will ensure that this >+ // path is not taken. >+ // >+ ASSERT (FALSE); >+ break; >+ } >+ } >+#endif >+ > for (Uint8Buffer = (UINT8 *)Buffer; Count > 0; Address += InStride, > Uint8Buffer += OutStride, Count--) { > if (OperationWidth == EfiCpuIoWidthUint8) { > IoWrite8 ((UINTN)Address, *Uint8Buffer); >diff --git a/UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.nasm >b/UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.nasm >new file mode 100644 >index 000000000000..daa90a99af37 >--- /dev/null >+++ b/UefiCpuPkg/CpuIo2Dxe/Ia32/IoFifo.nasm >@@ -0,0 +1,136 @@ >+;------------------------------------------------------------------------------ >+; >+; Copyright (c) 2006 - 2012, 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 >+; http://opensource.org/licenses/bsd-license.php. >+; >+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >IMPLIED. >+; >+;------------------------------------------------------------------------------ >+ >+ SECTION .text >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoReadFifo8 ( >+; IN UINTN Port, >+; IN UINTN Size, >+; IN VOID *Buffer >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoReadFifo8) >+ASM_PFX(IoReadFifo8): >+ push edi >+ cld >+ mov dx, [esp + 8] >+ mov ecx, [esp + 12] >+ mov edi, [esp + 16] >+rep insb >+ pop edi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoReadFifo16 ( >+; IN UINTN Port, >+; IN UINTN Size, >+; IN VOID *Buffer >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoReadFifo16) >+ASM_PFX(IoReadFifo16): >+ push edi >+ cld >+ mov dx, [esp + 8] >+ mov ecx, [esp + 12] >+ mov edi, [esp + 16] >+rep insw >+ pop edi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoReadFifo32 ( >+; IN UINTN Port, >+; IN UINTN Size, >+; IN VOID *Buffer >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoReadFifo32) >+ASM_PFX(IoReadFifo32): >+ push edi >+ cld >+ mov dx, [esp + 8] >+ mov ecx, [esp + 12] >+ mov edi, [esp + 16] >+rep insd >+ pop edi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoWriteFifo8 ( >+; IN UINTN Port, >+; IN UINTN Size, >+; IN VOID *Buffer >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoWriteFifo8) >+ASM_PFX(IoWriteFifo8): >+ push esi >+ cld >+ mov dx, [esp + 8] >+ mov ecx, [esp + 12] >+ mov esi, [esp + 16] >+rep outsb >+ pop esi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoWriteFifo16 ( >+; IN UINTN Port, >+; IN UINTN Size, >+; IN VOID *Buffer >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoWriteFifo16) >+ASM_PFX(IoWriteFifo16): >+ push esi >+ cld >+ mov dx, [esp + 8] >+ mov ecx, [esp + 12] >+ mov esi, [esp + 16] >+rep outsw >+ pop esi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoWriteFifo32 ( >+; IN UINTN Port, >+; IN UINTN Size, >+; IN VOID *Buffer >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoWriteFifo32) >+ASM_PFX(IoWriteFifo32): >+ push esi >+ cld >+ mov dx, [esp + 8] >+ mov ecx, [esp + 12] >+ mov esi, [esp + 16] >+rep outsd >+ pop esi >+ ret >+ >diff --git a/UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.nasm >b/UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.nasm >new file mode 100644 >index 000000000000..bb3d1da67af5 >--- /dev/null >+++ b/UefiCpuPkg/CpuIo2Dxe/X64/IoFifo.nasm >@@ -0,0 +1,125 @@ >+;------------------------------------------------------------------------------ >+; >+; Copyright (c) 2006 - 2012, 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 >+; http://opensource.org/licenses/bsd-license.php. >+; >+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >+; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >IMPLIED. >+; >+;------------------------------------------------------------------------------ >+ >+ DEFAULT REL >+ SECTION .text >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoReadFifo8 ( >+; IN UINTN Port, // rcx >+; IN UINTN Size, // rdx >+; IN VOID *Buffer // r8 >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoReadFifo8) >+ASM_PFX(IoReadFifo8): >+ cld >+ xchg rcx, rdx >+ xchg rdi, r8 ; rdi: buffer address; r8: save rdi >+rep insb >+ mov rdi, r8 ; restore rdi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoReadFifo16 ( >+; IN UINTN Port, // rcx >+; IN UINTN Size, // rdx >+; IN VOID *Buffer // r8 >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoReadFifo16) >+ASM_PFX(IoReadFifo16): >+ cld >+ xchg rcx, rdx >+ xchg rdi, r8 ; rdi: buffer address; r8: save rdi >+rep insw >+ mov rdi, r8 ; restore rdi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoReadFifo32 ( >+; IN UINTN Port, // rcx >+; IN UINTN Size, // rdx >+; IN VOID *Buffer // r8 >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoReadFifo32) >+ASM_PFX(IoReadFifo32): >+ cld >+ xchg rcx, rdx >+ xchg rdi, r8 ; rdi: buffer address; r8: save rdi >+rep insd >+ mov rdi, r8 ; restore rdi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoWriteFifo8 ( >+; IN UINTN Port, // rcx >+; IN UINTN Size, // rdx >+; IN VOID *Buffer // r8 >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoWriteFifo8) >+ASM_PFX(IoWriteFifo8): >+ cld >+ xchg rcx, rdx >+ xchg rsi, r8 ; rsi: buffer address; r8: save rsi >+rep outsb >+ mov rsi, r8 ; restore rsi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoWriteFifo16 ( >+; IN UINTN Port, // rcx >+; IN UINTN Size, // rdx >+; IN VOID *Buffer // r8 >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoWriteFifo16) >+ASM_PFX(IoWriteFifo16): >+ cld >+ xchg rcx, rdx >+ xchg rsi, r8 ; rsi: buffer address; r8: save rsi >+rep outsw >+ mov rsi, r8 ; restore rsi >+ ret >+ >+;------------------------------------------------------------------------------ >+; VOID >+; EFIAPI >+; IoWriteFifo32 ( >+; IN UINTN Port, // rcx >+; IN UINTN Size, // rdx >+; IN VOID *Buffer // r8 >+; ); >+;------------------------------------------------------------------------------ >+global ASM_PFX(IoWriteFifo32) >+ASM_PFX(IoWriteFifo32): >+ cld >+ xchg rcx, rdx >+ xchg rsi, r8 ; rsi: buffer address; r8: save rsi >+rep outsd >+ mov rsi, r8 ; restore rsi >+ ret >+ >-- >1.8.3.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
