[PATCH 0/2] Reenable timer mask quirk for Xen

[PATCH 0/2] Reenable timer mask quirk for Xen

[Ard Biesheuvel]

Reinstate the timer mask quirk for Xen that we recently removed under the
assumption that only ancient KVM host implementations still needed it.

Ard Biesheuvel (2):
  ArmPkg: add reenable hook to ArmGenericTimerCounterLib
  ArmVirtPkg: reinstate timer unmask quirk for Xen

 ArmPkg/Drivers/TimerDxe/TimerDxe.c            |   1 +
 .../Library/ArmGenericTimerCounterLib.h       |   6 +
 .../ArmGenericTimerPhyCounterLib.c            |   8 +
 .../ArmGenericTimerVirtCounterLib.c           |   8 +
 ArmVirtPkg/ArmVirtXen.dsc                     |   1 +
 .../XenArmGenericTimerVirtCounterLib.c        | 146 ++++++++++++++++++
 .../XenArmGenericTimerVirtCounterLib.inf      |  33 ++++
 7 files changed, 203 insertions(+)
 create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
 create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf

-- 
2.17.0

[PATCH 1/2] ArmPkg: add reenable hook to ArmGenericTimerCounterLib

[Ard Biesheuvel]

In preparation of selectively reinstating the timer enable quirk for Xen
that we removed in commit 411a373ed642 ("ArmPkg/TimerDxe: remove workaround
for KVM timer handling"), add a ArmGenericTimerReenableTimer() library
function to ArmGenericTimerCounterLib that we will populate for Xen only.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/TimerDxe/TimerDxe.c                                           | 1 +
 ArmPkg/Include/Library/ArmGenericTimerCounterLib.h                           | 6 ++++++
 ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c   | 8 ++++++++
 ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c | 8 ++++++++
 4 files changed, 23 insertions(+)

diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
index bd616d2efc73..478f1bece565 100644
--- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
+++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
@@ -337,6 +337,7 @@ TimerInterruptHandler (
 
     // Set next compare value
     ArmGenericTimerSetCompareVal (CompareValue);
+    ArmGenericTimerReenableTimer ();
     ArmInstructionSynchronizationBarrier ();
   }
 
diff --git a/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h b/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h
index 805025baa14b..47e28b79c033 100644
--- a/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h
+++ b/ArmPkg/Include/Library/ArmGenericTimerCounterLib.h
@@ -22,6 +22,12 @@ ArmGenericTimerEnableTimer (
   VOID
   );
 
+VOID
+EFIAPI
+ArmGenericTimerReenableTimer (
+  VOID
+  );
+
 VOID
 EFIAPI
 ArmGenericTimerDisableTimer (
diff --git a/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c b/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c
index d04e04fb334d..03519f50e79a 100644
--- a/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c
+++ b/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c
@@ -29,6 +29,14 @@ ArmGenericTimerEnableTimer (
   ArmWriteCntpCtl (TimerCtrlReg);
 }
 
+VOID
+EFIAPI
+ArmGenericTimerReenableTimer (
+  VOID
+  )
+{
+}
+
 VOID
 EFIAPI
 ArmGenericTimerDisableTimer (
diff --git a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
index c941895a3574..4bb1e1cde538 100644
--- a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
+++ b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
@@ -29,6 +29,14 @@ ArmGenericTimerEnableTimer (
   ArmWriteCntvCtl (TimerCtrlReg);
 }
 
+VOID
+EFIAPI
+ArmGenericTimerReenableTimer (
+  VOID
+  )
+{
+}
+
 VOID
 EFIAPI
 ArmGenericTimerDisableTimer (
-- 
2.17.0

[PATCH 2/2] ArmVirtPkg: reinstate timer unmask quirk for Xen

[Ard Biesheuvel]

Commit 411a373ed642 ("ArmPkg/TimerDxe: remove workaround for KVM timer
handling") removed the virtual timer handling quirk that cleared the
mask bit in the control register when enabling the timer, under the
assumption that only ancient KVM host implementations required it.
However, Julien reports that Xen also masks the timer interrupt in the
guest view of the timer control register, and therefore needs the same
quirk.

So let's reinstate it, but using a Xen specific implementation of the
timer support library, so that other virt platforms remain unchanged.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtXen.dsc                                                                |   1 +
 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c   | 146 ++++++++++++++++++++
 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf |  33 +++++
 3 files changed, 180 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index 175b56d10c8f..8878912e6bef 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -36,6 +36,7 @@ [LibraryClasses]
   RealTimeClockLib|ArmVirtPkg/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf
   XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
 
+  ArmGenericTimerCounterLib|ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
   ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
   ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
 
diff --git a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c b/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
new file mode 100644
index 000000000000..75c28168453e
--- /dev/null
+++ b/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
@@ -0,0 +1,146 @@
+/** @file
+
+  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2014 - 2018, Linaro Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Library/ArmGenericTimerCounterLib.h>
+#include <Library/ArmLib.h>
+
+VOID
+EFIAPI
+ArmGenericTimerEnableTimer (
+  VOID
+  )
+{
+  UINTN TimerCtrlReg;
+
+  TimerCtrlReg = ArmReadCntvCtl ();
+  TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE;
+  ArmWriteCntvCtl (TimerCtrlReg);
+}
+
+VOID
+EFIAPI
+ArmGenericTimerReenableTimer (
+  VOID
+  )
+{
+  UINTN TimerCtrlReg;
+
+  TimerCtrlReg = ArmReadCntvCtl ();
+  TimerCtrlReg |= ARM_ARCH_TIMER_ENABLE;
+
+  //
+  // When running under Xen, we need to unmask the interrupt on the timer side
+  // as Xen 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.
+  //
+  TimerCtrlReg &= ~ARM_ARCH_TIMER_IMASK;
+  ArmWriteCntvCtl (TimerCtrlReg);
+}
+
+VOID
+EFIAPI
+ArmGenericTimerDisableTimer (
+  VOID
+  )
+{
+  UINTN TimerCtrlReg;
+
+  TimerCtrlReg = ArmReadCntvCtl ();
+  TimerCtrlReg &= ~ARM_ARCH_TIMER_ENABLE;
+  ArmWriteCntvCtl (TimerCtrlReg);
+}
+
+VOID
+EFIAPI
+ArmGenericTimerSetTimerFreq (
+  IN   UINTN  FreqInHz
+  )
+{
+  ArmWriteCntFrq (FreqInHz);
+}
+
+UINTN
+EFIAPI
+ArmGenericTimerGetTimerFreq (
+  VOID
+  )
+{
+  return ArmReadCntFrq ();
+}
+
+UINTN
+EFIAPI
+ArmGenericTimerGetTimerVal (
+  VOID
+  )
+{
+  return ArmReadCntvTval ();
+}
+
+
+VOID
+EFIAPI
+ArmGenericTimerSetTimerVal (
+  IN   UINTN   Value
+  )
+{
+  ArmWriteCntvTval (Value);
+}
+
+UINT64
+EFIAPI
+ArmGenericTimerGetSystemCount (
+  VOID
+  )
+{
+  return ArmReadCntvCt ();
+}
+
+UINTN
+EFIAPI
+ArmGenericTimerGetTimerCtrlReg (
+  VOID
+  )
+{
+  return ArmReadCntvCtl ();
+}
+
+VOID
+EFIAPI
+ArmGenericTimerSetTimerCtrlReg (
+  UINTN Value
+  )
+{
+  ArmWriteCntvCtl (Value);
+}
+
+UINT64
+EFIAPI
+ArmGenericTimerGetCompareVal (
+  VOID
+  )
+{
+  return ArmReadCntvCval ();
+}
+
+VOID
+EFIAPI
+ArmGenericTimerSetCompareVal (
+  IN   UINT64   Value
+  )
+{
+  ArmWriteCntvCval (Value);
+}
diff --git a/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf b/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
new file mode 100644
index 000000000000..bd6ac8039844
--- /dev/null
+++ b/ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
@@ -0,0 +1,33 @@
+#/** @file
+#  Implement ArmGenericTimerCounterLib for Xen using the virtual timer
+#
+#  Copyright (c) 2014 - 2018, Linaro Ltd. All rights reserved.<BR>
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD License
+#  which accompanies this distribution. The full text of the license may be found at
+#  http://opensource.org/licenses/bsd-license.php
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = XenArmGenericTimerVirtCounterLib
+  FILE_GUID                      = e3913319-96ac-4ac0-808b-8edb8776a51c
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmGenericTimerCounterLib
+
+[Sources]
+  XenArmGenericTimerVirtCounterLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  ArmPkg/ArmPkg.dec
+
+[LibraryClasses]
+  ArmLib
+  BaseLib
-- 
2.17.0

Re: [PATCH 0/2] Reenable timer mask quirk for Xen

[Leif Lindholm]

On Mon, Apr 23, 2018 at 05:00:55PM +0200, Ard Biesheuvel wrote:
> Reinstate the timer mask quirk for Xen that we recently removed under the
> assumption that only ancient KVM host implementations still needed it.

>From my point of view, for the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

But I'd like Julien's R-b as well.

/
    Leif

> Ard Biesheuvel (2):
>   ArmPkg: add reenable hook to ArmGenericTimerCounterLib
>   ArmVirtPkg: reinstate timer unmask quirk for Xen
> 
>  ArmPkg/Drivers/TimerDxe/TimerDxe.c            |   1 +
>  .../Library/ArmGenericTimerCounterLib.h       |   6 +
>  .../ArmGenericTimerPhyCounterLib.c            |   8 +
>  .../ArmGenericTimerVirtCounterLib.c           |   8 +
>  ArmVirtPkg/ArmVirtXen.dsc                     |   1 +
>  .../XenArmGenericTimerVirtCounterLib.c        | 146 ++++++++++++++++++
>  .../XenArmGenericTimerVirtCounterLib.inf      |  33 ++++
>  7 files changed, 203 insertions(+)
>  create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
>  create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> 
> -- 
> 2.17.0
> 

Re: [PATCH 0/2] Reenable timer mask quirk for Xen

[Laszlo Ersek]

On 04/23/18 17:00, Ard Biesheuvel wrote:
> Reinstate the timer mask quirk for Xen that we recently removed under the
> assumption that only ancient KVM host implementations still needed it.
> 
> Ard Biesheuvel (2):
>   ArmPkg: add reenable hook to ArmGenericTimerCounterLib
>   ArmVirtPkg: reinstate timer unmask quirk for Xen
> 
>  ArmPkg/Drivers/TimerDxe/TimerDxe.c            |   1 +
>  .../Library/ArmGenericTimerCounterLib.h       |   6 +
>  .../ArmGenericTimerPhyCounterLib.c            |   8 +
>  .../ArmGenericTimerVirtCounterLib.c           |   8 +
>  ArmVirtPkg/ArmVirtXen.dsc                     |   1 +
>  .../XenArmGenericTimerVirtCounterLib.c        | 146 ++++++++++++++++++
>  .../XenArmGenericTimerVirtCounterLib.inf      |  33 ++++
>  7 files changed, 203 insertions(+)
>  create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
>  create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> 

For the series:

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

Thanks
Laszlo

Re: [PATCH 0/2] Reenable timer mask quirk for Xen

[Julien Grall]

Hi Ard,

Thank you for the series, I confirm it fixes EDK2 boot in Xen guest.

Tested-by: Julien Grall <julien.grall@arm.com>
Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

On 23/04/18 16:00, Ard Biesheuvel wrote:
> Reinstate the timer mask quirk for Xen that we recently removed under the
> assumption that only ancient KVM host implementations still needed it.
> 
> Ard Biesheuvel (2):
>    ArmPkg: add reenable hook to ArmGenericTimerCounterLib
>    ArmVirtPkg: reinstate timer unmask quirk for Xen
> 
>   ArmPkg/Drivers/TimerDxe/TimerDxe.c            |   1 +
>   .../Library/ArmGenericTimerCounterLib.h       |   6 +
>   .../ArmGenericTimerPhyCounterLib.c            |   8 +
>   .../ArmGenericTimerVirtCounterLib.c           |   8 +
>   ArmVirtPkg/ArmVirtXen.dsc                     |   1 +
>   .../XenArmGenericTimerVirtCounterLib.c        | 146 ++++++++++++++++++
>   .../XenArmGenericTimerVirtCounterLib.inf      |  33 ++++
>   7 files changed, 203 insertions(+)
>   create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
>   create mode 100644 ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
> 

-- 
Julien Grall

Re: [PATCH 0/2] Reenable timer mask quirk for Xen

[Ard Biesheuvel]

On 25 April 2018 at 19:26, Julien Grall <julien.grall@arm.com> wrote:
> Hi Ard,
>
> Thank you for the series, I confirm it fixes EDK2 boot in Xen guest.
>
> Tested-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Julien Grall <julien.grall@arm.com>
>

Pushed as 1eb72acddd61..ef42ef7e6dd7

Thanks all.


> On 23/04/18 16:00, Ard Biesheuvel wrote:
>>
>> Reinstate the timer mask quirk for Xen that we recently removed under the
>> assumption that only ancient KVM host implementations still needed it.
>>
>> Ard Biesheuvel (2):
>>    ArmPkg: add reenable hook to ArmGenericTimerCounterLib
>>    ArmVirtPkg: reinstate timer unmask quirk for Xen
>>
>>   ArmPkg/Drivers/TimerDxe/TimerDxe.c            |   1 +
>>   .../Library/ArmGenericTimerCounterLib.h       |   6 +
>>   .../ArmGenericTimerPhyCounterLib.c            |   8 +
>>   .../ArmGenericTimerVirtCounterLib.c           |   8 +
>>   ArmVirtPkg/ArmVirtXen.dsc                     |   1 +
>>   .../XenArmGenericTimerVirtCounterLib.c        | 146 ++++++++++++++++++
>>   .../XenArmGenericTimerVirtCounterLib.inf      |  33 ++++
>>   7 files changed, 203 insertions(+)
>>   create mode 100644
>> ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.c
>>   create mode 100644
>> ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/XenArmGenericTimerVirtCounterLib.inf
>>
>
> --
> Julien Grall