On 1 September 2014 17:19, Olivier Martin <[email protected]> wrote: > I have pushed your patxhes through our CI build to make sure there is no > regression. > The build system raised these errors: > # Line endings (CRLF): > - ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > # Trailing space: > - ArmPkg/Drivers/TimerDxe/TimerDxe.inf > > I would prefer you introduce new functions for the Virtual Timer for > instance: ArmArchTimerEnableVirtTimer(), > ArmArchTimerDisableVirtTimer(),ArmArchTimerGetVirtTimerVal(), etc instead of > using the new Feature PCD. > That would allow to use both counters (physical & virtual). >
Actually, I think the correct way would be to make ArmArchTimerLib depend on a control library which can be instantiated both in a physical version and in a virtual version. So this would mean creating an abstract ArmArchTimerControlLib, and implementations in ArmPhysArchTimerControlLib and ArmVirtArchTimerControlLib. Do you agree? > ArmLib is a helper library to access architectural registers. > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:[email protected]] >> Sent: 28 August 2014 15:14 >> To: [email protected]; Olivier Martin; edk2- >> [email protected]; [email protected]; >> [email protected]; [email protected]; >> [email protected]; [email protected] >> Cc: Michael Casadevall; Ard Biesheuvel >> Subject: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to be >> selected >> >> From: Michael Casadevall <[email protected]> >> >> For virtual machines, the physical architected timer may not be >> available, >> so we need to allow the virtual timer to be used instead. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Michael Casadevall <[email protected]> >> Acked-by: Laszlo Ersek <[email protected]> >> Signed-off-by: Ard Biesheuvel <[email protected]> >> --- >> ArmPkg/ArmPkg.dec | 3 + >> ArmPkg/Drivers/TimerDxe/TimerDxe.c | 6 ++ >> ArmPkg/Drivers/TimerDxe/TimerDxe.inf | 4 +- >> ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c | 74 >> +++++++++++++++++++---- >> ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 4 ++ >> ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf | 3 + >> ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf | 3 + >> ArmPkg/Library/ArmLib/ArmV7/ArmV7ArchTimer.c | 70 >> +++++++++++++++++---- >> ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf | 3 + >> ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf | 3 + >> ArmPkg/Library/ArmLib/ArmV7/ArmV7LibSec.inf | 3 + >> 11 files changed, 151 insertions(+), 25 deletions(-) >> >> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec >> index a8ca28fccc82..c2551d7c3307 100644 >> --- a/ArmPkg/ArmPkg.dec >> +++ b/ArmPkg/ArmPkg.dec >> @@ -69,6 +69,9 @@ >> # Linux (instead of PSCI) >> gArmTokenSpaceGuid.PcdArmLinuxSpinTable|FALSE|BOOLEAN|0x00000033 >> >> + # Whether to use the virtual rather than the physical architected >> timer >> + >> gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual|FALSE|BOOLEAN|0x0000003F >> + >> [PcdsFixedAtBuild.common] >> gArmTokenSpaceGuid.PcdTrustzoneSupport|FALSE|BOOLEAN|0x00000006 >> >> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> b/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> index 633876bea6bd..9227be8326b0 100644 >> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> @@ -347,6 +347,12 @@ TimerInitialize ( >> // Note: Because it is not possible to determine the security state >> of the >> // CPU dynamically, we just install interrupt handler for both sec >> and non-sec >> // timer PPI >> + Status = gInterrupt->RegisterInterruptSource (gInterrupt, PcdGet32 >> (PcdArmArchTimerVirtIntrNum), TimerInterruptHandler); >> + ASSERT_EFI_ERROR (Status); >> + >> + Status = gInterrupt->RegisterInterruptSource (gInterrupt, PcdGet32 >> (PcdArmArchTimerHypIntrNum), TimerInterruptHandler); >> + ASSERT_EFI_ERROR (Status); >> + >> Status = gInterrupt->RegisterInterruptSource (gInterrupt, PcdGet32 >> (PcdArmArchTimerSecIntrNum), TimerInterruptHandler); >> ASSERT_EFI_ERROR (Status); >> >> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.inf >> b/ArmPkg/Drivers/TimerDxe/TimerDxe.inf >> index 50477ba42a7a..98b09ba8d203 100644 >> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.inf >> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.inf >> @@ -52,7 +52,9 @@ >> gEmbeddedTokenSpaceGuid.PcdTimerPeriod >> gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum >> gArmTokenSpaceGuid.PcdArmArchTimerIntrNum >> - gArmTokenSpaceGuid.PcdArmArchTimerFreqInHz >> + gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum >> + gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum >> + gArmTokenSpaceGuid.PcdArmArchTimerFreqInHz >> >> [Depex] >> gHardwareInterruptProtocolGuid >> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c >> b/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c >> index fa4f7c741b15..f7ef69d5d4c1 100644 >> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c >> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c >> @@ -175,9 +175,25 @@ ArmArchTimerEnableTimer ( >> { >> UINTN TimerCtrlReg; >> >> - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> - TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; >> - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; >> + >> + /* >> + * When running under KVM, we need to unmask the interrupt on the >> timer side >> + * as KVM will mask it when servicing the interrupt at the >> hypervisor level >> + * and delivering the virtual timer interrupt to the guest. >> Otherwise, the >> + * interrupt will fire again, trapping into the hypervisor again, >> etc. etc. >> + * This is scheduled to be fixed on the KVM side, but there is no >> harm in >> + * leaving this in once KVM gets fixed. >> + */ >> + TimerCtrlReg &= ~ARM_ARCH_TIMER_IMASK; >> + ArmArchTimerWriteReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + } else { >> + ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; >> + ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + } >> } >> >> VOID >> @@ -188,9 +204,15 @@ ArmArchTimerDisableTimer ( >> { >> UINTN TimerCtrlReg; >> >> - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> - TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; >> - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; >> + ArmArchTimerWriteReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + } else { >> + ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; >> + ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + } >> } >> >> VOID >> @@ -220,7 +242,12 @@ ArmArchTimerGetTimerVal ( >> ) >> { >> UINTN ArchTimerVal; >> - ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvTval, (VOID *)&ArchTimerVal); >> + } else { >> + ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); >> + } >> + >> return ArchTimerVal; >> } >> >> @@ -231,7 +258,11 @@ ArmArchTimerSetTimerVal ( >> IN UINTN Val >> ) >> { >> - ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerWriteReg (CntvTval, (VOID *)&Val); >> + } else { >> + ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); >> + } >> } >> >> UINT64 >> @@ -241,7 +272,12 @@ ArmArchTimerGetSystemCount ( >> ) >> { >> UINT64 SystemCount; >> - ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCt, (VOID *)&SystemCount); >> + } else { >> + ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); >> + } >> + >> return SystemCount; >> } >> >> @@ -252,7 +288,13 @@ ArmArchTimerGetTimerCtrlReg ( >> ) >> { >> UINTN Val; >> - ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); >> + >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCtl, (VOID *)&Val); >> + } else { >> + ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); >> + } >> + >> return Val; >> } >> >> @@ -262,7 +304,11 @@ ArmArchTimerSetTimerCtrlReg ( >> UINTN Val >> ) >> { >> - ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerWriteReg (CntvCtl, (VOID *)&Val); >> + } else { >> + ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); >> + } >> } >> >> VOID >> @@ -271,5 +317,9 @@ ArmArchTimerSetCompareVal ( >> IN UINT64 Val >> ) >> { >> - ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerWriteReg (CntvCval, (VOID *)&Val); >> + } else { >> + ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); >> + } >> } >> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf >> b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf >> index e5247848b549..0dc2f26a21b5 100644 >> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf >> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf >> @@ -42,5 +42,9 @@ >> [Protocols] >> gEfiCpuArchProtocolGuid >> >> +[FeaturePcd] >> + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual >> + >> [FixedPcd] >> gArmTokenSpaceGuid.PcdArmCacheOperationThreshold >> + >> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf >> b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf >> index 3a99e1b713cc..081c6fb66cdc 100644 >> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf >> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf >> @@ -44,5 +44,8 @@ >> [Protocols] >> gEfiCpuArchProtocolGuid >> >> +[FeaturePcd] >> + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual >> + >> [FixedPcd] >> gArmTokenSpaceGuid.PcdArmCacheOperationThreshold >> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf >> b/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf >> index 57ac694cd733..1210b337b9c7 100644 >> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf >> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf >> @@ -39,5 +39,8 @@ >> [Protocols] >> gEfiCpuArchProtocolGuid >> >> +[FeaturePcd] >> + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual >> + >> [FixedPcd] >> gArmTokenSpaceGuid.PcdArmCacheOperationThreshold >> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7ArchTimer.c >> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7ArchTimer.c >> index 79083f56b708..150ae60272ca 100644 >> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7ArchTimer.c >> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7ArchTimer.c >> @@ -175,9 +175,25 @@ ArmArchTimerEnableTimer ( >> { >> UINTN TimerCtrlReg; >> >> - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> - TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; >> - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; >> + >> + /* >> + * When running under KVM, we need to unmask the interrupt on the >> timer side >> + * as KVM will mask it when servicing the interrupt at the >> hypervisor level >> + * and delivering the virtual timer interrupt to the guest. >> Otherwise, the >> + * interrupt will fire again, trapping into the hypervisor again, >> etc. etc. >> + * This is scheduled to be fixed on the KVM side, but there is no >> harm in >> + * leaving this in once KVM gets fixed. >> + */ >> + TimerCtrlReg &= ~ARM_ARCH_TIMER_IMASK; >> + ArmArchTimerWriteReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + } else { >> + ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; >> + ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + } >> } >> >> VOID >> @@ -188,9 +204,15 @@ ArmArchTimerDisableTimer ( >> { >> UINTN TimerCtrlReg; >> >> - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> - TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; >> - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; >> + ArmArchTimerWriteReg (CntvCtl, (VOID *)&TimerCtrlReg); >> + } else { >> + ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; >> + ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); >> + } >> } >> >> VOID >> @@ -220,7 +242,11 @@ ArmArchTimerGetTimerVal ( >> ) >> { >> UINTN ArchTimerVal; >> - ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvTval, (VOID *)&ArchTimerVal); >> + } else { >> + ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); >> + } >> return ArchTimerVal; >> } >> >> @@ -231,7 +257,11 @@ ArmArchTimerSetTimerVal ( >> IN UINTN Val >> ) >> { >> - ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerWriteReg (CntvTval, (VOID *)&Val); >> + } else { >> + ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); >> + } >> } >> >> UINT64 >> @@ -241,7 +271,11 @@ ArmArchTimerGetSystemCount ( >> ) >> { >> UINT64 SystemCount; >> - ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCt, (VOID *)&SystemCount); >> + } else { >> + ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); >> + } >> return SystemCount; >> } >> >> @@ -252,7 +286,11 @@ ArmArchTimerGetTimerCtrlReg ( >> ) >> { >> UINTN Val; >> - ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerReadReg (CntvCtl, (VOID *)&Val); >> + } else { >> + ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); >> + } >> return Val; >> } >> >> @@ -262,7 +300,11 @@ ArmArchTimerSetTimerCtrlReg ( >> UINTN Val >> ) >> { >> - ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerWriteReg (CntvCtl, (VOID *)&Val); >> + } else { >> + ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); >> + } >> } >> >> VOID >> @@ -271,5 +313,9 @@ ArmArchTimerSetCompareVal ( >> IN UINT64 Val >> ) >> { >> - ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); >> + if (FeaturePcdGet (PcdArmArchTimerUseVirtual)) { >> + ArmArchTimerWriteReg (CntvCval, (VOID *)&Val); >> + } else { >> + ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); >> + } >> } >> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf >> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf >> index 55c0ec661a81..5fce083fc428 100644 >> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf >> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf >> @@ -49,5 +49,8 @@ >> [Protocols] >> gEfiCpuArchProtocolGuid >> >> +[FeaturePcd] >> + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual >> + >> [FixedPcd] >> gArmTokenSpaceGuid.PcdArmCacheOperationThreshold >> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf >> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf >> index bc403d5613ca..bccd4b1f99bd 100644 >> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf >> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf >> @@ -49,5 +49,8 @@ >> [Protocols] >> gEfiCpuArchProtocolGuid >> >> +[FeaturePcd] >> + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual >> + >> [FixedPcd] >> gArmTokenSpaceGuid.PcdArmCacheOperationThreshold >> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibSec.inf >> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibSec.inf >> index 4081d1a3e563..5a87b2bc080a 100644 >> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibSec.inf >> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibSec.inf >> @@ -43,5 +43,8 @@ >> [Protocols] >> gEfiCpuArchProtocolGuid >> >> +[FeaturePcd] >> + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual >> + >> [FixedPcd] >> gArmTokenSpaceGuid.PcdArmCacheOperationThreshold >> -- >> 1.8.3.2 >> > > > > ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
