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

Ruiyu Ni posted 1 patch 6 years ago
Failed in applying to current master (apply log)
.../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(-)
[edk2] [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling
Posted by Ruiyu Ni 6 years ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling
Posted by Zeng, Star 5 years, 11 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling
Posted by Ni, Ruiyu 5 years, 11 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling
Posted by Ni, Ruiyu 5 years, 11 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling
Posted by Zeng, Star 5 years, 11 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PciHostBridge: Count the (mm)io overhead when polling
Posted by Zeng, Star 5 years, 11 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel