[PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Ard Biesheuvel]

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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 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.15.1

Re: [PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Marc Zyngier]

On 15/03/18 10:28, Ard Biesheuvel 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.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  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);
>  }
>  
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Leif Lindholm]

On Thu, Mar 15, 2018 at 10:28:26AM +0000, Ard Biesheuvel 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.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

I'm happy with this, with Marc's Ack.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

However, if this can affect old kernels running in vms, could you ping
cross-distro@lists.linaro.org as well, so it doesn't catch anyone by
surprise?

/
    Leif

> ---
>  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.15.1
> 

Re: [PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Laszlo Ersek]

On 03/15/18 20:01, Leif Lindholm wrote:
> On Thu, Mar 15, 2018 at 10:28:26AM +0000, Ard Biesheuvel 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.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> I'm happy with this, with Marc's Ack.
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> However, if this can affect old kernels running in vms, could you ping
> cross-distro@lists.linaro.org as well, so it doesn't catch anyone by
> surprise?

If I understand correctly, the patch could negatively affect old kernels
that run on *hosts*. (Discounting nested virtualization, which I'm
perfectly willing to ignore for this discussion.)

Emailing the cross-distro list shouldn't hurt in any case.

>From my side (since I was CC'd):

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

> 
> /
>     Leif
> 
>> ---
>>  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.15.1
>>

Re: [PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Ard Biesheuvel]

On 15 March 2018 at 19:01, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 15, 2018 at 10:28:26AM +0000, Ard Biesheuvel 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.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> I'm happy with this, with Marc's Ack.
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> However, if this can affect old kernels running in vms, could you ping
> cross-distro@lists.linaro.org as well, so it doesn't catch anyone by
> surprise?
>

It will affects VMs running new firmware on ancient host kernels (and
v4.2 *is* ancient when it comes to KVM/arm64 and server stuff imo)

>> ---
>>  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.15.1
>>

Re: [PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Leif Lindholm]

On Fri, Mar 16, 2018 at 11:28:21AM +0000, Ard Biesheuvel wrote:
> On 15 March 2018 at 19:01, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Mar 15, 2018 at 10:28:26AM +0000, Ard Biesheuvel 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.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > I'm happy with this, with Marc's Ack.
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> > However, if this can affect old kernels running in vms, could you ping
> > cross-distro@lists.linaro.org as well, so it doesn't catch anyone by
> > surprise?
> 
> It will affects VMs running new firmware on ancient host kernels (and
> v4.2 *is* ancient when it comes to KVM/arm64 and server stuff imo)

Oh, it is, but if we have
https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1744754
(Xenial guest), imagining people running a Zesty host isn't that much
more of a stretch (it's only EOL 2 months ago).

/
    Leif

> >> ---
> >>  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.15.1
> >>

Re: [PATCH] ArmPkg/TimerDxe: remove workaround for KVM timer handling

[Ard Biesheuvel]

On 16 March 2018 at 11:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Mar 16, 2018 at 11:28:21AM +0000, Ard Biesheuvel wrote:
>> On 15 March 2018 at 19:01, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Thu, Mar 15, 2018 at 10:28:26AM +0000, Ard Biesheuvel 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.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > I'm happy with this, with Marc's Ack.
>> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>> >
>> > However, if this can affect old kernels running in vms, could you ping
>> > cross-distro@lists.linaro.org as well, so it doesn't catch anyone by
>> > surprise?
>>
>> It will affects VMs running new firmware on ancient host kernels (and
>> v4.2 *is* ancient when it comes to KVM/arm64 and server stuff imo)
>
> Oh, it is, but if we have
> https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1744754
> (Xenial guest), imagining people running a Zesty host isn't that much
> more of a stretch (it's only EOL 2 months ago).
>

Xenial has v4.4, Zesty has v4.10, if I am not mistaken?

Still doesn't hurt to cc cross-distro, obviously. Just sayin'