|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [edk2] [PATCH RFC 11/14] OvmfPkg/XenOvmf: Adding XenTimerLocalApic
On 12/08/16 16:33, Anthony PERARD wrote:
> And replacing the ACPI Timer by this one based on the local APIC.
>
> ACPI Timer does not work in a PVH guest, but local APIC works on both
> PVH and HVM.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> OvmfPkg/XenOvmf.dsc | 18 +-
> OvmfPkg/XenOvmf.fdf | 2 +-
> OvmfPkg/XenTimerLocalApic/Timer.h | 191 ++++++++++++
> OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c | 386
> ++++++++++++++++++++++++
> OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf | 51 ++++
> 5 files changed, 640 insertions(+), 8 deletions(-)
> create mode 100644 OvmfPkg/XenTimerLocalApic/Timer.h
> create mode 100644 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c
> create mode 100644 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
comments below:
> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
> index ef32c33..31a2185 100644
> --- a/OvmfPkg/XenOvmf.dsc
> +++ b/OvmfPkg/XenOvmf.dsc
> @@ -81,7 +81,7 @@
>
> ################################################################################
> [LibraryClasses]
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> - TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -175,7 +175,7 @@
> !endif
>
> [LibraryClasses.common.SEC]
> - TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> !ifdef $(DEBUG_ON_SERIAL_PORT)
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> @@ -251,7 +251,7 @@
>
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -269,7 +269,7 @@
>
> [LibraryClasses.common.UEFI_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -284,7 +284,7 @@
>
> [LibraryClasses.common.DXE_DRIVER]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -314,7 +314,7 @@
>
> [LibraryClasses.common.UEFI_APPLICATION]
> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
(1) I suggest to split this patch if possible. In the first half, the
TimerLib class resolutions should be flipped, with the argument given in
the second paragraph of the commit message.
The subject line for the first patch should be something like
OvmfPkg/XenOvmf: use a TimerLib instance that depends only on the CPU
> @@ -546,7 +546,11 @@
> !endif
>
> MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> - PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf
> + OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
(2) The second patch should introduce the new DXE_DRIVER module:
OvmfPkg/XenOvmf: introduce XenTimerDxe
The commit message should document that
"PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf" is replaced with a
Xen-specific EFI_TIMER_ARCH_PROTOCOL implementation.
(3) The new DXE_DRIVER module should be located under "OvmfPkg/XenTimerDxe".
> + #{
> + #<LibraryClasses>
> +
> #LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> + #}
(4) Please don't just comment out unnecessary stuff; it's better to
remove those lines.
> UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
> UefiCpuPkg/CpuDxe/CpuDxe.inf
> PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf
> index c211f61..f6876d7 100644
> --- a/OvmfPkg/XenOvmf.fdf
> +++ b/OvmfPkg/XenOvmf.fdf
> @@ -207,7 +207,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> 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/XenTimerLocalApic/XenTimerLocalApic.inf
> INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
> INF UefiCpuPkg/CpuDxe/CpuDxe.inf
> INF PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
> diff --git a/OvmfPkg/XenTimerLocalApic/Timer.h
> b/OvmfPkg/XenTimerLocalApic/Timer.h
> new file mode 100644
> index 0000000..8d6f37c
> --- /dev/null
> +++ b/OvmfPkg/XenTimerLocalApic/Timer.h
> @@ -0,0 +1,191 @@
> +/** @file
> + Private data structures
> +
> +Copyright (c) 2005 - 2016, 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
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +**/
(5) You might want to add a Citrix copyright notice to the new files.
> +
> +#ifndef _TIMER_H_
> +#define _TIMER_H_
> +
> +#include <PiDxe.h>
> +
> +#include <Protocol/Cpu.h>
> +#include <Protocol/Legacy8259.h>
> +#include <Protocol/Timer.h>
> +
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.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
> +
> +//
> +// 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 occured 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/OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c
> b/OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c
> new file mode 100644
> index 0000000..7f3a8f0
> --- /dev/null
> +++ b/OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c
> @@ -0,0 +1,386 @@
> +/** @file
> + Timer Architectural Protocol as defined in the DXE CIS
> +
> +Copyright (c) 2005 - 2016, 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
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Register/LocalApic.h>
> +#include <Library/LocalApicLib.h>
> +#include <Library/PcdLib.h>
> +#include "Timer.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
> +//
> +/**
> + 8254 Timer #0 Interrupt Handler.
> +
> + @param InterruptType The type of interrupt that occured
> + @param SystemContext A pointer to the system context when the interrupt
> occured
> +**/
> +VOID
> +EFIAPI
> +TimerInterruptHandler (
> + IN EFI_EXCEPTION_TYPE InterruptType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + EFI_TPL OriginalTPL;
> +
> + OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> +
> + /* mLegacy8259->EndOfInterrupt (mLegacy8259, Efi8259Irq0); */
(6) Please remove this instead.
> +
> + if (mTimerNotifyFunction != NULL) {
> + //
> + // @bug : This does not handle missed timer interrupts
> + //
> + mTimerNotifyFunction (mTimerPeriod);
> + }
> + SendApicEoi();
> +
> + gBS->RestoreTPL (OriginalTPL);
> +}
> +
> +/**
> +
> + 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
> + //
> + DEBUG((EFI_D_ERROR, "%a %d\n", __FUNCTION__, __LINE__));
(7) This should either be removed, or turned into a DEBUG_VERBOSE message.
> + if (NotifyFunction == NULL && mTimerNotifyFunction == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (NotifyFunction != NULL && mTimerNotifyFunction != NULL) {
> + return EFI_ALREADY_STARTED;
> + }
> + DEBUG((EFI_D_ERROR, "%a %d\n", __FUNCTION__, __LINE__));
(8) Ditto.
> +
> + 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;
> + UINTN DivideValue;
> +
> + GetApicTimerState(&DivideValue, NULL, NULL);
> +
> + // XXX rework comment, copy from other lib
(9) Stale comment?
> + //
> + // 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
> + //
> + if (TimerPeriod == 0) {
> + //
> + // Disable timer interrupt for a TimerPeriod of 0
> + //
> + DisableApicTimerInterrupt();
> + } else {
> + TimerFrequency = PcdGet32(PcdFSBClock) / DivideValue;
> +
> + DEBUG((EFI_D_ERROR, "%a %d, fsbclock %d, freq %d\n",
> + __FUNCTION__, __LINE__,
> + PcdGet32(PcdFSBClock),
> + TimerFrequency));
(10) Should be logged on DEBUG_VERBOSE or DEBUG_INFO level, I think.
Also, please keep a space between the function / macro name, and the
paren that opens the argument list.
> +
> + //
> + // Convert TimerPeriod into local apic counts
> + //
> +
> + TimerCount = DivU64x32 (MultU64x32 (TimerFrequency,
> + (UINT32) TimerPeriod) + 500000,
> + 1000000);
> +
> + DEBUG((EFI_D_ERROR, "%a %d, new init val: %d\n", __FUNCTION__, __LINE__,
> TimerCount));
(11) DEBUG_VERBOSE or DEBUG_INFO, and spacing.
> +
> + //
> + // Program the timer with the new count value
> + //
> + WriteLocalApicReg (XAPIC_TIMER_INIT_COUNT_OFFSET, TimerCount);
> +
> + //
> + // 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);
> +
> + DisableApicTimerInterrupt();
> + 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 occured attempting to
> initialize the driver.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +TimerDriverInitialize (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 TimerVector;
> +
> + //
> + // 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
> + //
> + DisableApicTimerInterrupt();
> +
> + //
> + // Get the interrupt vector number corresponding to IRQ0 from the 8259
> driver
> + //
> + TimerVector = 32;
> + InitializeApicTimer(2, 0, 1, TimerVector);
> +
> + //
> + // Install interrupt handler for 8254 Timer #0 (ISA IRQ0)
> + //
> + Status = mCpu->RegisterInterruptHandler (mCpu, TimerVector,
> 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/OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
> b/OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
> new file mode 100644
> index 0000000..43e137c
> --- /dev/null
> +++ b/OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
> @@ -0,0 +1,51 @@
> +## @file
> +# 8254 timer driver that provides Timer Arch protocol.
> +#
> +# Copyright (c) 2005 - 2014, 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
> +# http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = ApicTimer
> + FILE_GUID = 52fe8196-f9de-4d07-b22f-51f77a0e7c41
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> +
> + ENTRY_POINT = TimerDriverInitialize
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + IntelFrameworkPkg/IntelFrameworkPkg.dec
> + UefiCpuPkg/UefiCpuPkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + UefiBootServicesTableLib
> + BaseLib
> + DebugLib
> + UefiDriverEntryPoint
> + IoLib
> + LocalApicLib
> +
> +[Sources]
> + Timer.h
> + XenTimerLocalApic.c
> +
> +[Protocols]
> + gEfiCpuArchProtocolGuid ## CONSUMES
> + gEfiTimerArchProtocolGuid ## PRODUCES
> +[Pcd.IA32, Pcd.X64]
> + gEfiMdePkgTokenSpaceGuid.PcdFSBClock ## CONSUMES
(12) Any particular reason for not using just [Pcd]?
> +
> +[Depex]
> + gEfiCpuArchProtocolGuid# AND gEfiLegacy8259ProtocolGuid
> +#[UserExtensions.TianoCore."ExtraFiles"]
> + #TimerExtra.uni
>
(13) Since gEfiLegacy8259ProtocolGuid is not consumed, please remove it
from the depex fully, don't just comment it out.
Same for the ExtraFiles section.
Thanks
Laszlo
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |