[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

Some systems out there (well, one type in particular - the Raspberry Pi series)
do have virtualization capabilities in the core, but no ARM GIC interrupt
controller.

To run on these systems, the cleanest route is to just handle all
interrupt delivery in user space and only deal with IRQ pins in the core
side in KVM.

This works pretty well already, but breaks when the guest starts to use
architected timers, as these are handled straight inside kernel space today.

This patch set allows user space to receive vtimer events as well as mask
them, so that we can handle all vtimer related interrupt injection from user
space, enabling us to use architected timer with user space gic emulation.

I have successfully run edk2 as well as Linux using these patches on a
Raspberry Pi 3 system with acceptable speed.

A branch with WIP QEMU code can be found here:

  https://github.com/agraf/qemu.git no-kvm-irqchip

To use the user space irqchip, just run it with

  $ qemu-system-aarch64 -M virt,kernel-irqchip=off

v1 -> v2:

  - Add back curly brace that got lost

v2 -> v3:

  - Fix "only only" in documentation
  - Split patches
  - Remove kvm_emulate.h include

Alexander Graf (2):
  KVM: arm/arm64: Add vcpu ENABLE_CAP functionality
  KVM: arm/arm64: Route vtimer events to user space

 Documentation/virtual/kvm/api.txt |  28 ++++++++-
 arch/arm/include/asm/kvm_host.h   |   3 +
 arch/arm/kvm/arm.c                |  47 +++++++++++---
 arch/arm64/include/asm/kvm_host.h |   3 +
 include/uapi/linux/kvm.h          |  14 +++++
 virt/kvm/arm/arch_timer.c         | 125 +++++++++++++++++++++++++++-----------
 6 files changed, 177 insertions(+), 43 deletions(-)

-- 
2.6.6

[PATCH v3 1/2] KVM: arm/arm64: Add vcpu ENABLE_CAP functionality

[Alexander Graf]

In a follow-up patch we will need to enable capabilities on demand for
backwards compatibility. This patch adds the generic framework to handle
vcpu cap enablement to the arm code base.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Documentation/virtual/kvm/api.txt |  4 +++-
 arch/arm/kvm/arm.c                | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 739db9a..23937e0 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -997,7 +997,9 @@ documentation when it pops into existence).
 
 Capability: KVM_CAP_ENABLE_CAP, KVM_CAP_ENABLE_CAP_VM
 Architectures: x86 (only KVM_CAP_ENABLE_CAP_VM),
-	       mips (only KVM_CAP_ENABLE_CAP), ppc, s390
+	       mips (only KVM_CAP_ENABLE_CAP), ppc, s390,
+	       arm (only KVM_CAP_ENABLE_CAP),
+	       arm64 (only KVM_CAP_ENABLE_CAP)
 Type: vcpu ioctl, vm ioctl (with KVM_CAP_ENABLE_CAP_VM)
 Parameters: struct kvm_enable_cap (in)
 Returns: 0 on success; -1 on error
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 75f130e..c84b6ad 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -878,6 +878,23 @@ static int kvm_arm_vcpu_has_attr(struct kvm_vcpu *vcpu,
 	return ret;
 }
 
+static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
+				     struct kvm_enable_cap *cap)
+{
+	int r;
+
+	if (cap->flags)
+		return -EINVAL;
+
+	switch (cap->cap) {
+	default:
+		r = -EINVAL;
+		break;
+	}
+
+	return r;
+}
+
 long kvm_arch_vcpu_ioctl(struct file *filp,
 			 unsigned int ioctl, unsigned long arg)
 {
@@ -941,6 +958,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 			return -EFAULT;
 		return kvm_arm_vcpu_has_attr(vcpu, &attr);
 	}
+	case KVM_ENABLE_CAP:
+	{
+		struct kvm_enable_cap cap;
+
+		if (copy_from_user(&cap, argp, sizeof(cap)))
+			return -EFAULT;
+		return kvm_vcpu_ioctl_enable_cap(vcpu, &cap);
+	}
 	default:
 		return -EINVAL;
 	}
-- 
2.6.6

[PATCH v3 2/2] KVM: arm/arm64: Route vtimer events to user space

[Alexander Graf]

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.

This patch fixes that by routing vtimer expiration events to user space.
With this patch I can successfully run edk2 and Linux with user space gic
emulation.

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

---

v1 -> v2:

  - Add back curly brace that got lost

v2 -> v3:

  - Split into patch set
---
 Documentation/virtual/kvm/api.txt |  24 +++++++-
 arch/arm/include/asm/kvm_host.h   |   3 +
 arch/arm/kvm/arm.c                |  22 ++++---
 arch/arm64/include/asm/kvm_host.h |   3 +
 include/uapi/linux/kvm.h          |  14 +++++
 virt/kvm/arm/arch_timer.c         | 125 +++++++++++++++++++++++++++-----------
 6 files changed, 149 insertions(+), 42 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 23937e0..dec1a78 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3202,8 +3202,10 @@ struct kvm_run {
 	/* in */
 	__u8 request_interrupt_window;
 
-Request that KVM_RUN return when it becomes possible to inject external
+[x86] Request that KVM_RUN return when it becomes possible to inject external
 interrupts into the guest.  Useful in conjunction with KVM_INTERRUPT.
+[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
+trigger forever. Useful with KVM_CAP_ARM_TIMER.
 
 	__u8 padding1[7];
 
@@ -3519,6 +3521,16 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
 event/message pages and to enable/disable SynIC messages/events processing
 in userspace.
 
+		/* KVM_EXIT_ARM_TIMER */
+		struct {
+			__u8 timesource;
+		} arm_timer;
+
+Indicates that a timer triggered that user space needs to handle and
+potentially mask with vcpu->run->request_interrupt_window to allow the
+guest to proceed. This only happens for timers that got enabled through
+KVM_CAP_ARM_TIMER.
+
 		/* Fix the size of the union. */
 		char padding[256];
 	};
@@ -3739,6 +3751,16 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
 accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
 the guest.
 
+6.11 KVM_CAP_ARM_TIMER
+
+Architectures: arm, arm64
+Target: vcpu
+Parameters: args[0] contains a bitmap of timers to enable
+
+This capability allows to route per-core timers into user space. When it's
+enabled, the enabled timers trigger KVM_EXIT_ARM_TIMER guest exits when they
+are pending, unless masked by vcpu->run->request_interrupt_window.
+
 7. Capabilities that can be enabled on VMs
 ------------------------------------------
 
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index de338d9..77d1f73 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -180,6 +180,9 @@ struct kvm_vcpu_arch {
 
 	/* Detect first run of a vcpu */
 	bool has_run_once;
+
+	/* User space wants timer notifications */
+	bool user_space_arm_timers;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index c84b6ad..57bdb71 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;
 }
@@ -601,6 +596,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			run->exit_reason = KVM_EXIT_INTR;
 		}
 
+		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
+			/* Tell user space about the pending vtimer */
+			ret = 0;
+			run->exit_reason = KVM_EXIT_ARM_TIMER;
+			run->arm_timer.timesource = KVM_ARM_TIMER_VTIMER;
+		}
+
 		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
 			vcpu->arch.power_off || vcpu->arch.pause) {
 			local_irq_enable();
@@ -887,6 +889,12 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	switch (cap->cap) {
+	case KVM_CAP_ARM_TIMER:
+		r = 0;
+		if (cap->args[0] != KVM_ARM_TIMER_VTIMER)
+			return -EINVAL;
+		vcpu->arch.user_space_arm_timers = true;
+		break;
 	default:
 		r = -EINVAL;
 		break;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3eda975..3d01481 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -270,6 +270,9 @@ struct kvm_vcpu_arch {
 
 	/* Detect first run of a vcpu */
 	bool has_run_once;
+
+	/* User space wants timer notifications */
+	bool user_space_arm_timers;
 };
 
 #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 300ef25..00f4948 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -205,6 +205,7 @@ struct kvm_hyperv_exit {
 #define KVM_EXIT_S390_STSI        25
 #define KVM_EXIT_IOAPIC_EOI       26
 #define KVM_EXIT_HYPERV           27
+#define KVM_EXIT_ARM_TIMER        28
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
@@ -361,6 +362,10 @@ struct kvm_run {
 		} eoi;
 		/* KVM_EXIT_HYPERV */
 		struct kvm_hyperv_exit hyperv;
+		/* KVM_EXIT_ARM_TIMER */
+		struct {
+			__u8 timesource;
+		} arm_timer;
 		/* Fix the size of the union. */
 		char padding[256];
 	};
@@ -870,6 +875,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 +1333,12 @@ 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->request_interrupt_window */
+#define KVM_IRQWINDOW_VTIMER		(1 << 0)
+
+/* Bits for run->arm_timer.timesource */
+#define KVM_ARM_TIMER_VTIMER		(1 << 0)
+
 #endif /* __LINUX_KVM_H */
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 4309b60..cbbb50dd 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -170,16 +170,45 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 {
 	int ret;
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct kvm_run *run = vcpu->run;
 
-	BUG_ON(!vgic_initialized(vcpu->kvm));
+	BUG_ON(irqchip_in_kernel(vcpu->kvm) && !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,
+	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);
+
+	if (irqchip_in_kernel(vcpu->kvm) && vgic_initialized(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);
+	} else if (!vcpu->arch.user_space_arm_timers) {
+		/* User space has not activated timer use */
+		ret = 0;
+	} else {
+		/*
+		 * Set PENDING_TIMER so that user space can handle the event if
+		 *
+		 *   1) Level is high
+		 *   2) The vtimer is not suppressed by user space
+		 *   3) We are not in the timer trigger exit path
+		 */
+		if (new_level &&
+		    !(run->request_interrupt_window & KVM_IRQWINDOW_VTIMER) &&
+		    (run->exit_reason != KVM_EXIT_ARM_TIMER)) {
+			/* KVM_REQ_PENDING_TIMER means vtimer triggered */
+			kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
+		}
+
+		/* Force a new level high check on next entry */
+		timer->irq.level = 0;
+
+		ret = 0;
+	}
+
 	WARN_ON(ret);
 }
 
@@ -197,7 +226,8 @@ 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)
+	if ((irqchip_in_kernel(vcpu->kvm) && !vgic_initialized(vcpu->kvm)) ||
+	    !timer->enabled)
 		return -ENODEV;
 
 	if (kvm_timer_should_fire(vcpu) != timer->irq.level)
@@ -275,35 +305,57 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	* to ensure that hardware interrupts from the timer triggers a guest
 	* exit.
 	*/
-	phys_active = timer->irq.level ||
-			kvm_vgic_map_is_active(vcpu, timer->irq.irq);
-
-	/*
-	 * We want to avoid hitting the (re)distributor as much as
-	 * possible, as this is a potentially expensive MMIO access
-	 * (not to mention locks in the irq layer), and a solution for
-	 * this is to cache the "active" state in memory.
-	 *
-	 * Things to consider: we cannot cache an "active set" state,
-	 * because the HW can change this behind our back (it becomes
-	 * "clear" in the HW). We must then restrict the caching to
-	 * the "clear" state.
-	 *
-	 * The cache is invalidated on:
-	 * - vcpu put, indicating that the HW cannot be trusted to be
-	 *   in a sane state on the next vcpu load,
-	 * - any change in the interrupt state
-	 *
-	 * Usage conditions:
-	 * - cached value is "active clear"
-	 * - value to be programmed is "active clear"
-	 */
-	if (timer->active_cleared_last && !phys_active)
-		return;
+	if (irqchip_in_kernel(vcpu->kvm) && vgic_initialized(vcpu->kvm)) {
+		phys_active = timer->irq.level ||
+				kvm_vgic_map_is_active(vcpu, timer->irq.irq);
+
+		/*
+		 * We want to avoid hitting the (re)distributor as much as
+		 * possible, as this is a potentially expensive MMIO access
+		 * (not to mention locks in the irq layer), and a solution for
+		 * this is to cache the "active" state in memory.
+		 *
+		 * Things to consider: we cannot cache an "active set" state,
+		 * because the HW can change this behind our back (it becomes
+		 * "clear" in the HW). We must then restrict the caching to
+		 * the "clear" state.
+		 *
+		 * The cache is invalidated on:
+		 * - vcpu put, indicating that the HW cannot be trusted to be
+		 *   in a sane state on the next vcpu load,
+		 * - any change in the interrupt state
+		 *
+		 * Usage conditions:
+		 * - cached value is "active clear"
+		 * - value to be programmed is "active clear"
+		 */
+		if (timer->active_cleared_last && !phys_active)
+			return;
+
+		ret = irq_set_irqchip_state(host_vtimer_irq,
+					    IRQCHIP_STATE_ACTIVE,
+					    phys_active);
+	} else {
+		/* User space tells us whether the timer is in active mode */
+		phys_active = vcpu->run->request_interrupt_window &
+			      KVM_IRQWINDOW_VTIMER;
+
+		/* However if the line is high, we exit anyway, so we want
+		 * to keep the IRQ masked */
+		phys_active = phys_active || timer->irq.level;
+
+		/*
+		 * So we can just explicitly mask or unmask the IRQ, gaining
+		 * more compatibility with oddball irq controllers.
+		 */
+		if (phys_active)
+			disable_percpu_irq(host_vtimer_irq);
+		else
+			enable_percpu_irq(host_vtimer_irq, 0);
+
+		ret = 0;
+	}
 
-	ret = irq_set_irqchip_state(host_vtimer_irq,
-				    IRQCHIP_STATE_ACTIVE,
-				    phys_active);
 	WARN_ON(ret);
 
 	timer->active_cleared_last = !phys_active;
@@ -479,6 +531,10 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (timer->enabled)
 		return 0;
 
+	/* No need to route physical IRQs when we don't use the vgic */
+	if (!irqchip_in_kernel(vcpu->kvm))
+		goto no_vgic;
+
 	/*
 	 * Find the physical IRQ number corresponding to the host_vtimer_irq
 	 */
@@ -502,6 +558,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.6.6

[PATCH v3 2/2] KVM: arm/arm64: Route vtimer events to user space

[Peter Maydell]

On 16 September 2016 at 07:26, Alexander Graf <agraf@suse.de> wrote:
> 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.
>
> This patch fixes that by routing vtimer expiration events to user space.
> With this patch I can successfully run edk2 and Linux with user space gic
> emulation.

Hi Alex. I have some comments just on the userspace API docs.
These are mostly requests for clarification or expansion, and they
boil down to "if you gave me this API document change I wouldn't be
able to deduce from it the necessary changes to QEMU or kvmtool to
use this functionality".

> Signed-off-by: Alexander Graf <agraf@suse.de>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 23937e0..dec1a78 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3202,8 +3202,10 @@ struct kvm_run {
>         /* in */
>         __u8 request_interrupt_window;
>
> -Request that KVM_RUN return when it becomes possible to inject external
> +[x86] Request that KVM_RUN return when it becomes possible to inject external
>  interrupts into the guest.  Useful in conjunction with KVM_INTERRUPT.
> +[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
> +trigger forever. Useful with KVM_CAP_ARM_TIMER.

It's only a _u8 and there's more than 8 IRQ lines -- which ones
can you mask this way?

>         __u8 padding1[7];
>
> @@ -3519,6 +3521,16 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
>  event/message pages and to enable/disable SynIC messages/events processing
>  in userspace.
>
> +               /* KVM_EXIT_ARM_TIMER */
> +               struct {
> +                       __u8 timesource;
> +               } arm_timer;
> +
> +Indicates that a timer triggered that user space needs to handle and
> +potentially mask with vcpu->run->request_interrupt_window to allow the
> +guest to proceed. This only happens for timers that got enabled through
> +KVM_CAP_ARM_TIMER.

What values can the timesource field contain?

> +
>                 /* Fix the size of the union. */
>                 char padding[256];
>         };
> @@ -3739,6 +3751,16 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
>  accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
>  the guest.
>
> +6.11 KVM_CAP_ARM_TIMER
> +
> +Architectures: arm, arm64
> +Target: vcpu
> +Parameters: args[0] contains a bitmap of timers to enable
> +
> +This capability allows to route per-core timers into user space. When it's
> +enabled, the enabled timers trigger KVM_EXIT_ARM_TIMER guest exits when they
> +are pending, unless masked by vcpu->run->request_interrupt_window.

How are the timers enumerated within the bitmap? Which timers can
be enabled like this?

Shouldn't this be talking about "timers to route to userspace"
rather than "timers to enable", since AIUI the timers are always
enabled regardless of what you set here ?

The KVM_CAP constant name seems rather generic given this is an
obscure corner of the API.


What is the mechanism for the kernel to tell userspace when the
timer *stops* being pending, so it can update the interrupt
level for it in userspace? (What you really want I think is
for the kernel to notify "timer level goes high" and "timer
level goes low" and handle the masking internally.)

> +
>  7. Capabilities that can be enabled on VMs
>  ------------------------------------------

thanks
-- PMM

[PATCH v3 2/2] KVM: arm/arm64: Route vtimer events to user space

[Alexander Graf]

> On 16 Sep 2016, at 11:11, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On 16 September 2016 at 07:26, Alexander Graf <agraf@suse.de> wrote:
>> 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.
>> 
>> This patch fixes that by routing vtimer expiration events to user space.
>> With this patch I can successfully run edk2 and Linux with user space gic
>> emulation.
> 
> Hi Alex. I have some comments just on the userspace API docs.
> These are mostly requests for clarification or expansion, and they
> boil down to "if you gave me this API document change I wouldn't be
> able to deduce from it the necessary changes to QEMU or kvmtool to
> use this functionality?.

Yeah, most of the documentation wouldn?t be enough to deduce the changes you need in user space applications, but I agree that that doesn?t mean we can?t improve going forward :).

> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 23937e0..dec1a78 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -3202,8 +3202,10 @@ struct kvm_run {
>>        /* in */
>>        __u8 request_interrupt_window;
>> 
>> -Request that KVM_RUN return when it becomes possible to inject external
>> +[x86] Request that KVM_RUN return when it becomes possible to inject external
>> interrupts into the guest.  Useful in conjunction with KVM_INTERRUPT.
>> +[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
>> +trigger forever. Useful with KVM_CAP_ARM_TIMER.
> 
> It's only a _u8 and there's more than 8 IRQ lines -- which ones
> can you mask this way?

There are defines for the individual event lines you can mask. I?ll clarify.

> 
>>        __u8 padding1[7];
>> 
>> @@ -3519,6 +3521,16 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
>> event/message pages and to enable/disable SynIC messages/events processing
>> in userspace.
>> 
>> +               /* KVM_EXIT_ARM_TIMER */
>> +               struct {
>> +                       __u8 timesource;
>> +               } arm_timer;
>> +
>> +Indicates that a timer triggered that user space needs to handle and
>> +potentially mask with vcpu->run->request_interrupt_window to allow the
>> +guest to proceed. This only happens for timers that got enabled through
>> +KVM_CAP_ARM_TIMER.
> 
> What values can the timesource field contain?

There are defines for that again, I?ll clarify :).

> 
>> +
>>                /* Fix the size of the union. */
>>                char padding[256];
>>        };
>> @@ -3739,6 +3751,16 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
>> accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
>> the guest.
>> 
>> +6.11 KVM_CAP_ARM_TIMER
>> +
>> +Architectures: arm, arm64
>> +Target: vcpu
>> +Parameters: args[0] contains a bitmap of timers to enable
>> +
>> +This capability allows to route per-core timers into user space. When it's
>> +enabled, the enabled timers trigger KVM_EXIT_ARM_TIMER guest exits when they
>> +are pending, unless masked by vcpu->run->request_interrupt_window.
> 
> How are the timers enumerated within the bitmap? Which timers can
> be enabled like this?

See above ;).

> Shouldn't this be talking about "timers to route to userspace"
> rather than "timers to enable", since AIUI the timers are always
> enabled regardless of what you set here ?

The counter is enabled, but the timer isn?t, as you can never receive an event. But yes, I agree that the wording isn?t explicit enough. I?ll see if I can come up with something better.

> The KVM_CAP constant name seems rather generic given this is an
> obscure corner of the API.

It basically enables the KVM_EXIT_ARM_TIMER capability - and I didn?t want to make the name too long. Any suggestions?

> What is the mechanism for the kernel to tell userspace when the
> timer *stops* being pending, so it can update the interrupt

It can?t, because it doesn?t know :(. We can?t trap that event.

> level for it in userspace? (What you really want I think is
> for the kernel to notify "timer level goes high" and "timer
> level goes low" and handle the masking internally.)

That?s what I want, but it?s not what hardware gives me. All I can do is poll whether it?s still up - and that?s basically what this interface does.


Alex

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Marc Zyngier]

Hi Alex,

On 16/09/16 07:26, Alexander Graf wrote:
> Some systems out there (well, one type in particular - the Raspberry Pi series)
> do have virtualization capabilities in the core, but no ARM GIC interrupt
> controller.
> 
> To run on these systems, the cleanest route is to just handle all
> interrupt delivery in user space and only deal with IRQ pins in the core
> side in KVM.
> 
> This works pretty well already, but breaks when the guest starts to use
> architected timers, as these are handled straight inside kernel space today.
> 
> This patch set allows user space to receive vtimer events as well as mask
> them, so that we can handle all vtimer related interrupt injection from user
> space, enabling us to use architected timer with user space gic emulation.

I have already voiced my concerns in the past, including face to face,
and I'm going to repeat it: I not keen at all on adding a new userspace
interface that is going to bitrot extremely quickly.

Let's face it, this new ABI will have a single user, with a limited
shelf life. I understand that the RPi is a popular product, but it looks
fairly obvious that this kind of sub-standard HW will eventually
disappear. We'll then be left with a userspace ABI that will break at
every single release, given that nobody in the RPi community actually
uses a mainline kernel.

And breaking this ABI will introduce userspace exploitable bugs, like
the one you've already shown. If anything, I would have loved to
completely kill the whole userspace GIC, because nobody cares. Yes, I
understand it is fun to have KVM running on the RPi. But the maintenance
costs far outweigh the fun aspect already.

You could still run KVM with an external emulated timer (not the arch
timer). No need for a new ABI for that.

Thanks,

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

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Paolo Bonzini]

On 16/09/2016 12:20, Marc Zyngier wrote:
> > This patch set allows user space to receive vtimer events as well as mask
> > them, so that we can handle all vtimer related interrupt injection from user
> > space, enabling us to use architected timer with user space gic emulation.
>
> I have already voiced my concerns in the past, including face to face,
> and I'm going to repeat it: I not keen at all on adding a new userspace
> interface that is going to bitrot extremely quickly.

You don't have automated tests set up?  It's not going to bitrot if you
test it, either with kvm-unit-tests or just by smoke-testing Linux.
It's _for_ the raspi, but it's not limited to it.

Paolo

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Christoffer Dall]

On Fri, Sep 16, 2016 at 02:18:45PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/09/2016 12:20, Marc Zyngier wrote:
> > > This patch set allows user space to receive vtimer events as well as mask
> > > them, so that we can handle all vtimer related interrupt injection from user
> > > space, enabling us to use architected timer with user space gic emulation.
> >
> > I have already voiced my concerns in the past, including face to face,
> > and I'm going to repeat it: I not keen at all on adding a new userspace
> > interface that is going to bitrot extremely quickly.
> 
> You don't have automated tests set up?  It's not going to bitrot if you
> test it, either with kvm-unit-tests or just by smoke-testing Linux.
> It's _for_ the raspi, but it's not limited to it.
> 

Our automated testing situation is not great, no.  Something we're
looking at, but have resource problems with.

-Christoffer

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Paolo Bonzini]

On 16/09/2016 14:30, Christoffer Dall wrote:
> > > > This patch set allows user space to receive vtimer events as well as mask
> > > > them, so that we can handle all vtimer related interrupt injection from user
> > > > space, enabling us to use architected timer with user space gic emulation.
> > >
> > > I have already voiced my concerns in the past, including face to face,
> > > and I'm going to repeat it: I not keen at all on adding a new userspace
> > > interface that is going to bitrot extremely quickly.
> > 
> > You don't have automated tests set up?  It's not going to bitrot if you
> > test it, either with kvm-unit-tests or just by smoke-testing Linux.
> > It's _for_ the raspi, but it's not limited to it.
> 
> Our automated testing situation is not great, no.  Something we're
> looking at, but have resource problems with.

But it's not a good reason to hold back a feature...

Paolo

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Christoffer Dall]

On Fri, Sep 16, 2016 at 02:31:42PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/09/2016 14:30, Christoffer Dall wrote:
> > > > > This patch set allows user space to receive vtimer events as well as mask
> > > > > them, so that we can handle all vtimer related interrupt injection from user
> > > > > space, enabling us to use architected timer with user space gic emulation.
> > > >
> > > > I have already voiced my concerns in the past, including face to face,
> > > > and I'm going to repeat it: I not keen at all on adding a new userspace
> > > > interface that is going to bitrot extremely quickly.
> > > 
> > > You don't have automated tests set up?  It's not going to bitrot if you
> > > test it, either with kvm-unit-tests or just by smoke-testing Linux.
> > > It's _for_ the raspi, but it's not limited to it.
> > 
> > Our automated testing situation is not great, no.  Something we're
> > looking at, but have resource problems with.
> 
> But it's not a good reason to hold back a feature...
> 

I didn't say that exactly, but choosing not to merge something we cannot
maintain and which we're not paid to look after and where there's a
minimal interest, is not entirely unreasonable.

That being said, I'm not categorically against these patches, but I
share Marc's view that we've already seen that non-vgic support had been
broken for multiple versions without anyone complaining, and without
automated testing or substantial interest in the work, the patches
really are likely to bit-rot.

But I haven't even looked at the patches in detail, I was just replying
to the comment about testing.

-Christoffer

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Andrew Jones]

On Fri, Sep 16, 2016 at 03:30:27PM +0200, Christoffer Dall wrote:
> On Fri, Sep 16, 2016 at 02:31:42PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 16/09/2016 14:30, Christoffer Dall wrote:
> > > > > > This patch set allows user space to receive vtimer events as well as mask
> > > > > > them, so that we can handle all vtimer related interrupt injection from user
> > > > > > space, enabling us to use architected timer with user space gic emulation.
> > > > >
> > > > > I have already voiced my concerns in the past, including face to face,
> > > > > and I'm going to repeat it: I not keen at all on adding a new userspace
> > > > > interface that is going to bitrot extremely quickly.
> > > > 
> > > > You don't have automated tests set up?  It's not going to bitrot if you
> > > > test it, either with kvm-unit-tests or just by smoke-testing Linux.
> > > > It's _for_ the raspi, but it's not limited to it.
> > > 
> > > Our automated testing situation is not great, no.  Something we're
> > > looking at, but have resource problems with.
> > 
> > But it's not a good reason to hold back a feature...
> > 
> 
> I didn't say that exactly, but choosing not to merge something we cannot
> maintain and which we're not paid to look after and where there's a
> minimal interest, is not entirely unreasonable.
> 
> That being said, I'm not categorically against these patches, but I
> share Marc's view that we've already seen that non-vgic support had been
> broken for multiple versions without anyone complaining, and without
> automated testing or substantial interest in the work, the patches
> really are likely to bit-rot.
> 
> But I haven't even looked at the patches in detail, I was just replying
> to the comment about testing.

This may be a great time to start encouraging feature writers to submit
kvm-unit-tests patches at the same time as the feature (Hi Alex :-) I'm
happy to help when a test isn't easy to write due to a lack of framework,
but don't have nearly enough bandwidth to write all the tests myself.

As for additional motivation for this series, I'll point out that it's
good for bug isolation. When a guest fails to boot over KVM I can try
TCG. If that works, then I've likely narrowed it to KVM. If I can
further try kernel_irqchip=no, then I may further narrow it down to
the vgic implementation.

Thanks,
drew

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Paolo Bonzini]

On 16/09/2016 15:46, Andrew Jones wrote:
> > But I haven't even looked at the patches in detail, I was just replying
> > to the comment about testing.
>
> This may be a great time to start encouraging feature writers to submit
> kvm-unit-tests patches at the same time as the feature (Hi Alex :-) I'm
> happy to help when a test isn't easy to write due to a lack of framework,
> but don't have nearly enough bandwidth to write all the tests myself.

In this case really a kernel smoke test can just work.  But yes,
architectural timer tests would be nice too.

> As for additional motivation for this series, I'll point out that it's
> good for bug isolation. When a guest fails to boot over KVM I can try
> TCG. If that works, then I've likely narrowed it to KVM. If I can
> further try kernel_irqchip=no, then I may further narrow it down to
> the vgic implementation.

(Or vice versa to the userspace GIC if you're looking at a TCG bug).

Paolo

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

> Am 16.09.2016 um 15:46 schrieb Andrew Jones <drjones@redhat.com>:
> 
>> On Fri, Sep 16, 2016 at 03:30:27PM +0200, Christoffer Dall wrote:
>>> On Fri, Sep 16, 2016 at 02:31:42PM +0200, Paolo Bonzini wrote:
>>> 
>>> 
>>> On 16/09/2016 14:30, Christoffer Dall wrote:
>>>>>>> This patch set allows user space to receive vtimer events as well as mask
>>>>>>> them, so that we can handle all vtimer related interrupt injection from user
>>>>>>> space, enabling us to use architected timer with user space gic emulation.
>>>>>> 
>>>>>> I have already voiced my concerns in the past, including face to face,
>>>>>> and I'm going to repeat it: I not keen at all on adding a new userspace
>>>>>> interface that is going to bitrot extremely quickly.
>>>>> 
>>>>> You don't have automated tests set up?  It's not going to bitrot if you
>>>>> test it, either with kvm-unit-tests or just by smoke-testing Linux.
>>>>> It's _for_ the raspi, but it's not limited to it.
>>>> 
>>>> Our automated testing situation is not great, no.  Something we're
>>>> looking at, but have resource problems with.
>>> 
>>> But it's not a good reason to hold back a feature...
>> 
>> I didn't say that exactly, but choosing not to merge something we cannot
>> maintain and which we're not paid to look after and where there's a
>> minimal interest, is not entirely unreasonable.
>> 
>> That being said, I'm not categorically against these patches, but I
>> share Marc's view that we've already seen that non-vgic support had been
>> broken for multiple versions without anyone complaining, and without
>> automated testing or substantial interest in the work, the patches
>> really are likely to bit-rot.
>> 
>> But I haven't even looked at the patches in detail, I was just replying
>> to the comment about testing.
> 
> This may be a great time to start encouraging feature writers to submit
> kvm-unit-tests patches at the same time as the feature (Hi Alex :-)

I actually started off implementing this with the help of kvm-unit-tests. It's awesome!

I'm lacking actual irq support to make the test reasonable though and wanted to get the kernel bits out first :). But I'll sit down on that again soon I hope.


Alex

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Andrew Jones]

On Fri, Sep 16, 2016 at 09:36:42PM +0200, Alexander Graf wrote:
> 
> 
> > Am 16.09.2016 um 15:46 schrieb Andrew Jones <drjones@redhat.com>:
> > 
> >> On Fri, Sep 16, 2016 at 03:30:27PM +0200, Christoffer Dall wrote:
> >>> On Fri, Sep 16, 2016 at 02:31:42PM +0200, Paolo Bonzini wrote:
> >>> 
> >>> 
> >>> On 16/09/2016 14:30, Christoffer Dall wrote:
> >>>>>>> This patch set allows user space to receive vtimer events as well as mask
> >>>>>>> them, so that we can handle all vtimer related interrupt injection from user
> >>>>>>> space, enabling us to use architected timer with user space gic emulation.
> >>>>>> 
> >>>>>> I have already voiced my concerns in the past, including face to face,
> >>>>>> and I'm going to repeat it: I not keen at all on adding a new userspace
> >>>>>> interface that is going to bitrot extremely quickly.
> >>>>> 
> >>>>> You don't have automated tests set up?  It's not going to bitrot if you
> >>>>> test it, either with kvm-unit-tests or just by smoke-testing Linux.
> >>>>> It's _for_ the raspi, but it's not limited to it.
> >>>> 
> >>>> Our automated testing situation is not great, no.  Something we're
> >>>> looking at, but have resource problems with.
> >>> 
> >>> But it's not a good reason to hold back a feature...
> >> 
> >> I didn't say that exactly, but choosing not to merge something we cannot
> >> maintain and which we're not paid to look after and where there's a
> >> minimal interest, is not entirely unreasonable.
> >> 
> >> That being said, I'm not categorically against these patches, but I
> >> share Marc's view that we've already seen that non-vgic support had been
> >> broken for multiple versions without anyone complaining, and without
> >> automated testing or substantial interest in the work, the patches
> >> really are likely to bit-rot.
> >> 
> >> But I haven't even looked at the patches in detail, I was just replying
> >> to the comment about testing.
> > 
> > This may be a great time to start encouraging feature writers to submit
> > kvm-unit-tests patches at the same time as the feature (Hi Alex :-)
> 
> I actually started off implementing this with the help of kvm-unit-tests. It's awesome!
> 
> I'm lacking actual irq support to make the test reasonable though and wanted to get the kernel bits out first :). But I'll sit down on that again soon I hope.

I'm glad it looks like a good base for you. I need to get this series
https://github.com/rhdrjones/kvm-unit-tests/commits/arm/gic refreshed and
merged, and also it's time to start looking into adding interrupt
injection to chr-testdev. With those in place I hope it'll be an even
better base for you.

Thanks,
drew

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

On 19.09.16 09:52, Andrew Jones wrote:
> On Fri, Sep 16, 2016 at 09:36:42PM +0200, Alexander Graf wrote:
>>
>>
>>> Am 16.09.2016 um 15:46 schrieb Andrew Jones <drjones@redhat.com>:
>>>
>>>> On Fri, Sep 16, 2016 at 03:30:27PM +0200, Christoffer Dall wrote:
>>>>> On Fri, Sep 16, 2016 at 02:31:42PM +0200, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 16/09/2016 14:30, Christoffer Dall wrote:
>>>>>>>>> This patch set allows user space to receive vtimer events as well as mask
>>>>>>>>> them, so that we can handle all vtimer related interrupt injection from user
>>>>>>>>> space, enabling us to use architected timer with user space gic emulation.
>>>>>>>>
>>>>>>>> I have already voiced my concerns in the past, including face to face,
>>>>>>>> and I'm going to repeat it: I not keen at all on adding a new userspace
>>>>>>>> interface that is going to bitrot extremely quickly.
>>>>>>>
>>>>>>> You don't have automated tests set up?  It's not going to bitrot if you
>>>>>>> test it, either with kvm-unit-tests or just by smoke-testing Linux.
>>>>>>> It's _for_ the raspi, but it's not limited to it.
>>>>>>
>>>>>> Our automated testing situation is not great, no.  Something we're
>>>>>> looking at, but have resource problems with.
>>>>>
>>>>> But it's not a good reason to hold back a feature...
>>>>
>>>> I didn't say that exactly, but choosing not to merge something we cannot
>>>> maintain and which we're not paid to look after and where there's a
>>>> minimal interest, is not entirely unreasonable.
>>>>
>>>> That being said, I'm not categorically against these patches, but I
>>>> share Marc's view that we've already seen that non-vgic support had been
>>>> broken for multiple versions without anyone complaining, and without
>>>> automated testing or substantial interest in the work, the patches
>>>> really are likely to bit-rot.
>>>>
>>>> But I haven't even looked at the patches in detail, I was just replying
>>>> to the comment about testing.
>>>
>>> This may be a great time to start encouraging feature writers to submit
>>> kvm-unit-tests patches at the same time as the feature (Hi Alex :-)
>>
>> I actually started off implementing this with the help of kvm-unit-tests. It's awesome!
>>
>> I'm lacking actual irq support to make the test reasonable though and wanted to get the kernel bits out first :). But I'll sit down on that again soon I hope.
> 
> I'm glad it looks like a good base for you. I need to get this series
> https://github.com/rhdrjones/kvm-unit-tests/commits/arm/gic refreshed and
> merged, and also it's time to start looking into adding interrupt
> injection to chr-testdev. With those in place I hope it'll be an even
> better base for you.

Awesome. Let me know when you're further ahead with the gic work then so
that we can actually trigger interrupts and measure irq latencies :).

Until then, I sent the simplistic version that I used for bringup to the
list.


Alex

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Gerd Hoffmann]

  Hi,

> That being said, I'm not categorically against these patches, but I
> share Marc's view that we've already seen that non-vgic support had been
> broken for multiple versions without anyone complaining,

Oh, did this ever work?  Work as in "can actually run a virtual
machine", not as in "kvm doesn't throw an error on initialization".

> and without
> automated testing or substantial interest in the work, the patches
> really are likely to bit-rot.

Well, as far I know the rpi3 is the *only* aarch64 hardware which is
easily available and can (with these patches) run kvm.

More powerful stuff with more ram and sata storage and gbit network
(which I'd love to have to play with arm virt on real hardware) is
announced to be available really soon now since ...  half a year at
least?

cheers,
  Gerd

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

On 16.09.16 15:30, Christoffer Dall wrote:
> On Fri, Sep 16, 2016 at 02:31:42PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 16/09/2016 14:30, Christoffer Dall wrote:
>>>>>> This patch set allows user space to receive vtimer events as well as mask
>>>>>> them, so that we can handle all vtimer related interrupt injection from user
>>>>>> space, enabling us to use architected timer with user space gic emulation.
>>>>>
>>>>> I have already voiced my concerns in the past, including face to face,
>>>>> and I'm going to repeat it: I not keen at all on adding a new userspace
>>>>> interface that is going to bitrot extremely quickly.
>>>>
>>>> You don't have automated tests set up?  It's not going to bitrot if you
>>>> test it, either with kvm-unit-tests or just by smoke-testing Linux.
>>>> It's _for_ the raspi, but it's not limited to it.
>>>
>>> Our automated testing situation is not great, no.  Something we're
>>> looking at, but have resource problems with.
>>
>> But it's not a good reason to hold back a feature...
>>
> 
> I didn't say that exactly, but choosing not to merge something we cannot
> maintain and which we're not paid to look after and where there's a
> minimal interest, is not entirely unreasonable.
> 
> That being said, I'm not categorically against these patches, but I
> share Marc's view that we've already seen that non-vgic support had been
> broken for multiple versions without anyone complaining, and without
> automated testing or substantial interest in the work, the patches
> really are likely to bit-rot.

I know that it's very hard to grasp from an upstream maintainer
perspective, but keep in mind where the bulk of execution of kernel code
lies. The average life cycle of a "stable" Linux distribution's kernel
is a few years.

So far all regressions in the user space gic code have been found within
less than 1y of the respective code release. I'd say that counts for
quite a well used feature.

> But I haven't even looked at the patches in detail, I was just replying
> to the comment about testing.

Also keep in mind that without the architected timer support (and/or
without qemu patches than enable user space timers) the user space gic
support is pretty unusable to most people, so you obviously get less
reports.


Alex

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Christoffer Dall]

On Mon, Sep 19, 2016 at 12:51:46PM +0200, Alexander Graf wrote:
> 
> 
> On 16.09.16 15:30, Christoffer Dall wrote:

[...]

> > 
> > That being said, I'm not categorically against these patches, but I
> > share Marc's view that we've already seen that non-vgic support had been
> > broken for multiple versions without anyone complaining, and without
> > automated testing or substantial interest in the work, the patches
> > really are likely to bit-rot.
> 
> I know that it's very hard to grasp from an upstream maintainer
> perspective, 

pfff

> but keep in mind where the bulk of execution of kernel code
> lies. The average life cycle of a "stable" Linux distribution's kernel
> is a few years.
> 
> So far all regressions in the user space gic code have been found within
> less than 1y of the respective code release. I'd say that counts for
> quite a well used feature.
> 

The only report I can think of about this was Pavel using an upstream
kernel for in-house Samsung development on non-public hardware.

But, again, I didn't look at the patches in detail yet, I'm not
categorically against them, I will take a careful look at them like I do
with all patches on the kvmarm list.  There's a risk they'll break in
mainline unless we sort out our testing story, and it may just be
something we'll have to live with.

> > But I haven't even looked at the patches in detail, I was just replying
> > to the comment about testing.
> 
> Also keep in mind that without the architected timer support (and/or
> without qemu patches than enable user space timers) the user space gic
> support is pretty unusable to most people, so you obviously get less
> reports.
>

I don't disagree with this.  I don't know what this has to do with the
part of my mail you're replying to, but I completely agree that the
current userspace irqchip support has limited value.

-Christoffer

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

> On 16 Sep 2016, at 12:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
> Hi Alex,
> 
> On 16/09/16 07:26, Alexander Graf wrote:
>> Some systems out there (well, one type in particular - the Raspberry Pi series)
>> do have virtualization capabilities in the core, but no ARM GIC interrupt
>> controller.
>> 
>> To run on these systems, the cleanest route is to just handle all
>> interrupt delivery in user space and only deal with IRQ pins in the core
>> side in KVM.
>> 
>> This works pretty well already, but breaks when the guest starts to use
>> architected timers, as these are handled straight inside kernel space today.
>> 
>> This patch set allows user space to receive vtimer events as well as mask
>> them, so that we can handle all vtimer related interrupt injection from user
>> space, enabling us to use architected timer with user space gic emulation.
> 
> I have already voiced my concerns in the past, including face to face,
> and I'm going to repeat it: I not keen at all on adding a new userspace
> interface that is going to bitrot extremely quickly.
> 
> Let's face it, this new ABI will have a single user, with a limited
> shelf life. I understand that the RPi is a popular product, but it looks
> fairly obvious that this kind of sub-standard HW will eventually
> disappear. We'll then be left with a userspace ABI that will break at

I?m not 100% convinced that this is the case. Emulating the GIC in user space can have other interesting use cases. For example, it might come in handy for nesting. It may be useful for migrating a gicv2 VM to a gicv3 host without gicv2 emulation as well.

> every single release, given that nobody in the RPi community actually
> uses a mainline kernel.

I actually verified all of this patch on 4.8-rc5 upstream, which is the only 64bit kernel you can find for the RPi. So I?d expect the situation to change with 64bit.

> And breaking this ABI will introduce userspace exploitable bugs, like
> the one you've already shown. If anything, I would have loved to
> completely kill the whole userspace GIC, because nobody cares. Yes, I
> understand it is fun to have KVM running on the RPi. But the maintenance
> costs far outweigh the fun aspect already.

Having CPU pins accessible is very useful for use cases of KVM that are beyond your traditional VM.

> You could still run KVM with an external emulated timer (not the arch
> timer). No need for a new ABI for that.

That?s what I thought too, but turns out that it?s not quite as simple as I hoped ;). Also, I much rather at least have a common notion of ?arch timers are always available on arm64? than ?kvm always uses the vgic?. The former has much more impact and much more reach.


Alex

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Christoffer Dall]

On Fri, Sep 16, 2016 at 02:25:01PM +0200, Alexander Graf wrote:
> 
> > On 16 Sep 2016, at 12:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > 
> > Hi Alex,
> > 
> > On 16/09/16 07:26, Alexander Graf wrote:
> >> Some systems out there (well, one type in particular - the Raspberry Pi series)
> >> do have virtualization capabilities in the core, but no ARM GIC interrupt
> >> controller.
> >> 
> >> To run on these systems, the cleanest route is to just handle all
> >> interrupt delivery in user space and only deal with IRQ pins in the core
> >> side in KVM.
> >> 
> >> This works pretty well already, but breaks when the guest starts to use
> >> architected timers, as these are handled straight inside kernel space today.
> >> 
> >> This patch set allows user space to receive vtimer events as well as mask
> >> them, so that we can handle all vtimer related interrupt injection from user
> >> space, enabling us to use architected timer with user space gic emulation.
> > 
> > I have already voiced my concerns in the past, including face to face,
> > and I'm going to repeat it: I not keen at all on adding a new userspace
> > interface that is going to bitrot extremely quickly.
> > 
> > Let's face it, this new ABI will have a single user, with a limited
> > shelf life. I understand that the RPi is a popular product, but it looks
> > fairly obvious that this kind of sub-standard HW will eventually
> > disappear. We'll then be left with a userspace ABI that will break at
> 
> I?m not 100% convinced that this is the case. Emulating the GIC in user space can have other interesting use cases. For example, it might come in handy for nesting. It may be useful for migrating a gicv2 VM to a gicv3 host without gicv2 emulation as well.
> 

I don't see why you'd do this; the VGIC hardware can perfectly well be
used for nesting as well, and this works rather well.

> > every single release, given that nobody in the RPi community actually
> > uses a mainline kernel.
> 
> I actually verified all of this patch on 4.8-rc5 upstream, which is the only 64bit kernel you can find for the RPi. So I?d expect the situation to change with 64bit.
> 
> > And breaking this ABI will introduce userspace exploitable bugs, like
> > the one you've already shown. If anything, I would have loved to
> > completely kill the whole userspace GIC, because nobody cares. Yes, I
> > understand it is fun to have KVM running on the RPi. But the maintenance
> > costs far outweigh the fun aspect already.
> 
> Having CPU pins accessible is very useful for use cases of KVM that are beyond your traditional VM.
> 
> > You could still run KVM with an external emulated timer (not the arch
> > timer). No need for a new ABI for that.
> 
> That?s what I thought too, but turns out that it?s not quite as simple as I hoped ;).

Why not?  I thought a few people had this running recently.  What were
the issues?  Perhaps I can convince someone to submit the patches they
used if useful.

Thanks,
-Christoffer

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Paolo Bonzini]

On 16/09/2016 14:29, Christoffer Dall wrote:
> > It may be useful for migrating a gicv2 VM to a gicv3 host without gicv2 emulation as well.
> 
> I don't see why you'd do this; the VGIC hardware can perfectly well be
> used for nesting as well, and this works rather well.

Can GICv3 emulate GICv2 in a guest?

Paolo

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

> On 16 Sep 2016, at 14:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 16/09/2016 14:29, Christoffer Dall wrote:
>>> It may be useful for migrating a gicv2 VM to a gicv3 host without gicv2 emulation as well.
>> 
>> I don't see why you'd do this; the VGIC hardware can perfectly well be
>> used for nesting as well, and this works rather well.
> 
> Can GICv3 emulate GICv2 in a guest?

It depends on the gicv3 configuration. As an SOC vendor you can either enable gicv2 compatibility or disable it. ThunderX for example is gicv3 only. LS2085 can handle gicv2 in the guest with gicv3 on the host.


Alex

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Paolo Bonzini]

On 16/09/2016 14:44, Alexander Graf wrote:
> 
>> On 16 Sep 2016, at 14:40, Paolo Bonzini <pbonzini@redhat.com>
>> wrote:
>> 
>> 
>> 
>> On 16/09/2016 14:29, Christoffer Dall wrote:
>>>> It may be useful for migrating a gicv2 VM to a gicv3 host
>>>> without gicv2 emulation as well.
>>> 
>>> I don't see why you'd do this; the VGIC hardware can perfectly
>>> well be used for nesting as well, and this works rather well.
>> 
>> Can GICv3 emulate GICv2 in a guest?
> 
> It depends on the gicv3 configuration. As an SOC vendor you can
> either enable gicv2 compatibility or disable it. ThunderX for example
> is gicv3 only.

And QEMU complains on startup and exits, I hope.

I am not too optimistic about having migration from kernel GIC to
userspace GIC, honestly.  But GICv2 emulation on GICv3-only hosts is a
very reasonable thing to want, if only for testing/debugging purposes.

Paolo

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Ard Biesheuvel]

On 16 September 2016 at 13:44, Alexander Graf <agraf@suse.de> wrote:
>
>> On 16 Sep 2016, at 14:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>>
>> On 16/09/2016 14:29, Christoffer Dall wrote:
>>>> It may be useful for migrating a gicv2 VM to a gicv3 host without gicv2 emulation as well.
>>>
>>> I don't see why you'd do this; the VGIC hardware can perfectly well be
>>> used for nesting as well, and this works rather well.
>>
>> Can GICv3 emulate GICv2 in a guest?
>
> It depends on the gicv3 configuration. As an SOC vendor you can either enable gicv2 compatibility or disable it. ThunderX for example is gicv3 only. LS2085 can handle gicv2 in the guest with gicv3 on the host.
>

Note that 'disabled' here means 'not implemented it in silicon', so
there is no way you will ever be able to re-enable GICv2 compatibility
on a ThunderX. Another thing to keep in mind is that GICv2
compatibility is disabled on the non-secure side if the secure side
elects to configure its view of the GIC as v3 (i.e., in order to
support >8 cores)

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Peter Maydell]

On 17 September 2016 at 16:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Another thing to keep in mind is that GICv2
> compatibility is disabled on the non-secure side if the secure side
> elects to configure its view of the GIC as v3 (i.e., in order to
> support >8 cores)

If I'm reading the 'legacy configurations' chapter of the GICv3
spec correctly, that is true for the NS host OS (ie the one
handling physical interrupts) but a guest OS can still use
the old GICv2-compat interface (assuming it was implemented
in silicon at all).

thanks
-- PMM

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Ard Biesheuvel]

On 17 September 2016 at 16:38, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 September 2016 at 16:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Another thing to keep in mind is that GICv2
>> compatibility is disabled on the non-secure side if the secure side
>> elects to configure its view of the GIC as v3 (i.e., in order to
>> support >8 cores)
>
> If I'm reading the 'legacy configurations' chapter of the GICv3
> spec correctly, that is true for the NS host OS (ie the one
> handling physical interrupts) but a guest OS can still use
> the old GICv2-compat interface (assuming it was implemented
> in silicon at all).
>

Ah right, apologies for spreading misinformation. But my first point
is still valid.

[PATCH v3 0/2] KVM: ARM: Enable vtimers with user space gic

[Alexander Graf]

> On 16 Sep 2016, at 14:29, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> 
> On Fri, Sep 16, 2016 at 02:25:01PM +0200, Alexander Graf wrote:
>> 
>>> On 16 Sep 2016, at 12:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> 
>>> Hi Alex,
>>> 
>>> On 16/09/16 07:26, Alexander Graf wrote:
>>>> Some systems out there (well, one type in particular - the Raspberry Pi series)
>>>> do have virtualization capabilities in the core, but no ARM GIC interrupt
>>>> controller.
>>>> 
>>>> To run on these systems, the cleanest route is to just handle all
>>>> interrupt delivery in user space and only deal with IRQ pins in the core
>>>> side in KVM.
>>>> 
>>>> This works pretty well already, but breaks when the guest starts to use
>>>> architected timers, as these are handled straight inside kernel space today.
>>>> 
>>>> This patch set allows user space to receive vtimer events as well as mask
>>>> them, so that we can handle all vtimer related interrupt injection from user
>>>> space, enabling us to use architected timer with user space gic emulation.
>>> 
>>> I have already voiced my concerns in the past, including face to face,
>>> and I'm going to repeat it: I not keen at all on adding a new userspace
>>> interface that is going to bitrot extremely quickly.
>>> 
>>> Let's face it, this new ABI will have a single user, with a limited
>>> shelf life. I understand that the RPi is a popular product, but it looks
>>> fairly obvious that this kind of sub-standard HW will eventually
>>> disappear. We'll then be left with a userspace ABI that will break at
>> 
>> I?m not 100% convinced that this is the case. Emulating the GIC in user space can have other interesting use cases. For example, it might come in handy for nesting. It may be useful for migrating a gicv2 VM to a gicv3 host without gicv2 emulation as well.
>> 
> 
> I don't see why you'd do this; the VGIC hardware can perfectly well be
> used for nesting as well, and this works rather well.

Mostly because it?s more. I like my problems self-contained, and simulating only nesting on the CPU level is less to worry about than simulating vgic switchover as well. Of course eventually with nesting you?d want a nested vgic ;).

> 
>>> every single release, given that nobody in the RPi community actually
>>> uses a mainline kernel.
>> 
>> I actually verified all of this patch on 4.8-rc5 upstream, which is the only 64bit kernel you can find for the RPi. So I?d expect the situation to change with 64bit.
>> 
>>> And breaking this ABI will introduce userspace exploitable bugs, like
>>> the one you've already shown. If anything, I would have loved to
>>> completely kill the whole userspace GIC, because nobody cares. Yes, I
>>> understand it is fun to have KVM running on the RPi. But the maintenance
>>> costs far outweigh the fun aspect already.
>> 
>> Having CPU pins accessible is very useful for use cases of KVM that are beyond your traditional VM.
>> 
>>> You could still run KVM with an external emulated timer (not the arch
>>> timer). No need for a new ABI for that.
>> 
>> That?s what I thought too, but turns out that it?s not quite as simple as I hoped ;).
> 
> Why not?  I thought a few people had this running recently.  What were
> the issues?  Perhaps I can convince someone to submit the patches they
> used if useful.

Just give it a try - I didn?t get any timer events and couldn?t quite figure out why. Patch is attached below.


Alex

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a193b5a..f118ab8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -91,6 +91,7 @@ typedef struct {
     bool secure;
     bool highmem;
     int32_t gic_version;
+    bool archtimer;
 } VirtMachineState;

 #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
@@ -177,6 +178,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
+    [VIRT_SP804] =              { 0x09050000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -195,6 +197,7 @@ static const int a15irqmap[] = {
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_GPIO] = 7,
     [VIRT_SECURE_UART] = 8,
+    [VIRT_SP804] = 9,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
@@ -352,11 +355,13 @@ static void fdt_add_timer_nodes(const VirtBoardInfo *vbi, int gictype)
                                 "arm,armv7-timer");
     }
     qemu_fdt_setprop(vbi->fdt, "/timer", "always-on", NULL, 0);
+#if 0
     qemu_fdt_setprop_cells(vbi->fdt, "/timer", "interrupts",
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_S_EL1_IRQ, irqflags,
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_NS_EL1_IRQ, irqflags,
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_VIRT_IRQ, irqflags,
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_NS_EL2_IRQ, irqflags);
+#endif
 }

 static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
@@ -655,6 +660,29 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
     g_free(nodename);
 }

+static void create_sp804(const VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    char *nodename;
+    hwaddr base = vbi->memmap[VIRT_SP804].base;
+    hwaddr size = vbi->memmap[VIRT_SP804].size;
+    int irq = vbi->irqmap[VIRT_SP804];
+    const char compat[] = "arm,sp804\0arm,primecell";
+
+    sysbus_create_simple("sp804", base, pic[irq]);
+
+    nodename = g_strdup_printf("/sp804@%" PRIx64, base);
+    qemu_fdt_add_subnode(vbi->fdt, nodename);
+    qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                 2, base, 2, size);
+    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
+                           GIC_FDT_IRQ_TYPE_SPI, irq,
+                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
+    qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
+    g_free(nodename);
+}
+
 static DeviceState *gpio_key_dev;
 static void virt_powerdown_req(Notifier *n, void *opaque)
 {
@@ -1354,6 +1385,10 @@ static void machvirt_init(MachineState *machine)

     create_rtc(vbi, pic);

+    if (!vms->archtimer) {
+        create_sp804(vbi, pic);
+    }
+
     create_pcie(vbi, pic, vms->highmem);

     create_gpio(vbi, pic);
@@ -1448,6 +1483,20 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
     }
 }

+static bool virt_get_archtimer(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->archtimer;
+}
+
+static void virt_set_archtimer(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->archtimer = value;
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1510,6 +1559,15 @@ static void virt_2_7_instance_init(Object *obj)
     object_property_set_description(obj, "gic-version",
                                     "Set GIC version. "
                                     "Valid values are 2, 3 and host", NULL);
+
+    /* Architected Timers are available by default */
+    vms->archtimer = true;
+    object_property_add_bool(obj, "archtimer", virt_get_archtimer,
+                             virt_set_archtimer, NULL);
+    object_property_set_description(obj, "archtimer",
+                                    "Set on/off to enable/disable exposure "
+                                    "of architected timers to the guest",
+                                    NULL);
 }

 static void virt_machine_2_7_options(MachineClass *mc)
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 111a16d..b71db7e 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 9650193..510cdc0 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -67,6 +67,7 @@ enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_SP804,
 };

 typedef struct MemMapEntry {