| Message ID | 1408994367-11143-4-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
some comments below, not affecting semantics On 08/25/14 21:19, 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. > > Signed-off-by: Michael Casadevall <michael.casadevall@linaro.org> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPkg/ArmPkg.dec | 5 ++ > ArmPkg/Drivers/TimerDxe/TimerDxe.c | 6 +++ > ArmPkg/Drivers/TimerDxe/TimerDxe.inf | 3 ++ > ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c | 66 ++++++++++++++++++----- > ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 2 + > ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf | 1 + > ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf | 1 + > 7 files changed, 72 insertions(+), 12 deletions(-) > > diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec > index 14f16b85169a..1d896892dafc 100644 > --- a/ArmPkg/ArmPkg.dec > +++ b/ArmPkg/ArmPkg.dec > @@ -143,6 +143,11 @@ > # ARM Architectural Timer Interrupt(GIC PPI) number > gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum|29|UINT32|0x00000035 > gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|30|UINT32|0x00000036 > + gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|27|UINT32|0x00000040 > + gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum|26|UINT32|0x00000041 > + > + # MC: not really sure what the last value is except I think it needs to be unique (1) Please drop this comment. > + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual|FALSE|BOOLEAN|0x0000003F > > [PcdsFixedAtBuild.ARM] > # > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > index a43a10f48cee..be346bf89c51 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 061fcbc688d9..fa48e569a17d 100644 > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.inf > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.inf > @@ -52,6 +52,9 @@ > gEmbeddedTokenSpaceGuid.PcdTimerPeriod > gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum > gArmTokenSpaceGuid.PcdArmArchTimerIntrNum > + gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum > + gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum > + > gArmTokenSpaceGuid.PcdArmArchTimerFreqInHz > > [Depex] > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c b/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c > index fa4f7c741b15..232d745b3d06 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c > @@ -175,9 +175,17 @@ ArmArchTimerEnableTimer ( > { > UINTN TimerCtrlReg; > > - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); > - TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; > - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { (2) Please drop "== TRUE". > + ArmArchTimerReadReg (CntvCtl, (VOID *)&TimerCtrlReg); > + /* FIXME: We need to clear IMASK when under KVM because KVM sets it. Unclear if this is a bug ... */ (3) If more information is available whether this is a bug or not, please clean up this comment. > + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; > + TimerCtrlReg &= ~ARM_ARCH_TIMER_IMASK; > + ArmArchTimerWriteReg (CntvCtl, (VOID *)&TimerCtrlReg); > + } else { > + ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); > + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; > + ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); > + } > } (4) Please drop the casts to (VOID *) with extreme prejudice. ArmArchTimerWriteReg() has the following prototype: VOID EFIAPI ArmArchTimerWriteReg ( IN ARM_ARCH_TIMER_REGS Reg, IN VOID *SrcBuf ); No need for the explicit cast. ArmArchTimerReadReg() is similar. I do realize that the preexistent code contains such casts. I don't insist. > > VOID > @@ -188,9 +196,15 @@ ArmArchTimerDisableTimer ( > { > UINTN TimerCtrlReg; > > - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); > - TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; > - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + 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); > + } > } > (2) and (4) apply. > VOID > @@ -220,7 +234,12 @@ ArmArchTimerGetTimerVal ( > ) > { > UINTN ArchTimerVal; > - ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + ArmArchTimerReadReg (CntvTval, (VOID *)&ArchTimerVal); > + } else { > + ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); > + } > + > return ArchTimerVal; > } (2) and (4) apply. > > @@ -231,7 +250,11 @@ ArmArchTimerSetTimerVal ( > IN UINTN Val > ) > { > - ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + ArmArchTimerWriteReg (CntvTval, (VOID *)&Val); > + } else { > + ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); > + } > } (2) and (4) apply. and so on... > > UINT64 > @@ -241,7 +264,12 @@ ArmArchTimerGetSystemCount ( > ) > { > UINT64 SystemCount; > - ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + ArmArchTimerReadReg (CntvCt, (VOID *)&SystemCount); > + } else { > + ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); > + } > + > return SystemCount; > } > > @@ -252,7 +280,13 @@ ArmArchTimerGetTimerCtrlReg ( > ) > { > UINTN Val; > - ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); > + > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + ArmArchTimerReadReg (CntvCtl, (VOID *)&Val); > + } else { > + ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); > + } > + > return Val; > } > > @@ -262,7 +296,11 @@ ArmArchTimerSetTimerCtrlReg ( > UINTN Val > ) > { > - ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + ArmArchTimerWriteReg (CntvCtl, (VOID *)&Val); > + } else { > + ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); > + } > } > > VOID > @@ -271,5 +309,9 @@ ArmArchTimerSetCompareVal ( > IN UINT64 Val > ) > { > - ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); > + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { > + ArmArchTimerWriteReg (CntvCval, (VOID *)&Val); > + } else { > + ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); > + } > } Then: > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > index e5247848b549..f7d50b9bd54c 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > @@ -44,3 +44,5 @@ > > [FixedPcd] > gArmTokenSpaceGuid.PcdArmCacheOperationThreshold > + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual > + > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf > index 3a99e1b713cc..698de2979f97 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf > @@ -46,3 +46,4 @@ > > [FixedPcd] > gArmTokenSpaceGuid.PcdArmCacheOperationThreshold > + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf > index 57ac694cd733..0eba4c04d404 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf > @@ -41,3 +41,4 @@ > > [FixedPcd] > gArmTokenSpaceGuid.PcdArmCacheOperationThreshold > + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual > (5) Now that I think of it, you might want to turn this BOOLEAN-typed, fixed-at-build PCD into a FeaturePcd. If you do that, then the referring INF files will have to be updated as well (and maybe some DSCs too, I'm not that far ahead); such PCDs are listed under the separate [FeaturePcd] section. (6) A generic note for the entire series: please consistently keep a space character between function (or macro) name and the opening paren. This covers function declarations, function definitions, function calls, and macro invocations. (Function-like macro definitions cannot adhere.) (In this patch, the PcdGetBool() calls break this "rule".) This is incredibly annoying, agreed, but that's the edk2 coding style. OTOH, Olivier might not insist. Wrt. semantics, I defer to Olivier. Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec index 14f16b85169a..1d896892dafc 100644 --- a/ArmPkg/ArmPkg.dec +++ b/ArmPkg/ArmPkg.dec @@ -143,6 +143,11 @@ # ARM Architectural Timer Interrupt(GIC PPI) number gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum|29|UINT32|0x00000035 gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|30|UINT32|0x00000036 + gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|27|UINT32|0x00000040 + gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum|26|UINT32|0x00000041 + + # MC: not really sure what the last value is except I think it needs to be unique + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual|FALSE|BOOLEAN|0x0000003F [PcdsFixedAtBuild.ARM] # diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c index a43a10f48cee..be346bf89c51 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 061fcbc688d9..fa48e569a17d 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.inf +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.inf @@ -52,6 +52,9 @@ gEmbeddedTokenSpaceGuid.PcdTimerPeriod gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum gArmTokenSpaceGuid.PcdArmArchTimerIntrNum + gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum + gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum + gArmTokenSpaceGuid.PcdArmArchTimerFreqInHz [Depex] diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c b/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c index fa4f7c741b15..232d745b3d06 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64ArchTimer.c @@ -175,9 +175,17 @@ ArmArchTimerEnableTimer ( { UINTN TimerCtrlReg; - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); - TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + ArmArchTimerReadReg (CntvCtl, (VOID *)&TimerCtrlReg); + /* FIXME: We need to clear IMASK when under KVM because KVM sets it. Unclear if this is a bug ... */ + TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE; + 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 +196,15 @@ ArmArchTimerDisableTimer ( { UINTN TimerCtrlReg; - ArmArchTimerReadReg (CntpCtl, (VOID *)&TimerCtrlReg); - TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE; - ArmArchTimerWriteReg (CntpCtl, (VOID *)&TimerCtrlReg); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + 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 +234,12 @@ ArmArchTimerGetTimerVal ( ) { UINTN ArchTimerVal; - ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + ArmArchTimerReadReg (CntvTval, (VOID *)&ArchTimerVal); + } else { + ArmArchTimerReadReg (CntpTval, (VOID *)&ArchTimerVal); + } + return ArchTimerVal; } @@ -231,7 +250,11 @@ ArmArchTimerSetTimerVal ( IN UINTN Val ) { - ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + ArmArchTimerWriteReg (CntvTval, (VOID *)&Val); + } else { + ArmArchTimerWriteReg (CntpTval, (VOID *)&Val); + } } UINT64 @@ -241,7 +264,12 @@ ArmArchTimerGetSystemCount ( ) { UINT64 SystemCount; - ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + ArmArchTimerReadReg (CntvCt, (VOID *)&SystemCount); + } else { + ArmArchTimerReadReg (CntPct, (VOID *)&SystemCount); + } + return SystemCount; } @@ -252,7 +280,13 @@ ArmArchTimerGetTimerCtrlReg ( ) { UINTN Val; - ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); + + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + ArmArchTimerReadReg (CntvCtl, (VOID *)&Val); + } else { + ArmArchTimerReadReg (CntpCtl, (VOID *)&Val); + } + return Val; } @@ -262,7 +296,11 @@ ArmArchTimerSetTimerCtrlReg ( UINTN Val ) { - ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + ArmArchTimerWriteReg (CntvCtl, (VOID *)&Val); + } else { + ArmArchTimerWriteReg (CntpCtl, (VOID *)&Val); + } } VOID @@ -271,5 +309,9 @@ ArmArchTimerSetCompareVal ( IN UINT64 Val ) { - ArmArchTimerWriteReg (CntpCval, (VOID *)&Val); + if (PcdGetBool(PcdArmArchTimerUseVirtual) == TRUE) { + 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..f7d50b9bd54c 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -44,3 +44,5 @@ [FixedPcd] gArmTokenSpaceGuid.PcdArmCacheOperationThreshold + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual + diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf index 3a99e1b713cc..698de2979f97 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPrePi.inf @@ -46,3 +46,4 @@ [FixedPcd] gArmTokenSpaceGuid.PcdArmCacheOperationThreshold + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf index 57ac694cd733..0eba4c04d404 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf @@ -41,3 +41,4 @@ [FixedPcd] gArmTokenSpaceGuid.PcdArmCacheOperationThreshold + gArmTokenSpaceGuid.PcdArmArchTimerUseVirtual