Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

["Wu, Hao A" <hao.a.wu@intel.com>]

Speaking of PcAtChipsetPkg/Library/AcpiTimerLib, the implementation of below instances:
* PeiAcpiTimerLib.inf
* DxeAcpiTimerLib.inf
* StandaloneMmAcpiTimerLib.inf
will not calculate the TSC frequency upon every call of GetPerformanceCounterProperties().

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Thursday, September 7, 2023 3:48 PM
> To: Mike Maslenkin <mike.maslenkin@gmail.com>; devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> Performance Timer for XHCI Timeouts
> 
> I do not have strong opinion on this considering it is an IO driver.
> The call of GetTimeInNanoSecond() is also likely to invoke
> GetPerformanceCounterProperties() call.
> 
> I will let the patch owner to decide on the open raised below.
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: Mike Maslenkin <mike.maslenkin@gmail.com>
> > Sent: Thursday, September 7, 2023 3:36 PM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>
> > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > Performance Timer for XHCI Timeouts
> >
> > Hello Hao Wu,
> >
> > Isn't GetPerformanceCounterProperties (&StartValue, &EndValue) call
> > too "heavy" for XHCI paths?
> > May be it's better to cache timer direction once?
> > I'm asking in a context of PcAtChipsetPkg/Library/AcpiTimerLib
> > instance used by a number of Intel's platform in edk-platforms.
> > For this  library GetPerformanceCounterProperties finally calls
> > InternalCalculateTscFrequency, that isn't simple.
> >
> > Regards,
> > Mike
> >
> > On Thu, Sep 7, 2023 at 4:27 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> > >
> > > Sorry for the late response.
> > > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Thursday, September 7, 2023 4:37 AM
> > > > To: devel@edk2.groups.io
> > > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > I sent this patch out a few weeks ago now but haven't seen a
> > > > reply, just checking in to make sure it didn't get missed.
> > > >
> > > > Thanks,
> > > > Patrick Henz
> > > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Henz, Patrick
> > > > Sent: Monday, August 14, 2023 10:52 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Henz, Patrick <patrick.henz@hpe.com>
> > > > Subject: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use
> > > > Performance Timer for XHCI Timeouts
> > > >
> > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948
> > > >
> > > > XhciDxe uses the timer functionality provided by the boot services
> > > > table to detect timeout conditions. This breaks the driver's
> > > > ExitBootServices call back, as CoreExitBootServices halts the
> > > > timer before signaling the ExitBootServices event. If the host
> > > > controller fails to halt in the call back, the timeout condition
> > > > will never occur and the boot gets stuck in an indefinite spin
> > > > loop. Use the free running timer provided by TimerLib to calculate
> > > > timeouts, avoiding
> > the potential hang.
> > > >
> > > > Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> > > > Reviewed-by:
> > > > ---
> > > >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 56 +++++++++++++++++++
> > > >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h      | 22 ++++++++
> > > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
> > > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +++++++++--------------
> > > >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68
> > > > +++++++++---------------
> > > >  5 files changed, 129 insertions(+), 86 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > > index 62682dd27c..1dcbe20512 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > > > @@ -1,6 +1,7 @@
> > > >  /** @file
> > > >    The XHCI controller driver.
> > > >
> > > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > > >  Copyright (c) 2011 - 2022, Intel Corporation. All rights
> > > > reserved.<BR>
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > > @@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
> > > >
> > > >    return EFI_SUCCESS;
> > > >  }
> > > > +
> > > > +/**
> > > > +  Computes and returns the elapsed time in nanoseconds since
> > > > +  PreviousTick. The value of PreviousTick is overwritten with the
> > > > +  current performance counter value.
> > > > +
> > > > +  @param  PreviousTick    Pointer to PreviousTick count.
> > > > +  @return The elapsed time in nanoseconds since PreviousCount.
> > > > +          PreviousCount is overwritten with the current performance
> > > > +          counter value.
> > > > +**/
> > > > +UINT64
> > > > +XhcGetElapsedTime (
> > > > +  IN OUT UINT64  *PreviousTick
> > > > +  )
> > > > +{
> > > > +  UINT64  StartValue;
> > > > +  UINT64  EndValue;
> > > > +  UINT64  CurrentTick;
> > > > +  UINT64  Delta;
> > > > +
> > > > +  GetPerformanceCounterProperties (&StartValue, &EndValue);
> > > > +
> > > > +  CurrentTick = GetPerformanceCounter ();
> > > > +
> > > > +  //
> > > > +  // Determine if the counter is counting up or down  //  if
> > > > + (StartValue < EndValue) {
> > > > +    //
> > > > +    // Counter counts upwards, check for an overflow condition
> > > > +    //
> > > > +    if (*PreviousTick > CurrentTick) {
> > > > +      Delta = (EndValue - *PreviousTick) + CurrentTick;
> > > > +    } else {
> > > > +      Delta = CurrentTick - *PreviousTick;
> > > > +    }
> > > > +  } else {
> > > > +    //
> > > > +    // Counter counts downwards, check for an underflow condition
> > > > +    //
> > > > +    if (*PreviousTick < CurrentTick) {
> > > > +      Delta = (StartValue - CurrentTick) + *PreviousTick;
> > > > +    } else {
> > > > +      Delta = *PreviousTick - CurrentTick;
> > > > +    }
> > > > +  }
> > > > +
> > > > +  //
> > > > +  // Set PreviousTick to CurrentTick  //  *PreviousTick =
> > > > + CurrentTick;
> > > > +
> > > > +  return GetTimeInNanoSecond (Delta); }
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > > index ca223bd20c..77feb66647 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> > > > @@ -2,6 +2,7 @@
> > > >
> > > >    Provides some data structure definitions used by the XHCI host
> > > > controller driver.
> > > >
> > > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > > >  Copyright (c) 2011 - 2017, Intel Corporation. All rights
> > > > reserved.<BR> Copyright (c) Microsoft Corporation.<BR>
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -26,6 +27,7 @@
> > > > SPDX-
> > > > License-Identifier: BSD-2-Clause-Patent  #include
> > > > <Library/UefiLib.h>  #include <Library/DebugLib.h>  #include
> > > > <Library/ReportStatusCodeLib.h>
> > > > +#include <Library/TimerLib.h>
> > > >
> > > >  #include <IndustryStandard/Pci.h>
> > > >
> > > > @@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXT
> > USB_DEV_CONTEXT;
> > > > #include "ComponentName.h"
> > > >  #include "UsbHcMem.h"
> > > >
> > > > +//
> > > > +// Converts a count from microseconds to nanoseconds // #define
> > > > +XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) *
> 1000)
> > > >  //
> > > >  // The unit is microsecond, setting it as 1us.
> > > >  //
> > > > @@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
> > > >    IN     VOID                                *Context
> > > >    );
> > > >
> > > > +/**
> > > > +  Computes and returns the elapsed time in nanoseconds since
> > > > +  PreviousTick. The value of PreviousTick is overwritten with the
> > > > +  current performance counter value.
> > > > +
> > > > +  @param  PreviousTick    Pointer to PreviousTick count.
> > > > +                          before calling this function.
> > > > +  @return The elapsed time in nanoseconds since PreviousCount.
> > > > +          PreviousCount is overwritten with the current performance
> > > > +          counter value.
> > > > +**/
> > > > +UINT64
> > > > +XhcGetElapsedTime (
> > > > +  IN OUT UINT64  *PreviousTick
> > > > +  );
> > > > +
> > > >  #endif
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > > index 5865d86822..18ef87916a 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
> > > > @@ -3,6 +3,7 @@
> > > >  #  It implements the interfaces of monitoring the status of all
> > > > ports and transferring  #  Control, Bulk, Interrupt and
> > > > Isochronous requests to those attached usb LS/FS/HS/SS devices.
> > > >  #
> > > > +#  (C) Copyright 2023 Hewlett Packard Enterprise Development
> > > > +LP<BR>
> > > >  #  Copyright (c) 2011 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>  #  #
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent @@ -54,6 +55,7 @@
> > > >    BaseMemoryLib
> > > >    DebugLib
> > > >    ReportStatusCodeLib
> > > > +  TimerLib
> > > >
> > > >  [Guids]
> > > >    gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES ##
> > > > Event
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > index 5700fc5fb8..07e57772b6 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> > > > @@ -2,6 +2,7 @@
> > > >
> > > >    The XHCI register operation routines.
> > > >
> > > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > > >  Copyright (c) 2011 - 2017, Intel Corporation. All rights
> > > > reserved.<BR>
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > > @@ -417,15 +418,14 @@ XhcClearOpRegBit (
> > > >    Wait the operation register's bit as specified by Bit
> > > >    to become set (or clear).
> > > >
> > > > -  @param  Xhc                    The XHCI Instance.
> > > > -  @param  Offset                 The offset of the operation register.
> > > > -  @param  Bit                    The bit of the register to wait for.
> > > > -  @param  WaitToSet              Wait the bit to set or clear.
> > > > -  @param  Timeout                The time to wait before abort (in
> millisecond,
> > > > ms).
> > > > +  @param  Xhc          The XHCI Instance.
> > > > +  @param  Offset       The offset of the operation register.
> > > > +  @param  Bit          The bit of the register to wait for.
> > > > +  @param  WaitToSet    Wait the bit to set or clear.
> > > > +  @param  Timeout      The time to wait before abort (in millisecond,
> ms).
> > > >
> > > > -  @retval EFI_SUCCESS            The bit successfully changed by host
> > controller.
> > > > -  @retval EFI_TIMEOUT            The time out occurred.
> > > > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
> > not
> > > > be allocated.
> > > > +  @retval EFI_SUCCESS  The bit successfully changed by host controller.
> > > > +  @retval EFI_TIMEOUT  The time out occurred.
> > > >
> > > >  **/
> > > >  EFI_STATUS
> > > > @@ -437,54 +437,35 @@ XhcWaitOpRegBit (
> > > >    IN UINT32             Timeout
> > > >    )
> > > >  {
> > > > -  EFI_STATUS  Status;
> > > > -  EFI_EVENT   TimeoutEvent;
> > > > -
> > > > -  TimeoutEvent = NULL;
> > > > +  UINT64  TimeoutTime;
> > > > +  UINT64  ElapsedTime;
> > > > +  UINT64  TimeDelta;
> > > > +  UINT64  CurrentTick;
> > > >
> > > >    if (Timeout == 0) {
> > > >      return EFI_TIMEOUT;
> > > >    }
> > > >
> > > > -  Status = gBS->CreateEvent (
> > > > -                  EVT_TIMER,
> > > > -                  TPL_CALLBACK,
> > > > -                  NULL,
> > > > -                  NULL,
> > > > -                  &TimeoutEvent
> > > > -                  );
> > > > -
> > > > -  if (EFI_ERROR (Status)) {
> > > > -    goto DONE;
> > > > -  }
> > > > -
> > > > -  Status = gBS->SetTimer (
> > > > -                  TimeoutEvent,
> > > > -                  TimerRelative,
> > > > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> > > > -                  );
> > > > -
> > > > -  if (EFI_ERROR (Status)) {
> > > > -    goto DONE;
> > > > -  }
> > > > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND
> > ((UINT64)Timeout
> > > > *
> > > > + XHC_1_MILLISECOND);  ElapsedTime = 0;  CurrentTick =
> > > > + GetPerformanceCounter ();
> > > >
> > > >    do {
> > > >      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> > > > -      Status = EFI_SUCCESS;
> > > > -      goto DONE;
> > > > +      return EFI_SUCCESS;
> > > >      }
> > > >
> > > >      gBS->Stall (XHC_1_MICROSECOND);
> > > > -  } while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent)));
> > > > -
> > > > -  Status = EFI_TIMEOUT;
> > > > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> > > > +    // Ensure that ElapsedTime is always incremented to avoid
> > > > + indefinite
> > > > hangs
> > > > +    if (TimeDelta == 0) {
> > > > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > > > (XHC_1_MICROSECOND);
> > > > +    }
> > > >
> > > > -DONE:
> > > > -  if (TimeoutEvent != NULL) {
> > > > -    gBS->CloseEvent (TimeoutEvent);
> > > > -  }
> > > > +    ElapsedTime += TimeDelta;
> > > > +  } while (ElapsedTime < TimeoutTime);
> > > >
> > > > -  return Status;
> > > > +  return EFI_TIMEOUT;
> > > >  }
> > > >
> > > >  /**
> > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > index 53421e64a8..7d4b7a769d 100644
> > > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > > > @@ -2,6 +2,7 @@
> > > >
> > > >    XHCI transfer scheduling routines.
> > > >
> > > > +(C) Copyright 2023 Hewlett Packard Enterprise Development LP<BR>
> > > >  Copyright (c) 2011 - 2020, Intel Corporation. All rights
> > > > reserved.<BR> Copyright (c) Microsoft Corporation.<BR>  Copyright
> > > > (C) 2022 Advanced Micro Devices, Inc. All rights reserved.<BR> @@
> > > > -
> > 1276,15 +1277,14 @@ EXIT:
> > > >  /**
> > > >    Execute the transfer by polling the URB. This is a synchronous
> operation.
> > > >
> > > > -  @param  Xhc                    The XHCI Instance.
> > > > -  @param  CmdTransfer            The executed URB is for cmd transfer or
> > not.
> > > > -  @param  Urb                    The URB to execute.
> > > > -  @param  Timeout                The time to wait before abort, in
> millisecond.
> > > > +  @param  Xhc               The XHCI Instance.
> > > > +  @param  CmdTransfer       The executed URB is for cmd transfer or
> not.
> > > > +  @param  Urb               The URB to execute.
> > > > +  @param  Timeout           The time to wait before abort, in millisecond.
> > > >
> > > > -  @return EFI_DEVICE_ERROR       The transfer failed due to transfer
> error.
> > > > -  @return EFI_TIMEOUT            The transfer failed due to time out.
> > > > -  @return EFI_SUCCESS            The transfer finished OK.
> > > > -  @retval EFI_OUT_OF_RESOURCES   Memory for the timer event could
> > not
> > > > be allocated.
> > > > +  @return EFI_DEVICE_ERROR  The transfer failed due to transfer error.
> > > > +  @return EFI_TIMEOUT       The transfer failed due to time out.
> > > > +  @return EFI_SUCCESS       The transfer finished OK.
> > > >
> > > >  **/
> > > >  EFI_STATUS
> > > > @@ -1299,12 +1299,14 @@ XhcExecTransfer (
> > > >    UINT8       SlotId;
> > > >    UINT8       Dci;
> > > >    BOOLEAN     Finished;
> > > > -  EFI_EVENT   TimeoutEvent;
> > > > +  UINT64      TimeoutTime;
> > > > +  UINT64      ElapsedTime;
> > > > +  UINT64      TimeDelta;
> > > > +  UINT64      CurrentTick;
> > > >    BOOLEAN     IndefiniteTimeout;
> > > >
> > > >    Status            = EFI_SUCCESS;
> > > >    Finished          = FALSE;
> > > > -  TimeoutEvent      = NULL;
> > > >    IndefiniteTimeout = FALSE;
> > > >
> > > >    if (CmdTransfer) {
> > > > @@ -1322,34 +1324,14 @@ XhcExecTransfer (
> > > >
> > > >    if (Timeout == 0) {
> > > >      IndefiniteTimeout = TRUE;
> > > > -    goto RINGDOORBELL;
> > > > -  }
> > > > -
> > > > -  Status = gBS->CreateEvent (
> > > > -                  EVT_TIMER,
> > > > -                  TPL_CALLBACK,
> > > > -                  NULL,
> > > > -                  NULL,
> > > > -                  &TimeoutEvent
> > > > -                  );
> > > > -
> > > > -  if (EFI_ERROR (Status)) {
> > > > -    goto DONE;
> > > >    }
> > > >
> > > > -  Status = gBS->SetTimer (
> > > > -                  TimeoutEvent,
> > > > -                  TimerRelative,
> > > > -                  EFI_TIMER_PERIOD_MILLISECONDS (Timeout)
> > > > -                  );
> > > > -
> > > > -  if (EFI_ERROR (Status)) {
> > > > -    goto DONE;
> > > > -  }
> > > > -
> > > > -RINGDOORBELL:
> > > >    XhcRingDoorBell (Xhc, SlotId, Dci);
> > > >
> > > > +  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND
> > ((UINT64)Timeout
> > > > *
> > > > + XHC_1_MILLISECOND);  ElapsedTime = 0;  CurrentTick =
> > > > + GetPerformanceCounter ();
> > > > +
> > > >    do {
> > > >      Finished = XhcCheckUrbResult (Xhc, Urb);
> > > >      if (Finished) {
> > > > @@ -1357,22 +1339,22 @@ RINGDOORBELL:
> > > >      }
> > > >
> > > >      gBS->Stall (XHC_1_MICROSECOND);
> > > > -  } while (IndefiniteTimeout || EFI_ERROR (gBS->CheckEvent
> > > > (TimeoutEvent)));
> > > > +    TimeDelta = XhcGetElapsedTime (&CurrentTick);
> > > > +    // Ensure that ElapsedTime is always incremented to avoid
> > > > + indefinite
> > > > hangs
> > > > +    if (TimeDelta == 0) {
> > > > +      TimeDelta = XHC_MICROSECOND_TO_NANOSECOND
> > > > (XHC_1_MICROSECOND);
> > > > +    }
> > > >
> > > > -DONE:
> > > > -  if (EFI_ERROR (Status)) {
> > > > -    Urb->Result = EFI_USB_ERR_NOTEXECUTE;
> > > > -  } else if (!Finished) {
> > > > +    ElapsedTime += TimeDelta;
> > > > +  } while (IndefiniteTimeout || ElapsedTime < TimeoutTime);
> > > > +
> > > > +  if (!Finished) {
> > > >      Urb->Result = EFI_USB_ERR_TIMEOUT;
> > > >      Status      = EFI_TIMEOUT;
> > > >    } else if (Urb->Result != EFI_USB_NOERROR) {
> > > >      Status = EFI_DEVICE_ERROR;
> > > >    }
> > > >
> > > > -  if (TimeoutEvent != NULL) {
> > > > -    gBS->CloseEvent (TimeoutEvent);
> > > > -  }
> > > > -
> > > >    return Status;
> > > >  }
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> > >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108364): https://edk2.groups.io/g/devel/message/108364
Mute This Topic: https://groups.io/mt/100739508/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-