[PATCH v3 0/5] Support userspace irqchip with arch timers

[PATCH v3 0/5] Support userspace irqchip with arch timers

[Christoffer Dall]

This series is the second version of the rework of the patches to support
architected timers with a userspace irqchip sent by Alexander Graf [1].

We first cleanup some of the timer code to make it easier to understand
what is being done in the later patches, and then define the ABI,
implement timers support, implement PMU support, and finally advertise
the features.

These patches are based on the recent work from Jintack to support the
physical timer in addition to the virtual timer.  This series including
its dependencies can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git irqs-to-user-v3

I tested this using Alex's QEMU patch with his fixes for SMP applied.  This
seems to be rock-solid.  The temporary-not-for-upstream-but-for-testing patch
can be found here (force-pushed and rebased since v2):

https://git.linaro.org/people/christoffer.dall/qemu-arm.git no-kvm-irqchip

I also tested it on 32-bit and it looks good there as well.

Changes since v2:
 - Actually push the right content to the kernel branch, sorry.
 - Rebased on kvmarm/queue as of this morning (v4.11-rc1+ stuff)
 - Changed IOCTL numbers as needed

Changes since v1:
 - Rework the ABI to support devices in general as opposed to just
   timers
 - Support the PMU in addition to timers
 - Also support the physical timer (rebased on Jintack's work)
 - Updated some comments where I noticed things were out of date.

Several changes have been made compared to v7 of the original single
patch, including:
 - Rewording ABI documentation to be more in line with the ARM
   architecture
 - Add an explicit check for needing to notify userspace of a level
   change instead of propagating the value
 - Changes to commenting throughout to more accurately describe the
   architecture concepts we try to maintain
 - Reword of functions, for example from sync to update when the date
   only flows one direction

[1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2016-September/021867.html
[2]: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next

Alexander Graf (2):
  KVM: arm/arm64: Add ARM user space interrupt signaling ABI
  KVM: arm/arm64: Support arch timers with a userspace gic

Christoffer Dall (3):
  KVM: arm/arm64: Cleanup the arch timer code's irqchip checking
  KVM: arm/arm64: Report PMU overflow interrupts to userspace irqchip
  KVM: arm/arm64: Advertise support for KVM_CAP_ARM_USER_IRQ

 Documentation/virtual/kvm/api.txt |  42 +++++++++++++
 arch/arm/include/uapi/asm/kvm.h   |   2 +
 arch/arm/kvm/arm.c                |  28 ++++++---
 arch/arm64/include/uapi/asm/kvm.h |   2 +
 include/kvm/arm_arch_timer.h      |   2 +
 include/kvm/arm_pmu.h             |   7 +++
 include/uapi/linux/kvm.h          |   8 +++
 virt/kvm/arm/arch_timer.c         | 127 ++++++++++++++++++++++++++++++--------
 virt/kvm/arm/pmu.c                |  42 +++++++++++--
 9 files changed, 221 insertions(+), 39 deletions(-)

-- 
2.9.0

[PATCH v3 1/5] KVM: arm/arm64: Cleanup the arch timer code's irqchip checking

[Christoffer Dall]

From: Christoffer Dall <christoffer.dall@linaro.org>

Currently we check if we have an in-kernel irqchip and if the vgic was
properly implemented several places in the arch timer code.  But, we
already predicate our enablement of the arm timers on having a valid
and initialized gic, so we can simply check if the timers are enabled or
not.

This also gets rid of the ugly "error that's not an error but used to
signal that the timer shouldn't poke the gic" construct we have.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 35d7100..363f0d2 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -189,8 +189,6 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 {
 	int ret;
 
-	BUG_ON(!vgic_initialized(vcpu->kvm));
-
 	timer_ctx->active_cleared_last = false;
 	timer_ctx->irq.level = new_level;
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
@@ -205,7 +203,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
  * Check if there was a change in the timer state (should we raise or lower
  * the line level to the GIC).
  */
-static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
+static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
@@ -217,16 +215,14 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	 * because the guest would never see the interrupt.  Instead wait
 	 * until we call this function from kvm_timer_flush_hwstate.
 	 */
-	if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
-		return -ENODEV;
+	if (!timer->enabled)
+		return;
 
 	if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
 		kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
 
 	if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
 		kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
-
-	return 0;
 }
 
 /* Schedule the background timer for the emulated timer. */
@@ -295,13 +291,16 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
  */
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 {
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	bool phys_active;
 	int ret;
 
-	if (kvm_timer_update_state(vcpu))
+	if (unlikely(!timer->enabled))
 		return;
 
+	kvm_timer_update_state(vcpu);
+
 	/* Set the background timer for the physical timer emulation. */
 	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
 
-- 
2.9.0

[PATCH v3 2/5] KVM: arm/arm64: Add ARM user space interrupt signaling ABI

[Christoffer Dall]

From: Alexander Graf <agraf@suse.de>

We have 2 modes for dealing with interrupts in the ARM world. We can
either handle them all using hardware acceleration through the vgic or
we can emulate a gic in user space and only drive CPU IRQ pins from
there.

Unfortunately, when driving IRQs from user space, we never tell user
space about events from devices emulated inside the kernel, which may
result in interrupt line state changes, so we lose out on for example
timer and PMU events if we run with user space gic emulation.

Define an ABI to publish such device output levels to userspace.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/virtual/kvm/api.txt | 42 +++++++++++++++++++++++++++++++++++++++
 arch/arm/include/uapi/asm/kvm.h   |  2 ++
 arch/arm64/include/uapi/asm/kvm.h |  2 ++
 include/uapi/linux/kvm.h          |  8 ++++++++
 4 files changed, 54 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 3c248f7..3b4e76e 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4147,3 +4147,45 @@ This capability, if KVM_CHECK_EXTENSION indicates that it is
 available, means that that the kernel can support guests using the
 hashed page table MMU defined in Power ISA V3.00 (as implemented in
 the POWER9 processor), including in-memory segment tables.
+
+
+8.5 KVM_CAP_ARM_USER_IRQ
+
+Architectures: arm, arm64
+This capability, if KVM_CHECK_EXTENSION indicates that it is available, means
+that if userspace creates a VM without an in-kernel interrupt controller, it
+will be notified of changes to the output level of in-kernel emulated devices,
+which can generate virtual interrupts, presented to the VM.
+For such VMs, on every return to userspace, the kernel
+updates the vcpu's run->s.regs.device_irq_level field to represent the actual
+output level of the device.
+
+Whenever kvm detects a change in the device output level, kvm guarantees at
+least one return to userspace before running the VM.  This exit could either
+be a KVM_EXIT_INTR or any other exit event, like KVM_EXIT_MMIO. This way,
+userspace can always sample the device output level and re-compute the state of
+the userspace interrupt controller.  Userspace should always check the state
+of run->s.regs.device_irq_level on every kvm exit.
+The value in run->s.regs.device_irq_level can represent both level and edge
+triggered interrupt signals, depending on the device.  Edge triggered interrupt
+signals will exit to userspace with the bit in run->s.regs.device_irq_level
+set exactly once per edge signal.
+
+The field run->s.regs.device_irq_level is available independent of
+run->kvm_valid_regs or run->kvm_dirty_regs bits.
+
+If KVM_CAP_ARM_USER_IRQ is supported, the KVM_CHECK_EXTENSION ioctl returns a
+number larger than 0 indicating the version of this capability is implemented
+and thereby which bits in in run->s.regs.device_irq_level can signal values.
+
+Currently the following bits are defined for the device_irq_level bitmap:
+
+  KVM_CAP_ARM_USER_IRQ >= 1:
+
+    KVM_ARM_DEV_EL1_VTIMER -  EL1 virtual timer
+    KVM_ARM_DEV_EL1_PTIMER -  EL1 physical timer
+    KVM_ARM_DEV_PMU        -  ARM PMU overflow interrupt signal
+
+Future versions of kvm may implement additional events. These will get
+indicated by returning a higher number from KVM_CHECK_EXTENSION and will be
+listed above.
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 6ebd3e6..a5838d6 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -114,6 +114,8 @@ struct kvm_debug_exit_arch {
 };
 
 struct kvm_sync_regs {
+	/* Used with KVM_CAP_ARM_USER_IRQ */
+	__u64 device_irq_level;
 };
 
 struct kvm_arch_memory_slot {
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index c286035..cd6bea4 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -143,6 +143,8 @@ struct kvm_debug_exit_arch {
 #define KVM_GUESTDBG_USE_HW		(1 << 17)
 
 struct kvm_sync_regs {
+	/* Used with KVM_CAP_ARM_USER_IRQ */
+	__u64 device_irq_level;
 };
 
 struct kvm_arch_memory_slot {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f51d508..6d6b9b2 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_MMU_RADIX 134
 #define KVM_CAP_PPC_MMU_HASH_V3 135
 #define KVM_CAP_IMMEDIATE_EXIT 136
+#define KVM_CAP_ARM_USER_IRQ 137
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1354,4 +1355,11 @@ struct kvm_assigned_msix_entry {
 #define KVM_X2APIC_API_USE_32BIT_IDS            (1ULL << 0)
 #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK  (1ULL << 1)
 
+/* Available with KVM_CAP_ARM_USER_IRQ */
+
+/* Bits for run->s.regs.device_irq_level */
+#define KVM_ARM_DEV_EL1_VTIMER		(1 << 0)
+#define KVM_ARM_DEV_EL1_PTIMER		(1 << 1)
+#define KVM_ARM_DEV_PMU			(1 << 2)
+
 #endif /* __LINUX_KVM_H */
-- 
2.9.0

[PATCH v3 3/5] KVM: arm/arm64: Support arch timers with a userspace gic

[Christoffer Dall]

From: Alexander Graf <agraf@suse.de>

If you're running with a userspace gic or other interrupt constroller
(that is no vgic in the kernel), then you have so far not been able to
use the architected timers, because the output of the architected
timers, which are driven inside the kernel, was a kernel-only construct
between the arch timer code and the vgic.

This patch implements the new KVM_CAP_ARM_USER_IRQ feature, where we use a
side channel on the kvm_run structure, run->s.regs.device_irq_level, to
always notify userspace of the timer output levels when using a userspace
irqchip.

This works by ensureing that before we enter the guest, if the timer
output level has changed compared to what we last told userspace, we
don't enter the guest, but instead return to userspace to notify it of
the new level.  If we are exiting, because of an MMIO for example, and
the level changed at the same time, the value is also updated and
userspace can sample the line as it needs.  This is nicely achieved
simply always updating the timer_irq_level field after the main run
loop.

Note that the kvm_timer_update_irq trace event is changed to show the
host IRQ number for the timer instead of the guest IRQ number, because
the kernel no longer know which IRQ userspace wires up the timer signal
to.

Also note that this patch implements all required functionality but does
not yet advertise the capability.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/arm.c           |  18 +++----
 include/kvm/arm_arch_timer.h |   2 +
 virt/kvm/arm/arch_timer.c    | 122 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 110 insertions(+), 32 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 7fa4898..efb16e5 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -515,13 +515,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 			return ret;
 	}
 
-	/*
-	 * Enable the arch timers only if we have an in-kernel VGIC
-	 * and it has been properly initialized, since we cannot handle
-	 * interrupts from the virtual timer with a userspace gic.
-	 */
-	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
-		ret = kvm_timer_enable(vcpu);
+	ret = kvm_timer_enable(vcpu);
 
 	return ret;
 }
@@ -640,9 +634,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		local_irq_disable();
 
 		/*
-		 * Re-check atomic conditions
+		 * If we have a singal pending, or need to notify a userspace
+		 * irqchip about timer level changes, then we exit (and update
+		 * the timer level state in kvm_timer_update_run below).
 		 */
-		if (signal_pending(current)) {
+		if (signal_pending(current) ||
+		    kvm_timer_should_notify_user(vcpu)) {
 			ret = -EINTR;
 			run->exit_reason = KVM_EXIT_INTR;
 		}
@@ -714,6 +711,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		ret = handle_exit(vcpu, run, ret);
 	}
 
+	/* Tell userspace about in-kernel device output levels */
+	kvm_timer_update_run(vcpu);
+
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 	return ret;
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index fe797d6..295584f 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -63,6 +63,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
+bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu);
+void kvm_timer_update_run(struct kvm_vcpu *vcpu);
 void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
 
 u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 363f0d2..5dc2167 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -184,6 +184,27 @@ bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
 	return cval <= now;
 }
 
+/*
+ * Reflect the timer output level into the kvm_run structure
+ */
+void kvm_timer_update_run(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+	struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm)))
+		return;
+
+	/* Populate the device bitmap with the timer states */
+	regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
+				    KVM_ARM_DEV_EL1_PTIMER);
+	if (vtimer->irq.level)
+		regs->device_irq_level |= KVM_ARM_DEV_EL1_VTIMER;
+	if (ptimer->irq.level)
+		regs->device_irq_level |= KVM_ARM_DEV_EL1_PTIMER;
+}
+
 static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 				 struct arch_timer_context *timer_ctx)
 {
@@ -194,9 +215,12 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
 				   timer_ctx->irq.level);
 
-	ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer_ctx->irq.irq,
-				  timer_ctx->irq.level);
-	WARN_ON(ret);
+	if (likely(irqchip_in_kernel(vcpu->kvm))) {
+		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
+					  timer_ctx->irq.irq,
+					  timer_ctx->irq.level);
+		WARN_ON(ret);
+	}
 }
 
 /*
@@ -215,7 +239,7 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	 * because the guest would never see the interrupt.  Instead wait
 	 * until we call this function from kvm_timer_flush_hwstate.
 	 */
-	if (!timer->enabled)
+	if (unlikely(!timer->enabled))
 		return;
 
 	if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
@@ -282,28 +306,12 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
 	timer_disarm(timer);
 }
 
-/**
- * kvm_timer_flush_hwstate - prepare to move the virt timer to the cpu
- * @vcpu: The vcpu pointer
- *
- * Check if the virtual timer has expired while we were running in the host,
- * and inject an interrupt if that was the case.
- */
-void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
+static void kvm_timer_flush_hwstate_vgic(struct kvm_vcpu *vcpu)
 {
-	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	bool phys_active;
 	int ret;
 
-	if (unlikely(!timer->enabled))
-		return;
-
-	kvm_timer_update_state(vcpu);
-
-	/* Set the background timer for the physical timer emulation. */
-	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
-
 	/*
 	* If we enter the guest with the virtual input level to the VGIC
 	* asserted, then we have already told the VGIC what we need to, and
@@ -355,11 +363,72 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	vtimer->active_cleared_last = !phys_active;
 }
 
+bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+	struct kvm_sync_regs *sregs = &vcpu->run->s.regs;
+	bool vlevel, plevel;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm)))
+		return false;
+
+	vlevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_VTIMER;
+	plevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_PTIMER;
+
+	return vtimer->irq.level != vlevel ||
+	       ptimer->irq.level != plevel;
+}
+
+static void kvm_timer_flush_hwstate_user(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+
+	/*
+	 * To prevent continuously exiting from the guest, we mask the
+	 * physical interrupt such that the guest can make forward progress.
+	 * Once we detect the output level being deasserted, we unmask the
+	 * interrupt again so that we exit from the guest when the timer
+	 * fires.
+	*/
+	if (vtimer->irq.level)
+		disable_percpu_irq(host_vtimer_irq);
+	else
+		enable_percpu_irq(host_vtimer_irq, 0);
+}
+
+/**
+ * kvm_timer_flush_hwstate - prepare timers before running the vcpu
+ * @vcpu: The vcpu pointer
+ *
+ * Check if the virtual timer has expired while we were running in the host,
+ * and inject an interrupt if that was the case, making sure the timer is
+ * masked or disabled on the host so that we keep executing.  Also schedule a
+ * software timer for the physical timer if it is enabled.
+ */
+void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	if (unlikely(!timer->enabled))
+		return;
+
+	kvm_timer_update_state(vcpu);
+
+	/* Set the background timer for the physical timer emulation. */
+	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
+
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
+		kvm_timer_flush_hwstate_user(vcpu);
+	else
+		kvm_timer_flush_hwstate_vgic(vcpu);
+}
+
 /**
  * kvm_timer_sync_hwstate - sync timer state from cpu
  * @vcpu: The vcpu pointer
  *
- * Check if the virtual timer has expired while we were running in the guest,
+ * Check if any of the timers have expired while we were running in the guest,
  * and inject an interrupt if that was the case.
  */
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
@@ -559,6 +628,13 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (timer->enabled)
 		return 0;
 
+	/* Without a VGIC we do not map virtual IRQs to physical IRQs */
+	if (!irqchip_in_kernel(vcpu->kvm))
+		goto no_vgic;
+
+	if (!vgic_initialized(vcpu->kvm))
+		return -ENODEV;
+
 	/*
 	 * Find the physical IRQ number corresponding to the host_vtimer_irq
 	 */
@@ -582,8 +658,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
+no_vgic:
 	timer->enabled = 1;
-
 	return 0;
 }
 
-- 
2.9.0

[PATCH v3 3/5] KVM: arm/arm64: Support arch timers with a userspace gic

[Alexander Graf]

On 05.04.17 11:28, Christoffer Dall wrote:
> From: Alexander Graf <agraf@suse.de>
>
> If you're running with a userspace gic or other interrupt constroller
> (that is no vgic in the kernel), then you have so far not been able to
> use the architected timers, because the output of the architected
> timers, which are driven inside the kernel, was a kernel-only construct
> between the arch timer code and the vgic.
>
> This patch implements the new KVM_CAP_ARM_USER_IRQ feature, where we use a
> side channel on the kvm_run structure, run->s.regs.device_irq_level, to
> always notify userspace of the timer output levels when using a userspace
> irqchip.
>
> This works by ensureing that before we enter the guest, if the timer
> output level has changed compared to what we last told userspace, we
> don't enter the guest, but instead return to userspace to notify it of
> the new level.  If we are exiting, because of an MMIO for example, and
> the level changed at the same time, the value is also updated and
> userspace can sample the line as it needs.  This is nicely achieved
> simply always updating the timer_irq_level field after the main run
> loop.
>
> Note that the kvm_timer_update_irq trace event is changed to show the
> host IRQ number for the timer instead of the guest IRQ number, because
> the kernel no longer know which IRQ userspace wires up the timer signal
> to.
>
> Also note that this patch implements all required functionality but does
> not yet advertise the capability.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/arm.c           |  18 +++----
>  include/kvm/arm_arch_timer.h |   2 +
>  virt/kvm/arm/arch_timer.c    | 122 +++++++++++++++++++++++++++++++++++--------
>  3 files changed, 110 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7fa4898..efb16e5 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -515,13 +515,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  			return ret;
>  	}
>
> -	/*
> -	 * Enable the arch timers only if we have an in-kernel VGIC
> -	 * and it has been properly initialized, since we cannot handle
> -	 * interrupts from the virtual timer with a userspace gic.
> -	 */
> -	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
> -		ret = kvm_timer_enable(vcpu);
> +	ret = kvm_timer_enable(vcpu);
>
>  	return ret;
>  }
> @@ -640,9 +634,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		local_irq_disable();
>
>  		/*
> -		 * Re-check atomic conditions
> +		 * If we have a singal pending, or need to notify a userspace
> +		 * irqchip about timer level changes, then we exit (and update
> +		 * the timer level state in kvm_timer_update_run below).
>  		 */
> -		if (signal_pending(current)) {
> +		if (signal_pending(current) ||
> +		    kvm_timer_should_notify_user(vcpu)) {
>  			ret = -EINTR;
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
> @@ -714,6 +711,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		ret = handle_exit(vcpu, run, ret);
>  	}
>
> +	/* Tell userspace about in-kernel device output levels */
> +	kvm_timer_update_run(vcpu);
> +
>  	if (vcpu->sigset_active)
>  		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
>  	return ret;
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index fe797d6..295584f 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -63,6 +63,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
> +bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu);
> +void kvm_timer_update_run(struct kvm_vcpu *vcpu);
>  void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
>
>  u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 363f0d2..5dc2167 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -184,6 +184,27 @@ bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
>  	return cval <= now;
>  }
>
> +/*
> + * Reflect the timer output level into the kvm_run structure
> + */
> +void kvm_timer_update_run(struct kvm_vcpu *vcpu)
> +{
> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +	struct kvm_sync_regs *regs = &vcpu->run->s.regs;
> +
> +	if (likely(irqchip_in_kernel(vcpu->kvm)))
> +		return;
> +
> +	/* Populate the device bitmap with the timer states */
> +	regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
> +				    KVM_ARM_DEV_EL1_PTIMER);
> +	if (vtimer->irq.level)
> +		regs->device_irq_level |= KVM_ARM_DEV_EL1_VTIMER;
> +	if (ptimer->irq.level)
> +		regs->device_irq_level |= KVM_ARM_DEV_EL1_PTIMER;
> +}
> +
>  static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>  				 struct arch_timer_context *timer_ctx)
>  {
> @@ -194,9 +215,12 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>  	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>  				   timer_ctx->irq.level);
>
> -	ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer_ctx->irq.irq,
> -				  timer_ctx->irq.level);
> -	WARN_ON(ret);
> +	if (likely(irqchip_in_kernel(vcpu->kvm))) {
> +		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
> +					  timer_ctx->irq.irq,
> +					  timer_ctx->irq.level);
> +		WARN_ON(ret);
> +	}
>  }
>
>  /*
> @@ -215,7 +239,7 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
>  	 * because the guest would never see the interrupt.  Instead wait
>  	 * until we call this function from kvm_timer_flush_hwstate.
>  	 */
> -	if (!timer->enabled)
> +	if (unlikely(!timer->enabled))
>  		return;
>
>  	if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
> @@ -282,28 +306,12 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
>  	timer_disarm(timer);
>  }
>
> -/**
> - * kvm_timer_flush_hwstate - prepare to move the virt timer to the cpu
> - * @vcpu: The vcpu pointer
> - *
> - * Check if the virtual timer has expired while we were running in the host,
> - * and inject an interrupt if that was the case.
> - */
> -void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> +static void kvm_timer_flush_hwstate_vgic(struct kvm_vcpu *vcpu)
>  {
> -	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>  	bool phys_active;
>  	int ret;
>
> -	if (unlikely(!timer->enabled))
> -		return;
> -
> -	kvm_timer_update_state(vcpu);
> -
> -	/* Set the background timer for the physical timer emulation. */
> -	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
> -
>  	/*
>  	* If we enter the guest with the virtual input level to the VGIC
>  	* asserted, then we have already told the VGIC what we need to, and
> @@ -355,11 +363,72 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  	vtimer->active_cleared_last = !phys_active;
>  }
>
> +bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
> +{
> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +	struct kvm_sync_regs *sregs = &vcpu->run->s.regs;
> +	bool vlevel, plevel;
> +
> +	if (likely(irqchip_in_kernel(vcpu->kvm)))
> +		return false;
> +
> +	vlevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_VTIMER;
> +	plevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_PTIMER;
> +
> +	return vtimer->irq.level != vlevel ||
> +	       ptimer->irq.level != plevel;
> +}
> +
> +static void kvm_timer_flush_hwstate_user(struct kvm_vcpu *vcpu)
> +{
> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> +
> +	/*
> +	 * To prevent continuously exiting from the guest, we mask the
> +	 * physical interrupt such that the guest can make forward progress.
> +	 * Once we detect the output level being deasserted, we unmask the
> +	 * interrupt again so that we exit from the guest when the timer
> +	 * fires.
> +	*/
> +	if (vtimer->irq.level)
> +		disable_percpu_irq(host_vtimer_irq);
> +	else
> +		enable_percpu_irq(host_vtimer_irq, 0);
> +}
> +
> +/**
> + * kvm_timer_flush_hwstate - prepare timers before running the vcpu
> + * @vcpu: The vcpu pointer
> + *
> + * Check if the virtual timer has expired while we were running in the host,
> + * and inject an interrupt if that was the case, making sure the timer is
> + * masked or disabled on the host so that we keep executing.  Also schedule a
> + * software timer for the physical timer if it is enabled.
> + */
> +void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> +{
> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +	if (unlikely(!timer->enabled))
> +		return;
> +
> +	kvm_timer_update_state(vcpu);
> +
> +	/* Set the background timer for the physical timer emulation. */
> +	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
> +
> +	if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
> +		kvm_timer_flush_hwstate_user(vcpu);
> +	else
> +		kvm_timer_flush_hwstate_vgic(vcpu);
> +}
> +
>  /**
>   * kvm_timer_sync_hwstate - sync timer state from cpu
>   * @vcpu: The vcpu pointer
>   *
> - * Check if the virtual timer has expired while we were running in the guest,
> + * Check if any of the timers have expired while we were running in the guest,
>   * and inject an interrupt if that was the case.
>   */
>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
> @@ -559,6 +628,13 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  	if (timer->enabled)
>  		return 0;
>
> +	/* Without a VGIC we do not map virtual IRQs to physical IRQs */
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		goto no_vgic;
> +
> +	if (!vgic_initialized(vcpu->kvm))
> +		return -ENODEV;
> +
>  	/*
>  	 * Find the physical IRQ number corresponding to the host_vtimer_irq
>  	 */
> @@ -582,8 +658,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  	if (ret)
>  		return ret;
>
> +no_vgic:
>  	timer->enabled = 1;

What happens if

   1) User space spawns a VM with user space irqchip
   2) Runs the VM
   3) Then adds a virtual gic device

Would we then access wrong data because we don't run through 
kvm_timer_enable() because timer->enabled is set?

Would thus (unprivileged) user space be able to DOS the host system?


Alex

[PATCH v3 3/5] KVM: arm/arm64: Support arch timers with a userspace gic

[Marc Zyngier]

On 06/04/17 09:16, Alexander Graf wrote:
> 
> 
> On 05.04.17 11:28, Christoffer Dall wrote:
>> From: Alexander Graf <agraf@suse.de>
>>
>> If you're running with a userspace gic or other interrupt constroller
>> (that is no vgic in the kernel), then you have so far not been able to
>> use the architected timers, because the output of the architected
>> timers, which are driven inside the kernel, was a kernel-only construct
>> between the arch timer code and the vgic.
>>
>> This patch implements the new KVM_CAP_ARM_USER_IRQ feature, where we use a
>> side channel on the kvm_run structure, run->s.regs.device_irq_level, to
>> always notify userspace of the timer output levels when using a userspace
>> irqchip.
>>
>> This works by ensureing that before we enter the guest, if the timer
>> output level has changed compared to what we last told userspace, we
>> don't enter the guest, but instead return to userspace to notify it of
>> the new level.  If we are exiting, because of an MMIO for example, and
>> the level changed at the same time, the value is also updated and
>> userspace can sample the line as it needs.  This is nicely achieved
>> simply always updating the timer_irq_level field after the main run
>> loop.
>>
>> Note that the kvm_timer_update_irq trace event is changed to show the
>> host IRQ number for the timer instead of the guest IRQ number, because
>> the kernel no longer know which IRQ userspace wires up the timer signal
>> to.
>>
>> Also note that this patch implements all required functionality but does
>> not yet advertise the capability.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  arch/arm/kvm/arm.c           |  18 +++----
>>  include/kvm/arm_arch_timer.h |   2 +
>>  virt/kvm/arm/arch_timer.c    | 122 +++++++++++++++++++++++++++++++++++--------
>>  3 files changed, 110 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7fa4898..efb16e5 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -515,13 +515,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>  			return ret;
>>  	}
>>
>> -	/*
>> -	 * Enable the arch timers only if we have an in-kernel VGIC
>> -	 * and it has been properly initialized, since we cannot handle
>> -	 * interrupts from the virtual timer with a userspace gic.
>> -	 */
>> -	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
>> -		ret = kvm_timer_enable(vcpu);
>> +	ret = kvm_timer_enable(vcpu);
>>
>>  	return ret;
>>  }
>> @@ -640,9 +634,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		local_irq_disable();
>>
>>  		/*
>> -		 * Re-check atomic conditions
>> +		 * If we have a singal pending, or need to notify a userspace
>> +		 * irqchip about timer level changes, then we exit (and update
>> +		 * the timer level state in kvm_timer_update_run below).
>>  		 */
>> -		if (signal_pending(current)) {
>> +		if (signal_pending(current) ||
>> +		    kvm_timer_should_notify_user(vcpu)) {
>>  			ret = -EINTR;
>>  			run->exit_reason = KVM_EXIT_INTR;
>>  		}
>> @@ -714,6 +711,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		ret = handle_exit(vcpu, run, ret);
>>  	}
>>
>> +	/* Tell userspace about in-kernel device output levels */
>> +	kvm_timer_update_run(vcpu);
>> +
>>  	if (vcpu->sigset_active)
>>  		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
>>  	return ret;
>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>> index fe797d6..295584f 100644
>> --- a/include/kvm/arm_arch_timer.h
>> +++ b/include/kvm/arm_arch_timer.h
>> @@ -63,6 +63,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
>>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
>>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
>> +bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu);
>> +void kvm_timer_update_run(struct kvm_vcpu *vcpu);
>>  void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
>>
>>  u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 363f0d2..5dc2167 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -184,6 +184,27 @@ bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
>>  	return cval <= now;
>>  }
>>
>> +/*
>> + * Reflect the timer output level into the kvm_run structure
>> + */
>> +void kvm_timer_update_run(struct kvm_vcpu *vcpu)
>> +{
>> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> +	struct kvm_sync_regs *regs = &vcpu->run->s.regs;
>> +
>> +	if (likely(irqchip_in_kernel(vcpu->kvm)))
>> +		return;
>> +
>> +	/* Populate the device bitmap with the timer states */
>> +	regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
>> +				    KVM_ARM_DEV_EL1_PTIMER);
>> +	if (vtimer->irq.level)
>> +		regs->device_irq_level |= KVM_ARM_DEV_EL1_VTIMER;
>> +	if (ptimer->irq.level)
>> +		regs->device_irq_level |= KVM_ARM_DEV_EL1_PTIMER;
>> +}
>> +
>>  static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>>  				 struct arch_timer_context *timer_ctx)
>>  {
>> @@ -194,9 +215,12 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>>  	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>>  				   timer_ctx->irq.level);
>>
>> -	ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer_ctx->irq.irq,
>> -				  timer_ctx->irq.level);
>> -	WARN_ON(ret);
>> +	if (likely(irqchip_in_kernel(vcpu->kvm))) {
>> +		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
>> +					  timer_ctx->irq.irq,
>> +					  timer_ctx->irq.level);
>> +		WARN_ON(ret);
>> +	}
>>  }
>>
>>  /*
>> @@ -215,7 +239,7 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
>>  	 * because the guest would never see the interrupt.  Instead wait
>>  	 * until we call this function from kvm_timer_flush_hwstate.
>>  	 */
>> -	if (!timer->enabled)
>> +	if (unlikely(!timer->enabled))
>>  		return;
>>
>>  	if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
>> @@ -282,28 +306,12 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
>>  	timer_disarm(timer);
>>  }
>>
>> -/**
>> - * kvm_timer_flush_hwstate - prepare to move the virt timer to the cpu
>> - * @vcpu: The vcpu pointer
>> - *
>> - * Check if the virtual timer has expired while we were running in the host,
>> - * and inject an interrupt if that was the case.
>> - */
>> -void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>> +static void kvm_timer_flush_hwstate_vgic(struct kvm_vcpu *vcpu)
>>  {
>> -	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>  	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>>  	bool phys_active;
>>  	int ret;
>>
>> -	if (unlikely(!timer->enabled))
>> -		return;
>> -
>> -	kvm_timer_update_state(vcpu);
>> -
>> -	/* Set the background timer for the physical timer emulation. */
>> -	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
>> -
>>  	/*
>>  	* If we enter the guest with the virtual input level to the VGIC
>>  	* asserted, then we have already told the VGIC what we need to, and
>> @@ -355,11 +363,72 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>  	vtimer->active_cleared_last = !phys_active;
>>  }
>>
>> +bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
>> +{
>> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> +	struct kvm_sync_regs *sregs = &vcpu->run->s.regs;
>> +	bool vlevel, plevel;
>> +
>> +	if (likely(irqchip_in_kernel(vcpu->kvm)))
>> +		return false;
>> +
>> +	vlevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_VTIMER;
>> +	plevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_PTIMER;
>> +
>> +	return vtimer->irq.level != vlevel ||
>> +	       ptimer->irq.level != plevel;
>> +}
>> +
>> +static void kvm_timer_flush_hwstate_user(struct kvm_vcpu *vcpu)
>> +{
>> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>> +
>> +	/*
>> +	 * To prevent continuously exiting from the guest, we mask the
>> +	 * physical interrupt such that the guest can make forward progress.
>> +	 * Once we detect the output level being deasserted, we unmask the
>> +	 * interrupt again so that we exit from the guest when the timer
>> +	 * fires.
>> +	*/
>> +	if (vtimer->irq.level)
>> +		disable_percpu_irq(host_vtimer_irq);
>> +	else
>> +		enable_percpu_irq(host_vtimer_irq, 0);
>> +}
>> +
>> +/**
>> + * kvm_timer_flush_hwstate - prepare timers before running the vcpu
>> + * @vcpu: The vcpu pointer
>> + *
>> + * Check if the virtual timer has expired while we were running in the host,
>> + * and inject an interrupt if that was the case, making sure the timer is
>> + * masked or disabled on the host so that we keep executing.  Also schedule a
>> + * software timer for the physical timer if it is enabled.
>> + */
>> +void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>> +{
>> +	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> +
>> +	if (unlikely(!timer->enabled))
>> +		return;
>> +
>> +	kvm_timer_update_state(vcpu);
>> +
>> +	/* Set the background timer for the physical timer emulation. */
>> +	kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
>> +
>> +	if (unlikely(!irqchip_in_kernel(vcpu->kvm)))
>> +		kvm_timer_flush_hwstate_user(vcpu);
>> +	else
>> +		kvm_timer_flush_hwstate_vgic(vcpu);
>> +}
>> +
>>  /**
>>   * kvm_timer_sync_hwstate - sync timer state from cpu
>>   * @vcpu: The vcpu pointer
>>   *
>> - * Check if the virtual timer has expired while we were running in the guest,
>> + * Check if any of the timers have expired while we were running in the guest,
>>   * and inject an interrupt if that was the case.
>>   */
>>  void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
>> @@ -559,6 +628,13 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>  	if (timer->enabled)
>>  		return 0;
>>
>> +	/* Without a VGIC we do not map virtual IRQs to physical IRQs */
>> +	if (!irqchip_in_kernel(vcpu->kvm))
>> +		goto no_vgic;
>> +
>> +	if (!vgic_initialized(vcpu->kvm))
>> +		return -ENODEV;
>> +
>>  	/*
>>  	 * Find the physical IRQ number corresponding to the host_vtimer_irq
>>  	 */
>> @@ -582,8 +658,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>  	if (ret)
>>  		return ret;
>>
>> +no_vgic:
>>  	timer->enabled = 1;
> 
> What happens if
> 
>    1) User space spawns a VM with user space irqchip
>    2) Runs the VM
>    3) Then adds a virtual gic device

As soon as a vcpu has run once, it is not possible to instantiate a vgic
(see virt/kvm/arm/vgic/vgic-init.c:kvm_vgic_create around line 101).

Thanks,

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

[PATCH v3 3/5] KVM: arm/arm64: Support arch timers with a userspace gic

[Alexander Graf]

On 06.04.17 10:25, Marc Zyngier wrote:
> On 06/04/17 09:16, Alexander Graf wrote:
>>
>>
>> On 05.04.17 11:28, Christoffer Dall wrote:
>>> From: Alexander Graf <agraf@suse.de>
>>>

>>> @@ -559,6 +628,13 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>>  	if (timer->enabled)
>>>  		return 0;
>>>
>>> +	/* Without a VGIC we do not map virtual IRQs to physical IRQs */
>>> +	if (!irqchip_in_kernel(vcpu->kvm))
>>> +		goto no_vgic;
>>> +
>>> +	if (!vgic_initialized(vcpu->kvm))
>>> +		return -ENODEV;
>>> +
>>>  	/*
>>>  	 * Find the physical IRQ number corresponding to the host_vtimer_irq
>>>  	 */
>>> @@ -582,8 +658,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>>  	if (ret)
>>>  		return ret;
>>>
>>> +no_vgic:
>>>  	timer->enabled = 1;
>>
>> What happens if
>>
>>    1) User space spawns a VM with user space irqchip
>>    2) Runs the VM
>>    3) Then adds a virtual gic device
>
> As soon as a vcpu has run once, it is not possible to instantiate a vgic
> (see virt/kvm/arm/vgic/vgic-init.c:kvm_vgic_create around line 101).

Ah, I was missing that part. Awesome, all problems solved :).


Alex

[PATCH v3 3/5] KVM: arm/arm64: Support arch timers with a userspace gic

[Marc Zyngier]

On 05/04/17 10:28, Christoffer Dall wrote:
> From: Alexander Graf <agraf@suse.de>
> 
> If you're running with a userspace gic or other interrupt constroller

controller

> (that is no vgic in the kernel), then you have so far not been able to
> use the architected timers, because the output of the architected
> timers, which are driven inside the kernel, was a kernel-only construct
> between the arch timer code and the vgic.
> 
> This patch implements the new KVM_CAP_ARM_USER_IRQ feature, where we use a
> side channel on the kvm_run structure, run->s.regs.device_irq_level, to
> always notify userspace of the timer output levels when using a userspace
> irqchip.
> 
> This works by ensureing that before we enter the guest, if the timer

ensuring

> output level has changed compared to what we last told userspace, we
> don't enter the guest, but instead return to userspace to notify it of
> the new level.  If we are exiting, because of an MMIO for example, and
> the level changed at the same time, the value is also updated and
> userspace can sample the line as it needs.  This is nicely achieved
> simply always updating the timer_irq_level field after the main run
> loop.
> 
> Note that the kvm_timer_update_irq trace event is changed to show the
> host IRQ number for the timer instead of the guest IRQ number, because
> the kernel no longer know which IRQ userspace wires up the timer signal
> to.
> 
> Also note that this patch implements all required functionality but does
> not yet advertise the capability.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Otherwise looks good.

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

[PATCH v3 4/5] KVM: arm/arm64: Report PMU overflow interrupts to userspace irqchip

[Christoffer Dall]

From: Christoffer Dall <christoffer.dall@linaro.org>

When not using an in-kernel VGIC, but instead emulating an interrupt
controller in userspace, we should report the PMU overflow status to
that userspace interrupt controller using the KVM_CAP_ARM_USER_IRQ
feature.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/arm.c    |  9 ++++++---
 include/kvm/arm_pmu.h |  7 +++++++
 virt/kvm/arm/pmu.c    | 42 ++++++++++++++++++++++++++++++++++++++----
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index efb16e5..f935383 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -635,11 +635,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		/*
 		 * If we have a singal pending, or need to notify a userspace
-		 * irqchip about timer level changes, then we exit (and update
-		 * the timer level state in kvm_timer_update_run below).
+		 * irqchip about timer or PMU level changes, then we exit (and
+		 * update the timer level state in kvm_timer_update_run
+		 * below).
 		 */
 		if (signal_pending(current) ||
-		    kvm_timer_should_notify_user(vcpu)) {
+		    kvm_timer_should_notify_user(vcpu) ||
+		    kvm_pmu_should_notify_user(vcpu)) {
 			ret = -EINTR;
 			run->exit_reason = KVM_EXIT_INTR;
 		}
@@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 	/* Tell userspace about in-kernel device output levels */
 	kvm_timer_update_run(vcpu);
+	kvm_pmu_update_run(vcpu);
 
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 92e7e97..1ab4633 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -50,6 +50,8 @@ void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_pmu_sync_hwstate(struct kvm_vcpu *vcpu);
+bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu);
+void kvm_pmu_update_run(struct kvm_vcpu *vcpu);
 void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
@@ -85,6 +87,11 @@ static inline void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u64 val) {}
 static inline void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val) {}
 static inline void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu) {}
 static inline void kvm_pmu_sync_hwstate(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+static inline void kvm_pmu_update_run(struct kvm_vcpu *vcpu) {}
 static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) {}
 static inline void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val) {}
 static inline void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu,
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 69ccce3..51218be 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -230,13 +230,47 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu)
 		return;
 
 	overflow = !!kvm_pmu_overflow_status(vcpu);
-	if (pmu->irq_level != overflow) {
-		pmu->irq_level = overflow;
-		kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
-				    pmu->irq_num, overflow);
+	if (pmu->irq_level == overflow)
+		return;
+
+	pmu->irq_level = overflow;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm))) {
+		int ret;
+		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
+					  pmu->irq_num, overflow);
+		WARN_ON(ret);
 	}
 }
 
+bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu)
+{
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	struct kvm_sync_regs *sregs = &vcpu->run->s.regs;
+	bool run_level = sregs->device_irq_level & KVM_ARM_DEV_PMU;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm)))
+		return false;
+
+	return pmu->irq_level != run_level;
+}
+
+/*
+ * Reflect the PMU overflow interrupt output level into the kvm_run structure
+ */
+void kvm_pmu_update_run(struct kvm_vcpu *vcpu)
+{
+	struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm)))
+		return;
+
+	/* Populate the timer bitmap for user space */
+	regs->device_irq_level &= ~KVM_ARM_DEV_PMU;
+	if (vcpu->arch.pmu.irq_level)
+		regs->device_irq_level |= KVM_ARM_DEV_PMU;
+}
+
 /**
  * kvm_pmu_flush_hwstate - flush pmu state to cpu
  * @vcpu: The vcpu pointer
-- 
2.9.0

[PATCH v3 4/5] KVM: arm/arm64: Report PMU overflow interrupts to userspace irqchip

[Marc Zyngier]

On 05/04/17 10:28, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
> 
> When not using an in-kernel VGIC, but instead emulating an interrupt
> controller in userspace, we should report the PMU overflow status to
> that userspace interrupt controller using the KVM_CAP_ARM_USER_IRQ
> feature.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/arm.c    |  9 ++++++---
>  include/kvm/arm_pmu.h |  7 +++++++
>  virt/kvm/arm/pmu.c    | 42 ++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index efb16e5..f935383 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -635,11 +635,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		/*
>  		 * If we have a singal pending, or need to notify a userspace
> -		 * irqchip about timer level changes, then we exit (and update
> -		 * the timer level state in kvm_timer_update_run below).
> +		 * irqchip about timer or PMU level changes, then we exit (and
> +		 * update the timer level state in kvm_timer_update_run
> +		 * below).
>  		 */
>  		if (signal_pending(current) ||
> -		    kvm_timer_should_notify_user(vcpu)) {
> +		    kvm_timer_should_notify_user(vcpu) ||
> +		    kvm_pmu_should_notify_user(vcpu)) {
>  			ret = -EINTR;
>  			run->exit_reason = KVM_EXIT_INTR;
>  		}
> @@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  	/* Tell userspace about in-kernel device output levels */
>  	kvm_timer_update_run(vcpu);
> +	kvm_pmu_update_run(vcpu);

Very minor nit: as we now have a couple of functions that are going to
check the same thing (irqchip_in_kernel), we could consider moving the
test here.  I don't have strong feelings about it though.

Thanks,

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

[PATCH v3 4/5] KVM: arm/arm64: Report PMU overflow interrupts to userspace irqchip

[Christoffer Dall]

On Thu, Apr 06, 2017 at 06:12:10PM +0100, Marc Zyngier wrote:
> On 05/04/17 10:28, Christoffer Dall wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> > 
> > When not using an in-kernel VGIC, but instead emulating an interrupt
> > controller in userspace, we should report the PMU overflow status to
> > that userspace interrupt controller using the KVM_CAP_ARM_USER_IRQ
> > feature.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  arch/arm/kvm/arm.c    |  9 ++++++---
> >  include/kvm/arm_pmu.h |  7 +++++++
> >  virt/kvm/arm/pmu.c    | 42 ++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 51 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > index efb16e5..f935383 100644
> > --- a/arch/arm/kvm/arm.c
> > +++ b/arch/arm/kvm/arm.c
> > @@ -635,11 +635,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  
> >  		/*
> >  		 * If we have a singal pending, or need to notify a userspace
> > -		 * irqchip about timer level changes, then we exit (and update
> > -		 * the timer level state in kvm_timer_update_run below).
> > +		 * irqchip about timer or PMU level changes, then we exit (and
> > +		 * update the timer level state in kvm_timer_update_run
> > +		 * below).
> >  		 */
> >  		if (signal_pending(current) ||
> > -		    kvm_timer_should_notify_user(vcpu)) {
> > +		    kvm_timer_should_notify_user(vcpu) ||
> > +		    kvm_pmu_should_notify_user(vcpu)) {
> >  			ret = -EINTR;
> >  			run->exit_reason = KVM_EXIT_INTR;
> >  		}
> > @@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  
> >  	/* Tell userspace about in-kernel device output levels */
> >  	kvm_timer_update_run(vcpu);
> > +	kvm_pmu_update_run(vcpu);
> 
> Very minor nit: as we now have a couple of functions that are going to
> check the same thing (irqchip_in_kernel), we could consider moving the
> test here.  I don't have strong feelings about it though.

I like it:

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index f935383..4263785 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -714,8 +714,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	}
 
 	/* Tell userspace about in-kernel device output levels */
-	kvm_timer_update_run(vcpu);
-	kvm_pmu_update_run(vcpu);
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm))) {
+		kvm_timer_update_run(vcpu);
+		kvm_pmu_update_run(vcpu);
+	}
 
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 5dc2167..5976609 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -193,9 +193,6 @@ void kvm_timer_update_run(struct kvm_vcpu *vcpu)
 	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 	struct kvm_sync_regs *regs = &vcpu->run->s.regs;
 
-	if (likely(irqchip_in_kernel(vcpu->kvm)))
-		return;
-
 	/* Populate the device bitmap with the timer states */
 	regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
 				    KVM_ARM_DEV_EL1_PTIMER);
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 51218be..4b43e7f 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -262,9 +262,6 @@ void kvm_pmu_update_run(struct kvm_vcpu *vcpu)
 {
 	struct kvm_sync_regs *regs = &vcpu->run->s.regs;
 
-	if (likely(irqchip_in_kernel(vcpu->kvm)))
-		return;
-
 	/* Populate the timer bitmap for user space */
 	regs->device_irq_level &= ~KVM_ARM_DEV_PMU;
 	if (vcpu->arch.pmu.irq_level)


Thanks,
-Christoffer

[PATCH v3 5/5] KVM: arm/arm64: Advertise support for KVM_CAP_ARM_USER_IRQ

[Christoffer Dall]

From: Christoffer Dall <christoffer.dall@linaro.org>

Now when we support both the virtual timer and PMU reporting interrupts
to userspace, we can advertise this support.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/arm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index f935383..b420aa9 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -229,6 +229,13 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		else
 			r = kvm->arch.vgic.msis_require_devid;
 		break;
+	case KVM_CAP_ARM_USER_IRQ:
+		/*
+		 * 1: EL1_VTIMER, EL1_PTIMER, and PMU.
+		 * (bump this number if adding more devices)
+		 */
+		r = 1;
+		break;
 	default:
 		r = kvm_arch_dev_ioctl_check_extension(kvm, ext);
 		break;
-- 
2.9.0

[PATCH v3 5/5] KVM: arm/arm64: Advertise support for KVM_CAP_ARM_USER_IRQ

[Marc Zyngier]

On 05/04/17 10:28, Christoffer Dall wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
> 
> Now when we support both the virtual timer and PMU reporting interrupts

Now that... both timers...

> to userspace, we can advertise this support.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/arm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index f935383..b420aa9 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -229,6 +229,13 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  		else
>  			r = kvm->arch.vgic.msis_require_devid;
>  		break;
> +	case KVM_CAP_ARM_USER_IRQ:
> +		/*
> +		 * 1: EL1_VTIMER, EL1_PTIMER, and PMU.
> +		 * (bump this number if adding more devices)
> +		 */
> +		r = 1;
> +		break;
>  	default:
>  		r = kvm_arch_dev_ioctl_check_extension(kvm, ext);
>  		break;
> 


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

[PATCH v3 0/5] Support userspace irqchip with arch timers

[Alexander Graf]

On 05.04.17 11:28, Christoffer Dall wrote:
> This series is the second version of the rework of the patches to support
> architected timers with a userspace irqchip sent by Alexander Graf [1].
>
> We first cleanup some of the timer code to make it easier to understand
> what is being done in the later patches, and then define the ABI,
> implement timers support, implement PMU support, and finally advertise
> the features.
>
> These patches are based on the recent work from Jintack to support the
> physical timer in addition to the virtual timer.  This series including
> its dependencies can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git irqs-to-user-v3
>
> I tested this using Alex's QEMU patch with his fixes for SMP applied.  This
> seems to be rock-solid.  The temporary-not-for-upstream-but-for-testing patch
> can be found here (force-pushed and rebased since v2):
>
> https://git.linaro.org/people/christoffer.dall/qemu-arm.git no-kvm-irqchip
>
> I also tested it on 32-bit and it looks good there as well.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[PATCH v3 0/5] Support userspace irqchip with arch timers

[Marc Zyngier]

On 05/04/17 10:28, Christoffer Dall wrote:
> This series is the second version of the rework of the patches to support
> architected timers with a userspace irqchip sent by Alexander Graf [1].
> 
> We first cleanup some of the timer code to make it easier to understand
> what is being done in the later patches, and then define the ABI,
> implement timers support, implement PMU support, and finally advertise
> the features.
> 
> These patches are based on the recent work from Jintack to support the
> physical timer in addition to the virtual timer.  This series including
> its dependencies can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git irqs-to-user-v3
> 
> I tested this using Alex's QEMU patch with his fixes for SMP applied.  This
> seems to be rock-solid.  The temporary-not-for-upstream-but-for-testing patch
> can be found here (force-pushed and rebased since v2):
> 
> https://git.linaro.org/people/christoffer.dall/qemu-arm.git no-kvm-irqchip
> 
> I also tested it on 32-bit and it looks good there as well.
> 
> Changes since v2:
>  - Actually push the right content to the kernel branch, sorry.
>  - Rebased on kvmarm/queue as of this morning (v4.11-rc1+ stuff)
>  - Changed IOCTL numbers as needed
> 
> Changes since v1:
>  - Rework the ABI to support devices in general as opposed to just
>    timers
>  - Support the PMU in addition to timers
>  - Also support the physical timer (rebased on Jintack's work)
>  - Updated some comments where I noticed things were out of date.
> 
> Several changes have been made compared to v7 of the original single
> patch, including:
>  - Rewording ABI documentation to be more in line with the ARM
>    architecture
>  - Add an explicit check for needing to notify userspace of a level
>    change instead of propagating the value
>  - Changes to commenting throughout to more accurately describe the
>    architecture concepts we try to maintain
>  - Reword of functions, for example from sync to update when the date
>    only flows one direction
> 
> [1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2016-September/021867.html
> [2]: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next

The couple of nits I mentioned notwithstanding, for the whole series:

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

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

[PATCH v3 0/5] Support userspace irqchip with arch timers

[Christoffer Dall]

On Thu, Apr 06, 2017 at 06:31:59PM +0100, Marc Zyngier wrote:
> On 05/04/17 10:28, Christoffer Dall wrote:
> > This series is the second version of the rework of the patches to support
> > architected timers with a userspace irqchip sent by Alexander Graf [1].
> > 
> > We first cleanup some of the timer code to make it easier to understand
> > what is being done in the later patches, and then define the ABI,
> > implement timers support, implement PMU support, and finally advertise
> > the features.
> > 
> > These patches are based on the recent work from Jintack to support the
> > physical timer in addition to the virtual timer.  This series including
> > its dependencies can be found here:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git irqs-to-user-v3
> > 
> > I tested this using Alex's QEMU patch with his fixes for SMP applied.  This
> > seems to be rock-solid.  The temporary-not-for-upstream-but-for-testing patch
> > can be found here (force-pushed and rebased since v2):
> > 
> > https://git.linaro.org/people/christoffer.dall/qemu-arm.git no-kvm-irqchip
> > 
> > I also tested it on 32-bit and it looks good there as well.
> > 
> > Changes since v2:
> >  - Actually push the right content to the kernel branch, sorry.
> >  - Rebased on kvmarm/queue as of this morning (v4.11-rc1+ stuff)
> >  - Changed IOCTL numbers as needed
> > 
> > Changes since v1:
> >  - Rework the ABI to support devices in general as opposed to just
> >    timers
> >  - Support the PMU in addition to timers
> >  - Also support the physical timer (rebased on Jintack's work)
> >  - Updated some comments where I noticed things were out of date.
> > 
> > Several changes have been made compared to v7 of the original single
> > patch, including:
> >  - Rewording ABI documentation to be more in line with the ARM
> >    architecture
> >  - Add an explicit check for needing to notify userspace of a level
> >    change instead of propagating the value
> >  - Changes to commenting throughout to more accurately describe the
> >    architecture concepts we try to maintain
> >  - Reword of functions, for example from sync to update when the date
> >    only flows one direction
> > 
> > [1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2016-September/021867.html
> > [2]: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
> 
> The couple of nits I mentioned notwithstanding, for the whole series:
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 

Thanks.   I have applied this to kvmarm/queue.

-Christoffer