[PATCH V2 28/28] OvmfPkg: Add LocalApicTimerDxe


Min Xu
 

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the supported
timer is 8254TimerDxe. So gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will check
the TimerSelector in its entry point. The default Timer is 8254.

TimerSelector will be set to LocalApicTimer by TdxDxe driver in Tdx guest.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/8254TimerDxe/8254Timer.inf | 3 +
OvmfPkg/8254TimerDxe/Timer.c | 5 +
OvmfPkg/8254TimerDxe/Timer.h | 1 +
OvmfPkg/8259InterruptControllerDxe/8259.c | 1 +
OvmfPkg/8259InterruptControllerDxe/8259.inf | 1 +
OvmfPkg/Include/Protocol/TimerSelector.h | 16 +
OvmfPkg/LocalApicTimerDxe/LocalApicTimer.c | 488 ++++++++++++++++++
.../LocalApicTimerDxe/LocalApicTimerDxe.inf | 52 ++
.../LocalApicTimerDxe/LocalApicTimerDxe.uni | 13 +
OvmfPkg/OvmfPkg.dec | 14 +
OvmfPkg/OvmfPkgX64.dsc | 4 +
OvmfPkg/OvmfPkgX64.fdf | 1 +
OvmfPkg/TdxDxe/TdxDxe.c | 3 +
OvmfPkg/TdxDxe/TdxDxe.inf | 1 +
14 files changed, 603 insertions(+)
create mode 100644 OvmfPkg/Include/Protocol/TimerSelector.h
create mode 100644 OvmfPkg/LocalApicTimerDxe/LocalApicTimer.c
create mode 100644 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
create mode 100644 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.uni

diff --git a/OvmfPkg/8254TimerDxe/8254Timer.inf b/OvmfPkg/8254TimerDxe/8254Timer.inf
index 8a07c8247ebe..f15792106944 100644
--- a/OvmfPkg/8254TimerDxe/8254Timer.inf
+++ b/OvmfPkg/8254TimerDxe/8254Timer.inf
@@ -36,6 +36,9 @@
gEfiLegacy8259ProtocolGuid ## CONSUMES
gEfiTimerArchProtocolGuid ## PRODUCES

+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
+
[Depex]
gEfiCpuArchProtocolGuid AND gEfiLegacy8259ProtocolGuid
[UserExtensions.TianoCore."ExtraFiles"]
diff --git a/OvmfPkg/8254TimerDxe/Timer.c b/OvmfPkg/8254TimerDxe/Timer.c
index fd1691beb3c7..b8d29acebf39 100644
--- a/OvmfPkg/8254TimerDxe/Timer.c
+++ b/OvmfPkg/8254TimerDxe/Timer.c
@@ -7,6 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
**/

#include "Timer.h"
+#include <Library/PcdLib.h>

//
// The handle onto which the Timer Architectural Protocol will be installed
@@ -340,6 +341,10 @@ TimerDriverInitialize (
EFI_STATUS Status;
UINT32 TimerVector;

+ if (PcdGet32 (PcdTimerSelector) != TimerSelector8254) {
+ return EFI_UNSUPPORTED;
+ }
+
//
// Initialize the pointer to our notify function.
//
diff --git a/OvmfPkg/8254TimerDxe/Timer.h b/OvmfPkg/8254TimerDxe/Timer.h
index 4c4b720d50dd..2beb7901fefd 100644
--- a/OvmfPkg/8254TimerDxe/Timer.h
+++ b/OvmfPkg/8254TimerDxe/Timer.h
@@ -13,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/Cpu.h>
#include <Protocol/Legacy8259.h>
#include <Protocol/Timer.h>
+#include <Protocol/TimerSelector.h>

#include <Library/UefiBootServicesTableLib.h>
#include <Library/BaseLib.h>
diff --git a/OvmfPkg/8259InterruptControllerDxe/8259.c b/OvmfPkg/8259InterruptControllerDxe/8259.c
index 1c2ac1039d40..eb69302cc12e 100644
--- a/OvmfPkg/8259InterruptControllerDxe/8259.c
+++ b/OvmfPkg/8259InterruptControllerDxe/8259.c
@@ -7,6 +7,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
**/

#include "8259.h"
+#include <Library/PcdLib.h>

//
// Global for the Legacy 8259 Protocol that is produced by this driver
diff --git a/OvmfPkg/8259InterruptControllerDxe/8259.inf b/OvmfPkg/8259InterruptControllerDxe/8259.inf
index 7320ff2490a7..fcd245720060 100644
--- a/OvmfPkg/8259InterruptControllerDxe/8259.inf
+++ b/OvmfPkg/8259InterruptControllerDxe/8259.inf
@@ -22,6 +22,7 @@
[Packages]
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+ UefiCpuPkg/UefiCpuPkg.dec

[LibraryClasses]
UefiBootServicesTableLib
diff --git a/OvmfPkg/Include/Protocol/TimerSelector.h b/OvmfPkg/Include/Protocol/TimerSelector.h
new file mode 100644
index 000000000000..b062ab94706e
--- /dev/null
+++ b/OvmfPkg/Include/Protocol/TimerSelector.h
@@ -0,0 +1,16 @@
+/** @file
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef TIMER_SELECTOR_H_
+#define TIMER_SELECTOR_H_
+
+typedef enum {
+ TimerSelector8254,
+ TimerSelectorLocalApic,
+} TIMER_SELECTOR;
+
+#endif
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimer.c b/OvmfPkg/LocalApicTimerDxe/LocalApicTimer.c
new file mode 100644
index 000000000000..fafb01ec841b
--- /dev/null
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimer.c
@@ -0,0 +1,488 @@
+/** @file
+ Timer Architectural Protocol module using Local APIC Timer
+
+ Copyright (c) 2011 - 2021, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <PiDxe.h>
+
+#include <Protocol/Cpu.h>
+#include <Protocol/Timer.h>
+#include <Protocol/TimerSelector.h>
+
+#include <Library/PcdLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/LocalApicLib.h>
+#include <Register/LocalApic.h>
+#include <ConfidentialComputingGuestAttr.h>
+
+/**
+ 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
+ );
+
+/**
+ 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
+ );
+
+///
+/// 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 = NULL;
+
+///
+/// The notification function to call on every timer interrupt.
+///
+EFI_TIMER_NOTIFY mTimerNotifyFunction = NULL;
+
+///
+/// The current period of the Local APIC timer interrupt in 100 ns units.
+///
+UINT64 mTimerPeriod = 0;
+
+///
+/// Counts the number of Local APIC Timer interrupts processed by this driver.
+/// Only required for debug.
+///
+volatile UINTN mNumTicks;
+
+/**
+ The interrupt handler for the Local APIC timer. This handler clears the Local
+ APIC interrupt and computes the amount of time that has passed since the last
+ Local APIC timer interrupt. If a notification function is registered, then
+ the amount of time since the last Local APIC timer interrupt is passed to that
+ notification function in 100 ns units. The Local APIC timer is updated to
+ generate another interrupt in the required time period.
+
+ @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
+ )
+{
+
+ EFI_TPL OriginalTPL;
+
+ OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+
+ //
+ // Count number of ticks
+ //
+ DEBUG_CODE (mNumTicks++;);
+
+ //
+ // Check to see if there is a registered notification function
+ //
+ if (mTimerNotifyFunction != NULL) {
+ mTimerNotifyFunction (mTimerPeriod);
+ }
+
+ gBS->RestoreTPL (OriginalTPL);
+
+ DisableInterrupts ();
+
+ SendApicEoi();
+}
+
+/**
+ 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;
+ }
+
+ //
+ // Cache the registered notification function
+ //
+ 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
+ )
+{
+ EFI_TPL Tpl;
+ UINTN Divisor;
+ UINT64 TimerCount;
+
+ //
+ // Disable interrupts
+ //
+ Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+
+ if (TimerPeriod == 0) {
+ //
+ // Disable timer interrupt for a TimerPeriod of 0
+ //
+ DisableApicTimerInterrupt ();
+ } else {
+ DisableApicTimerInterrupt ();
+
+ //
+ // Convert TimerPeriod in 100ns units to Local APIC Timer ticks.
+ //
+ GetApicTimerState (&Divisor, NULL, NULL);
+ TimerCount = DivU64x32 (
+ MultU64x32 (TimerPeriod, PcdGet32(PcdFSBClock)),
+ (UINT32)Divisor * 10000000
+ );
+
+ //
+ // Program the local APIC timer
+ //
+ InitializeApicTimer (0, (UINT32)TimerCount, TRUE, PcdGet8 (PcdHpetLocalApicVector));
+
+ EnableApicTimerInterrupt ();
+ }
+
+ //
+ // Save the new timer period
+ //
+ mTimerPeriod = TimerPeriod;
+
+ //
+ // Restore interrupts
+ //
+ gBS->RestoreTPL (Tpl);
+
+ 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
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+/**
+ 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;
+
+ if (PcdGet32 (PcdTimerSelector) != TimerSelectorLocalApic) {
+ return EFI_UNSUPPORTED;
+ }
+
+ //
+ // 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);
+
+ //
+ // Install interrupt handler for the Local APIC Timer
+ //
+ Status = mCpu->RegisterInterruptHandler (mCpu, PcdGet8 (PcdHpetLocalApicVector), TimerInterruptHandler);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Unable to register Local APIC interrupt with CPU Arch Protocol. Unload Local APIC timer driver.\n"));
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Force the Local APIC timer to be disabled while setting everything up
+ //
+ DisableApicTimerInterrupt ();
+ InitializeApicTimer (0, 0, FALSE, PcdGet8 (PcdHpetLocalApicVector));
+
+ //
+ // Force the Local APIC Timer to be enabled at its default period
+ //
+ Status = TimerDriverSetTimerPeriod (&mTimer, PcdGet64 (PcdHpetDefaultTimerPeriod));
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Unable to set Local APIC default timer rate. Unload Local APIC timer driver.\n"));
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Show state of enabled timer
+ //
+ DEBUG_CODE (
+ //
+ // Wait for a few timer interrupts to fire before continuing
+ //
+ while (mNumTicks < 10);
+ );
+
+ //
+ // 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/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
new file mode 100644
index 000000000000..70bc0ef6b2ea
--- /dev/null
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -0,0 +1,52 @@
+## @file
+# Timer Architectural Protocol module using Local APIC Timer
+#
+# Copyright (c) 2011 - 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = LocalApicTimerDxe
+ MODULE_UNI_FILE = LocalApicTimerDxe.uni
+ FILE_GUID = 74EB4D00-E63E-11EA-8B6E-0800200C9A66
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = TimerDriverInitialize
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = X64
+#
+#
+
+[Sources]
+ LocalApicTimer.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ UefiCpuPkg/UefiCpuPkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ PcdLib
+ DebugLib
+ UefiDriverEntryPoint
+ UefiBootServicesTableLib
+ BaseLib
+ LocalApicLib
+
+[Protocols]
+ gEfiTimerArchProtocolGuid ## PRODUCES
+ gEfiCpuArchProtocolGuid ## CONSUMES
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdHpetLocalApicVector ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdHpetDefaultTimerPeriod ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdFSBClock
+ gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
+
+[Depex]
+ gEfiCpuArchProtocolGuid
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.uni b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.uni
new file mode 100644
index 000000000000..7525d9493858
--- /dev/null
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.uni
@@ -0,0 +1,13 @@
+// /** @file
+// Timer Architectural Protocol module using Local APIC Timer
+//
+// Copyright (c) 2011 - 2020, Intel Corporation. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// **/
+
+
+#string STR_MODULE_ABSTRACT #language en-US "Timer Architectural Protocol module using Local APIC Timer"
+
+#string STR_MODULE_DESCRIPTION #language en-US "Timer Architectural Protocol module using Local APIC Timer."
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index dda83d81695b..56714d3311ad 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -369,6 +369,15 @@
## The Tdx accept page size. 0x1000(4k),0x200000(2M)
gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize|0x1000|UINT64|0x5a

+ ## This PCD specifies the Local APIC Interrupt Vector for the HPET Timer.
+ # @Prompt HPET local APIC vector.
+ gUefiOvmfPkgTokenSpaceGuid.PcdHpetLocalApicVector|0x40|UINT8|0x5b
+
+ ## This PCD specifies the default period of the HPET Timer in 100 ns units.
+ # The default value of 100000 100 ns units is the same as 10 ms.
+ # @Prompt Default period of HPET timer.
+ gUefiOvmfPkgTokenSpaceGuid.PcdHpetDefaultTimerPeriod|100000|UINT64|0x5c
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
@@ -411,6 +420,11 @@
## TDX relocated Mailbox base address
gUefiOvmfPkgTokenSpaceGuid.PcdTdRelocatedMailboxBase|0|UINT64|0x60

+ ## Timer selector
+ # There are multiple timer in Ovmf. This PCD indicates which Timer is installed.
+ # The default Timer is 8254 (0x0). See TimerSelector.h for more definition.
+ gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector|0|UINT32|0x61
+
[PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 455e901c2eb8..a17d6603af0b 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -777,6 +777,10 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
+ OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf {
+ <LibraryClasses>
+ LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
+ }
OvmfPkg/8254TimerDxe/8254Timer.inf
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index bbd9303ab14f..716297e52aff 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -236,6 +236,7 @@ INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
+INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/TdxDxe/TdxDxe.c b/OvmfPkg/TdxDxe/TdxDxe.c
index eecad8f6e050..fcfb972cc67c 100644
--- a/OvmfPkg/TdxDxe/TdxDxe.c
+++ b/OvmfPkg/TdxDxe/TdxDxe.c
@@ -23,6 +23,7 @@
#include <Library/UefiLib.h>
#include <Library/HobLib.h>
#include <Protocol/Cpu.h>
+#include <Protocol/TimerSelector.h>
#include <Library/UefiBootServicesTableLib.h>
#include <IndustryStandard/Tdx.h>
#include <IndustryStandard/IntelTdx.h>
@@ -131,6 +132,8 @@ TdxDxeEntryPoint (
return EFI_UNSUPPORTED;
}

+ PcdSet32S (PcdTimerSelector, TimerSelectorLocalApic);
+
PlatformInfo = (EFI_HOB_PLATFORM_INFO *) GET_GUID_HOB_DATA (GuidHob);

//
diff --git a/OvmfPkg/TdxDxe/TdxDxe.inf b/OvmfPkg/TdxDxe/TdxDxe.inf
index b77c6e5e9252..045c3c8c2ccc 100644
--- a/OvmfPkg/TdxDxe/TdxDxe.inf
+++ b/OvmfPkg/TdxDxe/TdxDxe.inf
@@ -60,3 +60,4 @@
gUefiOvmfPkgTokenSpaceGuid.PcdTdRelocatedMailboxBase
gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+ gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
--
2.29.2.windows.2


Gerd Hoffmann
 

On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the supported
timer is 8254TimerDxe. So gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will check
the TimerSelector in its entry point. The default Timer is 8254.
Hmm.

We already have a local apic timer implementation (XenTimerDxe). Works
fine with kvm, microvm already uses that. See commit 76602f45dcd9
("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").

So, first I'd suggest to just use that (maybe rename the thing to avoid
confusion as it isn't really Xen specific).

Next question is whenever there is a need for a runtime switch. I doubt
it is possible to create a virtual machine without lapic, so switching
ovmf from 8254 (aka pit) to lapic unconditionally should work fine.
Quick smoke test (patch below) shows no obvious problems.

take care,
Gerd

From 948aaf555a864da1d6f2ccc8dddc7e22cefb76e1 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 12 Oct 2021 14:58:18 +0200
Subject: [PATCH 1/1] OvmfPkgX64: use lapic timer

---
OvmfPkg/OvmfPkgX64.dsc | 4 ++--
OvmfPkg/OvmfPkgX64.fdf | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 52f7598cf1c7..687aae6e3e68 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -620,6 +620,7 @@ [PcdsDynamicDefault]
gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
!endif

+ gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0

# Set video resolution for text setup.
@@ -765,10 +766,9 @@ [Components]
}

MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
- OvmfPkg/8259InterruptControllerDxe/8259.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
- OvmfPkg/8254TimerDxe/8254Timer.inf
+ OvmfPkg/XenTimerDxe/XenTimerDxe.inf
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index b6cc3cabdd69..9ca723dc8604 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -232,10 +232,9 @@ [FV.DXEFV]
INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
-INF OvmfPkg/8254TimerDxe/8254Timer.inf
+INF OvmfPkg/XenTimerDxe/XenTimerDxe.inf
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
--
2.31.1


Min Xu
 

On October 12, 2021 9:02 PM, Gerd Hoffmann wrote:
On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the
supported timer is 8254TimerDxe. So
gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will check
the TimerSelector in its entry point. The default Timer is 8254.
Hmm.

We already have a local apic timer implementation (XenTimerDxe). Works fine
with kvm, microvm already uses that. See commit 76602f45dcd9
("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").

So, first I'd suggest to just use that (maybe rename the thing to avoid confusion
as it isn't really Xen specific).
Thanks for reminder. Let me first do some more investigation about the XenTimerDxe. It will be better to use an existing lapic timer than introducing a new one.

Next question is whenever there is a need for a runtime switch. I doubt it is
possible to create a virtual machine without lapic, so switching ovmf from 8254
(aka pit) to lapic unconditionally should work fine.
Quick smoke test (patch below) shows no obvious problems.
Let me do some more investigation.
Thanks.
Min


Yao, Jiewen
 

Good suggestion, Gerd.

I agree with both suggestion. We should rename XenTimerDxe to LocalApicTimerDxe.

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, October 14, 2021 1:20 PM
To: devel@edk2.groups.io; kraxel@redhat.com
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
<jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Erdem
Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>
Subject: RE: [edk2-devel] [PATCH V2 28/28] OvmfPkg: Add LocalApicTimerDxe

On October 12, 2021 9:02 PM, Gerd Hoffmann wrote:
On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the
supported timer is 8254TimerDxe. So
gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will check
the TimerSelector in its entry point. The default Timer is 8254.
Hmm.

We already have a local apic timer implementation (XenTimerDxe). Works fine
with kvm, microvm already uses that. See commit 76602f45dcd9
("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").

So, first I'd suggest to just use that (maybe rename the thing to avoid
confusion
as it isn't really Xen specific).
Thanks for reminder. Let me first do some more investigation about the
XenTimerDxe. It will be better to use an existing lapic timer than introducing a
new one.

Next question is whenever there is a need for a runtime switch. I doubt it is
possible to create a virtual machine without lapic, so switching ovmf from
8254
(aka pit) to lapic unconditionally should work fine.
Quick smoke test (patch below) shows no obvious problems.
Let me do some more investigation.
Thanks.
Min


Min Xu
 

On October 12, 2021 9:02 PM, Gerd Hoffmann wrote:
On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the
supported timer is 8254TimerDxe. So
gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will check
the TimerSelector in its entry point. The default Timer is 8254.
Hmm.

We already have a local apic timer implementation (XenTimerDxe). Works fine
with kvm, microvm already uses that. See commit 76602f45dcd9
("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").

So, first I'd suggest to just use that (maybe rename the thing to avoid confusion
as it isn't really Xen specific).
Thanks for reminder. We can use XenTimerDxe as the LocalApicTimerDxe in Tdx guest. There will be a separate patch to rename XenTimerDxe to LocalApicTimerDxe in the next version.

Next question is whenever there is a need for a runtime switch. I doubt it is
possible to create a virtual machine without lapic, so switching ovmf from 8254
(aka pit) to lapic unconditionally should work fine.
Quick smoke test (patch below) shows no obvious problems.
I am not quite sure if there will be any side effect if we switch ovmf (X64) from 8254 to lapic unconditionally. Quick smoke test does show no obvious problems (EDK2 CI shows no error either). But since 8254 timer has already been used in OvmfPkgX64, then there always a reason why 8254 is used.
I am thinking if it is a more secure way to introduce PcdTimerSelector (to select timer in run-time) this time.
We can revisit this proposal (switch ovmf from 8254 to lapic unconditionally) when we are pretty sure there is no side effect in the future.

Thanks.
Min


Gerd Hoffmann
 

On Mon, Oct 25, 2021 at 07:37:33AM +0000, Min Xu wrote:
On October 12, 2021 9:02 PM, Gerd Hoffmann wrote:
On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the
supported timer is 8254TimerDxe. So
gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will check
the TimerSelector in its entry point. The default Timer is 8254.
Hmm.

We already have a local apic timer implementation (XenTimerDxe). Works fine
with kvm, microvm already uses that. See commit 76602f45dcd9
("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").

So, first I'd suggest to just use that (maybe rename the thing to avoid confusion
as it isn't really Xen specific).
Thanks for reminder. We can use XenTimerDxe as the LocalApicTimerDxe in Tdx guest. There will be a separate patch to rename XenTimerDxe to LocalApicTimerDxe in the next version.
You can also split this off into a separate patch series as it
shouldn't have any tdx dependency.

I am not quite sure if there will be any side effect if we switch ovmf
(X64) from 8254 to lapic unconditionally. Quick smoke test does show
no obvious problems (EDK2 CI shows no error either). But since 8254
timer has already been used in OvmfPkgX64, then there always a reason
why 8254 is used.
Note that 8254TimerDxe was not written for OVMF, it was moved over
from PcAtChipsetPkg to OvmfPkg in 2019. Probably because OVMF was
the only user left.

Most likely the reason OVMF used 8254TimerDxe initially was that it
could just use the existing driver in PcAtChipsetPkg. And it simply
hasn't been changed ever.

Hmm, CSM support was moved in 2019 too, checking ...

Yes, CSM support depends on 8254 (and 8259) drivers.

I am thinking if it is a more secure way to introduce PcdTimerSelector
(to select timer in run-time) this time. We can revisit this proposal
(switch ovmf from 8254 to lapic unconditionally) when we are pretty
sure there is no side effect in the future.
I still think we don't need a runtime switch. Continue using
8254TimerDxe for CSM_ENABLE=TRUE builds should be enough.

take care,
Gerd


Min Xu
 

On October 25, 2021 7:28 PM, Gerd Hoffmann wrote:
On Mon, Oct 25, 2021 at 07:37:33AM +0000, Min Xu wrote:
On October 12, 2021 9:02 PM, Gerd Hoffmann wrote:
On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

TDX guest supports LocalApicTimer. But in current OvmfPkg the
supported timer is 8254TimerDxe. So
gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
is introduced to select the running Timer. The Timer driver will
check the TimerSelector in its entry point. The default Timer is 8254.
Hmm.

We already have a local apic timer implementation (XenTimerDxe).
Works fine with kvm, microvm already uses that. See commit
76602f45dcd9
("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").

So, first I'd suggest to just use that (maybe rename the thing to
avoid confusion as it isn't really Xen specific).
Thanks for reminder. We can use XenTimerDxe as the LocalApicTimerDxe in
Tdx guest. There will be a separate patch to rename XenTimerDxe to
LocalApicTimerDxe in the next version.

You can also split this off into a separate patch series as it shouldn't have any tdx
dependency.

I am not quite sure if there will be any side effect if we switch ovmf
(X64) from 8254 to lapic unconditionally. Quick smoke test does show
no obvious problems (EDK2 CI shows no error either). But since 8254
timer has already been used in OvmfPkgX64, then there always a reason
why 8254 is used.
Note that 8254TimerDxe was not written for OVMF, it was moved over from
PcAtChipsetPkg to OvmfPkg in 2019. Probably because OVMF was the only user
left.

Most likely the reason OVMF used 8254TimerDxe initially was that it could just
use the existing driver in PcAtChipsetPkg. And it simply hasn't been changed ever.

Hmm, CSM support was moved in 2019 too, checking ...

Yes, CSM support depends on 8254 (and 8259) drivers.

I am thinking if it is a more secure way to introduce PcdTimerSelector
(to select timer in run-time) this time. We can revisit this proposal
(switch ovmf from 8254 to lapic unconditionally) when we are pretty
sure there is no side effect in the future.
I still think we don't need a runtime switch. Continue using 8254TimerDxe for
CSM_ENABLE=TRUE builds should be enough.
Thanks for your detailed explanation. I agree we don't need a runtime switch. Just use CSM_ENABLE=TRUE in *.dsc/*.fdf to switch 8254 and lapic in build time.
I will submit a separate patch series for this change.

There are 4 .dsc which include the 8254Timer.
- OvmfPkg/AmdSev/AmdSevX64.dsc
- OvmfPkg/OvmfPkgIa32.dsc
- OvmfPkg/OvmfPkgIa32X64.dsc
- OvmfPkg/OvmfPkgX64.dsc

Do you think we should apply the changes to all above 4 .dsc?

Thanks
Min


Gerd Hoffmann
 

I still think we don't need a runtime switch. Continue using 8254TimerDxe for
CSM_ENABLE=TRUE builds should be enough.
Thanks for your detailed explanation. I agree we don't need a runtime switch. Just use CSM_ENABLE=TRUE in *.dsc/*.fdf to switch 8254 and lapic in build time.
I will submit a separate patch series for this change.

There are 4 .dsc which include the 8254Timer.
- OvmfPkg/AmdSev/AmdSevX64.dsc
- OvmfPkg/OvmfPkgIa32.dsc
- OvmfPkg/OvmfPkgIa32X64.dsc
- OvmfPkg/OvmfPkgX64.dsc

Do you think we should apply the changes to all above 4 .dsc?
For the AmdSev config it doesn't make sense to support a CSM.
So I' suggest to just remove support for CSM_ENABLE=TRUE (separate
patch), then use the lapic timer unconditionally.

For the three OvmfPkg* configs using 8254TimerDxe with CSM_ENABLE=TRUE
and LapicTimerDxe otherwise is fine.

take care,
Gerd