Re: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling

["Zeng, Star" <star.zeng@intel.com>]

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 
Sent: Wednesday, May 2, 2018 2:39 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; 'edk2-devel@lists.01.org' <edk2-devel@lists.01.org>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling

Reviewed-by: Star Zeng <star.zeng@intel.com> if it is updated. :)


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Wednesday, May 2, 2018 2:37 PM
To: Zeng, Star <star.zeng@intel.com>; 'edk2-devel@lists.01.org' <edk2-devel@lists.01.org>
Cc: Chiu, Chasel <chasel.chiu@intel.com>
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 <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.chiu@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io 
> overhead when polling
> 
> 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zeng, Star 
> > <star.zeng@intel.com>
> > 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: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Chiu, Chasel 
> > <chasel.chiu@intel.com>
> > 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 <ruiyu.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > ---
> >  .../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