Re: [edk2-devel] [PATCH v2 27/31] OvmfPkg/XenOvmf: Introduce XenTimerDxe

["Laszlo Ersek" <lersek@redhat.com>]

On 04/09/19 13:08, Anthony PERARD wrote:
> "PcAtChipsetPkg/8254TimerDxe" is replaced with a Xen-specific
> EFI_TIMER_ARCH_PROTOCOL implementation. Also remove
> 8259InterruptControllerDxe as it is not used anymore.
> 
> This Timer uses the local APIC timer as time source as it can work on
> both a Xen PVH guest and an HVM one.
> 
> Based on the "PcAtChipsetPkg/8254TimerDxe" implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v2:
>     - Use InitializeApicTimer instead of WriteLocalApicReg
>     - rework comments (remove many that don't apply)
>     - remove unused includes, and libs
>     - have a macro to the timervector.
>     - cleanup, copyright
>     - rework calculation of TimerCount, value to be use by the APIC timer
>     - check for overflow of TimerPeriod, with the apic timer, the period can
>       be up to about 42s on Xen (or even higher by changing the DivideValue).
> 
>  OvmfPkg/XenOvmf.dsc                                                              |   3 +-
>  OvmfPkg/XenOvmf.fdf                                                              |   3 +-
>  PcAtChipsetPkg/8254TimerDxe/8254Timer.inf => OvmfPkg/XenTimerDxe/XenTimerDxe.inf |  27 +++---
>  PcAtChipsetPkg/8254TimerDxe/Timer.h => OvmfPkg/XenTimerDxe/XenTimerDxe.h         |  32 +++----
>  PcAtChipsetPkg/8254TimerDxe/Timer.c => OvmfPkg/XenTimerDxe/XenTimerDxe.c         | 100 ++++++--------------
>  5 files changed, 55 insertions(+), 110 deletions(-)

For OVMF in general, the primary residence of 8254TimerDxe is now under
OvmfPkg, due to <https://bugzilla.tianocore.org/show_bug.cgi?id=1496>.

However, as long as the module continues to exist under PcAtChipsetPkg
(i.e. until <https://bugzilla.tianocore.org/show_bug.cgi?id=1604> is
completed), I think it is OK as the copy-origin here.

> 
> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
> index 54601c697f..35af05715b 100644
> --- a/OvmfPkg/XenOvmf.dsc
> +++ b/OvmfPkg/XenOvmf.dsc
> @@ -560,10 +560,9 @@ [Components]
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>  
>    MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> -  PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf
> +  OvmfPkg/XenTimerDxe/XenTimerDxe.inf
>    UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>    UefiCpuPkg/CpuDxe/CpuDxe.inf
> -  PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>    OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>    OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>    MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf
> index f9e58befd6..d614bdce1d 100644
> --- a/OvmfPkg/XenOvmf.fdf
> +++ b/OvmfPkg/XenOvmf.fdf
> @@ -284,10 +284,9 @@ [FV.DXEFV]
>  INF  MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>  INF  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>  INF  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> -INF  PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf
> +INF  OvmfPkg/XenTimerDxe/XenTimerDxe.inf
>  INF  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
>  INF  UefiCpuPkg/CpuDxe/CpuDxe.inf
> -INF  PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
>  INF  OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
>  INF  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
>  INF  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> diff --git a/PcAtChipsetPkg/8254TimerDxe/8254Timer.inf b/OvmfPkg/XenTimerDxe/XenTimerDxe.inf
> similarity index 64%
> copy from PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> copy to OvmfPkg/XenTimerDxe/XenTimerDxe.inf
> index 46cf01de39..5ad80b9368 100644
> --- a/PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> +++ b/OvmfPkg/XenTimerDxe/XenTimerDxe.inf
> @@ -1,7 +1,9 @@
>  ## @file
> -# 8254 timer driver that provides Timer Arch protocol.
> +# Local APIC timer driver that provides Timer Arch protocol.
> +#
> +# Copyright (c) 2005 - 2014, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2019, Citrix Systems, Inc.
>  #
> -# Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
>  # This program and the accompanying materials
>  # are licensed and made available under the terms and conditions of the BSD License
>  # which accompanies this distribution.  The full text of the license may be found at

I'm sure this is well-intended (you simply created the patch before the
Intel copyright notice was bumped under PcAtChipsetPkg), but now this
diff appears a bit questionable, due to "downgrading" the Intel
copyright. I understand it's technically correct (because the original
code, being copied & customized here, matches the copyright notice too).

(1) Still, could you please refresh this patch so that the Intel
copyright is not touched in the "find-copies-harder" diff? (Covering all
new files added in this patch.)

... Such a refresh would be justified by
<https://bugzilla.tianocore.org/show_bug.cgi?id=1373> anyway -- we
should preferably add new files only with SPDX license IDs now.

Otherwise I'd be OK to ACK the patch.

Thanks
Laszlo


> @@ -14,9 +16,8 @@
>  
>  [Defines]
>    INF_VERSION                    = 0x00010005
> -  BASE_NAME                      = Timer
> -  MODULE_UNI_FILE                = Timer.uni
> -  FILE_GUID                      = f2765dec-6b41-11d5-8e71-00902707b35e
> +  BASE_NAME                      = XenTimerDxe
> +  FILE_GUID                      = 52fe8196-f9de-4d07-b22f-51f77a0e7c41
>    MODULE_TYPE                    = DXE_DRIVER
>    VERSION_STRING                 = 1.0
>  
> @@ -25,24 +26,24 @@ [Defines]
>  [Packages]
>    MdePkg/MdePkg.dec
>    IntelFrameworkPkg/IntelFrameworkPkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +  OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
>    UefiBootServicesTableLib
>    BaseLib
>    DebugLib
>    UefiDriverEntryPoint
> -  IoLib
> +  LocalApicLib
>  
>  [Sources]
> -  Timer.h
> -  Timer.c
> +  XenTimerDxe.h
> +  XenTimerDxe.c
>  
>  [Protocols]
>    gEfiCpuArchProtocolGuid       ## CONSUMES
> -  gEfiLegacy8259ProtocolGuid    ## CONSUMES
>    gEfiTimerArchProtocolGuid     ## PRODUCES
> -
> +[Pcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock  ## CONSUMES
>  [Depex]
> -  gEfiCpuArchProtocolGuid AND gEfiLegacy8259ProtocolGuid
> -[UserExtensions.TianoCore."ExtraFiles"]
> -  TimerExtra.uni
> +  gEfiCpuArchProtocolGuid
> diff --git a/PcAtChipsetPkg/8254TimerDxe/Timer.h b/OvmfPkg/XenTimerDxe/XenTimerDxe.h
> similarity index 89%
> copy from PcAtChipsetPkg/8254TimerDxe/Timer.h
> copy to OvmfPkg/XenTimerDxe/XenTimerDxe.h
> index 9d70e3aa19..747259c162 100644
> --- a/PcAtChipsetPkg/8254TimerDxe/Timer.h
> +++ b/OvmfPkg/XenTimerDxe/XenTimerDxe.h
> @@ -1,7 +1,9 @@
>  /** @file
>    Private data structures
>  
> -Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2019, Citrix Systems, Inc.
> +
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -17,34 +19,24 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <PiDxe.h>
>  
>  #include <Protocol/Cpu.h>
> -#include <Protocol/Legacy8259.h>
>  #include <Protocol/Timer.h>
>  
> +#include <Register/LocalApic.h>
> +
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> -#include <Library/IoLib.h>
> +#include <Library/LocalApicLib.h>
> +#include <Library/PcdLib.h>
>  
> -//
> -// The PCAT 8253/8254 has an input clock at 1.193182 MHz and Timer 0 is
> -// initialized as a 16 bit free running counter that generates an interrupt(IRQ0)
> -// each time the counter rolls over.
> -//
> -//   65536 counts
> -// ---------------- * 1,000,000 uS/S = 54925.4 uS = 549254 * 100 ns
> -//   1,193,182 Hz
> -//
> -
> -//
> -// The maximum tick duration for 8254 timer
> -//
> -#define MAX_TIMER_TICK_DURATION     549254
> -//
>  // The default timer tick duration is set to 10 ms = 100000 100 ns units
>  //
>  #define DEFAULT_TIMER_TICK_DURATION 100000
> -#define TIMER_CONTROL_PORT          0x43
> -#define TIMER0_COUNT_PORT           0x40
> +
> +//
> +// The Timer Vector use for interrupt
> +//
> +#define LOCAL_APIC_TIMER_VECTOR 32
>  
>  //
>  // Function Prototypes
> diff --git a/PcAtChipsetPkg/8254TimerDxe/Timer.c b/OvmfPkg/XenTimerDxe/XenTimerDxe.c
> similarity index 77%
> copy from PcAtChipsetPkg/8254TimerDxe/Timer.c
> copy to OvmfPkg/XenTimerDxe/XenTimerDxe.c
> index 60799aadd3..fb39ce0cd3 100644
> --- a/PcAtChipsetPkg/8254TimerDxe/Timer.c
> +++ b/OvmfPkg/XenTimerDxe/XenTimerDxe.c
> @@ -1,7 +1,9 @@
>  /** @file
>    Timer Architectural Protocol as defined in the DXE CIS
>  
> -Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2019, Citrix Systems, Inc.
> +
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -12,7 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  **/
>  
> -#include "Timer.h"
> +#include "XenTimerDxe.h"
>  
>  //
>  // The handle onto which the Timer Architectural Protocol will be installed
> @@ -34,11 +36,6 @@ EFI_TIMER_ARCH_PROTOCOL   mTimer = {
>  //
>  EFI_CPU_ARCH_PROTOCOL     *mCpu;
>  
> -//
> -// Pointer to the Legacy 8259 Protocol instance
> -//
> -EFI_LEGACY_8259_PROTOCOL  *mLegacy8259;
> -
>  //
>  // The notification function to call on every timer interrupt.
>  // A bug in the compiler prevents us from initializing this here.
> @@ -54,22 +51,7 @@ volatile UINT64           mTimerPeriod = 0;
>  // Worker Functions
>  //
>  /**
> -  Sets the counter value for Timer #0 in a legacy 8254 timer.
> -
> -  @param Count    The 16-bit counter value to program into Timer #0 of the legacy 8254 timer.
> -**/
> -VOID
> -SetPitCount (
> -  IN UINT16  Count
> -  )
> -{
> -  IoWrite8 (TIMER_CONTROL_PORT, 0x36);
> -  IoWrite8 (TIMER0_COUNT_PORT, (UINT8)(Count & 0xff));
> -  IoWrite8 (TIMER0_COUNT_PORT, (UINT8)((Count >> 8) & 0xff));
> -}
> -
> -/**
> -  8254 Timer #0 Interrupt Handler.
> +  Interrupt Handler.
>  
>    @param InterruptType    The type of interrupt that occurred
>    @param SystemContext    A pointer to the system context when the interrupt occurred
> @@ -85,7 +67,7 @@ TimerInterruptHandler (
>  
>    OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
>  
> -  mLegacy8259->EndOfInterrupt (mLegacy8259, Efi8259Irq0);
> +  SendApicEoi();
>  
>    if (mTimerNotifyFunction != NULL) {
>      //
> @@ -187,49 +169,41 @@ TimerDriverSetTimerPeriod (
>    )
>  {
>    UINT64  TimerCount;
> +  UINT32  TimerFrequency;
> +  UINTN   DivideValue = 1;
>  
> -  //
> -  //  The basic clock is 1.19318 MHz or 0.119318 ticks per 100 ns.
> -  //  TimerPeriod * 0.119318 = 8254 timer divisor. Using integer arithmetic
> -  //  TimerCount = (TimerPeriod * 119318)/1000000.
> -  //
> -  //  Round up to next highest integer. This guarantees that the timer is
> -  //  equal to or slightly longer than the requested time.
> -  //  TimerCount = ((TimerPeriod * 119318) + 500000)/1000000
> -  //
> -  // Note that a TimerCount of 0 is equivalent to a count of 65,536
> -  //
> -  // Since TimerCount is limited to 16 bits for IA32, TimerPeriod is limited
> -  // to 20 bits.
> -  //
>    if (TimerPeriod == 0) {
>      //
>      // Disable timer interrupt for a TimerPeriod of 0
>      //
> -    mLegacy8259->DisableIrq (mLegacy8259, Efi8259Irq0);
> +    DisableApicTimerInterrupt();
>    } else {
> +    TimerFrequency = PcdGet32(PcdFSBClock) / DivideValue;
>  
>      //
> -    // Convert TimerPeriod into 8254 counts
> +    // Convert TimerPeriod into local APIC counts
>      //
> -    TimerCount = DivU64x32 (MultU64x32 (119318, (UINT32) TimerPeriod) + 500000, 1000000);
> +    // TimerPeriod is in 100ns
> +    // TimerPeriod/10000000 will be in seconds.
> +    TimerCount = DivU64x32 (MultU64x32 (TimerPeriod, TimerFrequency),
> +                            10000000);
>  
> -    //
>      // Check for overflow
> -    //
> -    if (TimerCount >= 65536) {
> -      TimerCount = 0;
> -      TimerPeriod = MAX_TIMER_TICK_DURATION;
> +    if (TimerCount > MAX_UINT32) {
> +      TimerCount = MAX_UINT32;
> +      /* TimerPeriod = (MAX_UINT32 / TimerFrequency) * 10000000; */
> +      TimerPeriod = 429496730;
>      }
> +
>      //
> -    // Program the 8254 timer with the new count value
> +    // Program the timer with the new count value
>      //
> -    SetPitCount ((UINT16) TimerCount);
> +    InitializeApicTimer(DivideValue, TimerCount, TRUE, LOCAL_APIC_TIMER_VECTOR);
>  
>      //
>      // Enable timer interrupt
>      //
> -    mLegacy8259->EnableIrq (mLegacy8259, Efi8259Irq0, FALSE);
> +    EnableApicTimerInterrupt();
>    }
>    //
>    // Save the new timer period
> @@ -294,16 +268,9 @@ TimerDriverGenerateSoftInterrupt (
>    IN EFI_TIMER_ARCH_PROTOCOL  *This
>    )
>  {
> -  EFI_STATUS  Status;
> -  UINT16      IRQMask;
>    EFI_TPL     OriginalTPL;
>  
> -  //
> -  // If the timer interrupt is enabled, then the registered handler will be invoked.
> -  //
> -  Status = mLegacy8259->GetMask (mLegacy8259, NULL, NULL, &IRQMask, NULL);
> -  ASSERT_EFI_ERROR (Status);
> -  if ((IRQMask & 0x1) == 0) {
> +  if (GetApicTimerInterruptState()) {
>      //
>      // Invoke the registered handler
>      //
> @@ -343,7 +310,6 @@ TimerDriverInitialize (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT32      TimerVector;
>  
>    //
>    // Initialize the pointer to our notify function.
> @@ -361,12 +327,6 @@ TimerDriverInitialize (
>    Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **) &mCpu);
>    ASSERT_EFI_ERROR (Status);
>  
> -  //
> -  // Find the Legacy8259 protocol.
> -  //
> -  Status = gBS->LocateProtocol (&gEfiLegacy8259ProtocolGuid, NULL, (VOID **) &mLegacy8259);
> -  ASSERT_EFI_ERROR (Status);
> -
>    //
>    // Force the timer to be disabled
>    //
> @@ -374,16 +334,10 @@ TimerDriverInitialize (
>    ASSERT_EFI_ERROR (Status);
>  
>    //
> -  // Get the interrupt vector number corresponding to IRQ0 from the 8259 driver
> +  // Install interrupt handler for Local APIC Timer
>    //
> -  TimerVector = 0;
> -  Status      = mLegacy8259->GetVector (mLegacy8259, Efi8259Irq0, (UINT8 *) &TimerVector);
> -  ASSERT_EFI_ERROR (Status);
> -
> -  //
> -  // Install interrupt handler for 8254 Timer #0 (ISA IRQ0)
> -  //
> -  Status = mCpu->RegisterInterruptHandler (mCpu, TimerVector, TimerInterruptHandler);
> +  Status = mCpu->RegisterInterruptHandler (mCpu, LOCAL_APIC_TIMER_VECTOR,
> +                                           TimerInterruptHandler);
>    ASSERT_EFI_ERROR (Status);
>  
>    //
>