[edk2-devel] Add LocalApicTimerDxe driver in UefiCpuPkg

[edk2-devel] Add LocalApicTimerDxe driver in UefiCpuPkg

[Ni, Ray]

Ray Ni (6):
  UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe
    driver
  UefiCpuPkg/LocalApicTimerDxe: Remove NestedInterruptTplLib call
  UefiCpuPkg: Add LocalApicTimerDxe driver in DSC file
  UefiCpuPkg/LocalApicTimerDxe: Enhance Timer Frequency calculation
    logic
  UefiCpuPkg/LocalApicTimerDxe: warn if APIC Timer is used by other code
  UefiCpuPkg/LocalApicTimerDxe: Passing fixed timer period is not a bug.

 .../LocalApicTimerDxe/LocalApicTimerDxe.c     | 464 ++++++++++++++++++
 .../LocalApicTimerDxe/LocalApicTimerDxe.h     | 180 +++++++
 .../LocalApicTimerDxe/LocalApicTimerDxe.inf   |  42 ++
 UefiCpuPkg/UefiCpuPkg.dsc                     |   3 +-
 4 files changed, 688 insertions(+), 1 deletion(-)
 create mode 100644 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
 create mode 100644 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
 create mode 100644 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf

-- 
2.39.1.windows.1



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

[edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Ni, Ray]

This commit only duplicates the OvmfPkg/LocalApicTimerDxe.
Following commits will enhance the driver.

FILE_GUID in INF file is updated with a new GUID.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 .../LocalApicTimerDxe/LocalApicTimerDxe.c     | 365 ++++++++++++++++++
 .../LocalApicTimerDxe/LocalApicTimerDxe.h     | 178 +++++++++
 .../LocalApicTimerDxe/LocalApicTimerDxe.inf   |  44 +++
 3 files changed, 587 insertions(+)
 create mode 100644 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
 create mode 100644 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
 create mode 100644 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf

diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
new file mode 100644
index 0000000000..67f4dcde37
--- /dev/null
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -0,0 +1,365 @@
+/** @file
+  Timer Architectural Protocol as defined in the DXE CIS
+
+Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2019, Citrix Systems, Inc.
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/NestedInterruptTplLib.h>
+
+#include "LocalApicTimerDxe.h"
+
+//
+// The handle onto which the Timer Architectural Protocol will be installed
+//
+EFI_HANDLE  mTimerHandle = NULL;
+
+//
+// The Timer Architectural Protocol that this driver produces
+//
+EFI_TIMER_ARCH_PROTOCOL  mTimer = {
+  TimerDriverRegisterHandler,
+  TimerDriverSetTimerPeriod,
+  TimerDriverGetTimerPeriod,
+  TimerDriverGenerateSoftInterrupt
+};
+
+//
+// Pointer to the CPU Architectural Protocol instance
+//
+EFI_CPU_ARCH_PROTOCOL  *mCpu;
+
+//
+// The notification function to call on every timer interrupt.
+// A bug in the compiler prevents us from initializing this here.
+//
+EFI_TIMER_NOTIFY  mTimerNotifyFunction;
+
+//
+// The current period of the timer interrupt
+//
+volatile UINT64  mTimerPeriod = 0;
+
+//
+// Worker Functions
+//
+
+/**
+  Interrupt Handler.
+
+  @param InterruptType    The type of interrupt that occurred
+  @param SystemContext    A pointer to the system context when the interrupt occurred
+**/
+VOID
+EFIAPI
+TimerInterruptHandler (
+  IN EFI_EXCEPTION_TYPE  InterruptType,
+  IN EFI_SYSTEM_CONTEXT  SystemContext
+  )
+{
+  STATIC NESTED_INTERRUPT_STATE  NestedInterruptState;
+  EFI_TPL                        OriginalTPL;
+
+  OriginalTPL = NestedInterruptRaiseTPL ();
+
+  SendApicEoi ();
+
+  if (mTimerNotifyFunction != NULL) {
+    //
+    // @bug : This does not handle missed timer interrupts
+    //
+    mTimerNotifyFunction (mTimerPeriod);
+  }
+
+  NestedInterruptRestoreTPL (OriginalTPL, SystemContext, &NestedInterruptState);
+}
+
+/**
+
+  This function registers the handler NotifyFunction so it is called every time
+  the timer interrupt fires.  It also passes the amount of time since the last
+  handler call to the NotifyFunction.  If NotifyFunction is NULL, then the
+  handler is unregistered.  If the handler is registered, then EFI_SUCCESS is
+  returned.  If the CPU does not support registering a timer interrupt handler,
+  then EFI_UNSUPPORTED is returned.  If an attempt is made to register a handler
+  when a handler is already registered, then EFI_ALREADY_STARTED is returned.
+  If an attempt is made to unregister a handler when a handler is not registered,
+  then EFI_INVALID_PARAMETER is returned.  If an error occurs attempting to
+  register the NotifyFunction with the timer interrupt, then EFI_DEVICE_ERROR
+  is returned.
+
+
+  @param This             The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param NotifyFunction   The function to call when a timer interrupt fires.  This
+                          function executes at TPL_HIGH_LEVEL.  The DXE Core will
+                          register a handler for the timer interrupt, so it can know
+                          how much time has passed.  This information is used to
+                          signal timer based events.  NULL will unregister the handler.
+
+  @retval        EFI_SUCCESS            The timer handler was registered.
+  @retval        EFI_UNSUPPORTED        The platform does not support timer interrupts.
+  @retval        EFI_ALREADY_STARTED    NotifyFunction is not NULL, and a handler is already
+                                        registered.
+  @retval        EFI_INVALID_PARAMETER  NotifyFunction is NULL, and a handler was not
+                                        previously registered.
+  @retval        EFI_DEVICE_ERROR       The timer handler could not be registered.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverRegisterHandler (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This,
+  IN EFI_TIMER_NOTIFY         NotifyFunction
+  )
+{
+  //
+  // Check for invalid parameters
+  //
+  if ((NotifyFunction == NULL) && (mTimerNotifyFunction == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((NotifyFunction != NULL) && (mTimerNotifyFunction != NULL)) {
+    return EFI_ALREADY_STARTED;
+  }
+
+  mTimerNotifyFunction = NotifyFunction;
+
+  return EFI_SUCCESS;
+}
+
+/**
+
+  This function adjusts the period of timer interrupts to the value specified
+  by TimerPeriod.  If the timer period is updated, then the selected timer
+  period is stored in EFI_TIMER.TimerPeriod, and EFI_SUCCESS is returned.  If
+  the timer hardware is not programmable, then EFI_UNSUPPORTED is returned.
+  If an error occurs while attempting to update the timer period, then the
+  timer hardware will be put back in its state prior to this call, and
+  EFI_DEVICE_ERROR is returned.  If TimerPeriod is 0, then the timer interrupt
+  is disabled.  This is not the same as disabling the CPU's interrupts.
+  Instead, it must either turn off the timer hardware, or it must adjust the
+  interrupt controller so that a CPU interrupt is not generated when the timer
+  interrupt fires.
+
+
+  @param This            The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param TimerPeriod     The rate to program the timer interrupt in 100 nS units.  If
+                         the timer hardware is not programmable, then EFI_UNSUPPORTED is
+                         returned.  If the timer is programmable, then the timer period
+                         will be rounded up to the nearest timer period that is supported
+                         by the timer hardware.  If TimerPeriod is set to 0, then the
+                         timer interrupts will be disabled.
+
+  @retval        EFI_SUCCESS       The timer period was changed.
+  @retval        EFI_UNSUPPORTED   The platform cannot change the period of the timer interrupt.
+  @retval        EFI_DEVICE_ERROR  The timer period could not be changed due to a device error.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverSetTimerPeriod (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This,
+  IN UINT64                   TimerPeriod
+  )
+{
+  UINT64  TimerCount;
+  UINT32  TimerFrequency;
+  UINT32  DivideValue = 1;
+
+  if (TimerPeriod == 0) {
+    //
+    // Disable timer interrupt for a TimerPeriod of 0
+    //
+    DisableApicTimerInterrupt ();
+  } else {
+    TimerFrequency = PcdGet32 (PcdFSBClock) / (UINT32)DivideValue;
+
+    //
+    // Convert TimerPeriod into local APIC counts
+    //
+    // TimerPeriod is in 100ns
+    // TimerPeriod/10000000 will be in seconds.
+    TimerCount = DivU64x32 (
+                   MultU64x32 (TimerPeriod, TimerFrequency),
+                   10000000
+                   );
+
+    // Check for overflow
+    if (TimerCount > MAX_UINT32) {
+      TimerCount = MAX_UINT32;
+      /* TimerPeriod = (MAX_UINT32 / TimerFrequency) * 10000000; */
+      TimerPeriod = 429496730;
+    }
+
+    //
+    // Program the timer with the new count value
+    //
+    InitializeApicTimer (DivideValue, (UINT32)TimerCount, TRUE, LOCAL_APIC_TIMER_VECTOR);
+
+    //
+    // Enable timer interrupt
+    //
+    EnableApicTimerInterrupt ();
+  }
+
+  //
+  // Save the new timer period
+  //
+  mTimerPeriod = TimerPeriod;
+
+  return EFI_SUCCESS;
+}
+
+/**
+
+  This function retrieves the period of timer interrupts in 100 ns units,
+  returns that value in TimerPeriod, and returns EFI_SUCCESS.  If TimerPeriod
+  is NULL, then EFI_INVALID_PARAMETER is returned.  If a TimerPeriod of 0 is
+  returned, then the timer is currently disabled.
+
+
+  @param This            The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param TimerPeriod     A pointer to the timer period to retrieve in 100 ns units.  If
+                         0 is returned, then the timer is currently disabled.
+
+  @retval EFI_SUCCESS            The timer period was returned in TimerPeriod.
+  @retval EFI_INVALID_PARAMETER  TimerPeriod is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverGetTimerPeriod (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This,
+  OUT UINT64                  *TimerPeriod
+  )
+{
+  if (TimerPeriod == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *TimerPeriod = mTimerPeriod;
+
+  return EFI_SUCCESS;
+}
+
+/**
+
+  This function generates a soft timer interrupt. If the platform does not support soft
+  timer interrupts, then EFI_UNSUPPORTED is returned. Otherwise, EFI_SUCCESS is returned.
+  If a handler has been registered through the EFI_TIMER_ARCH_PROTOCOL.RegisterHandler()
+  service, then a soft timer interrupt will be generated. If the timer interrupt is
+  enabled when this service is called, then the registered handler will be invoked. The
+  registered handler should not be able to distinguish a hardware-generated timer
+  interrupt from a software-generated timer interrupt.
+
+
+  @param This              The EFI_TIMER_ARCH_PROTOCOL instance.
+
+  @retval EFI_SUCCESS       The soft timer interrupt was generated.
+  @retval EFI_UNSUPPORTED   The platform does not support the generation of soft timer interrupts.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverGenerateSoftInterrupt (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This
+  )
+{
+  EFI_TPL  OriginalTPL;
+
+  if (GetApicTimerInterruptState ()) {
+    //
+    // Invoke the registered handler
+    //
+    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+
+    if (mTimerNotifyFunction != NULL) {
+      //
+      // @bug : This does not handle missed timer interrupts
+      //
+      mTimerNotifyFunction (mTimerPeriod);
+    }
+
+    gBS->RestoreTPL (OriginalTPL);
+  } else {
+    return EFI_UNSUPPORTED;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Initialize the Timer Architectural Protocol driver
+
+  @param ImageHandle     ImageHandle of the loaded driver
+  @param SystemTable     Pointer to the System Table
+
+  @retval EFI_SUCCESS            Timer Architectural Protocol created
+  @retval EFI_OUT_OF_RESOURCES   Not enough resources available to initialize driver.
+  @retval EFI_DEVICE_ERROR       A device error occurred attempting to initialize the driver.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverInitialize (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  //
+  // Initialize the pointer to our notify function.
+  //
+  mTimerNotifyFunction = NULL;
+
+  //
+  // Make sure the Timer Architectural Protocol is not already installed in the system
+  //
+  ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiTimerArchProtocolGuid);
+
+  //
+  // Find the CPU architectural protocol.
+  //
+  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Force the timer to be disabled
+  //
+  Status = TimerDriverSetTimerPeriod (&mTimer, 0);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Install interrupt handler for Local APIC Timer
+  //
+  Status = mCpu->RegisterInterruptHandler (
+                   mCpu,
+                   LOCAL_APIC_TIMER_VECTOR,
+                   TimerInterruptHandler
+                   );
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Force the timer to be enabled at its default period
+  //
+  Status = TimerDriverSetTimerPeriod (&mTimer, DEFAULT_TIMER_TICK_DURATION);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Install the Timer Architectural Protocol onto a new handle
+  //
+  Status = gBS->InstallMultipleProtocolInterfaces (
+                  &mTimerHandle,
+                  &gEfiTimerArchProtocolGuid,
+                  &mTimer,
+                  NULL
+                  );
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
+}
diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
new file mode 100644
index 0000000000..93706995f8
--- /dev/null
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
@@ -0,0 +1,178 @@
+/** @file
+  Private data structures
+
+Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2019, Citrix Systems, Inc.
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef LOCAL_APIC_TIMER_H_
+#define LOCAL_APIC_TIMER_H_
+
+#include <PiDxe.h>
+
+#include <Protocol/Cpu.h>
+#include <Protocol/Timer.h>
+
+#include <Register/LocalApic.h>
+
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/LocalApicLib.h>
+#include <Library/PcdLib.h>
+
+// The default timer tick duration is set to 10 ms = 100000 100 ns units
+//
+#define DEFAULT_TIMER_TICK_DURATION  100000
+
+//
+// The Timer Vector use for interrupt
+//
+#define LOCAL_APIC_TIMER_VECTOR  32
+
+//
+// Function Prototypes
+//
+
+/**
+  Initialize the Timer Architectural Protocol driver
+
+  @param ImageHandle     ImageHandle of the loaded driver
+  @param SystemTable     Pointer to the System Table
+
+  @retval EFI_SUCCESS            Timer Architectural Protocol created
+  @retval EFI_OUT_OF_RESOURCES   Not enough resources available to initialize driver.
+  @retval EFI_DEVICE_ERROR       A device error occurred attempting to initialize the driver.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverInitialize (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+;
+
+/**
+
+  This function adjusts the period of timer interrupts to the value specified
+  by TimerPeriod.  If the timer period is updated, then the selected timer
+  period is stored in EFI_TIMER.TimerPeriod, and EFI_SUCCESS is returned.  If
+  the timer hardware is not programmable, then EFI_UNSUPPORTED is returned.
+  If an error occurs while attempting to update the timer period, then the
+  timer hardware will be put back in its state prior to this call, and
+  EFI_DEVICE_ERROR is returned.  If TimerPeriod is 0, then the timer interrupt
+  is disabled.  This is not the same as disabling the CPU's interrupts.
+  Instead, it must either turn off the timer hardware, or it must adjust the
+  interrupt controller so that a CPU interrupt is not generated when the timer
+  interrupt fires.
+
+
+  @param This            The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param NotifyFunction  The rate to program the timer interrupt in 100 nS units.  If
+                         the timer hardware is not programmable, then EFI_UNSUPPORTED is
+                         returned.  If the timer is programmable, then the timer period
+                         will be rounded up to the nearest timer period that is supported
+                         by the timer hardware.  If TimerPeriod is set to 0, then the
+                         timer interrupts will be disabled.
+
+  @retval        EFI_SUCCESS       The timer period was changed.
+  @retval        EFI_UNSUPPORTED   The platform cannot change the period of the timer interrupt.
+  @retval        EFI_DEVICE_ERROR  The timer period could not be changed due to a device error.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverRegisterHandler (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This,
+  IN EFI_TIMER_NOTIFY         NotifyFunction
+  )
+;
+
+/**
+
+  This function adjusts the period of timer interrupts to the value specified
+  by TimerPeriod.  If the timer period is updated, then the selected timer
+  period is stored in EFI_TIMER.TimerPeriod, and EFI_SUCCESS is returned.  If
+  the timer hardware is not programmable, then EFI_UNSUPPORTED is returned.
+  If an error occurs while attempting to update the timer period, then the
+  timer hardware will be put back in its state prior to this call, and
+  EFI_DEVICE_ERROR is returned.  If TimerPeriod is 0, then the timer interrupt
+  is disabled.  This is not the same as disabling the CPU's interrupts.
+  Instead, it must either turn off the timer hardware, or it must adjust the
+  interrupt controller so that a CPU interrupt is not generated when the timer
+  interrupt fires.
+
+
+  @param This            The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param TimerPeriod     The rate to program the timer interrupt in 100 nS units.  If
+                         the timer hardware is not programmable, then EFI_UNSUPPORTED is
+                         returned.  If the timer is programmable, then the timer period
+                         will be rounded up to the nearest timer period that is supported
+                         by the timer hardware.  If TimerPeriod is set to 0, then the
+                         timer interrupts will be disabled.
+
+  @retval        EFI_SUCCESS       The timer period was changed.
+  @retval        EFI_UNSUPPORTED   The platform cannot change the period of the timer interrupt.
+  @retval        EFI_DEVICE_ERROR  The timer period could not be changed due to a device error.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverSetTimerPeriod (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This,
+  IN UINT64                   TimerPeriod
+  )
+;
+
+/**
+
+  This function retrieves the period of timer interrupts in 100 ns units,
+  returns that value in TimerPeriod, and returns EFI_SUCCESS.  If TimerPeriod
+  is NULL, then EFI_INVALID_PARAMETER is returned.  If a TimerPeriod of 0 is
+  returned, then the timer is currently disabled.
+
+
+  @param This            The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param TimerPeriod     A pointer to the timer period to retrieve in 100 ns units.  If
+                         0 is returned, then the timer is currently disabled.
+
+  @retval EFI_SUCCESS            The timer period was returned in TimerPeriod.
+  @retval EFI_INVALID_PARAMETER  TimerPeriod is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverGetTimerPeriod (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This,
+  OUT UINT64                  *TimerPeriod
+  )
+;
+
+/**
+
+  This function generates a soft timer interrupt. If the platform does not support soft
+  timer interrupts, then EFI_UNSUPPORTED is returned. Otherwise, EFI_SUCCESS is returned.
+  If a handler has been registered through the EFI_TIMER_ARCH_PROTOCOL.RegisterHandler()
+  service, then a soft timer interrupt will be generated. If the timer interrupt is
+  enabled when this service is called, then the registered handler will be invoked. The
+  registered handler should not be able to distinguish a hardware-generated timer
+  interrupt from a software-generated timer interrupt.
+
+
+  @param This              The EFI_TIMER_ARCH_PROTOCOL instance.
+
+  @retval EFI_SUCCESS       The soft timer interrupt was generated.
+  @retval EFI_UNSUPPORTED   The platform does not support the generation of soft timer interrupts.
+
+**/
+EFI_STATUS
+EFIAPI
+TimerDriverGenerateSoftInterrupt (
+  IN EFI_TIMER_ARCH_PROTOCOL  *This
+  )
+;
+
+#endif
diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
new file mode 100644
index 0000000000..6c711bd163
--- /dev/null
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -0,0 +1,44 @@
+## @file
+# Local APIC timer driver that provides Timer Arch protocol.
+# PcdFSBClock is defined in MdePkg and it should be set by the consumer.
+#
+# Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2019, Citrix Systems, Inc.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = LocalApicTimerDxe
+  FILE_GUID                      = e3e805df-94ed-488d-a58a-38203fb24bb2
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+
+  ENTRY_POINT                    = TimerDriverInitialize
+
+[Packages]
+  MdePkg/MdePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  UefiBootServicesTableLib
+  BaseLib
+  DebugLib
+  NestedInterruptTplLib
+  UefiDriverEntryPoint
+  LocalApicLib
+
+[Sources]
+  LocalApicTimerDxe.h
+  LocalApicTimerDxe.c
+
+[Protocols]
+  gEfiCpuArchProtocolGuid       ## CONSUMES
+  gEfiTimerArchProtocolGuid     ## PRODUCES
+[Pcd]
+  gEfiMdePkgTokenSpaceGuid.PcdFSBClock  ## CONSUMES
+[Depex]
+  gEfiCpuArchProtocolGuid
-- 
2.39.1.windows.1



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

[edk2-devel] [PATCH 2/6] UefiCpuPkg/LocalApicTimerDxe: Remove NestedInterruptTplLib call

[Ni, Ray]

a086f4a adds NestedInterruptTplLib call in OvmfPkg/LocalApicTimerDxe to
avoid stack overflow due to interrupt storm in VM guest.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2815
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4162

The interrupt storm only happens in VM guest environment when the VM is
heavily loaded.
The UefiCpuPkg/LocalApicTimerDxe at current stage does not support
the VM guest environment. So, this patch removes the NestedInterruptTplLib
call.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c   | 11 ++++-------
 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf |  4 +---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
index 67f4dcde37..f36a0e6bf3 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -1,15 +1,13 @@
 /** @file
   Timer Architectural Protocol as defined in the DXE CIS
 
-Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2005 - 2024, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2019, Citrix Systems, Inc.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
-#include <Library/NestedInterruptTplLib.h>
-
 #include "LocalApicTimerDxe.h"
 
 //
@@ -60,10 +58,9 @@ TimerInterruptHandler (
   IN EFI_SYSTEM_CONTEXT  SystemContext
   )
 {
-  STATIC NESTED_INTERRUPT_STATE  NestedInterruptState;
-  EFI_TPL                        OriginalTPL;
+  EFI_TPL  OriginalTPL;
 
-  OriginalTPL = NestedInterruptRaiseTPL ();
+  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
 
   SendApicEoi ();
 
@@ -74,7 +71,7 @@ TimerInterruptHandler (
     mTimerNotifyFunction (mTimerPeriod);
   }
 
-  NestedInterruptRestoreTPL (OriginalTPL, SystemContext, &NestedInterruptState);
+  gBS->RestoreTPL (OriginalTPL);
 }
 
 /**
diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
index 6c711bd163..874d58fa17 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -2,7 +2,7 @@
 # Local APIC timer driver that provides Timer Arch protocol.
 # PcdFSBClock is defined in MdePkg and it should be set by the consumer.
 #
-# Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2005 - 2024, Intel Corporation. All rights reserved.<BR>
 # Copyright (c) 2019, Citrix Systems, Inc.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -21,13 +21,11 @@
 [Packages]
   MdePkg/MdePkg.dec
   UefiCpuPkg/UefiCpuPkg.dec
-  OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
   UefiBootServicesTableLib
   BaseLib
   DebugLib
-  NestedInterruptTplLib
   UefiDriverEntryPoint
   LocalApicLib
 
-- 
2.39.1.windows.1



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

[edk2-devel] [PATCH 3/6] UefiCpuPkg: Add LocalApicTimerDxe driver in DSC file

[Ni, Ray]

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/UefiCpuPkg.dsc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 28eed85bce..580d860960 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -1,7 +1,7 @@
 ## @file
 #  UefiCpuPkg Package
 #
-#  Copyright (c) 2007 - 2023, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2007 - 2024, Intel Corporation. All rights reserved.<BR>
 #  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -137,6 +137,7 @@
   UefiCpuPkg/CpuIo2Smm/CpuIo2Smm.inf
   UefiCpuPkg/CpuIo2Smm/CpuIo2StandaloneMm.inf
   UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+  UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
   UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
   UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
   UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
-- 
2.39.1.windows.1



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

[edk2-devel] [PATCH 4/6] UefiCpuPkg/LocalApicTimerDxe: Enhance Timer Frequency calculation logic

[Ni, Ray]

Old logic is simply to get the APIC Timer Frequency from PcdFSBClock.

New logic follows the SDM to firstly get the crystal clock frequency from
CPUID.0x15 leaf. If CPUID.0x15 is not supported, the crystal clock frequency
is retrieved from PcdCpuCoreCrystalClockFrequency.
Then the new logic finds a proper divisor that could support longer timer
period. Usually divisor 1 is good enough. But when the timer period is very
long, divisor 4, 8, or even 128 could be used.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 .../LocalApicTimerDxe/LocalApicTimerDxe.c     | 120 +++++++++++++++---
 .../LocalApicTimerDxe/LocalApicTimerDxe.h     |   4 +-
 .../LocalApicTimerDxe/LocalApicTimerDxe.inf   |   2 +-
 3 files changed, 107 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
index f36a0e6bf3..babf2476e3 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -128,6 +128,50 @@ TimerDriverRegisterHandler (
   return EFI_SUCCESS;
 }
 
+/**
+  Return the Crystal Clock Frequency in Hz.
+
+  If CPUID.0x15 is supported, the Crystal Clock Frequency is retrieved from CPUID.0x15.ECX.
+  Otherwise, the Crystal Clock Frequency is retrieved from PcdCpuCoreCrystalClockFrequency.
+
+  @return the Crystal Clock Frequency in Hz.
+**/
+UINT64
+GetCoreXtalClockFrequency (
+  VOID
+  )
+{
+  UINT32  MaxLeaf;
+  UINT32  RegEax;
+  UINT32  RegEbx;
+  UINT32  CoreXtalClockFrequency;
+
+  CoreXtalClockFrequency = 0;
+  AsmCpuid (CPUID_SIGNATURE, &MaxLeaf, NULL, NULL, NULL);
+  if (MaxLeaf >= CPUID_TIME_STAMP_COUNTER) {
+    //
+    // Use CPUID leaf 0x15 Time Stamp Counter and Nominal Core Crystal Clock Information
+    // EBX returns 0 if not supported. ECX, if non zero, provides Core Xtal Frequency in hertz.
+    // TSC frequency = (ECX, Core Xtal Frequency) * EBX/EAX.
+    //
+    AsmCpuid (CPUID_TIME_STAMP_COUNTER, &RegEax, &RegEbx, (UINT32 *)&CoreXtalClockFrequency, NULL);
+
+    //
+    // If EAX or EBX returns 0, the Crystal ratio is not enumerated.
+    //
+    if ((RegEax == 0) || (RegEbx == 0)) {
+      CoreXtalClockFrequency = 0;
+    }
+  }
+
+  if (CoreXtalClockFrequency != 0) {
+    return CoreXtalClockFrequency;
+  } else {
+    DEBUG ((DEBUG_INFO, "%a: Using PcdCpuCoreCrystalClockFrequency (%ld)\n", __func__, PcdGet64 (PcdCpuCoreCrystalClockFrequency)));
+    return PcdGet64 (PcdCpuCoreCrystalClockFrequency);
+  }
+}
+
 /**
 
   This function adjusts the period of timer interrupts to the value specified
@@ -163,9 +207,13 @@ TimerDriverSetTimerPeriod (
   IN UINT64                   TimerPeriod
   )
 {
-  UINT64  TimerCount;
-  UINT32  TimerFrequency;
-  UINT32  DivideValue = 1;
+  UINT64  InitialCount;
+  UINT64  CoreXtalClockFrequency;
+  UINT64  ApicTimerFrequency;
+  UINT8   Divisor;
+  UINT32  TimerPeriodDivisor;
+
+  DEBUG ((DEBUG_INFO, "%a: TimerPeriod = %d (100ns)\n", __func__, TimerPeriod));
 
   if (TimerPeriod == 0) {
     //
@@ -173,29 +221,67 @@ TimerDriverSetTimerPeriod (
     //
     DisableApicTimerInterrupt ();
   } else {
-    TimerFrequency = PcdGet32 (PcdFSBClock) / (UINT32)DivideValue;
+    CoreXtalClockFrequency = GetCoreXtalClockFrequency ();
+    //
+    // Find a good Divisor that can support the TimerPeriod.
+    //
+    for (Divisor = 1; Divisor <= 128; Divisor *= 2) {
+      ApicTimerFrequency = DivU64x32 (CoreXtalClockFrequency, Divisor);
+
+      //
+      //                   TimerPeriod
+      // InitialCount =   ---------------- * ApicTimerFrequency
+      //                  10 * 1000 * 1000
+      //
+      // So,
+      //
+      //                        InitialCount * 10 * 1000 * 1000
+      // ApicTimerFrequency =   ------------------------------
+      //                                TimerPeriod
+      //
+      // Because InitialCount is a UINT32, the maximum ApicTimerFrequency is:
+      //
+      //                        MAX_UINT32 * 10 * 1000 * 1000
+      //                        ------------------------------
+      //                                TimerPeriod
+      //
+      if (ApicTimerFrequency <= DivU64x64Remainder (MultU64x32 (MAX_UINT32, 10 * 1000 * 1000), TimerPeriod, NULL)) {
+        break;
+      }
+    }
+
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: ApicTimerFrequency (%d) = CoreXtalClockFrequency (%d) / Divisor (%d)\n",
+      __func__,
+      ApicTimerFrequency,
+      CoreXtalClockFrequency,
+      Divisor
+      ));
 
     //
-    // Convert TimerPeriod into local APIC counts
+    // Convert TimerPeriod (in 100ns) into local APIC counts
+    //                   TimerPeriod
+    // InitialCount =   ---------------- * ApicTimerFrequency
+    //                10 * 1000 * 1000
     //
-    // TimerPeriod is in 100ns
-    // TimerPeriod/10000000 will be in seconds.
-    TimerCount = DivU64x32 (
-                   MultU64x32 (TimerPeriod, TimerFrequency),
-                   10000000
-                   );
+    TimerPeriodDivisor = 10 * 1000 * 1000;
 
-    // Check for overflow
-    if (TimerCount > MAX_UINT32) {
-      TimerCount = MAX_UINT32;
-      /* TimerPeriod = (MAX_UINT32 / TimerFrequency) * 10000000; */
-      TimerPeriod = 429496730;
+    //
+    // If TimerPeriod * ApicTimerFrequency > MAX_UINT64, divide TimerPeriod by 10 until the result <= MAX_UINT64.
+    //
+    while (TimerPeriod > DivU64x64Remainder (MAX_UINT64, ApicTimerFrequency, NULL)) {
+      TimerPeriod         = DivU64x32 (TimerPeriod, 10);
+      TimerPeriodDivisor /= 10;
     }
 
+    InitialCount = DivU64x64Remainder (MultU64x64 (TimerPeriod, ApicTimerFrequency), TimerPeriodDivisor, NULL);
+    DEBUG ((DEBUG_INFO, "%a: InitialCount = %d\n", __func__, InitialCount));
+
     //
     // Program the timer with the new count value
     //
-    InitializeApicTimer (DivideValue, (UINT32)TimerCount, TRUE, LOCAL_APIC_TIMER_VECTOR);
+    InitializeApicTimer (Divisor, (UINT32)InitialCount, TRUE, LOCAL_APIC_TIMER_VECTOR);
 
     //
     // Enable timer interrupt
diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
index 93706995f8..d7f38a3dc3 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
@@ -1,7 +1,7 @@
 /** @file
   Private data structures
 
-Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2005 - 2024, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2019, Citrix Systems, Inc.
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -16,12 +16,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/Timer.h>
 
 #include <Register/LocalApic.h>
+#include <Register/Cpuid.h>
 
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/LocalApicLib.h>
 #include <Library/PcdLib.h>
+#include <Library/TimerLib.h>
 
 // The default timer tick duration is set to 10 ms = 100000 100 ns units
 //
diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
index 874d58fa17..a468f09566 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -37,6 +37,6 @@
   gEfiCpuArchProtocolGuid       ## CONSUMES
   gEfiTimerArchProtocolGuid     ## PRODUCES
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdFSBClock  ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency  ## CONSUMES
 [Depex]
   gEfiCpuArchProtocolGuid
-- 
2.39.1.windows.1



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

[edk2-devel] [PATCH 5/6] UefiCpuPkg/LocalApicTimerDxe: warn if APIC Timer is used by other code

[Ni, Ray]

LocalApicTimerDxe uses APIC Timer as the timer interrupt source.
Other code should not program the APIC Timer as it may cause the timer
interrupt be signaled at the incorrect timer period or the timer interrupt
might be disabled.

The patch prints warning debug messages in driver entrypoint when the
Initial Count is not the power-on default value 0.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
index babf2476e3..14ab8fd03d 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -400,6 +400,15 @@ TimerDriverInitialize (
   //
   mTimerNotifyFunction = NULL;
 
+  if (GetApicTimerInitCount () != 0) {
+    DEBUG ((
+      DEBUG_WARN,
+      "Warning: APIC timer might be used by other drivers.\n"
+      "         The timer period may be changed unexpectedly.\n"
+      "         Please make sure no other code programs the APIC timer.\n"
+      ));
+  }
+
   //
   // Make sure the Timer Architectural Protocol is not already installed in the system
   //
-- 
2.39.1.windows.1



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

[edk2-devel] [PATCH 6/6] UefiCpuPkg/LocalApicTimerDxe: Passing fixed timer period is not a bug.

[Ni, Ray]

The patch adds detailed comments to explain that passing fixed timer period
to mTimerNotifyFunction() is not a bug.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
index 14ab8fd03d..51c5d39b29 100644
--- a/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
@@ -66,7 +66,17 @@ TimerInterruptHandler (
 
   if (mTimerNotifyFunction != NULL) {
     //
-    // @bug : This does not handle missed timer interrupts
+    // When mTimerPeriod is 10ms but 1st call of mTimerNotifyFunction() takes 1 hour, the timer
+    // interrupt will be queued and TimerInterruptHandler() will be called immediately after the
+    // interrupt is re-enabled with gBS->RestoreTPL(). The most precise implementation should pass
+    // 1 hour as the time period to 2nd call of mTimerNotifyFunction().
+    // Always passing 10ms to mTimerNotifyFunction() might cause some timer event callbacks to be
+    // invoked later than expected.
+    //
+    // But in reality, DxeCore implements the mTimerNotifyFunction() as a simple function which
+    // runs at TPL_HIGH_LEVEL. Considering almost all UEFI services require to run at TPL lower
+    // than TPL_HIGH_LEVEL, the mTimerNotifyFunction() does not have much heavy work to do. It
+    // should not take longer.
     //
     mTimerNotifyFunction (mTimerPeriod);
   }
@@ -361,9 +371,6 @@ TimerDriverGenerateSoftInterrupt (
     OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
 
     if (mTimerNotifyFunction != NULL) {
-      //
-      // @bug : This does not handle missed timer interrupts
-      //
       mTimerNotifyFunction (mTimerPeriod);
     }
 
-- 
2.39.1.windows.1



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Pedro Falcato]

On Mon, Jan 15, 2024 at 8:04 AM Ni, Ray <ray.ni@intel.com> wrote:
>
> This commit only duplicates the OvmfPkg/LocalApicTimerDxe.
> Following commits will enhance the driver.

Hi,

Please describe why you're doing this change. i.e what's your use case
for LocalApicTimerDxe, and why are you duplicating this instead of
moving OvmfPkg's (why do we need to maintain 2 separate versions of
what is essentially the same driver)?

Thanks,
Pedro


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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael D Kinney]

Hi Ray,

I think nesting may be possible in physical platforms, but very hard to induce.

One option is to consolidate to a single LocalApicTimerDxe implementation
in the UefiCpuPkg, but allow the platform DSC to either specify a Null
NestedInterruptTplLib for physical platforms or the full one from the
OvmfPkg for VM use cases.

The other changes could be included for OvmfPkg use cases.  If the VM
does not support the CPUID leafs, then the PCD value should be used.

Mike

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Monday, January 15, 2024 1:00 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desimone@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe:
> Duplicate OvmfPkg/LocalApicTimerDxe driver
> 
> On Mon, Jan 15, 2024 at 8:04 AM Ni, Ray <ray.ni@intel.com> wrote:
> >
> > This commit only duplicates the OvmfPkg/LocalApicTimerDxe.
> > Following commits will enhance the driver.
> 
> Hi,
> 
> Please describe why you're doing this change. i.e what's your use case
> for LocalApicTimerDxe, and why are you duplicating this instead of
> moving OvmfPkg's (why do we need to maintain 2 separate versions of
> what is essentially the same driver)?
> 
> Thanks,
> Pedro


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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Laszlo Ersek]

On 1/15/24 09:59, Pedro Falcato wrote:
> On Mon, Jan 15, 2024 at 8:04 AM Ni, Ray <ray.ni@intel.com> wrote:
>>
>> This commit only duplicates the OvmfPkg/LocalApicTimerDxe.
>> Following commits will enhance the driver.
> 
> Hi,
> 
> Please describe why you're doing this change. i.e what's your use case
> for LocalApicTimerDxe,

Right, I have the same question -- although, admittedly, I've not been
CC'd on the cover letter (0/6), and the reason could be captured there.
(I'm not going to check my edk2-devel folder again today, only finishing
up my personal INBOX. So tomorrow when I check edk2-devel, I might
actually find the reason in the cover letter.)

On a second thought, I'm (retroactively?) surprised that
LocalApicTimerDxe was (apparently?) first introduced under OvmfPkg --
i.e., for VMs? That's not impossible, but feels a bit strange.

> and why are you duplicating this instead of
> moving OvmfPkg's (why do we need to maintain 2 separate versions of
> what is essentially the same driver)?

Not wanting the NestedInterruptTplLib dependency / functionality in
UefiCpuPkg's instance of this driver seems justified (patch 2). Also,
patch 4 calculates the timer frequency based on CPUID; might not be
straightforward to use on VMs without prior verification.

The tricky parts to review in this series are the math in patch 4 (such
code is usually prone to integer overflows, so that's my concern by
default there), plus the big comment in patch 6.

I'll try to look at them later.

Meta-comment for Ray: patches like patch#1 are best formatted with the
option "--find-copies-harder" (try it please, the output speaks for itself).

Thanks
Laszlo



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Gerd Hoffmann]

  Hi,

> Right, I have the same question -- although, admittedly, I've not been
> CC'd on the cover letter (0/6), and the reason could be captured there.

It wasn't.

> On a second thought, I'm (retroactively?) surprised that
> LocalApicTimerDxe was (apparently?) first introduced under OvmfPkg --
> i.e., for VMs? That's not impossible, but feels a bit strange.

I think the reason is that the next timer interrupt arriving while the
handler is running still is *much* more likely in virtual machines
because the vCPU does not get 100% of the of the physical CPU time
slice.

So on OVMF we will continue to need NestedInterruptTplLib.  I like the
idea to have a Null version of the library, so OVMF does not need its
own version of the driver just because of NestedInterruptTplLib.

take care,
  Gerd



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 16/01/2024 08:47, Gerd Hoffmann wrote:
> I think the reason is that the next timer interrupt arriving while the
> handler is running still is *much* more likely in virtual machines
> because the vCPU does not get 100% of the of the physical CPU time
> slice.

The interrupt handler can run for an arbitrary length of time, because 
the call to RestoreTPL() can end up calling an application callback 
which in turn waits on further timer interrupts.

This is a legitimate use case within UEFI, so all timer interrupt 
handlers (not just in virtual machines) need to allow for the 
possibility that nested interrupts will occur.

> So on OVMF we will continue to need NestedInterruptTplLib.  I like the
> idea to have a Null version of the library, so OVMF does not need its
> own version of the driver just because of NestedInterruptTplLib.

I agree that there should not need to be two versions of LocalApicTimerDxe.

I would suggest moving NestedInterruptTplLib from OvmfPkg to MdeModulePkg.

The code is complex, but for the caller the complexity is completely 
hidden behind the calls to NestedInterruptRaiseTPL() and 
NestedInterruptRestoreTPL(), which straightforwardly replace what would 
otherwise be (unsafe) calls to RaiseTPL() and RestoreTPL(), as shown in

https://github.com/tianocore/edk2/commit/a086f4a#diff-1418ec21e24e4ce2f3d621113585e3bea11fbcfa3b77d54046e61be7928e0c92

For a new CPU architecture, the only requirement is to provide a short 
fragment (~5 lines) of code that can clear the interrupts-enabled flag 
in the EFI_SYSTEM_CONTEXT, as shown in 
OvmfPkg/Library/NestedInterruptTplLib/Iret.c.

I'm happy to send a patch to migrate NestedInterruptTplLib to 
MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do 
this?

Thanks,

Michael



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Laszlo Ersek]

On 1/15/24 19:10, Kinney, Michael D wrote:
> Hi Ray,
>
> I think nesting may be possible in physical platforms, but very hard
> to induce.
>
> One option is to consolidate to a single LocalApicTimerDxe
> implementation in the UefiCpuPkg, but allow the platform DSC to either
> specify a Null NestedInterruptTplLib for physical platforms or the
> full one from the OvmfPkg for VM use cases.
>
> The other changes could be included for OvmfPkg use cases.  If the VM
> does not support the CPUID leafs, then the PCD value should be used.

All of this sounds great!

(And I do think that *some* nesting should not be a problem, on either
physical or virtual platforms, as (IIRC) the interrupt handler stack can
accommodate a certain level of reentrancy. It's the "storm" that is a
problem, but that should never occur on physical machines, I reckon.)

Assuming a v2 is coming up, my advance request would be for Ray to
re-review the math in patch #4, before posting v2, focusing on integer
overflows, and SafeIntLib (if needed). I've not looked at the patch in
detail yet, but I reviewed something similar not so long ago [1]. The
latter frequency calculation code had been quite commonly used in edk2,
and I needed hours to review just that one patch. Plus, the review
turned up a number of issues to fix. So, preferably such a patch should
not only prevent any integer overflows, but also clarify, in comments,
why overflows are impossible, and/or how they are prevented.

[1] https://edk2.groups.io/g/devel/message/111613
    http://mid.mail-archive.com/2e42db7c-a927-f47b-7d80-632895b11e1b@redhat.com

Thanks!
Laszlo



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Laszlo Ersek]

On 1/16/24 10:48, Michael Brown wrote:
> On 16/01/2024 08:47, Gerd Hoffmann wrote:
>> I think the reason is that the next timer interrupt arriving while the
>> handler is running still is *much* more likely in virtual machines
>> because the vCPU does not get 100% of the of the physical CPU time
>> slice.
> 
> The interrupt handler can run for an arbitrary length of time, because
> the call to RestoreTPL() can end up calling an application callback
> which in turn waits on further timer interrupts.
> 
> This is a legitimate use case within UEFI, so all timer interrupt
> handlers (not just in virtual machines) need to allow for the
> possibility that nested interrupts will occur.

The more naive, original interrupt handler implementation (i.e., the one
without NestedInterruptTplLib) still "allowed" for nesting, simply by
virtue of letting itself be interrupted, if I remember correctly. That
in itself didn't cause a problem; the problem arose when this reentrant
interruption got limitlessly deep, due to interrupts having been queued
on the host side, and then being injected as a "storm" in the guest.

The naive handler impl. effectively translated the host-side "interrupt
queue" to a "guest side stack". "queue length" became "stack depth",
leading to stack overflow.

Thus, even the original (more naive) handler could work fine (for
nesting too) as long as there was no storm (put differently, as long as
queue length, aka stack depth, were small); is that correct? (I admit
that I can't really recall the details at this point.)

The sophistication of NestedInterruptTplLib is that it supports nesting
while *resisting* a storm (= preventing infinite nesting by careful
masking of interrupt delivery, IIRC). It does not eagerly slurp all
pending (queued) interrupts into the handler stack.

IOW, my impression is that NestedInterruptTplLib can certainly handle
all scenarios thrown at it, but where it really matters is in the face
of an interrupt storm (not just "normal nesting"), and a storm is
unlikely (or even impossible?) on physical hardware.

... Oh, scratch that. "Interrupt storm" simply means that interrupts are
being delivered at a rate higher than the handler routine can service
them. IOW, the "storm" is not that interrupts are delivered *very
rapidly* in an absoulte sense. If interrupts are delivered at normal
frequency, but the handler is too slow to service *even that rate*, then
that also qualifies as "storm", because the nesting depth will *keep
growing*. It's not really the growth rate that matters; what matter is
the *trend*, i.e., the fact that there *is* growth (the stack gets
deeper and deeper). The stack might not overflow immediately, and if the
handler speeds up (for whatever reason), the stack might recover, but
there is nothing to prevent an overflow.

So, in the end, I think you've convinced me.

> 
>> So on OVMF we will continue to need NestedInterruptTplLib.  I like the
>> idea to have a Null version of the library, so OVMF does not need its
>> own version of the driver just because of NestedInterruptTplLib.
> 
> I agree that there should not need to be two versions of LocalApicTimerDxe.
> 
> I would suggest moving NestedInterruptTplLib from OvmfPkg to MdeModulePkg.
> 
> The code is complex, but for the caller the complexity is completely
> hidden behind the calls to NestedInterruptRaiseTPL() and
> NestedInterruptRestoreTPL(), which straightforwardly replace what would
> otherwise be (unsafe) calls to RaiseTPL() and RestoreTPL(), as shown in
> 
> https://github.com/tianocore/edk2/commit/a086f4a#diff-1418ec21e24e4ce2f3d621113585e3bea11fbcfa3b77d54046e61be7928e0c92
> 
> For a new CPU architecture, the only requirement is to provide a short
> fragment (~5 lines) of code that can clear the interrupts-enabled flag
> in the EFI_SYSTEM_CONTEXT, as shown in
> OvmfPkg/Library/NestedInterruptTplLib/Iret.c.
> 
> I'm happy to send a patch to migrate NestedInterruptTplLib to
> MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do
> this?

Sounds like a valid idea to me.

Could be greatly supported by a test case (to be run on the bare metal)
installing a slow handler that *eventually* exhausted the stack, when
not using NestedInterruptTplLib.

(FWIW, IIRC, the UEFI spec warns about this -- it says something like,
"return from TPL_HIGH as soon as you can, otherwise the system will
become unstable".)

Sorry for the wall of text, I find this very difficult to reason about.

Laszlo



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 16/01/2024 14:34, Laszlo Ersek wrote:
> On 1/16/24 10:48, Michael Brown wrote:
> IOW, my impression is that NestedInterruptTplLib can certainly handle
> all scenarios thrown at it, but where it really matters is in the face
> of an interrupt storm (not just "normal nesting"), and a storm is
> unlikely (or even impossible?) on physical hardware.
> 
> ... Oh, scratch that. "Interrupt storm" simply means that interrupts are
> being delivered at a rate higher than the handler routine can service
> them. IOW, the "storm" is not that interrupts are delivered *very
> rapidly* in an absoulte sense. If interrupts are delivered at normal
> frequency, but the handler is too slow to service *even that rate*, then
> that also qualifies as "storm", because the nesting depth will *keep
> growing*. It's not really the growth rate that matters; what matter is
> the *trend*, i.e., the fact that there *is* growth (the stack gets
> deeper and deeper). The stack might not overflow immediately, and if the
> handler speeds up (for whatever reason), the stack might recover, but
> there is nothing to prevent an overflow.
> 
> So, in the end, I think you've convinced me.

:)

>> I'm happy to send a patch to migrate NestedInterruptTplLib to
>> MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do
>> this?
> 
> Sounds like a valid idea to me.
> 
> Could be greatly supported by a test case (to be run on the bare metal)
> installing a slow handler that *eventually* exhausted the stack, when
> not using NestedInterruptTplLib.
> 
> (FWIW, IIRC, the UEFI spec warns about this -- it says something like,
> "return from TPL_HIGH as soon as you can, otherwise the system will
> become unstable".)
> 
> Sorry for the wall of text, I find this very difficult to reason about.

I also find it very difficult to reason about, which is why 
NestedInterruptRestoreTpl() has 126 lines of comments providing a 
semi-formal proof of correctness for a mere 15 statements of C code!

In particular, I find it difficult to reason about when it would be safe 
for a platform to *not* use NestedInterruptTplLib.  It's clearly 
empirically difficult to trigger stack underflow via an interrupt 
"storm" on physical hardware, but I'm not convinced it's impossible.

I find it mentally easier to rely on the hard guarantee that 
NestedInterruptTplLib provides: that nested interrupts will continue to 
be delivered but that the number of interrupt-induced stack frames is 
bounded by the (small, finite) number of distinct TPL levels in existence.



While developing NestedInterruptTplLib, I did hack together a test case 
for a slow handler that would deliberately induce an interrupt storm, 
since I needed this to test that my code was working.  When triggered, 
this test would cause the machine to effectively hang due to servicing 
an endless storm of timer interrupts.  Before NestedInterruptTplLib, the 
stack would soon underflow and would typically cause a reboot (or other 
crash).  With NestedInterruptTplLib the machine would continue to 
service interrupts indefinitely.

How might such a test case be included in upstream EDK2?  I'm 
peripherally aware of EDK2 test infrastructure such as UEFI SCT, but 
I've never interacted with it yet.

Thanks,

Michael



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Laszlo Ersek]

On 1/16/24 16:16, Michael Brown wrote:
> On 16/01/2024 14:34, Laszlo Ersek wrote:
>> On 1/16/24 10:48, Michael Brown wrote:
>> IOW, my impression is that NestedInterruptTplLib can certainly handle
>> all scenarios thrown at it, but where it really matters is in the face
>> of an interrupt storm (not just "normal nesting"), and a storm is
>> unlikely (or even impossible?) on physical hardware.
>>
>> ... Oh, scratch that. "Interrupt storm" simply means that interrupts are
>> being delivered at a rate higher than the handler routine can service
>> them. IOW, the "storm" is not that interrupts are delivered *very
>> rapidly* in an absoulte sense. If interrupts are delivered at normal
>> frequency, but the handler is too slow to service *even that rate*, then
>> that also qualifies as "storm", because the nesting depth will *keep
>> growing*. It's not really the growth rate that matters; what matter is
>> the *trend*, i.e., the fact that there *is* growth (the stack gets
>> deeper and deeper). The stack might not overflow immediately, and if the
>> handler speeds up (for whatever reason), the stack might recover, but
>> there is nothing to prevent an overflow.
>>
>> So, in the end, I think you've convinced me.
> 
> :)
> 
>>> I'm happy to send a patch to migrate NestedInterruptTplLib to
>>> MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do
>>> this?
>>
>> Sounds like a valid idea to me.
>>
>> Could be greatly supported by a test case (to be run on the bare metal)
>> installing a slow handler that *eventually* exhausted the stack, when
>> not using NestedInterruptTplLib.
>>
>> (FWIW, IIRC, the UEFI spec warns about this -- it says something like,
>> "return from TPL_HIGH as soon as you can, otherwise the system will
>> become unstable".)
>>
>> Sorry for the wall of text, I find this very difficult to reason about.
> 
> I also find it very difficult to reason about, which is why
> NestedInterruptRestoreTpl() has 126 lines of comments providing a
> semi-formal proof of correctness for a mere 15 statements of C code!
> 
> In particular, I find it difficult to reason about when it would be safe
> for a platform to *not* use NestedInterruptTplLib.  It's clearly
> empirically difficult to trigger stack underflow via an interrupt
> "storm" on physical hardware, but I'm not convinced it's impossible.
> 
> I find it mentally easier to rely on the hard guarantee that
> NestedInterruptTplLib provides: that nested interrupts will continue to
> be delivered but that the number of interrupt-induced stack frames is
> bounded by the (small, finite) number of distinct TPL levels in existence.
> 
> 
> 
> While developing NestedInterruptTplLib, I did hack together a test case
> for a slow handler that would deliberately induce an interrupt storm,
> since I needed this to test that my code was working.  When triggered,
> this test would cause the machine to effectively hang due to servicing
> an endless storm of timer interrupts.  Before NestedInterruptTplLib, the
> stack would soon underflow and would typically cause a reboot (or other
> crash).  With NestedInterruptTplLib the machine would continue to
> service interrupts indefinitely.
> 
> How might such a test case be included in upstream EDK2?  I'm
> peripherally aware of EDK2 test infrastructure such as UEFI SCT, but
> I've never interacted with it yet.

I'm vaguely aware of a unit test framework inside edk2, but the best I
can give you is just this link:

https://github.com/tianocore/edk2/tree/master/UnitTestFrameworkPkg#unit-test-framework-package

There are some files under the directory "MdeModulePkg/Test" too;
git-log on that subdir, and perhaps the MdeModulePkg maintainers, might
provide more pointers.

The end of the readme linked above says to ask Bret, Mike and Sean, as well.

Laszlo



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael D Kinney]

Unit tests for the math calculations would help with reviews too.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, January 16, 2024 2:03 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato
> <pedro.falcato@gmail.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kumar, Rahul
> R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe:
> Duplicate OvmfPkg/LocalApicTimerDxe driver
> 
> On 1/15/24 19:10, Kinney, Michael D wrote:
> > Hi Ray,
> >
> > I think nesting may be possible in physical platforms, but very hard
> > to induce.
> >
> > One option is to consolidate to a single LocalApicTimerDxe
> > implementation in the UefiCpuPkg, but allow the platform DSC to either
> > specify a Null NestedInterruptTplLib for physical platforms or the
> > full one from the OvmfPkg for VM use cases.
> >
> > The other changes could be included for OvmfPkg use cases.  If the VM
> > does not support the CPUID leafs, then the PCD value should be used.
> 
> All of this sounds great!
> 
> (And I do think that *some* nesting should not be a problem, on either
> physical or virtual platforms, as (IIRC) the interrupt handler stack can
> accommodate a certain level of reentrancy. It's the "storm" that is a
> problem, but that should never occur on physical machines, I reckon.)
> 
> Assuming a v2 is coming up, my advance request would be for Ray to
> re-review the math in patch #4, before posting v2, focusing on integer
> overflows, and SafeIntLib (if needed). I've not looked at the patch in
> detail yet, but I reviewed something similar not so long ago [1]. The
> latter frequency calculation code had been quite commonly used in edk2,
> and I needed hours to review just that one patch. Plus, the review
> turned up a number of issues to fix. So, preferably such a patch should
> not only prevent any integer overflows, but also clarify, in comments,
> why overflows are impossible, and/or how they are prevented.
> 
> [1] https://edk2.groups.io/g/devel/message/111613
>     http://mid.mail-archive.com/2e42db7c-a927-f47b-7d80-
> 632895b11e1b@redhat.com
> 
> Thanks!
> Laszlo



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Ni, Ray]

Laszlo, Michael,

When timer interrupt happens, the calling flow is:
[Timer Interrupt #1] CPU IDT handler calls into LocalApicTimerDxe::TimerInterruptHandler(), which
   [Timer Interrupt #1]1. RaiseTPL (HIGH) from APPLICATION causing CPU interrupt be disabled.
   [Timer Interrupt #1]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts)
   [Timer Interrupt #1]3. Call DxeCore::CoreTimerTick()
   [Timer Interrupt #1]4. RestoreTPL (APPLICATION) from HIGH. (All callbacks registered at NOTIFY and CALLBACK will run.)
      [Timer Interrupt #1]4.1. When there are Callbacks registered at NOTIFY, current TPL is set to NOTIFY and interrupt is enabled. CoreDispatchEventNotifies() is called to run the NOTIFY callbacks.
         [Timer Interrupt #2] Immediately after interrupt is enabled, CPU runs to LocalApicTimerDxe::TimerInterruptHandler(). But stack is not fully popped to the initial state.
            [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled.
            [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts)
            [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick()
            [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled.
               [Timer Interrupt #3] Immediately after interrupt is enabled, CPU runs to LocalApicTimerDxe::TimerInterruptHandler(). But stack is not fully popped to the initial state.
                  [Timer Interrupt #3]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled.
                  [Timer Interrupt #3]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts)
                  [Timer Interrupt #3]3. Call DxeCore::CoreTimerTick()
                  [Timer Interrupt #3]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled.
                     [Timer Interrupt #4] Immediately after interrupt is enabled, CPU runs to LocalApicTimerDxe::TimerInterruptHandler(). But stack is not fully popped to the initial state.
                        [Timer Interrupt #4]...


The above flow shows endless re-entrance of timer interrupt handler.

But, my question is: above flow only can happen in real platform when the below 4 steps occupies more time than the timer period (usually 10ms).
            [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled.
            [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts)
            [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick()
            [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled.

But, in my opinion, it's impossible.


Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, January 16, 2024 11:37 PM
> To: devel@edk2.groups.io; mcb30@ipxe.org; kraxel@redhat.com
> Cc: Pedro Falcato <pedro.falcato@gmail.com>; Ni, Ray <ray.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe:
> Duplicate OvmfPkg/LocalApicTimerDxe driver
> 
> On 1/16/24 16:16, Michael Brown wrote:
> > On 16/01/2024 14:34, Laszlo Ersek wrote:
> >> On 1/16/24 10:48, Michael Brown wrote:
> >> IOW, my impression is that NestedInterruptTplLib can certainly handle
> >> all scenarios thrown at it, but where it really matters is in the face
> >> of an interrupt storm (not just "normal nesting"), and a storm is
> >> unlikely (or even impossible?) on physical hardware.
> >>
> >> ... Oh, scratch that. "Interrupt storm" simply means that interrupts are
> >> being delivered at a rate higher than the handler routine can service
> >> them. IOW, the "storm" is not that interrupts are delivered *very
> >> rapidly* in an absoulte sense. If interrupts are delivered at normal
> >> frequency, but the handler is too slow to service *even that rate*, then
> >> that also qualifies as "storm", because the nesting depth will *keep
> >> growing*. It's not really the growth rate that matters; what matter is
> >> the *trend*, i.e., the fact that there *is* growth (the stack gets
> >> deeper and deeper). The stack might not overflow immediately, and if the
> >> handler speeds up (for whatever reason), the stack might recover, but
> >> there is nothing to prevent an overflow.
> >>
> >> So, in the end, I think you've convinced me.
> >
> > :)
> >
> >>> I'm happy to send a patch to migrate NestedInterruptTplLib to
> >>> MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I
> do
> >>> this?
> >>
> >> Sounds like a valid idea to me.
> >>
> >> Could be greatly supported by a test case (to be run on the bare metal)
> >> installing a slow handler that *eventually* exhausted the stack, when
> >> not using NestedInterruptTplLib.
> >>
> >> (FWIW, IIRC, the UEFI spec warns about this -- it says something like,
> >> "return from TPL_HIGH as soon as you can, otherwise the system will
> >> become unstable".)
> >>
> >> Sorry for the wall of text, I find this very difficult to reason about.
> >
> > I also find it very difficult to reason about, which is why
> > NestedInterruptRestoreTpl() has 126 lines of comments providing a
> > semi-formal proof of correctness for a mere 15 statements of C code!
> >
> > In particular, I find it difficult to reason about when it would be safe
> > for a platform to *not* use NestedInterruptTplLib.  It's clearly
> > empirically difficult to trigger stack underflow via an interrupt
> > "storm" on physical hardware, but I'm not convinced it's impossible.
> >
> > I find it mentally easier to rely on the hard guarantee that
> > NestedInterruptTplLib provides: that nested interrupts will continue to
> > be delivered but that the number of interrupt-induced stack frames is
> > bounded by the (small, finite) number of distinct TPL levels in existence.
> >
> >
> >
> > While developing NestedInterruptTplLib, I did hack together a test case
> > for a slow handler that would deliberately induce an interrupt storm,
> > since I needed this to test that my code was working.  When triggered,
> > this test would cause the machine to effectively hang due to servicing
> > an endless storm of timer interrupts.  Before NestedInterruptTplLib, the
> > stack would soon underflow and would typically cause a reboot (or other
> > crash).  With NestedInterruptTplLib the machine would continue to
> > service interrupts indefinitely.
> >
> > How might such a test case be included in upstream EDK2?  I'm
> > peripherally aware of EDK2 test infrastructure such as UEFI SCT, but
> > I've never interacted with it yet.
> 
> I'm vaguely aware of a unit test framework inside edk2, but the best I
> can give you is just this link:
> 
> https://github.com/tianocore/edk2/tree/master/UnitTestFrameworkPkg#unit
> -test-framework-package
> 
> There are some files under the directory "MdeModulePkg/Test" too;
> git-log on that subdir, and perhaps the MdeModulePkg maintainers, might
> provide more pointers.
> 
> The end of the readme linked above says to ask Bret, Mike and Sean, as well.
> 
> Laszlo



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 17/01/2024 07:11, Ni, Ray wrote:
> The above flow shows endless re-entrance of timer interrupt handler.
> 
> But, my question is: above flow only can happen in real platform when the below 4 steps occupies more time than the timer period (usually 10ms).
>              [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled.
>              [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts)
>              [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick()
>              [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled.
> 
> But, in my opinion, it's impossible.

As is thoroughly documented in NestedInterruptRestoreTpl(), the 
potential for unbounded stack consumption arises when an interrupt 
occurs after the point that RestoreTPL() completes dispatching all 
notifications but before the IRET (or equivalent) instruction pops the 
original stack frame.

Since dispatching notifications can take an unbounded amount of time, 
there is absolutely no guarantee that this will be less than 10ms after 
the previous interrupt.  It could easily be 30 seconds later.

The problematic flow is a subtle variation on what you described:

   [IRQ#1] timer interrupt at TPL_APPLICATION
     [ISR#1] RaiseTPL from TPL_APPLICATION -> TPL_HIGH_LEVEL
     [ISR#1] Send APIC EOI
     [ISR#1] Call CoreTimerTick()
     [ISR#1] RestoreTPL from TPL_HIGH_LEVEL -> TPL_APPLICATION
       [ISR#1] Callbacks for TPL_NOTIFY are run
       [ISR#1] Callbacks for TPL_CALLBACK are run
       ... these may take several *seconds* to complete, during
           which further interrupts are raised, the details of
           which are not shown here...
       [ISR#1] TPL is now restored to TPL_APPLICATION
       [IRQ#N] timer interrupt at TPL_APPLICATION
         [ISR#N] RaiseTPL from TPL_APPLICATION -> TPL_HIGH_LEVEL
         ... continues ...

The root cause is that the ISR reaches a state in which:

   a) an arbitrary amount of time has elapsed since the triggering
      interrupt (due to unknown callbacks being invoked, which may
      themselves wait for further timer interrupts), and

   b) the TPL has been fully restored back to the TPL at the point
      the triggering interrupt occurred (i.e. TPL_APPLICATION in
      this example), and

   c) the timer interrupt source is enabled, and

   d) CPU interrupts are enabled

At this point, there is nothing preventing another interrupt from 
occurring.  It will occur at TPL_APPLICATION and it will be one stack 
frame deeper than the previous interrupt at TPL_APPLICATION.

Rinse and repeat, and you have unbounded stack consumption.

Hence the requirement for NestedInterruptTplLib, even on physical hardware.

Michael



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 6559 bytes --]

Michael,

Thanks for the explanation.
I tried to expand the code flow in below and help the discussion..

TimerInterruptHandler()
    gBS->RaiseTPL (HIGH)
    gBS->RestoreTPL (APPLICATION) // expand in below.
        For Tpl = {NOTIFY, CALLBACK}:          ---- [for-loop]
            if PendingBit is not set:
                continue
            gCurrentTpl = Tpl
            EnableInterrupt()                  ---- [1]
            CoreDispatchEventNotifies(Tpl) // expand in below.
                gBS->RaiseTPL (HIGH)
                gBS->RestoreTPL (Tpl)
                NotifyFunction()               ---- [2]
                gBS->RaiseTPL (HIGH)
                gBS->RestoreTPL (Tpl)
        End-For
        gCurrentTpl = APPLICATION              ---- [3]
        EnableInterrupt()                      ---- [4]
IRET


  1.  Agree that the stack overflow could happen in real platform.
The interrupt-enabled env when CPU runs gBS->RestoreTPL(APPLICATION)  could be 3 cases: When gCurrentTpl is NOTIFY, CALLBACK or APPLICATION.
Let's name them as "env:NOTIFY", "env:CALLBACK" and "env:APPLICATION".
CPU enters "env:NOTIFY" and "env:CALLBACK" in [1].
CPU enters "env:APPLICATION" in [4], or [3] when the interrupt is already enabled in the [for-loop].

When interrupt happens in "env:NOTIFY", the inner interrupt handler calls gBS->RestoreTPL(NOTIFY). The interrupt-enabled env in the inner RestoreTPL(NOTIFY) is "env:NOTIFY" only.
When interrupt happens in "env:CALLBACK", the inner interrupt handler calls gBS->RestoreTPL(CALLBACK). The interrupt-enabled env in the inner RestoreTPL(CALLBACK) can be: "env:NOTIFY" and "env:CALLBACK".

So, the interrupt re-entrance we want to avoid is "env:NOTIFY"  -> "env:NOTIFY", or "env:CALLBACK" -> "env:CALLBACK", or "env:APPLICATION" -> "env:APPLICATION". Because it's endless.
NestedTplInterruptLib was written to avoid it.

However, it's ok for: "env:APPLICATION" -> "env:CALLBACK" -> "env:NOTIFY", or "env:CALLBACK" -> "env:NOTIFY".

In real platform, it's possible that interrupt happens just after [4], and in worst case, the RestoreTpl() call in the inner timer interrupt handler is interrupted after [4] again, and again.


  1.  Some questions on NestedInterruptTplLib.

  1.  Can we remove DisableInterruptsOnIret()? That means the inner interrupt handler would returns to the outer world with interrupt enabled and TPL==HIGH. But I don't see any issue with that.
  2.  If DxeCore can be changed, do you have an easier-to-understand solution? It really took me 2 days to understand why NestedInterruptTplLib is written in today's way.


thanks,
ray
________________________________
From: Michael Brown <mcb30@ipxe.org>
Sent: Wednesday, January 17, 2024 6:46:59 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; kraxel@redhat.com <kraxel@redhat.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

On 17/01/2024 07:11, Ni, Ray wrote:
> The above flow shows endless re-entrance of timer interrupt handler.
>
> But, my question is: above flow only can happen in real platform when the below 4 steps occupies more time than the timer period (usually 10ms).
>              [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU interrupt be disabled.
>              [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received so APIC can continue generate interrupts)
>              [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick()
>              [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback runs as no callback can be registered at TPL > NOTIFY. In the end of RestoreTPL(), CPU interrupt is enabled.
>
> But, in my opinion, it's impossible.

As is thoroughly documented in NestedInterruptRestoreTpl(), the
potential for unbounded stack consumption arises when an interrupt
occurs after the point that RestoreTPL() completes dispatching all
notifications but before the IRET (or equivalent) instruction pops the
original stack frame.

Since dispatching notifications can take an unbounded amount of time,
there is absolutely no guarantee that this will be less than 10ms after
the previous interrupt.  It could easily be 30 seconds later.

The problematic flow is a subtle variation on what you described:

   [IRQ#1] timer interrupt at TPL_APPLICATION
     [ISR#1] RaiseTPL from TPL_APPLICATION -> TPL_HIGH_LEVEL
     [ISR#1] Send APIC EOI
     [ISR#1] Call CoreTimerTick()
     [ISR#1] RestoreTPL from TPL_HIGH_LEVEL -> TPL_APPLICATION
       [ISR#1] Callbacks for TPL_NOTIFY are run
       [ISR#1] Callbacks for TPL_CALLBACK are run
       ... these may take several *seconds* to complete, during
           which further interrupts are raised, the details of
           which are not shown here...
       [ISR#1] TPL is now restored to TPL_APPLICATION
       [IRQ#N] timer interrupt at TPL_APPLICATION
         [ISR#N] RaiseTPL from TPL_APPLICATION -> TPL_HIGH_LEVEL
         ... continues ...

The root cause is that the ISR reaches a state in which:

   a) an arbitrary amount of time has elapsed since the triggering
      interrupt (due to unknown callbacks being invoked, which may
      themselves wait for further timer interrupts), and

   b) the TPL has been fully restored back to the TPL at the point
      the triggering interrupt occurred (i.e. TPL_APPLICATION in
      this example), and

   c) the timer interrupt source is enabled, and

   d) CPU interrupts are enabled

At this point, there is nothing preventing another interrupt from
occurring.  It will occur at TPL_APPLICATION and it will be one stack
frame deeper than the previous interrupt at TPL_APPLICATION.

Rinse and repeat, and you have unbounded stack consumption.

Hence the requirement for NestedInterruptTplLib, even on physical hardware.

Michael


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



[-- Attachment #2: Type: text/html, Size: 21900 bytes --]

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 19/01/2024 13:14, Ni, Ray wrote:
> So, the interrupt re-entrance we want to avoid is “env:NOTIFY”  -> 
> “env:NOTIFY”, or “env:CALLBACK” -> “env:CALLBACK”, or “env:APPLICATION” 
> -> “env:APPLICATION”. Because it’s endless.
> 
> NestedTplInterruptLib was written to avoid it.

Yes, precisely this.

>  2. Some questions on NestedInterruptTplLib.
> 
>  1. Can we remove DisableInterruptsOnIret()? That means the inner
>     interrupt handler would returns to the outer world with interrupt
>     enabled and TPL==HIGH. But I don’t see any issue with that.
Using DisableInterruptsOnIret() allows us to guarantee that absolutely 
nothing happens between the "DEFERRAL INVOCATION POINT" and "DEFERRAL 
RETURN POINT" described in the comments in Tpl.c.

If we don't use DisableInterruptsOnIret() then we lose this guarantee, 
and the situation becomes even more complex than it already is.

I don't personally feel able to reason through all the possible 
circumstances that could arise if an interrupt were to occur between 
"DEFERRAL INVOCATION POINT" and "DEFERRAL RETURN POINT", so I don't feel 
safe removing the use of DisableInterruptsOnIret().

I have a vague memory that I was still experiencing some kind of crashes 
before I added DisableInterruptsOnIret(), but I cannot now remember any 
details, sorry.

>  2. If DxeCore can be changed, do you have an easier-to-understand
>     solution? It really took me 2 days to understand why
>     NestedInterruptTplLib is written in today’s way.

The ability to change DxeCore doesn't help, unfortunately.

If we could change the prototype of RaiseTPL() and RestoreTPL() to 
include a flag indicating whether or not interrupts should be enabled at 
the point that RestoreTPL() returns, then that would allow for an 
easier-to-understand solution.

This would require making a breaking change to the UEFI specification, 
though, so it's not a viable solution.


I do appreciate that it's difficult to understand the internals of 
NestedInterruptTplLib.  It's fundamentally having to solve a very 
difficult problem within the constraints of the UEFI API.  I think the 
solution that NestedInterruptTplLib provides is as simple as it's 
possible to get, and it does at least have the advantage that all of the 
complexity is hidden inside the library: the caller gets to just change 
two lines:

- OriginalTPL = gBS->RaiseTPL(TPL_HIGH_LEVEL);
+ OriginalTPL = NestedInterruptRaiseTPL();
   ...
- gBS->RestoreTPL(OriginalTPL);
+ NestedInterruptRestoreTPL(OriginalTPL, Context, &State);


I'll send through a patch to move NestedInterruptTplLib to MdeModulePkg.

Thanks,

Michael



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 4254 bytes --]

Michael,
I still want to see if the RestoreTpl2 that does not enable interrupt is added as a protocol, and how simple the lib could be.
The reason is about maintainability.
I can image that one day people would question the Lib implementation if some timer event issue appears. If the Lib is easy to understand, the suspicion could be avoided.
And if the correctness of the Lib can be proven by a thorough test, that will be better. But it seems to me the Lib can only be proven as correct with careful code review, like some multi-threaded logic.



thanks,
ray
________________________________
From: Michael Brown <mcb30@ipxe.org>
Sent: Saturday, January 20, 2024 1:42 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; kraxel@redhat.com <kraxel@redhat.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

On 19/01/2024 13:14, Ni, Ray wrote:
> So, the interrupt re-entrance we want to avoid is “env:NOTIFY”  ->
> “env:NOTIFY”, or “env:CALLBACK” -> “env:CALLBACK”, or “env:APPLICATION”
> -> “env:APPLICATION”. Because it’s endless.
>
> NestedTplInterruptLib was written to avoid it.

Yes, precisely this.

>  2. Some questions on NestedInterruptTplLib.
>
>  1. Can we remove DisableInterruptsOnIret()? That means the inner
>     interrupt handler would returns to the outer world with interrupt
>     enabled and TPL==HIGH. But I don’t see any issue with that.
Using DisableInterruptsOnIret() allows us to guarantee that absolutely
nothing happens between the "DEFERRAL INVOCATION POINT" and "DEFERRAL
RETURN POINT" described in the comments in Tpl.c.

If we don't use DisableInterruptsOnIret() then we lose this guarantee,
and the situation becomes even more complex than it already is.

I don't personally feel able to reason through all the possible
circumstances that could arise if an interrupt were to occur between
"DEFERRAL INVOCATION POINT" and "DEFERRAL RETURN POINT", so I don't feel
safe removing the use of DisableInterruptsOnIret().

I have a vague memory that I was still experiencing some kind of crashes
before I added DisableInterruptsOnIret(), but I cannot now remember any
details, sorry.

>  2. If DxeCore can be changed, do you have an easier-to-understand
>     solution? It really took me 2 days to understand why
>     NestedInterruptTplLib is written in today’s way.

The ability to change DxeCore doesn't help, unfortunately.

If we could change the prototype of RaiseTPL() and RestoreTPL() to
include a flag indicating whether or not interrupts should be enabled at
the point that RestoreTPL() returns, then that would allow for an
easier-to-understand solution.

This would require making a breaking change to the UEFI specification,
though, so it's not a viable solution.


I do appreciate that it's difficult to understand the internals of
NestedInterruptTplLib.  It's fundamentally having to solve a very
difficult problem within the constraints of the UEFI API.  I think the
solution that NestedInterruptTplLib provides is as simple as it's
possible to get, and it does at least have the advantage that all of the
complexity is hidden inside the library: the caller gets to just change
two lines:

- OriginalTPL = gBS->RaiseTPL(TPL_HIGH_LEVEL);
+ OriginalTPL = NestedInterruptRaiseTPL();
   ...
- gBS->RestoreTPL(OriginalTPL);
+ NestedInterruptRestoreTPL(OriginalTPL, Context, &State);


I'll send through a patch to move NestedInterruptTplLib to MdeModulePkg.

Thanks,

Michael



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



[-- Attachment #2: Type: text/html, Size: 6203 bytes --]

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 19/01/2024 23:44, Ni, Ray wrote:
> I still want to see if the RestoreTpl2 that does not enable interrupt is 
> added as a protocol, and how simple the lib could be.

RestoreTpl() always has to enable interrupts during its execution, since 
interrupts must be allowed to occur while callbacks are running 
(otherwise the callbacks may break due to the system time freezing).

The only alternative approach I am aware of would be to add a 
RestoreTPLEx() call to EFI_BOOT_SERVICES, with an additional parameter 
EnableInterruptsAtRestoredTpl.

RestoreTPLEx() would then:

1. For each TPL between EfiCurrentTpl and OldTpl:
    a) enable interrupts
    b) dispatch any callbacks registered at this TPL
    c) disable interrupts

2. Re-enable interrupts before returning if 
EnableInterruptsAtRestoredTpl is TRUE.

The implementation of RestoreTPL() would then become just a call to 
RestoreTPLEx(OldTPl, (OldTpl < TPL_HIGH_LEVEL)).

This would require a change to the EFI_BOOT_SERVICES table definition, 
which is something that I don't think has happened in the 18 years since 
the UEFI specification was released.  There's a very good chance that 
such a table change would break something, somewhere.

RestoreTPLEx() could be installed as a protocol instead, but it seems 
very messy to have something so fundamental as TPL management and event 
dispatch handled through an installable (and therefore uninstallable) 
protocol.  Are there any other instances where deep internals of DxeCore 
are exposed in this way?

Lastly: I think the RestoreTPLEx approach ought to work, but I have not 
done any testing on it (and have no intention of trying it, unless Intel 
wants to fund the work).  NestedInterruptTplLib has been quite 
thoroughly tested by now.

> The reason is about maintainability.
> I can image that one day people would question the Lib implementation if 
> some timer event issue appears. If the Lib is easy to understand, the 
> suspicion could be avoided.
> And if the correctness of the Lib can be proven by a thorough test, that 
> will be better. But it seems to me the Lib can only be proven as correct 
> with careful code review, like some multi-threaded logic.

It's relatively easy to test with a deliberately broken ISR: that's how 
I tested it during development.

The semi-formal proof is an added bonus.  Testing shows that the 
symptoms have gone away, but the semi-formal proof is what gives 
confidence (to me, at least) that the problem has actually been fixed 
properly.

Thanks,

Michael



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

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Gerd Hoffmann]

  Hi,

> I do appreciate that it's difficult to understand the internals of
> NestedInterruptTplLib.  It's fundamentally having to solve a very difficult
> problem within the constraints of the UEFI API.  I think the solution that
> NestedInterruptTplLib provides is as simple as it's possible to get, and it
> does at least have the advantage that all of the complexity is hidden inside
> the library.

I think NestedInterruptTplLib is a good solution for the problem and I
fully support moving it to MdeModulePkg.

Yes, the library is complex.  I reviewed it when it was merged to
OvmfPkg, and even with the very good comments it took me a few hours to
fully understand the logic.  But the problem is complex too, I also
think there is no simpler way to solve it.

take care,
  Gerd



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