| Message ID | 1409235244-25783-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/28/14 16:13, Ard Biesheuvel wrote: > From: Michael Casadevall <michael.casadevall@linaro.org> > > 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 <michael.casadevall@linaro.org> > Acked-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > 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(-) Hm, the ArmV7 changes are new in this version of the patch, but from a cursory look, they seem to mirror the AArch64 hunks. I guess my ACK stands. Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
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). ArmLib is a helper library to access architectural registers. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 28 August 2014 15:14 > To: lersek@redhat.com; Olivier Martin; edk2- > devel@lists.sourceforge.net; peter.maydell@linaro.org; > christoffer.dall@linaro.org; drjones@redhat.com; > ilias.biris@linaro.org; leif.lindholm@linaro.org > Cc: Michael Casadevall; Ard Biesheuvel > Subject: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to be > selected > > From: Michael Casadevall <michael.casadevall@linaro.org> > > 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 <michael.casadevall@linaro.org> > Acked-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > 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/
On 1 September 2014 17:19, Olivier Martin <olivier.martin@arm.com> 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:ard.biesheuvel@linaro.org] >> Sent: 28 August 2014 15:14 >> To: lersek@redhat.com; Olivier Martin; edk2- >> devel@lists.sourceforge.net; peter.maydell@linaro.org; >> christoffer.dall@linaro.org; drjones@redhat.com; >> ilias.biris@linaro.org; leif.lindholm@linaro.org >> Cc: Michael Casadevall; Ard Biesheuvel >> Subject: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to be >> selected >> >> From: Michael Casadevall <michael.casadevall@linaro.org> >> >> 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 <michael.casadevall@linaro.org> >> Acked-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> 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/
That's actually what I also had in mind when I saw your Feature PCD and your TimerDxe changes. I was waiting the v6 to get a better understanding of the impact of this new library interface. My suggestion would be to use this interface and libraries: - ArmPkg/Include/Library/ArmGenericTimerCounterLib.h: Definition of the Generic Timer Counter library interface - ArmPkg/Library/ArmGenericTimerPhyCounterLib: implementation for the Generic Timer Physical Counter - ArmPkg/Library/ArmGenericTimerVirtCounterLib: implementation for the Generic Timer Virtual Counter The ARM ARM refers this timer as the 'Generic Timer'. And the idea is to encapsulate the physical and virtual counters under a same interface - both part of the Generic Timer (controller). The question is should these changes move the Architectural Timer code from ArmLib to these new libraries? I am not sure. If we do so, we might have problems if a EDK2 modules need to access both counter registers. My suggestion would be to: 1) Add Virtual Counter support to ArmLib 2) Introduce the new Generic Timer library interface and library implementations > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 01 September 2014 19:18 > To: Olivier Martin > Cc: Laszlo Ersek; edk2-devel@lists.sourceforge.net; Peter Maydell; > Christoffer Dall; Andrew Jones; Ilias Biris; Leif Lindholm; Michael > Casadevall > Subject: Re: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to > be selected > > On 1 September 2014 17:19, Olivier Martin <olivier.martin@arm.com> > 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:ard.biesheuvel@linaro.org] > >> Sent: 28 August 2014 15:14 > >> To: lersek@redhat.com; Olivier Martin; edk2- > >> devel@lists.sourceforge.net; peter.maydell@linaro.org; > >> christoffer.dall@linaro.org; drjones@redhat.com; > >> ilias.biris@linaro.org; leif.lindholm@linaro.org > >> Cc: Michael Casadevall; Ard Biesheuvel > >> Subject: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to be > >> selected > >> > >> From: Michael Casadevall <michael.casadevall@linaro.org> > >> > >> 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 <michael.casadevall@linaro.org> > >> Acked-by: Laszlo Ersek <lersek@redhat.com> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> --- > >> 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/
On 1 September 2014 20:42, Olivier Martin <olivier.martin@arm.com> wrote: > That's actually what I also had in mind when I saw your Feature PCD and your TimerDxe changes. I was waiting the v6 to get a better understanding of the impact of this new library interface. > > My suggestion would be to use this interface and libraries: > - ArmPkg/Include/Library/ArmGenericTimerCounterLib.h: Definition of the Generic Timer Counter library interface > - ArmPkg/Library/ArmGenericTimerPhyCounterLib: implementation for the Generic Timer Physical Counter > - ArmPkg/Library/ArmGenericTimerVirtCounterLib: implementation for the Generic Timer Virtual Counter > > The ARM ARM refers this timer as the 'Generic Timer'. > And the idea is to encapsulate the physical and virtual counters under a same interface - both part of the Generic Timer (controller). > > The question is should these changes move the Architectural Timer code from ArmLib to these new libraries? > I am not sure. If we do so, we might have problems if a EDK2 modules need to access both counter registers. > > My suggestion would be to: > 1) Add Virtual Counter support to ArmLib > 2) Introduce the new Generic Timer library interface and library implementations > OK, fair enough. Now that you have added calls to ArmReadCntPct() and ArmWriteCntpCval() etc to TimerDxe. I suppose that means we should have a VirtTimerDxe under ArmVirtualizationPkg which uses the virt interface. But how does that map onto the PhyCounterLib and VirtCounterLib above? And what is the point of having the ArmArchTimerXXX functions if you also call ArmReadCntpXXX directly? > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 01 September 2014 19:18 >> To: Olivier Martin >> Cc: Laszlo Ersek; edk2-devel@lists.sourceforge.net; Peter Maydell; >> Christoffer Dall; Andrew Jones; Ilias Biris; Leif Lindholm; Michael >> Casadevall >> Subject: Re: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to >> be selected >> >> On 1 September 2014 17:19, Olivier Martin <olivier.martin@arm.com> >> 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:ard.biesheuvel@linaro.org] >> >> Sent: 28 August 2014 15:14 >> >> To: lersek@redhat.com; Olivier Martin; edk2- >> >> devel@lists.sourceforge.net; peter.maydell@linaro.org; >> >> christoffer.dall@linaro.org; drjones@redhat.com; >> >> ilias.biris@linaro.org; leif.lindholm@linaro.org >> >> Cc: Michael Casadevall; Ard Biesheuvel >> >> Subject: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to be >> >> selected >> >> >> >> From: Michael Casadevall <michael.casadevall@linaro.org> >> >> >> >> 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 <michael.casadevall@linaro.org> >> >> Acked-by: Laszlo Ersek <lersek@redhat.com> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> --- >> >> 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/
Ok, let's give a bit more Timer context. We should get something like: - ArmPkg/Library/ArmLib: Helper functions to access the generic timer registers (including physical and virtual counters) - ArmPkg/Include/Library/ArmGenericTimerCounterLib.h: Definition of the Generic Timer Counter library interface - ArmPkg/Library/ArmGenericTimerPhyCounterLib: implementation for the Generic Timer Physical Counter - ArmPkg/Library/ArmGenericTimerVirtCounterLib: implementation for the Generic Timer Virtual Counter - ArmPkg/Library/ArmArchTimerLib: Implement TimerLib for Generic Timer. Should use ArmGenericTimerCounterLib to use either the Physical or Virtual counters - ArmPkg/Drivers/TimerDxe: Implement EFI_TIMER_ARCH_PROTOCOL using ArmGenericTimerCounterLib to use either the Physical or Virtual counters In theory, you should make ArmArchTimerLib and TimerDxe only depend on ArmGenericTimerCounterLib to use either counters. Does it make more sense? Have I missed something? > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 02 September 2014 07:41 > To: Olivier Martin > Cc: Laszlo Ersek; edk2-devel@lists.sourceforge.net; Peter Maydell; > Christoffer Dall; Andrew Jones; Ilias Biris; Leif Lindholm; Michael > Casadevall > Subject: Re: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to > be selected > > On 1 September 2014 20:42, Olivier Martin <olivier.martin@arm.com> > wrote: > > That's actually what I also had in mind when I saw your Feature PCD > and your TimerDxe changes. I was waiting the v6 to get a better > understanding of the impact of this new library interface. > > > > My suggestion would be to use this interface and libraries: > > - ArmPkg/Include/Library/ArmGenericTimerCounterLib.h: Definition of > the Generic Timer Counter library interface > > - ArmPkg/Library/ArmGenericTimerPhyCounterLib: implementation for > the Generic Timer Physical Counter > > - ArmPkg/Library/ArmGenericTimerVirtCounterLib: implementation for > the Generic Timer Virtual Counter > > > > The ARM ARM refers this timer as the 'Generic Timer'. > > And the idea is to encapsulate the physical and virtual counters > under a same interface - both part of the Generic Timer (controller). > > > > The question is should these changes move the Architectural Timer > code from ArmLib to these new libraries? > > I am not sure. If we do so, we might have problems if a EDK2 modules > need to access both counter registers. > > > > My suggestion would be to: > > 1) Add Virtual Counter support to ArmLib > > 2) Introduce the new Generic Timer library interface and library > implementations > > > > OK, fair enough. Now that you have added calls to ArmReadCntPct() and > ArmWriteCntpCval() etc to TimerDxe. I suppose that means we should > have a VirtTimerDxe under ArmVirtualizationPkg which uses the virt > interface. But how does that map onto the PhyCounterLib and > VirtCounterLib above? And what is the point of having the > ArmArchTimerXXX functions if you also call ArmReadCntpXXX directly? > > > > > > > > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >> Sent: 01 September 2014 19:18 > >> To: Olivier Martin > >> Cc: Laszlo Ersek; edk2-devel@lists.sourceforge.net; Peter Maydell; > >> Christoffer Dall; Andrew Jones; Ilias Biris; Leif Lindholm; Michael > >> Casadevall > >> Subject: Re: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer > to > >> be selected > >> > >> On 1 September 2014 17:19, Olivier Martin <olivier.martin@arm.com> > >> 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:ard.biesheuvel@linaro.org] > >> >> Sent: 28 August 2014 15:14 > >> >> To: lersek@redhat.com; Olivier Martin; edk2- > >> >> devel@lists.sourceforge.net; peter.maydell@linaro.org; > >> >> christoffer.dall@linaro.org; drjones@redhat.com; > >> >> ilias.biris@linaro.org; leif.lindholm@linaro.org > >> >> Cc: Michael Casadevall; Ard Biesheuvel > >> >> Subject: [PATCH v4 01/16] ArmPkg/TimerDxe: allow virtual timer to > be > >> >> selected > >> >> > >> >> From: Michael Casadevall <michael.casadevall@linaro.org> > >> >> > >> >> 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 <michael.casadevall@linaro.org> > >> >> Acked-by: Laszlo Ersek <lersek@redhat.com> > >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> >> --- > >> >> 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/
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