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

["Henz, Patrick" <patrick.henz@hpe.com>]

I can certainly make that change.

For what it's worth I have been working on adding the new function, GetElapsedTicks, to the various implementations of TimerLib. I've finished up testing, I would just need to clean up the commits for a patch. Should I move forward with that, or would we rather I just add XhcGetElapsedTime to XhciDxe for the time being?

Thanks,
Patrick Henz

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Sunday, July 30, 2023 9:57 PM
To: devel@edk2.groups.io; Henz, Patrick <patrick.henz@hpe.com>; Michael Brown <mcb30@ipxe.org>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

For the 2 occurrences of in XhcWaitOpRegBit & XhcExecTransfer:
  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND (Timeout * XHC_1_MILLISECOND); How about changing them to:
  TimeoutTime = XHC_MICROSECOND_TO_NANOSECOND ((UINT64) Timeout * XHC_1_MILLISECOND); to address possible overflow during "Timeout * XHC_1_MILLISECOND"?

For extending XhcGetElapsedTime as a TimerLib API, I am fine to put it in XhciDxe at this moment.
If package maintainers suggest to make it as a public library API, my take is that this should be done in a separate commit.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz, 
> Patrick
> Sent: Thursday, July 6, 2023 4:16 AM
> To: devel@edk2.groups.io
> Cc: Henz, Patrick <patrick.henz@hpe.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance 
> Timer for XHCI Timeouts
> 
> REF:INVALID URI REMOVED
> g.cgi?id=2948__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5
> _B3K1Lcta452Gx-1twRt8At3cueAYDq_n$
> 
> 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..2499698715 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 (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 298fb88b81..5177886e52 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>
> 
> @@ -1273,15 +1274,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
> 
> @@ -1296,12 +1296,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) {
> 
> @@ -1319,34 +1321,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 (Timeout *
> XHC_1_MILLISECOND);
> 
> +  ElapsedTime = 0;
> 
> +  CurrentTick = GetPerformanceCounter ();
> 
> +
> 
>    do {
> 
>      Finished = XhcCheckUrbResult (Xhc, Urb);
> 
>      if (Finished) {
> 
> @@ -1354,22 +1336,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 (#106668):
> INVALID URI REMOVED
> 668__;!!NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5_B3K1Lcta4
> 52Gx-1twRt8At3cuWxQsO18$ Mute This Topic: 
> https://groups.io/mt/99972791/1768737
> NpxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5_B3K1Lcta452Gx-1tw
> Rt8At3cubh2HDyk$ Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: 
> https://edk2.groups.io/g/devel/unsub
> pxR!kyFQM5IkKYAG9CRBO4xphwBnzi_jhb2oU-NKbMjOV-lctg5_B3K1Lcta452Gx-1twR
> t8At3cuWYMMXVU$  [hao.a.wu@intel.com] -=-=-=-=-=-=
> 








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