If Multiplicand * Multiplier + Remainder = MAX_UINT64, Even Multiplicand = MAX_UINT64 / Multiplier, Overflow still happens.
So ">=" is used here. Thanks/Ray > -----Original Message----- > From: Zeng, Star > Sent: Wednesday, May 2, 2018 11:44 AM > To: Ni, Ruiyu <[email protected]>; [email protected] > Cc: Chiu, Chasel <[email protected]>; Zeng, Star <[email protected]> > Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io > overhead when polling > > Is it more accurate > if (Multiplicand >= 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: [email protected] > Cc: Zeng, Star <[email protected]>; Chiu, Chasel <[email protected]> > Subject: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io > overhead when polling > > RootBridgeIo.PollMem()/PollIo() originally don't count the IO/MMIO access > overhead when delaying. > The patch changes the implementation to count the access overhead so that > the actually delay equals to user required delay. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni <[email protected]> > Cc: Star Zeng <[email protected]> > Cc: Chasel Chiu <[email protected]> > --- > .../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 reserved.<BR> > +# Copyright (c) 2009 - 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 @@ -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 reserved.<BR> > +Copyright (c) 1999 - 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 > @@ -36,6 +36,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, EITHER EXPRESS OR IMPLIED. > #include <Library/BaseLib.h> > #include <Library/PciSegmentLib.h> > #include <Library/UefiLib.h> > +#include <Library/TimerLib.h> > #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 reserved.<BR> > +Copyright (c) 1999 - 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 > @@ -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 >= DivU64x64Remainder (MAX_UINT64, Multiplier, NULL)) > { > + // > + // Make sure Multiplicand is the bigger one. > + // > + if (Multiplicand < Multiplier) { > + Uint64 = Multiplicand; > + Multiplicand = Multiplier; > + Multiplier = Uint64; > + } > + // > + // Because Multiplicand * Multiplier overflows, > + // Multiplicand * Multiplier / Divisor > + // = (2 * Multiplicand' + 1) * Multiplier / Divisor > + // = 2 * (Multiplicand' * Multiplier / Divisor) + Multiplier / Divisor > + // > + Uint64 = MultThenDivU64x64x32 (RShiftU64 (Multiplicand, 1), Multiplier, > Divisor, &LocalRemainder); > + Uint64 = LShiftU64 (Uint64, 1); > + Uint32 = 0; > + if ((Multiplicand & 0x1) == 1) { > + Uint64 += DivU64x32Remainder (Multiplier, Divisor, &Uint32); > + } > + return Uint64 + DivU64x32Remainder (Uint32 + LShiftU64 > +(LocalRemainder, 1), Divisor, Remainder); > + } else { > + return DivU64x32Remainder (MultU64x64 (Multiplicand, 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 when it > + rolls over. > + @param EndTick The value that the performance counter ends with > before > + it rolls over. > + > + @return The elapsed tick count from CurrentTick. > +**/ > +UINT64 > +GetElapsedTick ( > + UINT64 *CurrentTick, > + UINT64 StartTick, > + UINT64 EndTick > + ) > +{ > + UINT64 PreviousTick; > + > + PreviousTick = *CurrentTick; > + *CurrentTick = GetPerformanceCounter(); > + if (StartTick < EndTick) { > + return *CurrentTick - PreviousTick; > + } else { > + return PreviousTick - *CurrentTick; > + } > +} > + > /** > Polls an address in memory mapped I/O space until an exit 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 == NULL) { > return EFI_INVALID_PARAMETER; > @@ -542,28 +626,18 @@ RootBridgeIoPollMem ( > return EFI_SUCCESS; > > } else { > - > - // > - // Determine the proper # of metronome ticks to wait for polling the > - // 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 = Frenquency * Delay / > EFI_TIMER_PERIOD_SECONDS(1) > // > - NumberOfTicks = DivU64x32Remainder (Delay, (UINT32) mMetronome- > >TickPeriod, > - &Remainder); > - if (Remainder != 0) { > - NumberOfTicks += 1; > + Frequency = GetPerformanceCounterProperties (&StartTick, &EndTick); > + NumberOfTicks = MultThenDivU64x64x32 (Frequency, Delay, > (UINT32)EFI_TIMER_PERIOD_SECONDS(1), &Remainder); > + if (Remainder >= (UINTN)EFI_TIMER_PERIOD_SECONDS(1) / 2) { > + NumberOfTicks++; > } > - NumberOfTicks += 1; > - > - while (NumberOfTicks != 0) { > - > - mMetronome->WaitForTick (mMetronome, 1); > - > + for ( ElapsedTick = 0, CurrentTick = GetPerformanceCounter() > + ; ElapsedTick <= NumberOfTicks > + ; ElapsedTick += GetElapsedTick (&CurrentTick, StartTick, EndTick) > + ) { > Status = This->Mem.Read (This, Width, Address, 1, Result); > if (EFI_ERROR (Status)) { > return Status; > @@ -572,8 +646,6 @@ RootBridgeIoPollMem ( > if ((*Result & Mask) == Value) { > return EFI_SUCCESS; > } > - > - NumberOfTicks -= 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 the > - // 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 = Frenquency * Delay / > EFI_TIMER_PERIOD_SECONDS(1) > // > - NumberOfTicks = DivU64x32Remainder (Delay, (UINT32)mMetronome- > >TickPeriod, > - &Remainder); > - if (Remainder != 0) { > - NumberOfTicks += 1; > + Frequency = GetPerformanceCounterProperties (&StartTick, &EndTick); > + NumberOfTicks = MultThenDivU64x64x32 (Frequency, Delay, > (UINT32)EFI_TIMER_PERIOD_SECONDS(1), &Remainder); > + if (Remainder >= (UINTN)EFI_TIMER_PERIOD_SECONDS(1) / 2) { > + NumberOfTicks++; > } > - NumberOfTicks += 1; > - > - while (NumberOfTicks != 0) { > - > - mMetronome->WaitForTick (mMetronome, 1); > - > + for ( ElapsedTick = 0, CurrentTick = GetPerformanceCounter() > + ; ElapsedTick <= NumberOfTicks > + ; ElapsedTick += GetElapsedTick (&CurrentTick, StartTick, EndTick) > + ) { > Status = This->Io.Read (This, Width, Address, 1, Result); > if (EFI_ERROR (Status)) { > return Status; > @@ -678,13 +748,14 @@ RootBridgeIoPollIo ( > if ((*Result & Mask) == Value) { > return EFI_SUCCESS; > } > - > - NumberOfTicks -= 1; > } > } > return EFI_TIMEOUT; > } > > + > + > + > /** > Enables a PCI driver to access PCI controller registers in the PCI root > bridge memory space. > -- > 2.16.1.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
