When we first ported EDK2 to KVM/arm, we implemented a workaround for the quirky timer handling on the KVM side. This has been fixed in Linux commit f120cd6533d2 ("KVM: arm/arm64: timer: Allow the timer to control the active state") dated 23 June 2014, which was incorporated into Linux release 4.3.
So almost 4 years later, it should be safe to drop this workaround on the EDK2 side.
This reverts commit b1a633434ddc.
Cc: cross-distro@lists.linaro.org Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org Acked-by: Marc Zyngier marc.zyngier@arm.com Reviewed-by: Leif Lindholm leif.lindholm@linaro.org Acked-by: Laszlo Ersek lersek@redhat.com --- v2: add acks
Note to cross-distro readers: this means guest firmware built with this patch will not work on KVM/ARM hosts using kernel v4.2 or earlier.
ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 - ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c | 10 ---------- 2 files changed, 11 deletions(-)
diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c index a3202fa056f3..bd616d2efc73 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c @@ -337,7 +337,6 @@ TimerInterruptHandler (
// Set next compare value ArmGenericTimerSetCompareVal (CompareValue); - ArmGenericTimerEnableTimer (); ArmInstructionSynchronizationBarrier (); }
diff --git a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c index 69a4ceb62db6..c941895a3574 100644 --- a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c +++ b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c @@ -26,16 +26,6 @@ ArmGenericTimerEnableTimer (
TimerCtrlReg = ArmReadCntvCtl (); 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; ArmWriteCntvCtl (TimerCtrlReg); }
On 17 April 2018 at 08:03, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
When we first ported EDK2 to KVM/arm, we implemented a workaround for the quirky timer handling on the KVM side. This has been fixed in Linux commit f120cd6533d2 ("KVM: arm/arm64: timer: Allow the timer to control the active state") dated 23 June 2014, which was incorporated into Linux release 4.3.
So almost 4 years later, it should be safe to drop this workaround on the EDK2 side.
This reverts commit b1a633434ddc.
Cc: cross-distro@lists.linaro.org Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org Acked-by: Marc Zyngier marc.zyngier@arm.com Reviewed-by: Leif Lindholm leif.lindholm@linaro.org Acked-by: Laszlo Ersek lersek@redhat.com
Pushed as 411a373ed6426fb1bff253905b6a59ada44e18ad
Thanks all
v2: add acks
Note to cross-distro readers: this means guest firmware built with this patch will not work on KVM/ARM hosts using kernel v4.2 or earlier.
ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 - ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c | 10 ---------- 2 files changed, 11 deletions(-)
diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c index a3202fa056f3..bd616d2efc73 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c @@ -337,7 +337,6 @@ TimerInterruptHandler (
// Set next compare value ArmGenericTimerSetCompareVal (CompareValue);
- ArmGenericTimerEnableTimer (); ArmInstructionSynchronizationBarrier (); }
diff --git a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c index 69a4ceb62db6..c941895a3574 100644 --- a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c +++ b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c @@ -26,16 +26,6 @@ ArmGenericTimerEnableTimer (
TimerCtrlReg = ArmReadCntvCtl (); 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; ArmWriteCntvCtl (TimerCtrlReg);
}
-- 2.17.0
Hi Ard,
Sorry for the late reply.
On 19/04/18 09:16, Ard Biesheuvel wrote:
On 17 April 2018 at 08:03, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
When we first ported EDK2 to KVM/arm, we implemented a workaround for the quirky timer handling on the KVM side. This has been fixed in Linux commit f120cd6533d2 ("KVM: arm/arm64: timer: Allow the timer to control the active state") dated 23 June 2014, which was incorporated into Linux release 4.3.
So almost 4 years later, it should be safe to drop this workaround on the EDK2 side.
This reverts commit b1a633434ddc.
Cc: cross-distro@lists.linaro.org Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org Acked-by: Marc Zyngier marc.zyngier@arm.com Reviewed-by: Leif Lindholm leif.lindholm@linaro.org Acked-by: Laszlo Ersek lersek@redhat.com
Pushed as 411a373ed6426fb1bff253905b6a59ada44e18ad
While this was added for KVM, I believe that code is also needed by Xen. Indeed before injecting the interrupt the hypervisor will mask the interrupt.
So would it be possible to revert that patch?
Cheers,
Thanks all
v2: add acks
Note to cross-distro readers: this means guest firmware built with this patch will not work on KVM/ARM hosts using kernel v4.2 or earlier.
ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 - ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c | 10 ---------- 2 files changed, 11 deletions(-)
diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c index a3202fa056f3..bd616d2efc73 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c @@ -337,7 +337,6 @@ TimerInterruptHandler (
// Set next compare value ArmGenericTimerSetCompareVal (CompareValue);
- ArmGenericTimerEnableTimer (); ArmInstructionSynchronizationBarrier (); }
diff --git a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c index 69a4ceb62db6..c941895a3574 100644 --- a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c +++ b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c @@ -26,16 +26,6 @@ ArmGenericTimerEnableTimer (
TimerCtrlReg = ArmReadCntvCtl (); 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; ArmWriteCntvCtl (TimerCtrlReg); }
-- 2.17.0
cross-distro mailing list cross-distro@lists.linaro.org https://lists.linaro.org/mailman/listinfo/cross-distro
On 19 April 2018 at 16:23, Julien Grall julien.grall@arm.com wrote:
Hi Ard,
Sorry for the late reply.
On 19/04/18 09:16, Ard Biesheuvel wrote:
On 17 April 2018 at 08:03, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
When we first ported EDK2 to KVM/arm, we implemented a workaround for the quirky timer handling on the KVM side. This has been fixed in Linux commit f120cd6533d2 ("KVM: arm/arm64: timer: Allow the timer to control the active state") dated 23 June 2014, which was incorporated into Linux release 4.3.
So almost 4 years later, it should be safe to drop this workaround on the EDK2 side.
This reverts commit b1a633434ddc.
Cc: cross-distro@lists.linaro.org Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org Acked-by: Marc Zyngier marc.zyngier@arm.com Reviewed-by: Leif Lindholm leif.lindholm@linaro.org Acked-by: Laszlo Ersek lersek@redhat.com
Pushed as 411a373ed6426fb1bff253905b6a59ada44e18ad
While this was added for KVM, I believe that code is also needed by Xen. Indeed before injecting the interrupt the hypervisor will mask the interrupt.
So would it be possible to revert that patch?
Given that this is now a Xen-only quirk, I'd rather work around it by creating a separate ArmGenericTimerCounterLib implementation for Xen.
I will try to put something together beginning of next week.
On 19/04/18 15:34, Ard Biesheuvel wrote:
On 19 April 2018 at 16:23, Julien Grall julien.grall@arm.com wrote:
Hi Ard,
Sorry for the late reply.
On 19/04/18 09:16, Ard Biesheuvel wrote:
On 17 April 2018 at 08:03, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
When we first ported EDK2 to KVM/arm, we implemented a workaround for the quirky timer handling on the KVM side. This has been fixed in Linux commit f120cd6533d2 ("KVM: arm/arm64: timer: Allow the timer to control the active state") dated 23 June 2014, which was incorporated into Linux release 4.3.
So almost 4 years later, it should be safe to drop this workaround on the EDK2 side.
This reverts commit b1a633434ddc.
Cc: cross-distro@lists.linaro.org Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org Acked-by: Marc Zyngier marc.zyngier@arm.com Reviewed-by: Leif Lindholm leif.lindholm@linaro.org Acked-by: Laszlo Ersek lersek@redhat.com
Pushed as 411a373ed6426fb1bff253905b6a59ada44e18ad
While this was added for KVM, I believe that code is also needed by Xen. Indeed before injecting the interrupt the hypervisor will mask the interrupt.
So would it be possible to revert that patch?
Given that this is now a Xen-only quirk, I'd rather work around it by creating a separate ArmGenericTimerCounterLib implementation for Xen.
That would work for me.
I will try to put something together beginning of next week.
I am happy to test it.
Cheers,