From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 D0FD820957AEA for ; Wed, 2 May 2018 00:57:23 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 May 2018 00:57:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,354,1520924400"; d="scan'208";a="50805100" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga004.fm.intel.com with ESMTP; 02 May 2018 00:57:23 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 2 May 2018 00:57:23 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 2 May 2018 00:57:22 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.79]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.240]) with mapi id 14.03.0319.002; Wed, 2 May 2018 15:57:20 +0800 From: "Zeng, Star" To: "Ni, Ruiyu" , "'edk2-devel@lists.01.org'" CC: "Chiu, Chasel" , "Zeng, Star" Thread-Topic: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling Thread-Index: AQHT3QWm6PIcDGMxxUGjlMmkIZLg0qQb1TAw//+S3QCAABfGgIAAhlIggAAV1sA= Date: Wed, 2 May 2018 07:57:20 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BAE6864@shsmsx102.ccr.corp.intel.com> References: <20180426022356.66540-1-ruiyu.ni@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BAE658E@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BC84B22@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BC84D55@SHSMSX104.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BAE670E@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BAE670E@shsmsx102.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 07:57:24 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Another minor comment below. There are three lines added with no code, I guess they are added accidently= . It is better to remove them. + + + Thanks, Star -----Original Message----- From: Zeng, Star=20 Sent: Wednesday, May 2, 2018 2:39 PM To: Ni, Ruiyu ; 'edk2-devel@lists.01.org' Cc: Chiu, Chasel ; Zeng, Star Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead = when polling Reviewed-by: Star Zeng if it is updated. :) Thanks, Star -----Original Message----- From: Ni, Ruiyu Sent: Wednesday, May 2, 2018 2:37 PM To: Zeng, Star ; 'edk2-devel@lists.01.org' Cc: Chiu, Chasel Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead = when polling Star, You are correct. ">" is enough here. I will change it when committing the patch. Thanks/Ray > -----Original Message----- > From: Ni, Ruiyu > Sent: Wednesday, May 2, 2018 1:12 PM > To: Zeng, Star ; edk2-devel@lists.01.org > Cc: Chiu, Chasel > Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io=20 > overhead when polling >=20 > If Multiplicand * Multiplier + Remainder =3D MAX_UINT64, Even=20 > Multiplicand =3D > MAX_UINT64 / Multiplier, Overflow still happens. >=20 > So ">=3D" is used here. >=20 >=20 >=20 > Thanks/Ray >=20 > > -----Original Message----- > > From: Zeng, Star > > Sent: Wednesday, May 2, 2018 11:44 AM > > To: Ni, Ruiyu ; edk2-devel@lists.01.org > > Cc: Chiu, Chasel ; Zeng, Star=20 > > > > Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io=20 > > overhead when polling > > > > Is it more accurate > > if (Multiplicand >=3D DivU64x64Remainder (MAX_UINT64, Multiplier, > > NULL)) { > > -> > > if (Multiplicand > DivU64x64Remainder (MAX_UINT64, Multiplier, > > NULL)) { > > > > > > Thanks, > > Star > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Thursday, April 26, 2018 10:24 AM > > To: edk2-devel@lists.01.org > > Cc: Zeng, Star ; Chiu, Chasel=20 > > > > Subject: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io > overhead > > when polling > > > > RootBridgeIo.PollMem()/PollIo() originally don't count the IO/MMIO=20 > > access overhead when delaying. > > The patch changes the implementation to count the access overhead so=20 > > that the actually delay equals to user required delay. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ruiyu Ni > > Cc: Star Zeng > > Cc: Chasel Chiu > > --- > > .../Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf | 3 +- > > .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h | 3 +- > > .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 151 > +++++++++++++++- > > ----- > > 3 files changed, 115 insertions(+), 42 deletions(-) > > > > diff --git > > a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf > > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf > > index 42bd8a23cb..2e56959a8f 100644 > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf > > @@ -1,7 +1,7 @@ > > ## @file > > # Generic PCI Host Bridge driver. > > # > > -# Copyright (c) 2009 - 2016, Intel Corporation. All rights=20 > > reserved.
> > +# Copyright (c) 2009 - 2018, Intel Corporation. All rights=20 > > +reserved.
> > # > > # This program and the accompanying materials # are licensed and=20 > > made available under the terms and conditions of the BSD License @@ > > -43,6 +43,7 @@ [LibraryClasses] > > PciSegmentLib > > UefiLib > > PciHostBridgeLib > > + TimerLib > > > > [Protocols] > > gEfiMetronomeArchProtocolGuid ## CONSUMES > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h > > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h > > index d3dfb57fc6..e2f651aee4 100644 > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridge.h > > @@ -2,7 +2,7 @@ > > > > The PCI Root Bridge header file. > > > > -Copyright (c) 1999 - 2017, Intel Corporation. All rights=20 > > reserved.
> > +Copyright (c) 1999 - 2018, Intel Corporation. All rights=20 > > +reserved.
> > This program and the accompanying materials are licensed and made=20 > > available under the terms and conditions of the BSD License which=20 > > accompanies this distribution. The full text of the license may be=20 > > found at @@ -36,6 +36,7 @@ WITHOUT WARRANTIES OR > REPRESENTATIONS OF > > ANY KIND, EITHER EXPRESS OR IMPLIED. > > #include > > #include > > #include > > +#include > > #include "PciHostResource.h" > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > > index 5764c2f49f..459e962fe7 100644 > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > > @@ -2,7 +2,7 @@ > > > > PCI Root Bridge Io Protocol code. > > > > -Copyright (c) 1999 - 2017, Intel Corporation. All rights=20 > > reserved.
> > +Copyright (c) 1999 - 2018, Intel Corporation. All rights=20 > > +reserved.
> > This program and the accompanying materials are licensed and made=20 > > available under the terms and conditions of the BSD License which=20 > > accompanies this distribution. The full text of the license may be=20 > > found at @@ -468,6 +468,85 @@ > RootBridgeIoGetMemTranslationByAddress ( > > return EFI_SUCCESS; > > } > > > > +/** > > + Return the result of (Multiplicand * Multiplier / Divisor). > > + > > + @param Multiplicand A 64-bit unsigned value. > > + @param Multiplier A 64-bit unsigned value. > > + @param Divisor A 32-bit unsigned value. > > + @param Remainder A pointer to a 32-bit unsigned value. This > parameter > > is > > + optional and may be NULL. > > + > > + @return Multiplicand * Multiplier / Divisor. > > +**/ > > +UINT64 > > +MultThenDivU64x64x32 ( > > + IN UINT64 Multiplicand, > > + IN UINT64 Multiplier, > > + IN UINT32 Divisor, > > + OUT UINT32 *Remainder OPTIONAL > > + ) > > +{ > > + UINT64 Uint64; > > + UINT32 LocalRemainder; > > + UINT32 Uint32; > > + if (Multiplicand >=3D DivU64x64Remainder (MAX_UINT64, Multiplier, > > +NULL)) > > { > > + // > > + // Make sure Multiplicand is the bigger one. > > + // > > + if (Multiplicand < Multiplier) { > > + Uint64 =3D Multiplicand; > > + Multiplicand =3D Multiplier; > > + Multiplier =3D Uint64; > > + } > > + // > > + // Because Multiplicand * Multiplier overflows, > > + // Multiplicand * Multiplier / Divisor > > + // =3D (2 * Multiplicand' + 1) * Multiplier / Divisor > > + // =3D 2 * (Multiplicand' * Multiplier / Divisor) + Multiplier / D= ivisor > > + // > > + Uint64 =3D MultThenDivU64x64x32 (RShiftU64 (Multiplicand, 1),=20 > > + Multiplier, > > Divisor, &LocalRemainder); > > + Uint64 =3D LShiftU64 (Uint64, 1); > > + Uint32 =3D 0; > > + if ((Multiplicand & 0x1) =3D=3D 1) { > > + Uint64 +=3D DivU64x32Remainder (Multiplier, Divisor, &Uint32); > > + } > > + return Uint64 + DivU64x32Remainder (Uint32 + LShiftU64=20 > > +(LocalRemainder, 1), Divisor, Remainder); > > + } else { > > + return DivU64x32Remainder (MultU64x64 (Multiplicand,=20 > > +Multiplier), Divisor, Remainder); > > + } > > +} > > + > > +/** > > + Return the elapsed tick count from CurrentTick. > > + > > + @param CurrentTick On input, the previous tick count. > > + On output, the current tick count. > > + @param StartTick The value the performance counter starts with w= hen > it > > + rolls over. > > + @param EndTick The value that the performance counter ends wit= h > > before > > + it rolls over. > > + > > + @return The elapsed tick count from CurrentTick. > > +**/ > > +UINT64 > > +GetElapsedTick ( > > + UINT64 *CurrentTick, > > + UINT64 StartTick, > > + UINT64 EndTick > > + ) > > +{ > > + UINT64 PreviousTick; > > + > > + PreviousTick =3D *CurrentTick; > > + *CurrentTick =3D GetPerformanceCounter(); > > + if (StartTick < EndTick) { > > + return *CurrentTick - PreviousTick; > > + } else { > > + return PreviousTick - *CurrentTick; > > + } > > +} > > + > > /** > > Polls an address in memory mapped I/O space until an exit=20 > > condition is > met, > > or a timeout occurs. > > @@ -517,6 +596,11 @@ RootBridgeIoPollMem ( > > EFI_STATUS Status; > > UINT64 NumberOfTicks; > > UINT32 Remainder; > > + UINT64 StartTick; > > + UINT64 EndTick; > > + UINT64 CurrentTick; > > + UINT64 ElapsedTick; > > + UINT64 Frequency; > > > > if (Result =3D=3D NULL) { > > return EFI_INVALID_PARAMETER; > > @@ -542,28 +626,18 @@ RootBridgeIoPollMem ( > > return EFI_SUCCESS; > > > > } else { > > - > > - // > > - // Determine the proper # of metronome ticks to wait for polling t= he > > - // location. The nuber of ticks is Roundup (Delay / > > - // mMetronome->TickPeriod)+1 > > - // The "+1" to account for the possibility of the first tick being= short > > - // because we started in the middle of a tick. > > // > > - // BugBug: overriding mMetronome->TickPeriod with UINT32 until > > Metronome > > - // protocol definition is updated. > > + // NumberOfTicks =3D Frenquency * Delay / > > EFI_TIMER_PERIOD_SECONDS(1) > > // > > - NumberOfTicks =3D DivU64x32Remainder (Delay, (UINT32) mMetronome- > > >TickPeriod, > > - &Remainder); > > - if (Remainder !=3D 0) { > > - NumberOfTicks +=3D 1; > > + Frequency =3D GetPerformanceCounterProperties (&StartTick, > &EndTick); > > + NumberOfTicks =3D MultThenDivU64x64x32 (Frequency, Delay, > > (UINT32)EFI_TIMER_PERIOD_SECONDS(1), &Remainder); > > + if (Remainder >=3D (UINTN)EFI_TIMER_PERIOD_SECONDS(1) / 2) { > > + NumberOfTicks++; > > } > > - NumberOfTicks +=3D 1; > > - > > - while (NumberOfTicks !=3D 0) { > > - > > - mMetronome->WaitForTick (mMetronome, 1); > > - > > + for ( ElapsedTick =3D 0, CurrentTick =3D GetPerformanceCounter() > > + ; ElapsedTick <=3D NumberOfTicks > > + ; ElapsedTick +=3D GetElapsedTick (&CurrentTick, StartTick, En= dTick) > > + ) { > > Status =3D This->Mem.Read (This, Width, Address, 1, Result); > > if (EFI_ERROR (Status)) { > > return Status; > > @@ -572,8 +646,6 @@ RootBridgeIoPollMem ( > > if ((*Result & Mask) =3D=3D Value) { > > return EFI_SUCCESS; > > } > > - > > - NumberOfTicks -=3D 1; > > } > > } > > return EFI_TIMEOUT; > > @@ -626,6 +698,11 @@ RootBridgeIoPollIo ( > > EFI_STATUS Status; > > UINT64 NumberOfTicks; > > UINT32 Remainder; > > + UINT64 StartTick; > > + UINT64 EndTick; > > + UINT64 CurrentTick; > > + UINT64 ElapsedTick; > > + UINT64 Frequency; > > > > // > > // No matter what, always do a single poll. > > @@ -651,25 +728,18 @@ RootBridgeIoPollIo ( > > return EFI_SUCCESS; > > > > } else { > > - > > // > > - // Determine the proper # of metronome ticks to wait for polling t= he > > - // location. The number of ticks is Roundup (Delay / > > - // mMetronome->TickPeriod)+1 > > - // The "+1" to account for the possibility of the first tick being= short > > - // because we started in the middle of a tick. > > + // NumberOfTicks =3D Frenquency * Delay / > > EFI_TIMER_PERIOD_SECONDS(1) > > // > > - NumberOfTicks =3D DivU64x32Remainder (Delay, (UINT32)mMetronome- > > >TickPeriod, > > - &Remainder); > > - if (Remainder !=3D 0) { > > - NumberOfTicks +=3D 1; > > + Frequency =3D GetPerformanceCounterProperties (&StartTick, > &EndTick); > > + NumberOfTicks =3D MultThenDivU64x64x32 (Frequency, Delay, > > (UINT32)EFI_TIMER_PERIOD_SECONDS(1), &Remainder); > > + if (Remainder >=3D (UINTN)EFI_TIMER_PERIOD_SECONDS(1) / 2) { > > + NumberOfTicks++; > > } > > - NumberOfTicks +=3D 1; > > - > > - while (NumberOfTicks !=3D 0) { > > - > > - mMetronome->WaitForTick (mMetronome, 1); > > - > > + for ( ElapsedTick =3D 0, CurrentTick =3D GetPerformanceCounter() > > + ; ElapsedTick <=3D NumberOfTicks > > + ; ElapsedTick +=3D GetElapsedTick (&CurrentTick, StartTick, En= dTick) > > + ) { > > Status =3D This->Io.Read (This, Width, Address, 1, Result); > > if (EFI_ERROR (Status)) { > > return Status; > > @@ -678,13 +748,14 @@ RootBridgeIoPollIo ( > > if ((*Result & Mask) =3D=3D Value) { > > return EFI_SUCCESS; > > } > > - > > - NumberOfTicks -=3D 1; > > } > > } > > return EFI_TIMEOUT; > > } > > > > + > > + > > + > > /** > > Enables a PCI driver to access PCI controller registers in the PCI r= oot > > bridge memory space. > > -- > > 2.16.1.windows.1