[PATCH 0/3] Support userspace irqchip with arch timers

[PATCH 0/3] Support userspace irqchip with arch timers

[Christoffer Dall]

Hi Alex,

Marc and I have been looking at this during Linaro connect and have
slightly reworked your patch into this small series.

It would be good if you could have a look at it and test it out.

I've tested it with your QEMU, and it works for UP, but secondary CPUs
fail to come up, and it looks like the kernel never gets an IPI for
those CPUs from userspace.  Any chance you're willing to take a look at
that?

Also, let me know if the split of your patch with preserving your
authorship is ok with you.

Thanks,
-Christoffer

---

This series slightly reworks 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 split up the
implementation into two separate patches (maintaining original
authorship).

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

These patches are based on two fixes necessary to test them which deal
with running kernel code without an in-kernl irqchip, and they are both
in kvmarm/next[2].

[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 arch timer interrupts ABI
  KVM: arm/arm64: Support arch timers with a userspace gic

Christoffer Dall (1):
  KVM: arm/arm64: Cleanup the arch timer code's irqchip checking

 Documentation/virtual/kvm/api.txt |  29 ++++++++++
 arch/arm/include/uapi/asm/kvm.h   |   2 +
 arch/arm/kvm/arm.c                |  19 +++----
 arch/arm64/include/uapi/asm/kvm.h |   2 +
 include/kvm/arm_arch_timer.h      |   2 +
 include/uapi/linux/kvm.h          |   6 +++
 virt/kvm/arm/arch_timer.c         | 111 ++++++++++++++++++++++++++++++--------
 7 files changed, 139 insertions(+), 32 deletions(-)

-- 
2.9.0

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

[Christoffer Dall]

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 | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 27a1f63..824ed26 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -171,8 +171,6 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 	int ret;
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
-	BUG_ON(!vgic_initialized(vcpu->kvm));
-
 	timer->active_cleared_last = false;
 	timer->irq.level = new_level;
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
@@ -187,7 +185,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;
 
@@ -197,13 +195,11 @@ 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(vcpu) != timer->irq.level)
 		kvm_timer_update_irq(vcpu, !timer->irq.level);
-
-	return 0;
 }
 
 /*
@@ -255,9 +251,11 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	bool phys_active;
 	int ret;
 
-	if (kvm_timer_update_state(vcpu))
+	if (unlikely(!timer->enabled))
 		return;
 
+	kvm_timer_update_state(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
-- 
2.9.0

[PATCH 2/3] KVM: arm/arm64: Add ARM arch timer interrupts 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 timer events that may result in interrupt line state
changes, so we lose out on timer events if we run with user space gic
emulation.

Define an ABI to publish the timer output level 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 | 29 +++++++++++++++++++++++++++++
 arch/arm/include/uapi/asm/kvm.h   |  2 ++
 arch/arm64/include/uapi/asm/kvm.h |  2 ++
 include/uapi/linux/kvm.h          |  6 ++++++
 4 files changed, 39 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 739db9a..2adf600 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3928,3 +3928,32 @@ In order to use SynIC, it has to be activated by setting this
 capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
 will disable the use of APIC hardware virtualization even if supported
 by the CPU, as it's incompatible with SynIC auto-EOI behavior.
+
+8.3 KVM_CAP_ARM_TIMER
+
+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 ARM architected timers
+presented to the VM.  For such VMs, on every return to userspace, the kernel
+updates the vcpu's run->s.regs.timer_irq_level field to represent the actual
+output level of the timers.
+
+Whenever kvm detects a change in the timer 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 timer output level and re-compute the state of
+the userspace interrupt controller.  Userspace should always check the state
+of run->s.regs.timer_irq_level on every kvm exit.  The value in
+run->s.regs.timer_irq_level should be considered a level triggered interrupt
+signal.
+
+The field run->s.regs.timer_irq_level is available independent of
+run->kvm_valid_regs or run->kvm_dirty_regs bits.
+
+Currently the following bits are defined for the timer_irq_level bitmap:
+
+    KVM_ARM_TIMER_VTIMER  -  virtual timer
+
+Future versions of kvm may implement additional timer events. These will get
+indicated by additional KVM_CAP extensions.
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index b38c10c..23c2e77 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -112,6 +112,8 @@ struct kvm_debug_exit_arch {
 };
 
 struct kvm_sync_regs {
+	/* Used with KVM_CAP_ARM_TIMER */
+	u8 timer_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 3051f86..411d62a 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_TIMER */
+	u8 timer_irq_level;
 };
 
 struct kvm_arch_memory_slot {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 300ef25..c293fc9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -870,6 +870,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_S390_USER_INSTR0 130
 #define KVM_CAP_MSI_DEVID 131
 #define KVM_CAP_PPC_HTM 132
+#define KVM_CAP_ARM_TIMER 133
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1327,4 +1328,9 @@ 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_TIMER */
+
+/* Bits for run->s.regs.timer_irq_level */
+#define KVM_ARM_TIMER_VTIMER		(1 << 0)
+
 #endif /* __LINUX_KVM_H */
-- 
2.9.0

[PATCH 3/3] 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_TIMER feature, where we use a
side channel on the kvm_run structure, run->s.regs.timer_irq_level, to
always notify userspace of the timer output level 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.

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

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 85a3f90..712695d 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_PSCI_0_2:
 	case KVM_CAP_READONLY_MEM:
 	case KVM_CAP_MP_STATE:
+	case KVM_CAP_ARM_TIMER:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -474,13 +475,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;
 }
@@ -594,9 +589,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;
 		}
@@ -668,6 +666,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		ret = handle_exit(vcpu, run, ret);
 	}
 
+	/* Tell userspace about the arch timer output level */
+	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 dda39d8..7157e73 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -65,6 +65,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 824ed26..c645b7d 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -166,6 +166,22 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
 	return cval <= now;
 }
 
+/*
+ * Reflect the timer output level into the kvm_run structure
+ */
+void kvm_timer_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->timer_irq_level &= ~KVM_ARM_TIMER_VTIMER;
+	if (vcpu->arch.timer_cpu.irq.level)
+		regs->timer_irq_level |= KVM_ARM_TIMER_VTIMER;
+}
+
 static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 {
 	int ret;
@@ -173,12 +189,17 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 
 	timer->active_cleared_last = false;
 	timer->irq.level = new_level;
-	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
+	trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
 				   timer->irq.level);
-	ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
-					 timer->irq.irq,
-					 timer->irq.level);
-	WARN_ON(ret);
+
+	if (likely(irqchip_in_kernel(vcpu->kvm))) {
+		/* Fire the timer in the VGIC */
+		ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
+						 timer->irq.irq,
+						 timer->irq.level);
+
+		WARN_ON(ret);
+	}
 }
 
 /*
@@ -195,7 +216,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(vcpu) != timer->irq.level)
@@ -238,24 +259,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;
 	bool phys_active;
 	int ret;
 
-	if (unlikely(!timer->enabled))
-		return;
-
-	kvm_timer_update_state(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
@@ -307,6 +316,56 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	timer->active_cleared_last = !phys_active;
 }
 
+bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	bool run_level = vcpu->run->s.regs.timer_irq_level & KVM_ARM_TIMER_VTIMER;
+
+	if (likely(irqchip_in_kernel(vcpu->kvm)))
+		return false;
+
+	return timer->irq.level != run_level;
+}
+
+static void kvm_timer_flush_hwstate_user(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	/*
+	 * 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 (timer->irq.level)
+		disable_percpu_irq(host_vtimer_irq);
+	else
+		enable_percpu_irq(host_vtimer_irq, 0);
+}
+
+/**
+ * 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)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	if (unlikely(!timer->enabled))
+		return;
+
+	kvm_timer_update_state(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
@@ -473,6 +532,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
 	 */
@@ -496,6 +562,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
+no_vgic:
 
 	/*
 	 * There is a potential race here between VCPUs starting for the first
-- 
2.9.0

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

On 09/27/2016 09:08 PM, Christoffer Dall wrote:
> Hi Alex,
>
> Marc and I have been looking at this during Linaro connect and have
> slightly reworked your patch into this small series.
>
> It would be good if you could have a look at it and test it out.

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

Works fine so far :).

> I've tested it with your QEMU, and it works for UP, but secondary CPUs
> fail to come up, and it looks like the kernel never gets an IPI for
> those CPUs from userspace.  Any chance you're willing to take a look at
> that?

I can try, but not very soon. I doubt it's something fundamental - we 
probably just don't synchronize kvm/qemu state properly on IPIs (read: a 
QEMU bug).

> Also, let me know if the split of your patch with preserving your
> authorship is ok with you.

Works for me :)


Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

On 27.09.16 21:08, Christoffer Dall wrote:
> Hi Alex,
> 
> Marc and I have been looking at this during Linaro connect and have
> slightly reworked your patch into this small series.
> 
> It would be good if you could have a look at it and test it out.
> 
> I've tested it with your QEMU, and it works for UP, but secondary CPUs
> fail to come up, and it looks like the kernel never gets an IPI for
> those CPUs from userspace.  Any chance you're willing to take a look at
> that?

I still need to see whether I can come up with a prettier solution, but
for now this works:

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f1ad805..8b9a084 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2393,7 +2393,11 @@ static int kvm_get_mp_state(X86CPU *cpu)
         return ret;
     }
     env->mp_state = mp_state.mp_state;
+#ifdef CONFIG_ARM
+    if (kvm_enabled()) {
+#else
     if (kvm_irqchip_in_kernel()) {
+#endif
         cs->halted = (mp_state.mp_state == KVM_MP_STATE_HALTED);
     }
     return 0;


Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

On 30.09.16 16:54, Alexander Graf wrote:
> 
> 
> On 27.09.16 21:08, Christoffer Dall wrote:
>> Hi Alex,
>>
>> Marc and I have been looking at this during Linaro connect and have
>> slightly reworked your patch into this small series.
>>
>> It would be good if you could have a look at it and test it out.
>>
>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>> fail to come up, and it looks like the kernel never gets an IPI for
>> those CPUs from userspace.  Any chance you're willing to take a look at
>> that?
> 
> I still need to see whether I can come up with a prettier solution, but
> for now this works:
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c

Eh, no, not in i386 code :). But the problem seems to be a missing
mpstate sync.


Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Christoffer Dall]

On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
> 
> 
> On 30.09.16 16:54, Alexander Graf wrote:
> > 
> > 
> > On 27.09.16 21:08, Christoffer Dall wrote:
> >> Hi Alex,
> >>
> >> Marc and I have been looking at this during Linaro connect and have
> >> slightly reworked your patch into this small series.
> >>
> >> It would be good if you could have a look at it and test it out.
> >>
> >> I've tested it with your QEMU, and it works for UP, but secondary CPUs
> >> fail to come up, and it looks like the kernel never gets an IPI for
> >> those CPUs from userspace.  Any chance you're willing to take a look at
> >> that?
> > 
> > I still need to see whether I can come up with a prettier solution, but
> > for now this works:
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> 
> Eh, no, not in i386 code :). But the problem seems to be a missing
> mpstate sync.
> 
Yeah, that looked really dodgy.  Have you tested it? :)

-Christoffer

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

On 30.09.16 17:43, Christoffer Dall wrote:
> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>
>>
>> On 30.09.16 16:54, Alexander Graf wrote:
>>>
>>>
>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>> Hi Alex,
>>>>
>>>> Marc and I have been looking at this during Linaro connect and have
>>>> slightly reworked your patch into this small series.
>>>>
>>>> It would be good if you could have a look at it and test it out.
>>>>
>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>> those CPUs from userspace.  Any chance you're willing to take a look at
>>>> that?
>>>
>>> I still need to see whether I can come up with a prettier solution, but
>>> for now this works:
>>>
>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>
>> Eh, no, not in i386 code :). But the problem seems to be a missing
>> mpstate sync.
>>
> Yeah, that looked really dodgy.  Have you tested it? :)

I have, but I ran the wrong command line and by accident used -M
...,kernel-irqchip=on :)


Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

On 30.09.16 17:43, Christoffer Dall wrote:
> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>
>>
>> On 30.09.16 16:54, Alexander Graf wrote:
>>>
>>>
>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>> Hi Alex,
>>>>
>>>> Marc and I have been looking at this during Linaro connect and have
>>>> slightly reworked your patch into this small series.
>>>>
>>>> It would be good if you could have a look at it and test it out.
>>>>
>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>> those CPUs from userspace.  Any chance you're willing to take a look at
>>>> that?
>>>
>>> I still need to see whether I can come up with a prettier solution, but
>>> for now this works:
>>>
>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>
>> Eh, no, not in i386 code :). But the problem seems to be a missing
>> mpstate sync.
>>
> Yeah, that looked really dodgy.  Have you tested it? :)

This time around tested with the correct command line parameters I hope
:). I'll send a pretty patch later.

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b4c8fe2..b549f00 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
      */
     kvm_async_interrupts_allowed = true;

+    /*
+     * PSCI wakes up secondary cores, so we always need to
+     * have vCPUs waiting in kernel space
+     */
+    kvm_halt_in_kernel_allowed = true;
+
     cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);

     type_register_static(&host_arm_cpu_type_info);


Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Marc Zyngier]

Alex,

On 30/09/16 20:31, Alexander Graf wrote:
> 
> 
> On 30.09.16 17:43, Christoffer Dall wrote:
>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 30.09.16 16:54, Alexander Graf wrote:
>>>>
>>>>
>>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>>> Hi Alex,
>>>>>
>>>>> Marc and I have been looking at this during Linaro connect and have
>>>>> slightly reworked your patch into this small series.
>>>>>
>>>>> It would be good if you could have a look at it and test it out.
>>>>>
>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>>> those CPUs from userspace.  Any chance you're willing to take a look at
>>>>> that?
>>>>
>>>> I still need to see whether I can come up with a prettier solution, but
>>>> for now this works:
>>>>
>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>
>>> Eh, no, not in i386 code :). But the problem seems to be a missing
>>> mpstate sync.
>>>
>> Yeah, that looked really dodgy.  Have you tested it? :)
> 
> This time around tested with the correct command line parameters I hope
> :). I'll send a pretty patch later.
> 
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index b4c8fe2..b549f00 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       */
>      kvm_async_interrupts_allowed = true;
> 
> +    /*
> +     * PSCI wakes up secondary cores, so we always need to
> +     * have vCPUs waiting in kernel space
> +     */
> +    kvm_halt_in_kernel_allowed = true;
> +
>      cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
> 
>      type_register_static(&host_arm_cpu_type_info);

What the status of userspace for this thing? Are QEMU patches being
posted and reviewed?

Thanks,

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

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

> Am 28.10.2016 um 16:38 schrieb Marc Zyngier <marc.zyngier@arm.com>:
> 
> Alex,
> 
>> On 30/09/16 20:31, Alexander Graf wrote:
>> 
>> 
>>> On 30.09.16 17:43, Christoffer Dall wrote:
>>>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>>> 
>>>> 
>>>>> On 30.09.16 16:54, Alexander Graf wrote:
>>>>> 
>>>>> 
>>>>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>>>> Hi Alex,
>>>>>> 
>>>>>> Marc and I have been looking at this during Linaro connect and have
>>>>>> slightly reworked your patch into this small series.
>>>>>> 
>>>>>> It would be good if you could have a look at it and test it out.
>>>>>> 
>>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>>>> those CPUs from userspace.  Any chance you're willing to take a look at
>>>>>> that?
>>>>> 
>>>>> I still need to see whether I can come up with a prettier solution, but
>>>>> for now this works:
>>>>> 
>>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>> 
>>>> Eh, no, not in i386 code :). But the problem seems to be a missing
>>>> mpstate sync.
>>>> 
>>> Yeah, that looked really dodgy.  Have you tested it? :)
>> 
>> This time around tested with the correct command line parameters I hope
>> :). I'll send a pretty patch later.
>> 
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index b4c8fe2..b549f00 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>      */
>>     kvm_async_interrupts_allowed = true;
>> 
>> +    /*
>> +     * PSCI wakes up secondary cores, so we always need to
>> +     * have vCPUs waiting in kernel space
>> +     */
>> +    kvm_halt_in_kernel_allowed = true;
>> +
>>     cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
>> 
>>     type_register_static(&host_arm_cpu_type_info);
> 
> What the status of userspace for this thing? Are QEMU patches being
> posted and reviewed?

I didn't see a notification that the patches were merged. Are they in Linus' tree yet? Then I can post enablement to qemu-devel.

Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Marc Zyngier]

On 28/10/16 16:52, Alexander Graf wrote:
> 
> 
>> Am 28.10.2016 um 16:38 schrieb Marc Zyngier <marc.zyngier@arm.com>:
>>
>> Alex,
>>
>>> On 30/09/16 20:31, Alexander Graf wrote:
>>>
>>>
>>>> On 30.09.16 17:43, Christoffer Dall wrote:
>>>>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>>>>
>>>>>
>>>>>> On 30.09.16 16:54, Alexander Graf wrote:
>>>>>>
>>>>>>
>>>>>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> Marc and I have been looking at this during Linaro connect and have
>>>>>>> slightly reworked your patch into this small series.
>>>>>>>
>>>>>>> It would be good if you could have a look at it and test it out.
>>>>>>>
>>>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>>>>> those CPUs from userspace.  Any chance you're willing to take a look at
>>>>>>> that?
>>>>>>
>>>>>> I still need to see whether I can come up with a prettier solution, but
>>>>>> for now this works:
>>>>>>
>>>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>>>
>>>>> Eh, no, not in i386 code :). But the problem seems to be a missing
>>>>> mpstate sync.
>>>>>
>>>> Yeah, that looked really dodgy.  Have you tested it? :)
>>>
>>> This time around tested with the correct command line parameters I hope
>>> :). I'll send a pretty patch later.
>>>
>>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>>> index b4c8fe2..b549f00 100644
>>> --- a/target-arm/kvm.c
>>> +++ b/target-arm/kvm.c
>>> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>      */
>>>     kvm_async_interrupts_allowed = true;
>>>
>>> +    /*
>>> +     * PSCI wakes up secondary cores, so we always need to
>>> +     * have vCPUs waiting in kernel space
>>> +     */
>>> +    kvm_halt_in_kernel_allowed = true;
>>> +
>>>     cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
>>>
>>>     type_register_static(&host_arm_cpu_type_info);
>>
>> What the status of userspace for this thing? Are QEMU patches being
>> posted and reviewed?
> 
> I didn't see a notification that the patches were merged. Are they in
> Linus' tree yet? Then I can post enablement to qemu-devel.

I think you got it backward. I have no intention of merging them until I
see a vague consensus on the userspace API, and a set of patches ready
to be merged in QEMU.

Thanks,

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

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

> Am 28.10.2016 um 17:57 schrieb Marc Zyngier <marc.zyngier@arm.com>:
> 
>> On 28/10/16 16:52, Alexander Graf wrote:
>> 
>> 
>>> Am 28.10.2016 um 16:38 schrieb Marc Zyngier <marc.zyngier@arm.com>:
>>> 
>>> Alex,
>>> 
>>>> On 30/09/16 20:31, Alexander Graf wrote:
>>>> 
>>>> 
>>>>>> On 30.09.16 17:43, Christoffer Dall wrote:
>>>>>> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 30.09.16 16:54, Alexander Graf wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>>>>>> Hi Alex,
>>>>>>>> 
>>>>>>>> Marc and I have been looking at this during Linaro connect and have
>>>>>>>> slightly reworked your patch into this small series.
>>>>>>>> 
>>>>>>>> It would be good if you could have a look at it and test it out.
>>>>>>>> 
>>>>>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>>>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>>>>>> those CPUs from userspace.  Any chance you're willing to take a look at
>>>>>>>> that?
>>>>>>> 
>>>>>>> I still need to see whether I can come up with a prettier solution, but
>>>>>>> for now this works:
>>>>>>> 
>>>>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>>>> 
>>>>>> Eh, no, not in i386 code :). But the problem seems to be a missing
>>>>>> mpstate sync.
>>>>>> 
>>>>> Yeah, that looked really dodgy.  Have you tested it? :)
>>>> 
>>>> This time around tested with the correct command line parameters I hope
>>>> :). I'll send a pretty patch later.
>>>> 
>>>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>>>> index b4c8fe2..b549f00 100644
>>>> --- a/target-arm/kvm.c
>>>> +++ b/target-arm/kvm.c
>>>> @@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>>     */
>>>>    kvm_async_interrupts_allowed = true;
>>>> 
>>>> +    /*
>>>> +     * PSCI wakes up secondary cores, so we always need to
>>>> +     * have vCPUs waiting in kernel space
>>>> +     */
>>>> +    kvm_halt_in_kernel_allowed = true;
>>>> +
>>>>    cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
>>>> 
>>>>    type_register_static(&host_arm_cpu_type_info);
>>> 
>>> What the status of userspace for this thing? Are QEMU patches being
>>> posted and reviewed?
>> 
>> I didn't see a notification that the patches were merged. Are they in
>> Linus' tree yet? Then I can post enablement to qemu-devel.
> 
> I think you got it backward. I have no intention of merging them until I
> see a vague consensus on the userspace API, and a set of patches ready
> to be merged in QEMU.

That's not how kvm apis are made.


Alex

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Paolo Bonzini]

> > > > What the status of userspace for this thing? Are QEMU patches being
> > > > posted and reviewed?
> > > 
> > > I didn't see a notification that the patches were merged. Are they in
> > > Linus' tree yet? Then I can post enablement to qemu-devel.
> > 
> > I think you got it backward. I have no intention of merging them until I
> > see a vague consensus on the userspace API, and a set of patches ready
> > to be merged in QEMU.
> 
> That's not how kvm apis are made.

Actually I think it's always been like this, depending on what Marc meant for
"ready to be merged in QEMU".  It doesn't make sense to merge KVM APIs without
having userspace patches at least posted as RFC to qemu-devel, and without
having at least a positive response from the QEMU architecture maintainer.
ARM does require a bit more care because there's no overlap between kernel
and userspace maintainers, so perhaps that's the source of the confusion?

Now, of course merging the patches in QEMU may take a month or two depending
on the timing (because you have to wait for the patches to be merged into
Linus's tree and for the KVM headers to be updated in QEMU---which is not
going to happen during freeze of course).  So of course the KVM patch thus
can be committed even if QEMU is in freeze, as long as the QEMU architecture
maintainer gives an overall green light.

In fact a couple weeks from now would be the perfect time to post the patches
to QEMU.  Then you can get a review from Peter once he's back from vacation
(beginning of December), the KVM patches can easily make it for the 4.10 merge
window (end of December), and the userspace patches will hopefully be merged in
QEMU 2.9 in January 2017.

Thanks,

Paolo

Re: [PATCH 0/3] Support userspace irqchip with arch timers

[Alexander Graf]

> On 29 Oct 2016, at 15:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>>>>> What the status of userspace for this thing? Are QEMU patches being
>>>>> posted and reviewed?
>>>> 
>>>> I didn't see a notification that the patches were merged. Are they in
>>>> Linus' tree yet? Then I can post enablement to qemu-devel.
>>> 
>>> I think you got it backward. I have no intention of merging them until I
>>> see a vague consensus on the userspace API, and a set of patches ready
>>> to be merged in QEMU.
>> 
>> That's not how kvm apis are made.
> 
> Actually I think it's always been like this, depending on what Marc meant for
> "ready to be merged in QEMU".  It doesn't make sense to merge KVM APIs without
> having userspace patches at least posted as RFC to qemu-devel, and without
> having at least a positive response from the QEMU architecture maintainer.

I halfway agree. I do agree that there needs to be a reference implementation that proves the API to make sense. That bit is referenced in the cover letter of the patch set.

Peter as the QEMU architecture maintainer has been part of the review process and involved from the beginning. In fact, the current approach was his idea.

Do we need to fly the loop over qemu-devel? I doubt it, but if it makes you happy I can post an RFC there too.

> ARM does require a bit more care because there's no overlap between kernel
> and userspace maintainers, so perhaps that's the source of the confusion?
> 
> Now, of course merging the patches in QEMU may take a month or two depending
> on the timing (because you have to wait for the patches to be merged into
> Linus's tree and for the KVM headers to be updated in QEMU---which is not
> going to happen during freeze of course).  So of course the KVM patch thus
> can be committed even if QEMU is in freeze, as long as the QEMU architecture
> maintainer gives an overall green light.

Right. My plan was to make sure that people agree on the KVM interface. Then directly send non-RFC patches to qemu-devel, which can only happen when the KVM patches are merged. I didn’t see any reason why that approach would be controversial, since everyone who really cared was involved.

But again, I don’t care strongly enough to make a fuss. If people are happier with RFC patches on the ML rather than a github link to RFC patches, I’ll send them.


Alex

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm/arm64: Add ARM arch timer interrupts ABI

[Peter Maydell]

On 27 September 2016 at 20:08, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> 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 timer events that may result in interrupt line state
> changes, so we lose out on timer events if we run with user space gic
> emulation.
>
> Define an ABI to publish the timer output level 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 | 29 +++++++++++++++++++++++++++++
>  arch/arm/include/uapi/asm/kvm.h   |  2 ++
>  arch/arm64/include/uapi/asm/kvm.h |  2 ++
>  include/uapi/linux/kvm.h          |  6 ++++++
>  4 files changed, 39 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 739db9a..2adf600 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3928,3 +3928,32 @@ In order to use SynIC, it has to be activated by setting this
>  capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
>  will disable the use of APIC hardware virtualization even if supported
>  by the CPU, as it's incompatible with SynIC auto-EOI behavior.
> +
> +8.3 KVM_CAP_ARM_TIMER
> +
> +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 ARM architected timers
> +presented to the VM.  For such VMs, on every return to userspace, the kernel
> +updates the vcpu's run->s.regs.timer_irq_level field to represent the actual
> +output level of the timers.
> +
> +Whenever kvm detects a change in the timer 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 timer output level and re-compute the state of
> +the userspace interrupt controller.  Userspace should always check the state
> +of run->s.regs.timer_irq_level on every kvm exit.  The value in
> +run->s.regs.timer_irq_level should be considered a level triggered interrupt
> +signal.
> +
> +The field run->s.regs.timer_irq_level is available independent of
> +run->kvm_valid_regs or run->kvm_dirty_regs bits.
> +
> +Currently the following bits are defined for the timer_irq_level bitmap:
> +
> +    KVM_ARM_TIMER_VTIMER  -  virtual timer
> +
> +Future versions of kvm may implement additional timer events. These will get
> +indicated by additional KVM_CAP extensions.

This API looks good to me generally. My only question is whether we
want to name the struct fields so they're not specifically talking
about timer interrupts. For instance we probably want to expose the
vPMU interrupt line to userspace too. We could do that by adding another
struct field pmu_irq_level, but we could equally just assign it a bit
in the existing irq_level field.

Possible current and future outbound interrupt lines (some of these
would only show up in some unlikely or lots-of-implementation-needed
cases, I'm just trying to produce an exhaustive list):
 * virtual timer
 * physical timer
 * hyp timer (nested virtualization case)
 * secure timer (unlikely but maybe if EL3 is ever supported inside a VM)
 * gic maintenance interrupt (nested virt again)
 * PMU interrupt

The kernel doesn't know which interrupt number these would be wired
up to, so they're all just arbitrary outputs, and you could put them
in one field or split them up into multiple fields, it doesn't make
much difference.

thanks
-- PMM

Re: [PATCH 2/3] KVM: arm/arm64: Add ARM arch timer interrupts ABI

[Christoffer Dall]

On Tue, Nov 01, 2016 at 11:26:54AM +0000, Peter Maydell wrote:
> On 27 September 2016 at 20:08, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > 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 timer events that may result in interrupt line state
> > changes, so we lose out on timer events if we run with user space gic
> > emulation.
> >
> > Define an ABI to publish the timer output level 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 | 29 +++++++++++++++++++++++++++++
> >  arch/arm/include/uapi/asm/kvm.h   |  2 ++
> >  arch/arm64/include/uapi/asm/kvm.h |  2 ++
> >  include/uapi/linux/kvm.h          |  6 ++++++
> >  4 files changed, 39 insertions(+)
> >
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 739db9a..2adf600 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -3928,3 +3928,32 @@ In order to use SynIC, it has to be activated by setting this
> >  capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
> >  will disable the use of APIC hardware virtualization even if supported
> >  by the CPU, as it's incompatible with SynIC auto-EOI behavior.
> > +
> > +8.3 KVM_CAP_ARM_TIMER
> > +
> > +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 ARM architected timers
> > +presented to the VM.  For such VMs, on every return to userspace, the kernel
> > +updates the vcpu's run->s.regs.timer_irq_level field to represent the actual
> > +output level of the timers.
> > +
> > +Whenever kvm detects a change in the timer 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 timer output level and re-compute the state of
> > +the userspace interrupt controller.  Userspace should always check the state
> > +of run->s.regs.timer_irq_level on every kvm exit.  The value in
> > +run->s.regs.timer_irq_level should be considered a level triggered interrupt
> > +signal.
> > +
> > +The field run->s.regs.timer_irq_level is available independent of
> > +run->kvm_valid_regs or run->kvm_dirty_regs bits.
> > +
> > +Currently the following bits are defined for the timer_irq_level bitmap:
> > +
> > +    KVM_ARM_TIMER_VTIMER  -  virtual timer
> > +
> > +Future versions of kvm may implement additional timer events. These will get
> > +indicated by additional KVM_CAP extensions.
> 
> This API looks good to me generally. My only question is whether we
> want to name the struct fields so they're not specifically talking
> about timer interrupts. For instance we probably want to expose the
> vPMU interrupt line to userspace too. We could do that by adding another
> struct field pmu_irq_level, but we could equally just assign it a bit
> in the existing irq_level field.
> 
> Possible current and future outbound interrupt lines (some of these
> would only show up in some unlikely or lots-of-implementation-needed
> cases, I'm just trying to produce an exhaustive list):
>  * virtual timer
>  * physical timer
>  * hyp timer (nested virtualization case)
>  * secure timer (unlikely but maybe if EL3 is ever supported inside a VM)
>  * gic maintenance interrupt (nested virt again)
>  * PMU interrupt

Thanks for the list, that's good to have around for the future.

There's also the potential of the EL2 virtual timer for nested VHE
support, right?

> 
> The kernel doesn't know which interrupt number these would be wired
> up to, so they're all just arbitrary outputs, and you could put them
> in one field or split them up into multiple fields, it doesn't make
> much difference.
> 

So if we keep this we're kind of suggesting that we'll have a field per
device type later on.  Since this is a u8 and we are talking about up 5
5 timers already, there's not much waste currently, and we have plenty
of padding.  I suppose we an always add a 'other_devices' thing later,
so I prefer just sticking with this one for now.

Thanks,
-Christoffer

Re: [PATCH 2/3] KVM: arm/arm64: Add ARM arch timer interrupts ABI

[Peter Maydell]

On 1 November 2016 at 14:50, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Tue, Nov 01, 2016 at 11:26:54AM +0000, Peter Maydell wrote:
>> Possible current and future outbound interrupt lines (some of these
>> would only show up in some unlikely or lots-of-implementation-needed
>> cases, I'm just trying to produce an exhaustive list):
>>  * virtual timer
>>  * physical timer
>>  * hyp timer (nested virtualization case)
>>  * secure timer (unlikely but maybe if EL3 is ever supported inside a VM)
>>  * gic maintenance interrupt (nested virt again)
>>  * PMU interrupt
>
> Thanks for the list, that's good to have around for the future.
>
> There's also the potential of the EL2 virtual timer for nested VHE
> support, right?

That's the one I meant by "hyp timer".

>> The kernel doesn't know which interrupt number these would be wired
>> up to, so they're all just arbitrary outputs, and you could put them
>> in one field or split them up into multiple fields, it doesn't make
>> much difference.
>>
>
> So if we keep this we're kind of suggesting that we'll have a field per
> device type later on.  Since this is a u8 and we are talking about up 5
> 5 timers already,

4.

> there's not much waste currently, and we have plenty
> of padding.  I suppose we an always add a 'other_devices' thing later,
> so I prefer just sticking with this one for now.

Yeah, it's not a big deal either way.

thanks
-- PMM

Re: [PATCH 2/3] KVM: arm/arm64: Add ARM arch timer interrupts ABI

[Christoffer Dall]

On Tue, Nov 01, 2016 at 02:54:11PM +0000, Peter Maydell wrote:
> On 1 November 2016 at 14:50, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Tue, Nov 01, 2016 at 11:26:54AM +0000, Peter Maydell wrote:
> >> Possible current and future outbound interrupt lines (some of these
> >> would only show up in some unlikely or lots-of-implementation-needed
> >> cases, I'm just trying to produce an exhaustive list):
> >>  * virtual timer
> >>  * physical timer
> >>  * hyp timer (nested virtualization case)
> >>  * secure timer (unlikely but maybe if EL3 is ever supported inside a VM)
> >>  * gic maintenance interrupt (nested virt again)
> >>  * PMU interrupt
> >
> > Thanks for the list, that's good to have around for the future.
> >
> > There's also the potential of the EL2 virtual timer for nested VHE
> > support, right?
> 
> That's the one I meant by "hyp timer".
> 

there's the hyp timer, and then there's the ARMv8.1 virtual hyp timer.

> >> The kernel doesn't know which interrupt number these would be wired
> >> up to, so they're all just arbitrary outputs, and you could put them
> >> in one field or split them up into multiple fields, it doesn't make
> >> much difference.
> >>
> >
> > So if we keep this we're kind of suggesting that we'll have a field per
> > device type later on.  Since this is a u8 and we are talking about up 5
> > 5 timers already,
> 
> 4.
> 

virtual
physical
hyp
virtual hyp
secure

Am I missing something?

-Christoffer

Re: [PATCH 2/3] KVM: arm/arm64: Add ARM arch timer interrupts ABI

[Marc Zyngier]

On Tue, Nov 01 2016 at 02:54:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 November 2016 at 14:50, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
>> On Tue, Nov 01, 2016 at 11:26:54AM +0000, Peter Maydell wrote:
>>> Possible current and future outbound interrupt lines (some of these
>>> would only show up in some unlikely or lots-of-implementation-needed
>>> cases, I'm just trying to produce an exhaustive list):
>>>  * virtual timer
>>>  * physical timer
>>>  * hyp timer (nested virtualization case)
>>>  * secure timer (unlikely but maybe if EL3 is ever supported inside a VM)
>>>  * gic maintenance interrupt (nested virt again)
>>>  * PMU interrupt
>>
>> Thanks for the list, that's good to have around for the future.
>>
>> There's also the potential of the EL2 virtual timer for nested VHE
>> support, right?
>
> That's the one I meant by "hyp timer".

VHE also adds an extra virtual timer, for symmetry with what EL1
provides (and on which CNTVOFF doesn't have any effect) - see section
B8.1.1 of the ARMv8.1 addendum. So we effectively have:

- Secure physical EL3
- Non-secure physical EL1
- Non-secure virtual EL1
- Non-secure physical EL2
- Non-secure virtual EL2

Thanks,

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