[PATCH v4 0/5] KVM/ARM Architected Timers support

[PATCH v4 0/5] KVM/ARM Architected Timers support

[Christoffer Dall]

The following series implements support for the architected generic
timers for KVM/ARM.

This is an unmodified repost of the previously submitted series.

This patch series can also be pulled from:
    git://github.com/virtualopensystems/linux-kvm-arm.git
            branch: kvm-arm-v13-vgic-timers

---

Marc Zyngier (5):
      ARM: arch_timers: switch to physical timers if HYP mode is available
      ARM: KVM: arch_timers: Add minimal infrastructure
      ARM: KVM: arch_timers: Add guest timer core support
      ARM: KVM: arch_timers: Add timer world switch
      ARM: KVM: arch_timers: Wire the init code and config option


 arch/arm/include/asm/kvm_arch_timer.h |  100 ++++++++++++++++
 arch/arm/include/asm/kvm_host.h       |    5 +
 arch/arm/kernel/arch_timer.c          |    7 +
 arch/arm/kernel/asm-offsets.c         |    8 +
 arch/arm/kvm/Kconfig                  |    7 +
 arch/arm/kvm/Makefile                 |    1 
 arch/arm/kvm/arm.c                    |   14 ++
 arch/arm/kvm/interrupts.S             |    2 
 arch/arm/kvm/interrupts_head.S        |   60 ++++++++++
 arch/arm/kvm/reset.c                  |    9 +
 arch/arm/kvm/timer.c                  |  204 +++++++++++++++++++++++++++++++++
 arch/arm/kvm/vgic.c                   |    1 
 12 files changed, 417 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/kvm_arch_timer.h
 create mode 100644 arch/arm/kvm/timer.c

-- 

[PATCH v4 1/5] ARM: arch_timers: switch to physical timers if HYP mode is available

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

If we're booted in HYP mode, it is possible that we'll run some
kind of virtualized environment. In this case, it is a better to
switch to the physical timers, and leave the virtual timers to
guests.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kernel/arch_timer.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index c8ef207..8adcd04 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -26,6 +26,7 @@
 #include <asm/arch_timer.h>
 #include <asm/system_info.h>
 #include <asm/sched_clock.h>
+#include <asm/virt.h>
 
 static unsigned long arch_timer_rate;
 
@@ -489,10 +490,14 @@ int __init arch_timer_of_register(void)
 		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
 
 	/*
+	 * If HYP mode is available, we know that the physical timer
+	 * has been configured to be accessible from PL1. Use it, so
+	 * that a guest can use the virtual timer instead.
+	 *
 	 * If no interrupt provided for virtual timer, we'll have to
 	 * stick to the physical timer. It'd better be accessible...
 	 */
-	if (!arch_timer_ppi[VIRT_PPI]) {
+	if (is_hyp_mode_available() || !arch_timer_ppi[VIRT_PPI]) {
 		arch_timer_use_virtual = false;
 
 		if (!arch_timer_ppi[PHYS_SECURE_PPI] ||

[PATCH v4 2/5] ARM: KVM: arch_timers: Add minimal infrastructure

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

Add some very minimal architected timer related infrastructure.
For the moment, we just provide empty structures, and enable/disable
access to the physical timer across world switch.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
---
 arch/arm/include/asm/kvm_arch_timer.h |   45 +++++++++++++++++++++++++++++++++
 arch/arm/include/asm/kvm_host.h       |    5 ++++
 arch/arm/kvm/interrupts.S             |    2 +
 arch/arm/kvm/interrupts_head.S        |   19 ++++++++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 arch/arm/include/asm/kvm_arch_timer.h

diff --git a/arch/arm/include/asm/kvm_arch_timer.h b/arch/arm/include/asm/kvm_arch_timer.h
new file mode 100644
index 0000000..513b852
--- /dev/null
+++ b/arch/arm/include/asm/kvm_arch_timer.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2012 ARM Ltd.
+ * Author: Marc Zyngier <marc.zyngier@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef __ASM_ARM_KVM_ARCH_TIMER_H
+#define __ASM_ARM_KVM_ARCH_TIMER_H
+
+struct arch_timer_kvm {
+};
+
+struct arch_timer_cpu {
+};
+
+#ifndef CONFIG_KVM_ARM_TIMER
+static inline int kvm_timer_hyp_init(void)
+{
+	return 0;
+};
+
+static inline int kvm_timer_init(struct kvm *kvm)
+{
+	return 0;
+}
+
+static inline void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) {}
+static inline void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu) {}
+static inline void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu) {}
+static inline void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu) {}
+#endif
+
+#endif
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 49ba25a..41012ef 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -24,6 +24,7 @@
 #include <asm/fpstate.h>
 #include <asm/kvm_decode.h>
 #include <asm/kvm_vgic.h>
+#include <asm/kvm_arch_timer.h>
 
 #define KVM_MAX_VCPUS CONFIG_KVM_ARM_MAX_VCPUS
 #define KVM_MEMORY_SLOTS 32
@@ -48,6 +49,9 @@ struct kvm_arch {
 	/* VTTBR value associated with below pgd and vmid */
 	u64    vttbr;
 
+	/* Timer */
+	struct arch_timer_kvm	timer;
+
 	/*
 	 * Anything that is not used directly from assembly code goes
 	 * here.
@@ -98,6 +102,7 @@ struct kvm_vcpu_arch {
 
 	/* VGIC state */
 	struct vgic_cpu vgic_cpu;
+	struct arch_timer_cpu timer_cpu;
 
 	/*
 	 * Anything that is not used directly from assembly code goes
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index e418c9b..5a09e89 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -92,6 +92,7 @@ ENTRY(__kvm_vcpu_run)
 	save_host_regs
 
 	restore_vgic_state r0
+	restore_timer_state r0
 
 	@ Store hardware CP15 state and load guest state
 	read_cp15_state
@@ -186,6 +187,7 @@ after_vfp_restore:
 	read_cp15_state 1, r1
 	write_cp15_state
 
+	save_timer_state r1
 	save_vgic_state	r1
 
 	restore_host_regs
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index c2423d8..0003aab 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -418,6 +418,25 @@
 #endif
 .endm
 
+#define CNTHCTL_PL1PCTEN	(1 << 0)
+#define CNTHCTL_PL1PCEN		(1 << 1)
+
+.macro save_timer_state	vcpup
+	@ Allow physical timer/counter access for the host
+	mrc	p15, 4, r2, c14, c1, 0	@ CNTHCTL
+	orr	r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)
+	mcr	p15, 4, r2, c14, c1, 0	@ CNTHCTL
+.endm
+
+.macro restore_timer_state vcpup
+	@ Disallow physical timer access for the guest
+	@ Physical counter access is allowed
+	mrc	p15, 4, r2, c14, c1, 0	@ CNTHCTL
+	orr	r2, r2, #CNTHCTL_PL1PCTEN
+	bic	r2, r2, #CNTHCTL_PL1PCEN
+	mcr	p15, 4, r2, c14, c1, 0	@ CNTHCTL
+.endm
+
 /* Configures the HSTR (Hyp System Trap Register) on entry/return
  * (hardware reset value is 0) */
 .macro set_hstr entry

[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

We can inject a timer interrupt into the guest as a result of
three possible events:
- The virtual timer interrupt has fired while we were still
  executing the guest
- The timer interrupt hasn't fired, but it expired while we
  were doing the world switch
- A hrtimer we programmed earlier has fired

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
---
 arch/arm/include/asm/kvm_arch_timer.h |   57 +++++++++
 arch/arm/kvm/reset.c                  |    9 +
 arch/arm/kvm/timer.c                  |  204 +++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/kvm/timer.c

diff --git a/arch/arm/include/asm/kvm_arch_timer.h b/arch/arm/include/asm/kvm_arch_timer.h
index 513b852..bd5e501 100644
--- a/arch/arm/include/asm/kvm_arch_timer.h
+++ b/arch/arm/include/asm/kvm_arch_timer.h
@@ -19,13 +19,68 @@
 #ifndef __ASM_ARM_KVM_ARCH_TIMER_H
 #define __ASM_ARM_KVM_ARCH_TIMER_H
 
+#include <linux/clocksource.h>
+#include <linux/hrtimer.h>
+#include <linux/workqueue.h>
+
 struct arch_timer_kvm {
+#ifdef CONFIG_KVM_ARM_TIMER
+	/* Is the timer enabled */
+	bool			enabled;
+
+	/*
+	 * Virtual offset (kernel access it through cntvoff, HYP code
+	 * access it as two 32bit values).
+	 */
+	union {
+		cycle_t		cntvoff;
+		struct {
+			u32	low; 	/* Restored only */
+			u32	high;  	/* Restored only */
+		} cntvoff32;
+	};
+#endif
 };
 
 struct arch_timer_cpu {
+#ifdef CONFIG_KVM_ARM_TIMER
+	/* Registers: control register, timer value */
+	u32				cntv_ctl;	/* Saved/restored */
+	union {
+		cycle_t			cntv_cval;
+		struct {
+			u32		low;		/* Saved/restored */
+			u32		high;		/* Saved/restored */
+		} cntv_cval32;
+	};
+
+	/*
+	 * Anything that is not used directly from assembly code goes
+	 * here.
+	 */
+
+	/* Background timer used when the guest is not running */
+	struct hrtimer			timer;
+
+	/* Work queued with the above timer expires */
+	struct work_struct		expired;
+
+	/* Background timer active */
+	bool				armed;
+
+	/* Timer IRQ */
+	const struct kvm_irq_level	*irq;
+#endif
 };
 
-#ifndef CONFIG_KVM_ARM_TIMER
+#ifdef CONFIG_KVM_ARM_TIMER
+int kvm_timer_hyp_init(void);
+int kvm_timer_init(struct kvm *kvm);
+void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
+void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu);
+void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu);
+void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
+#else
 static inline int kvm_timer_hyp_init(void)
 {
 	return 0;
diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index b80256b..7463f5b 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
 	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
 };
 
+#ifdef CONFIG_KVM_ARM_TIMER
+static const struct kvm_irq_level a15_virt_timer_ppi = {
+	{ .irq = 27 },	/* irq: A7/A15 specific */
+	.level = 1	/* level */
+};
+#endif
 
 /*******************************************************************************
  * Exported reset function
@@ -59,6 +65,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 			return -EINVAL;
 		cpu_reset = &a15_regs_reset;
 		vcpu->arch.midr = read_cpuid_id();
+#ifdef CONFIG_KVM_ARM_TIMER
+		vcpu->arch.timer_cpu.irq = &a15_virt_timer_ppi;
+#endif
 		break;
 	default:
 		return -ENODEV;
diff --git a/arch/arm/kvm/timer.c b/arch/arm/kvm/timer.c
new file mode 100644
index 0000000..a241298
--- /dev/null
+++ b/arch/arm/kvm/timer.c
@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2012 ARM Ltd.
+ * Author: Marc Zyngier <marc.zyngier@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/of_irq.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <linux/interrupt.h>
+
+#include <asm/arch_timer.h>
+
+#include <asm/kvm_vgic.h>
+#include <asm/kvm_arch_timer.h>
+
+static struct timecounter *timecounter;
+static struct workqueue_struct *wqueue;
+
+static cycle_t kvm_phys_timer_read(void)
+{
+	return timecounter->cc->read(timecounter->cc);
+}
+
+static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	timer->cntv_ctl |= 1 << 1; /* Mask the interrupt in the guest */
+	kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
+			    vcpu->arch.timer_cpu.irq->irq,
+			    vcpu->arch.timer_cpu.irq->level);
+}
+
+static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
+{
+	struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
+
+	/*
+	 * We disable the timer in the world switch and let it be
+	 * handled by kvm_timer_sync_from_cpu(). Getting a timer
+	 * interrupt at this point is a sure sign of some major
+	 * breakage.
+	 */
+	pr_warn("Unexpected interrupt %d on vcpu %p\n", irq, vcpu);
+	return IRQ_HANDLED;
+}
+
+static void kvm_timer_inject_irq_work(struct work_struct *work)
+{
+	struct kvm_vcpu *vcpu;
+
+	vcpu = container_of(work, struct kvm_vcpu, arch.timer_cpu.expired);
+	vcpu->arch.timer_cpu.armed = false;
+	kvm_timer_inject_irq(vcpu);
+}
+
+static enum hrtimer_restart kvm_timer_expire(struct hrtimer *hrt)
+{
+	struct arch_timer_cpu *timer;
+	timer = container_of(hrt, struct arch_timer_cpu, timer);
+	queue_work(wqueue, &timer->expired);
+	return HRTIMER_NORESTART;
+}
+
+void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	/*
+	 * We're about to run this vcpu again, so there is no need to
+	 * keep the background timer running, as we're about to
+	 * populate the CPU timer again.
+	 */
+	if (timer->armed) {
+		hrtimer_cancel(&timer->timer);
+		cancel_work_sync(&timer->expired);
+		timer->armed = false;
+	}
+}
+
+void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	cycle_t cval, now;
+	u64 ns;
+
+	/* Check if the timer is enabled and unmasked first */
+	if ((timer->cntv_ctl & 3) != 1)
+		return;
+
+	cval = timer->cntv_cval;
+	now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
+
+	BUG_ON(timer->armed);
+
+	if (cval <= now) {
+		/*
+		 * Timer has already expired while we were not
+		 * looking. Inject the interrupt and carry on.
+		 */
+		kvm_timer_inject_irq(vcpu);
+		return;
+	}
+
+	timer->armed = true;
+	ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
+	hrtimer_start(&timer->timer, ktime_add_ns(ktime_get(), ns),
+		      HRTIMER_MODE_ABS);
+}
+
+void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
+	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+	timer->timer.function = kvm_timer_expire;
+}
+
+static void kvm_timer_init_interrupt(void *info)
+{
+	unsigned int *irqp = info;
+
+	enable_percpu_irq(*irqp, 0);
+}
+
+
+static const struct of_device_id arch_timer_of_match[] = {
+	{ .compatible	= "arm,armv7-timer",	},
+	{},
+};
+
+int kvm_timer_hyp_init(void)
+{
+	struct device_node *np;
+	unsigned int ppi;
+	int err;
+
+	timecounter = arch_timer_get_timecounter();
+	if (!timecounter)
+		return -ENODEV;
+
+	np = of_find_matching_node(NULL, arch_timer_of_match);
+	if (!np) {
+		kvm_err("kvm_arch_timer: can't find DT node\n");
+		return -ENODEV;
+	}
+
+	ppi = irq_of_parse_and_map(np, 2);
+	if (!ppi) {
+		kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
+		return -EINVAL;
+	}
+
+	err = request_percpu_irq(ppi, kvm_arch_timer_handler,
+				 "kvm guest timer", kvm_get_running_vcpus());
+	if (err) {
+		kvm_err("kvm_arch_timer: can't request interrupt %d (%d)\n",
+			ppi, err);
+		return err;
+	}
+
+	wqueue = create_singlethread_workqueue("kvm_arch_timer");
+	if (!wqueue) {
+		free_percpu_irq(ppi, kvm_get_running_vcpus());
+		return -ENOMEM;
+	}
+
+	kvm_info("%s IRQ%d\n", np->name, ppi);
+	on_each_cpu(kvm_timer_init_interrupt, &ppi, 1);
+
+	return 0;
+}
+
+void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	hrtimer_cancel(&timer->timer);
+	cancel_work_sync(&timer->expired);
+}
+
+int kvm_timer_init(struct kvm *kvm)
+{
+	if (timecounter && wqueue) {
+		kvm->arch.timer.cntvoff = kvm_phys_timer_read();
+		kvm->arch.timer.enabled = 1;
+	}
+
+	return 0;
+}

[PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

Do the necessary save/restore dance for the timers in the world
switch code. In the process, allow the guest to read the physical
counter, which is useful for its own clock_event_device.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
---
 arch/arm/kernel/asm-offsets.c  |    8 ++++++++
 arch/arm/kvm/arm.c             |    3 +++
 arch/arm/kvm/interrupts_head.S |   41 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 39b6221..50df318 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -177,6 +177,14 @@ int main(void)
   DEFINE(VGIC_CPU_APR,		offsetof(struct vgic_cpu, vgic_apr));
   DEFINE(VGIC_CPU_LR,		offsetof(struct vgic_cpu, vgic_lr));
   DEFINE(VGIC_CPU_NR_LR,	offsetof(struct vgic_cpu, nr_lr));
+#ifdef CONFIG_KVM_ARM_TIMER
+  DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
+  DEFINE(VCPU_TIMER_CNTV_CVALH,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high));
+  DEFINE(VCPU_TIMER_CNTV_CVALL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low));
+  DEFINE(KVM_TIMER_CNTVOFF_H,	offsetof(struct kvm, arch.timer.cntvoff32.high));
+  DEFINE(KVM_TIMER_CNTVOFF_L,	offsetof(struct kvm, arch.timer.cntvoff32.low));
+  DEFINE(KVM_TIMER_ENABLED,	offsetof(struct kvm, arch.timer.enabled));
+#endif
   DEFINE(KVM_VGIC_VCTRL,	offsetof(struct kvm, arch.vgic.vctrl_base));
 #endif
   DEFINE(KVM_VTTBR,		offsetof(struct kvm, arch.vttbr));
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 1716f12..8cdef69 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		update_vttbr(vcpu->kvm);
 
 		kvm_vgic_sync_to_cpu(vcpu);
+		kvm_timer_sync_to_cpu(vcpu);
 
 		local_irq_disable();
 
@@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
 			local_irq_enable();
+			kvm_timer_sync_from_cpu(vcpu);
 			kvm_vgic_sync_from_cpu(vcpu);
 			continue;
 		}
@@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		 * Back from guest
 		 *************************************************************/
 
+		kvm_timer_sync_from_cpu(vcpu);
 		kvm_vgic_sync_from_cpu(vcpu);
 
 		ret = handle_exit(vcpu, run, ret);
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 0003aab..ece84d1 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -422,6 +422,25 @@
 #define CNTHCTL_PL1PCEN		(1 << 1)
 
 .macro save_timer_state	vcpup
+#ifdef CONFIG_KVM_ARM_TIMER
+	ldr	r4, [\vcpup, #VCPU_KVM]
+	ldr	r2, [r4, #KVM_TIMER_ENABLED]
+	cmp	r2, #0
+	beq	1f
+
+	mrc	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+	and	r2, #3
+	str	r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
+	bic	r2, #1			@ Clear ENABLE
+	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+	isb
+
+	mrrc	p15, 3, r2, r3, c14	@ CNTV_CVAL
+	str	r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
+	str	r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
+
+1:
+#endif
 	@ Allow physical timer/counter access for the host
 	mrc	p15, 4, r2, c14, c1, 0	@ CNTHCTL
 	orr	r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)
@@ -435,6 +454,28 @@
 	orr	r2, r2, #CNTHCTL_PL1PCTEN
 	bic	r2, r2, #CNTHCTL_PL1PCEN
 	mcr	p15, 4, r2, c14, c1, 0	@ CNTHCTL
+
+#ifdef CONFIG_KVM_ARM_TIMER
+	ldr	r4, [\vcpup, #VCPU_KVM]
+	ldr	r2, [r4, #KVM_TIMER_ENABLED]
+	cmp	r2, #0
+	beq	1f
+
+	ldr	r3, [r4, #KVM_TIMER_CNTVOFF_H]
+	ldr	r2, [r4, #KVM_TIMER_CNTVOFF_L]
+	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
+	isb
+
+	ldr	r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
+	ldr	r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
+	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
+
+	ldr	r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
+	and	r2, #3
+	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+	isb
+1:
+#endif
 .endm
 
 /* Configures the HSTR (Hyp System Trap Register) on entry/return

[PATCH v4 5/5] ARM: KVM: arch_timers: Wire the init code and config option

[Christoffer Dall]

From: Marc Zyngier <marc.zyngier@arm.com>

It is now possible to select CONFIG_KVM_ARM_TIMER to enable the
KVM architected timer support.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
---
 arch/arm/kvm/Kconfig  |    7 +++++++
 arch/arm/kvm/Makefile |    1 +
 arch/arm/kvm/arm.c    |   11 +++++++++++
 arch/arm/kvm/vgic.c   |    1 +
 4 files changed, 20 insertions(+)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 3c979ce..eaecb9f 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -58,6 +58,13 @@ config KVM_ARM_VGIC
 	---help---
 	  Adds support for a hardware assisted, in-kernel GIC emulation.
 
+config KVM_ARM_TIMER
+        bool "KVM support for Architected Timers"
+	depends on KVM_ARM_VGIC && ARM_ARCH_TIMER
+	select HAVE_KVM_IRQCHIP
+	---help---
+	  Adds support for the Architected Timers in virtual machines
+
 source drivers/virtio/Kconfig
 
 endif # VIRTUALIZATION
diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index c019f02..7c5a635 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += $(addprefix ../../../virt/kvm/, kvm_main.o coalesc
 obj-$(CONFIG_KVM_ARM_HOST) += arm.o guest.o mmu.o emulate.o reset.o
 obj-$(CONFIG_KVM_ARM_HOST) += coproc.o coproc_a15.o mmio.o decode.o
 obj-$(CONFIG_KVM_ARM_VGIC) += vgic.o
+obj-$(CONFIG_KVM_ARM_TIMER) += timer.o
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 8cdef69..e62ba49 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -286,6 +286,7 @@ out:
 void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
 {
 	kvm_mmu_free_memory_caches(vcpu);
+	kvm_timer_vcpu_terminate(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vcpu);
 }
 
@@ -323,6 +324,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 	/* Set up VGIC */
 	kvm_vgic_vcpu_init(vcpu);
 
+	/* Set up the timer */
+	kvm_timer_vcpu_init(vcpu);
+
 	return 0;
 }
 
@@ -1048,6 +1052,13 @@ static int init_hyp_mode(void)
 	if (!err)
 		vgic_present = true;
 
+	/*
+	 * Init HYP architected timer support
+	 */
+	err = kvm_timer_hyp_init();
+	if (err)
+		goto out_free_mappings;
+
 	return 0;
 out_free_vfp:
 	free_percpu(kvm_host_vfp_state);
diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index 146de1d..090ea79 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -1168,6 +1168,7 @@ int kvm_vgic_init(struct kvm *kvm)
 	for (i = 32; i < VGIC_NR_IRQS; i += 4)
 		vgic_set_target_reg(kvm, 0, i);
 
+	kvm_timer_init(kvm);
 	kvm->arch.vgic.ready = true;
 out:
 	mutex_unlock(&kvm->lock);

[PATCH v4 1/5] ARM: arch_timers: switch to physical timers if HYP mode is available

[Will Deacon]

On Sat, Nov 10, 2012 at 03:46:05PM +0000, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> If we're booted in HYP mode, it is possible that we'll run some
> kind of virtualized environment. In this case, it is a better to
> switch to the physical timers, and leave the virtual timers to
> guests.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Seems simple enough.

Reviewed-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH v4 2/5] ARM: KVM: arch_timers: Add minimal infrastructure

[Will Deacon]

On Sat, Nov 10, 2012 at 03:46:12PM +0000, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> Add some very minimal architected timer related infrastructure.
> For the moment, we just provide empty structures, and enable/disable
> access to the physical timer across world switch.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>

Is it even worth having this patch? It doesn't seem to do much other than
stub everything out, so I'd be inclined to fold it in with the next one.

Will

[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

[Will Deacon]

On Sat, Nov 10, 2012 at 03:46:19PM +0000, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> We can inject a timer interrupt into the guest as a result of
> three possible events:
> - The virtual timer interrupt has fired while we were still
>   executing the guest
> - The timer interrupt hasn't fired, but it expired while we
>   were doing the world switch
> - A hrtimer we programmed earlier has fired
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
> ---
>  arch/arm/include/asm/kvm_arch_timer.h |   57 +++++++++
>  arch/arm/kvm/reset.c                  |    9 +
>  arch/arm/kvm/timer.c                  |  204 +++++++++++++++++++++++++++++++++
>  3 files changed, 269 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/kvm/timer.c
> 
> diff --git a/arch/arm/include/asm/kvm_arch_timer.h b/arch/arm/include/asm/kvm_arch_timer.h
> index 513b852..bd5e501 100644
> --- a/arch/arm/include/asm/kvm_arch_timer.h
> +++ b/arch/arm/include/asm/kvm_arch_timer.h
> @@ -19,13 +19,68 @@
>  #ifndef __ASM_ARM_KVM_ARCH_TIMER_H
>  #define __ASM_ARM_KVM_ARCH_TIMER_H
> 
> +#include <linux/clocksource.h>
> +#include <linux/hrtimer.h>
> +#include <linux/workqueue.h>
> +
>  struct arch_timer_kvm {
> +#ifdef CONFIG_KVM_ARM_TIMER
> +       /* Is the timer enabled */
> +       bool                    enabled;
> +
> +       /*
> +        * Virtual offset (kernel access it through cntvoff, HYP code
> +        * access it as two 32bit values).
> +        */
> +       union {
> +               cycle_t         cntvoff;
> +               struct {
> +                       u32     low;    /* Restored only */
> +                       u32     high;   /* Restored only */
> +               } cntvoff32;
> +       };
> +#endif

Endianness?

>  };
> 
>  struct arch_timer_cpu {
> +#ifdef CONFIG_KVM_ARM_TIMER
> +       /* Registers: control register, timer value */
> +       u32                             cntv_ctl;       /* Saved/restored */
> +       union {
> +               cycle_t                 cntv_cval;
> +               struct {
> +                       u32             low;            /* Saved/restored */
> +                       u32             high;           /* Saved/restored */
> +               } cntv_cval32;
> +       };

Similarly.

> +       /*
> +        * Anything that is not used directly from assembly code goes
> +        * here.
> +        */
> +
> +       /* Background timer used when the guest is not running */
> +       struct hrtimer                  timer;
> +
> +       /* Work queued with the above timer expires */
> +       struct work_struct              expired;
> +
> +       /* Background timer active */
> +       bool                            armed;
> +
> +       /* Timer IRQ */
> +       const struct kvm_irq_level      *irq;
> +#endif
>  };
> 
> -#ifndef CONFIG_KVM_ARM_TIMER
> +#ifdef CONFIG_KVM_ARM_TIMER
> +int kvm_timer_hyp_init(void);
> +int kvm_timer_init(struct kvm *kvm);
> +void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
> +void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu);
> +void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu);
> +void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
> +#else
>  static inline int kvm_timer_hyp_init(void)
>  {
>         return 0;
> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> index b80256b..7463f5b 100644
> --- a/arch/arm/kvm/reset.c
> +++ b/arch/arm/kvm/reset.c
> @@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
>         .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>  };
> 
> +#ifdef CONFIG_KVM_ARM_TIMER
> +static const struct kvm_irq_level a15_virt_timer_ppi = {
> +       { .irq = 27 },  /* irq: A7/A15 specific */

This should be parameterised by the vCPU type.

> +       .level = 1      /* level */
> +};
> +#endif
> 
>  /*******************************************************************************
>   * Exported reset function
> @@ -59,6 +65,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>                         return -EINVAL;
>                 cpu_reset = &a15_regs_reset;
>                 vcpu->arch.midr = read_cpuid_id();
> +#ifdef CONFIG_KVM_ARM_TIMER
> +               vcpu->arch.timer_cpu.irq = &a15_virt_timer_ppi;
> +#endif
>                 break;
>         default:
>                 return -ENODEV;
> diff --git a/arch/arm/kvm/timer.c b/arch/arm/kvm/timer.c
> new file mode 100644
> index 0000000..a241298
> --- /dev/null
> +++ b/arch/arm/kvm/timer.c

Maybe kvm/arch_timer.c since it seems to be specific to that device and
matches the naming for the main driver under kernel/? If we get new virtual
timer devices in the future, I guess this would need to be split up so the
arch-timer-specific bits are separate from generic hrtimer and kvm code.

> @@ -0,0 +1,204 @@
> +/*
> + * Copyright (C) 2012 ARM Ltd.
> + * Author: Marc Zyngier <marc.zyngier@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <linux/of_irq.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <linux/interrupt.h>
> +
> +#include <asm/arch_timer.h>
> +
> +#include <asm/kvm_vgic.h>
> +#include <asm/kvm_arch_timer.h>
> +
> +static struct timecounter *timecounter;
> +static struct workqueue_struct *wqueue;
> +
> +static cycle_t kvm_phys_timer_read(void)
> +{
> +       return timecounter->cc->read(timecounter->cc);
> +}
> +
> +static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu)
> +{
> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +       timer->cntv_ctl |= 1 << 1; /* Mask the interrupt in the guest */

This is ARCH_TIMER_CTRL_IT_MASK right? We should make that definition
available if it is needed here.

> +       kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
> +                           vcpu->arch.timer_cpu.irq->irq,
> +                           vcpu->arch.timer_cpu.irq->level);
> +}
> +
> +static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> +{
> +       struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
> +
> +       /*
> +        * We disable the timer in the world switch and let it be
> +        * handled by kvm_timer_sync_from_cpu(). Getting a timer
> +        * interrupt at this point is a sure sign of some major
> +        * breakage.
> +        */
> +       pr_warn("Unexpected interrupt %d on vcpu %p\n", irq, vcpu);
> +       return IRQ_HANDLED;

IRQ_NONE?

> +}
> +
> +static void kvm_timer_inject_irq_work(struct work_struct *work)
> +{
> +       struct kvm_vcpu *vcpu;
> +
> +       vcpu = container_of(work, struct kvm_vcpu, arch.timer_cpu.expired);
> +       vcpu->arch.timer_cpu.armed = false;
> +       kvm_timer_inject_irq(vcpu);
> +}
> +
> +static enum hrtimer_restart kvm_timer_expire(struct hrtimer *hrt)
> +{
> +       struct arch_timer_cpu *timer;
> +       timer = container_of(hrt, struct arch_timer_cpu, timer);
> +       queue_work(wqueue, &timer->expired);
> +       return HRTIMER_NORESTART;
> +}
> +
> +void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu)
> +{
> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +       /*
> +        * We're about to run this vcpu again, so there is no need to
> +        * keep the background timer running, as we're about to
> +        * populate the CPU timer again.
> +        */
> +       if (timer->armed) {
> +               hrtimer_cancel(&timer->timer);
> +               cancel_work_sync(&timer->expired);
> +               timer->armed = false;
> +       }
> +}

I think some helper functions like timer_is_armed, timer_arm and
timer_disarm would make this more readable (resisting arm_timer, which
confuses things more!).

> +
> +void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu)
> +{
> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +       cycle_t cval, now;
> +       u64 ns;
> +
> +       /* Check if the timer is enabled and unmasked first */
> +       if ((timer->cntv_ctl & 3) != 1)
> +               return;

Again, we should create/use ARCH_TIMER #defines for this hardware bits.

> +       cval = timer->cntv_cval;
> +       now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;

We probably want to add an isb() *before* the mrrc in
arch_timer_counter_read if you want to do things like this, because the
counter can be speculated in advance.

> +       BUG_ON(timer->armed);
> +
> +       if (cval <= now) {
> +               /*
> +                * Timer has already expired while we were not
> +                * looking. Inject the interrupt and carry on.
> +                */
> +               kvm_timer_inject_irq(vcpu);
> +               return;
> +       }

Does this buy you much? You still have to cope with the timer expiring here
anyway.

> +       timer->armed = true;
> +       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
> +       hrtimer_start(&timer->timer, ktime_add_ns(ktime_get(), ns),
> +                     HRTIMER_MODE_ABS);
> +}
> +
> +void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
> +{
> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +       INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
> +       hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> +       timer->timer.function = kvm_timer_expire;
> +}
> +
> +static void kvm_timer_init_interrupt(void *info)
> +{
> +       unsigned int *irqp = info;
> +
> +       enable_percpu_irq(*irqp, 0);

IRQ_TYPE_NONE

> +}
> +
> +
> +static const struct of_device_id arch_timer_of_match[] = {
> +       { .compatible   = "arm,armv7-timer",    },
> +       {},
> +};
> +
> +int kvm_timer_hyp_init(void)
> +{
> +       struct device_node *np;
> +       unsigned int ppi;
> +       int err;
> +
> +       timecounter = arch_timer_get_timecounter();
> +       if (!timecounter)
> +               return -ENODEV;
> +
> +       np = of_find_matching_node(NULL, arch_timer_of_match);
> +       if (!np) {
> +               kvm_err("kvm_arch_timer: can't find DT node\n");
> +               return -ENODEV;
> +       }
> +
> +       ppi = irq_of_parse_and_map(np, 2);
> +       if (!ppi) {
> +               kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
> +               return -EINVAL;
> +       }
> +
> +       err = request_percpu_irq(ppi, kvm_arch_timer_handler,
> +                                "kvm guest timer", kvm_get_running_vcpus());
> +       if (err) {
> +               kvm_err("kvm_arch_timer: can't request interrupt %d (%d)\n",
> +                       ppi, err);
> +               return err;
> +       }
> +
> +       wqueue = create_singlethread_workqueue("kvm_arch_timer");
> +       if (!wqueue) {
> +               free_percpu_irq(ppi, kvm_get_running_vcpus());
> +               return -ENOMEM;
> +       }
> +
> +       kvm_info("%s IRQ%d\n", np->name, ppi);
> +       on_each_cpu(kvm_timer_init_interrupt, &ppi, 1);

on_each_cpu currently just returns 0, but you could use that as your return
value for good measure anyway.

> +       return 0;
> +}
> +
> +void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
> +{
> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +
> +       hrtimer_cancel(&timer->timer);
> +       cancel_work_sync(&timer->expired);
> +}
> +
> +int kvm_timer_init(struct kvm *kvm)
> +{
> +       if (timecounter && wqueue) {
> +               kvm->arch.timer.cntvoff = kvm_phys_timer_read();

Shouldn't this be initialised to 0 and then updated on world switch?

Will

[PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch

[Will Deacon]

On Sat, Nov 10, 2012 at 03:46:25PM +0000, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> Do the necessary save/restore dance for the timers in the world
> switch code. In the process, allow the guest to read the physical
> counter, which is useful for its own clock_event_device.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
> ---
>  arch/arm/kernel/asm-offsets.c  |    8 ++++++++
>  arch/arm/kvm/arm.c             |    3 +++
>  arch/arm/kvm/interrupts_head.S |   41 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index 39b6221..50df318 100644
> --- a/arch/arm/kernel/asm-offsets.c
> +++ b/arch/arm/kernel/asm-offsets.c
> @@ -177,6 +177,14 @@ int main(void)
>    DEFINE(VGIC_CPU_APR,		offsetof(struct vgic_cpu, vgic_apr));
>    DEFINE(VGIC_CPU_LR,		offsetof(struct vgic_cpu, vgic_lr));
>    DEFINE(VGIC_CPU_NR_LR,	offsetof(struct vgic_cpu, nr_lr));
> +#ifdef CONFIG_KVM_ARM_TIMER
> +  DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
> +  DEFINE(VCPU_TIMER_CNTV_CVALH,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high));
> +  DEFINE(VCPU_TIMER_CNTV_CVALL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low));
> +  DEFINE(KVM_TIMER_CNTVOFF_H,	offsetof(struct kvm, arch.timer.cntvoff32.high));
> +  DEFINE(KVM_TIMER_CNTVOFF_L,	offsetof(struct kvm, arch.timer.cntvoff32.low));
> +  DEFINE(KVM_TIMER_ENABLED,	offsetof(struct kvm, arch.timer.enabled));
> +#endif
>    DEFINE(KVM_VGIC_VCTRL,	offsetof(struct kvm, arch.vgic.vctrl_base));
>  #endif
>    DEFINE(KVM_VTTBR,		offsetof(struct kvm, arch.vttbr));
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 1716f12..8cdef69 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		update_vttbr(vcpu->kvm);
>  
>  		kvm_vgic_sync_to_cpu(vcpu);
> +		kvm_timer_sync_to_cpu(vcpu);
>  
>  		local_irq_disable();
>  
> @@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>  			local_irq_enable();
> +			kvm_timer_sync_from_cpu(vcpu);
>  			kvm_vgic_sync_from_cpu(vcpu);
>  			continue;
>  		}
> @@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		 * Back from guest
>  		 *************************************************************/
>  
> +		kvm_timer_sync_from_cpu(vcpu);
>  		kvm_vgic_sync_from_cpu(vcpu);
>  
>  		ret = handle_exit(vcpu, run, ret);
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 0003aab..ece84d1 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -422,6 +422,25 @@
>  #define CNTHCTL_PL1PCEN		(1 << 1)
>  
>  .macro save_timer_state	vcpup
> +#ifdef CONFIG_KVM_ARM_TIMER
> +	ldr	r4, [\vcpup, #VCPU_KVM]
> +	ldr	r2, [r4, #KVM_TIMER_ENABLED]
> +	cmp	r2, #0
> +	beq	1f
> +
> +	mrc	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> +	and	r2, #3

Why do you need this and (you do the masking on the restore path)?

> +	str	r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
> +	bic	r2, #1			@ Clear ENABLE
> +	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> +	isb
> +
> +	mrrc	p15, 3, r2, r3, c14	@ CNTV_CVAL
> +	str	r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
> +	str	r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
> +
> +1:
> +#endif
>  	@ Allow physical timer/counter access for the host
>  	mrc	p15, 4, r2, c14, c1, 0	@ CNTHCTL
>  	orr	r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)
> @@ -435,6 +454,28 @@
>  	orr	r2, r2, #CNTHCTL_PL1PCTEN
>  	bic	r2, r2, #CNTHCTL_PL1PCEN
>  	mcr	p15, 4, r2, c14, c1, 0	@ CNTHCTL
> +
> +#ifdef CONFIG_KVM_ARM_TIMER
> +	ldr	r4, [\vcpup, #VCPU_KVM]
> +	ldr	r2, [r4, #KVM_TIMER_ENABLED]
> +	cmp	r2, #0
> +	beq	1f
> +
> +	ldr	r3, [r4, #KVM_TIMER_CNTVOFF_H]
> +	ldr	r2, [r4, #KVM_TIMER_CNTVOFF_L]
> +	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
> +	isb
> +
> +	ldr	r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
> +	ldr	r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
> +	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
> +
> +	ldr	r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
> +	and	r2, #3

Please use the long form syntax for these instructions (and r2, r2, #3) as
toolchains have historically been picky about accepting these when not
targetting Thumb and it also makes it consistent with the rest of arch/arm/.

> +	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> +	isb
> +1:
> +#endif

It's pretty horrible to take a macro argument (vcpup) and then open-code all
the other register numbers without moving the argument somewhere nice. Are
callers just expected to understand that you clobber {r2,r3,r4} and avoid
them? If so, why not just use the PCS so that people don't have to learn a
new convention?

Will

[PATCH v4 5/5] ARM: KVM: arch_timers: Wire the init code and config option

[Will Deacon]

On Sat, Nov 10, 2012 at 03:46:32PM +0000, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> It is now possible to select CONFIG_KVM_ARM_TIMER to enable the
> KVM architected timer support.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
> ---
>  arch/arm/kvm/Kconfig  |    7 +++++++
>  arch/arm/kvm/Makefile |    1 +
>  arch/arm/kvm/arm.c    |   11 +++++++++++
>  arch/arm/kvm/vgic.c   |    1 +
>  4 files changed, 20 insertions(+)
> 
> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
> index 3c979ce..eaecb9f 100644
> --- a/arch/arm/kvm/Kconfig
> +++ b/arch/arm/kvm/Kconfig
> @@ -58,6 +58,13 @@ config KVM_ARM_VGIC
>  	---help---
>  	  Adds support for a hardware assisted, in-kernel GIC emulation.
>  
> +config KVM_ARM_TIMER
> +        bool "KVM support for Architected Timers"
> +	depends on KVM_ARM_VGIC && ARM_ARCH_TIMER
> +	select HAVE_KVM_IRQCHIP
> +	---help---
> +	  Adds support for the Architected Timers in virtual machines
> +

This should probably be default y

Will

[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

[Marc Zyngier]

On 23/11/12 16:17, Will Deacon wrote:
> On Sat, Nov 10, 2012 at 03:46:19PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> We can inject a timer interrupt into the guest as a result of
>> three possible events:
>> - The virtual timer interrupt has fired while we were still
>>   executing the guest
>> - The timer interrupt hasn't fired, but it expired while we
>>   were doing the world switch
>> - A hrtimer we programmed earlier has fired
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
>> ---
>>  arch/arm/include/asm/kvm_arch_timer.h |   57 +++++++++
>>  arch/arm/kvm/reset.c                  |    9 +
>>  arch/arm/kvm/timer.c                  |  204 +++++++++++++++++++++++++++++++++
>>  3 files changed, 269 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm/kvm/timer.c
>>
>> diff --git a/arch/arm/include/asm/kvm_arch_timer.h b/arch/arm/include/asm/kvm_arch_timer.h
>> index 513b852..bd5e501 100644
>> --- a/arch/arm/include/asm/kvm_arch_timer.h
>> +++ b/arch/arm/include/asm/kvm_arch_timer.h
>> @@ -19,13 +19,68 @@
>>  #ifndef __ASM_ARM_KVM_ARCH_TIMER_H
>>  #define __ASM_ARM_KVM_ARCH_TIMER_H
>>
>> +#include <linux/clocksource.h>
>> +#include <linux/hrtimer.h>
>> +#include <linux/workqueue.h>
>> +
>>  struct arch_timer_kvm {
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +       /* Is the timer enabled */
>> +       bool                    enabled;
>> +
>> +       /*
>> +        * Virtual offset (kernel access it through cntvoff, HYP code
>> +        * access it as two 32bit values).
>> +        */
>> +       union {
>> +               cycle_t         cntvoff;
>> +               struct {
>> +                       u32     low;    /* Restored only */
>> +                       u32     high;   /* Restored only */
>> +               } cntvoff32;
>> +       };
>> +#endif
> 
> Endianness?

Yup. I'll fix that.

>>  };
>>
>>  struct arch_timer_cpu {
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +       /* Registers: control register, timer value */
>> +       u32                             cntv_ctl;       /* Saved/restored */
>> +       union {
>> +               cycle_t                 cntv_cval;
>> +               struct {
>> +                       u32             low;            /* Saved/restored */
>> +                       u32             high;           /* Saved/restored */
>> +               } cntv_cval32;
>> +       };
> 
> Similarly.

Ditto.

> 
>> +       /*
>> +        * Anything that is not used directly from assembly code goes
>> +        * here.
>> +        */
>> +
>> +       /* Background timer used when the guest is not running */
>> +       struct hrtimer                  timer;
>> +
>> +       /* Work queued with the above timer expires */
>> +       struct work_struct              expired;
>> +
>> +       /* Background timer active */
>> +       bool                            armed;
>> +
>> +       /* Timer IRQ */
>> +       const struct kvm_irq_level      *irq;
>> +#endif
>>  };
>>
>> -#ifndef CONFIG_KVM_ARM_TIMER
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +int kvm_timer_hyp_init(void);
>> +int kvm_timer_init(struct kvm *kvm);
>> +void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
>> +void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu);
>> +void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu);
>> +void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
>> +#else
>>  static inline int kvm_timer_hyp_init(void)
>>  {
>>         return 0;
>> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
>> index b80256b..7463f5b 100644
>> --- a/arch/arm/kvm/reset.c
>> +++ b/arch/arm/kvm/reset.c
>> @@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
>>         .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>>  };
>>
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +static const struct kvm_irq_level a15_virt_timer_ppi = {
>> +       { .irq = 27 },  /* irq: A7/A15 specific */
> 
> This should be parameterised by the vCPU type.

This is already A15 specific, and assigned in an A15 specific code
section below.

>> +       .level = 1      /* level */
>> +};
>> +#endif
>>
>>  /*******************************************************************************
>>   * Exported reset function
>> @@ -59,6 +65,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>                         return -EINVAL;
>>                 cpu_reset = &a15_regs_reset;
>>                 vcpu->arch.midr = read_cpuid_id();
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +               vcpu->arch.timer_cpu.irq = &a15_virt_timer_ppi;
>> +#endif
>>                 break;
>>         default:
>>                 return -ENODEV;
>> diff --git a/arch/arm/kvm/timer.c b/arch/arm/kvm/timer.c
>> new file mode 100644
>> index 0000000..a241298
>> --- /dev/null
>> +++ b/arch/arm/kvm/timer.c
> 
> Maybe kvm/arch_timer.c since it seems to be specific to that device and
> matches the naming for the main driver under kernel/? If we get new virtual
> timer devices in the future, I guess this would need to be split up so the
> arch-timer-specific bits are separate from generic hrtimer and kvm code.

Sure.

>> @@ -0,0 +1,204 @@
>> +/*
>> + * Copyright (C) 2012 ARM Ltd.
>> + * Author: Marc Zyngier <marc.zyngier@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + */
>> +
>> +#include <linux/of_irq.h>
>> +#include <linux/kvm.h>
>> +#include <linux/kvm_host.h>
>> +#include <linux/interrupt.h>
>> +
>> +#include <asm/arch_timer.h>
>> +
>> +#include <asm/kvm_vgic.h>
>> +#include <asm/kvm_arch_timer.h>
>> +
>> +static struct timecounter *timecounter;
>> +static struct workqueue_struct *wqueue;
>> +
>> +static cycle_t kvm_phys_timer_read(void)
>> +{
>> +       return timecounter->cc->read(timecounter->cc);
>> +}
>> +
>> +static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu)
>> +{
>> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> +
>> +       timer->cntv_ctl |= 1 << 1; /* Mask the interrupt in the guest */
> 
> This is ARCH_TIMER_CTRL_IT_MASK right? We should make that definition
> available if it is needed here.

OK.

>> +       kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
>> +                           vcpu->arch.timer_cpu.irq->irq,
>> +                           vcpu->arch.timer_cpu.irq->level);
>> +}
>> +
>> +static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>> +{
>> +       struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
>> +
>> +       /*
>> +        * We disable the timer in the world switch and let it be
>> +        * handled by kvm_timer_sync_from_cpu(). Getting a timer
>> +        * interrupt at this point is a sure sign of some major
>> +        * breakage.
>> +        */
>> +       pr_warn("Unexpected interrupt %d on vcpu %p\n", irq, vcpu);
>> +       return IRQ_HANDLED;
> 
> IRQ_NONE?

I don't think so. We're actually handling the interrupt (admittedly in a
very basic way), and as it is a per-cpu interrupt, there will be noone
else to take care of it.

>> +}
>> +
>> +static void kvm_timer_inject_irq_work(struct work_struct *work)
>> +{
>> +       struct kvm_vcpu *vcpu;
>> +
>> +       vcpu = container_of(work, struct kvm_vcpu, arch.timer_cpu.expired);
>> +       vcpu->arch.timer_cpu.armed = false;
>> +       kvm_timer_inject_irq(vcpu);
>> +}
>> +
>> +static enum hrtimer_restart kvm_timer_expire(struct hrtimer *hrt)
>> +{
>> +       struct arch_timer_cpu *timer;
>> +       timer = container_of(hrt, struct arch_timer_cpu, timer);
>> +       queue_work(wqueue, &timer->expired);
>> +       return HRTIMER_NORESTART;
>> +}
>> +
>> +void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu)
>> +{
>> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> +
>> +       /*
>> +        * We're about to run this vcpu again, so there is no need to
>> +        * keep the background timer running, as we're about to
>> +        * populate the CPU timer again.
>> +        */
>> +       if (timer->armed) {
>> +               hrtimer_cancel(&timer->timer);
>> +               cancel_work_sync(&timer->expired);
>> +               timer->armed = false;
>> +       }
>> +}
> 
> I think some helper functions like timer_is_armed, timer_arm and
> timer_disarm would make this more readable (resisting arm_timer, which
> confuses things more!).

OK.

>> +
>> +void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu)
>> +{
>> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> +       cycle_t cval, now;
>> +       u64 ns;
>> +
>> +       /* Check if the timer is enabled and unmasked first */
>> +       if ((timer->cntv_ctl & 3) != 1)
>> +               return;
> 
> Again, we should create/use ARCH_TIMER #defines for this hardware bits.
> 
>> +       cval = timer->cntv_cval;
>> +       now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
> 
> We probably want to add an isb() *before* the mrrc in
> arch_timer_counter_read if you want to do things like this, because the
> counter can be speculated in advance.

Indeed.

>> +       BUG_ON(timer->armed);
>> +
>> +       if (cval <= now) {
>> +               /*
>> +                * Timer has already expired while we were not
>> +                * looking. Inject the interrupt and carry on.
>> +                */
>> +               kvm_timer_inject_irq(vcpu);
>> +               return;
>> +       }
> 
> Does this buy you much? You still have to cope with the timer expiring here
> anyway.

It definitely does from a latency point of view. Programming a timer
that will expire right away, calling the interrupt handler, queuing the
work queue, waiting for the workqueue to be scheduled and finally
delivering the interrupt... If we can catch a few of these early (and we
do), it is worth it.

>> +       timer->armed = true;
>> +       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
>> +       hrtimer_start(&timer->timer, ktime_add_ns(ktime_get(), ns),
>> +                     HRTIMER_MODE_ABS);
>> +}
>> +
>> +void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>> +{
>> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> +
>> +       INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
>> +       hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>> +       timer->timer.function = kvm_timer_expire;
>> +}
>> +
>> +static void kvm_timer_init_interrupt(void *info)
>> +{
>> +       unsigned int *irqp = info;
>> +
>> +       enable_percpu_irq(*irqp, 0);
> 
> IRQ_TYPE_NONE
> 
>> +}
>> +
>> +
>> +static const struct of_device_id arch_timer_of_match[] = {
>> +       { .compatible   = "arm,armv7-timer",    },
>> +       {},
>> +};
>> +
>> +int kvm_timer_hyp_init(void)
>> +{
>> +       struct device_node *np;
>> +       unsigned int ppi;
>> +       int err;
>> +
>> +       timecounter = arch_timer_get_timecounter();
>> +       if (!timecounter)
>> +               return -ENODEV;
>> +
>> +       np = of_find_matching_node(NULL, arch_timer_of_match);
>> +       if (!np) {
>> +               kvm_err("kvm_arch_timer: can't find DT node\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       ppi = irq_of_parse_and_map(np, 2);
>> +       if (!ppi) {
>> +               kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = request_percpu_irq(ppi, kvm_arch_timer_handler,
>> +                                "kvm guest timer", kvm_get_running_vcpus());
>> +       if (err) {
>> +               kvm_err("kvm_arch_timer: can't request interrupt %d (%d)\n",
>> +                       ppi, err);
>> +               return err;
>> +       }
>> +
>> +       wqueue = create_singlethread_workqueue("kvm_arch_timer");
>> +       if (!wqueue) {
>> +               free_percpu_irq(ppi, kvm_get_running_vcpus());
>> +               return -ENOMEM;
>> +       }
>> +
>> +       kvm_info("%s IRQ%d\n", np->name, ppi);
>> +       on_each_cpu(kvm_timer_init_interrupt, &ppi, 1);
> 
> on_each_cpu currently just returns 0, but you could use that as your return
> value for good measure anyway.

Sure.

>> +       return 0;
>> +}
>> +
>> +void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>> +{
>> +       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>> +
>> +       hrtimer_cancel(&timer->timer);
>> +       cancel_work_sync(&timer->expired);
>> +}
>> +
>> +int kvm_timer_init(struct kvm *kvm)
>> +{
>> +       if (timecounter && wqueue) {
>> +               kvm->arch.timer.cntvoff = kvm_phys_timer_read();
> 
> Shouldn't this be initialised to 0 and then updated on world switch?

No. You do not want your virtual offset to drift. Otherwise you'll
observe something like time dilatation, and your clocks will drift.
Plus, you really want all your vcpus to be synchronized. Allowing them
to drift apart could be an interesting experience... ;-)

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

[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

[Will Deacon]

On Fri, Nov 23, 2012 at 04:52:12PM +0000, Marc Zyngier wrote:
> On 23/11/12 16:17, Will Deacon wrote:
> >> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> >> index b80256b..7463f5b 100644
> >> --- a/arch/arm/kvm/reset.c
> >> +++ b/arch/arm/kvm/reset.c
> >> @@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
> >>         .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
> >>  };
> >>
> >> +#ifdef CONFIG_KVM_ARM_TIMER
> >> +static const struct kvm_irq_level a15_virt_timer_ppi = {
> >> +       { .irq = 27 },  /* irq: A7/A15 specific */
> >
> > This should be parameterised by the vCPU type.
> 
> This is already A15 specific, and assigned in an A15 specific code
> section below.

Right, but we can take the interrupt number from the device-tree, like we do
for the host anyway.

> >> +static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> >> +{
> >> +       struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
> >> +
> >> +       /*
> >> +        * We disable the timer in the world switch and let it be
> >> +        * handled by kvm_timer_sync_from_cpu(). Getting a timer
> >> +        * interrupt at this point is a sure sign of some major
> >> +        * breakage.
> >> +        */
> >> +       pr_warn("Unexpected interrupt %d on vcpu %p\n", irq, vcpu);
> >> +       return IRQ_HANDLED;
> >
> > IRQ_NONE?
> 
> I don't think so. We're actually handling the interrupt (admittedly in a
> very basic way), and as it is a per-cpu interrupt, there will be noone
> else to take care of it.

For an SPI, returning IRQ_NONE would (eventually) silence a screaming
interrupt because the generic IRQ bits would disable it. I'm not sure if that
applies to PPIs or not but if it does, I'd say that's a good reason to use it.

> 
> >> +       BUG_ON(timer->armed);
> >> +
> >> +       if (cval <= now) {
> >> +               /*
> >> +                * Timer has already expired while we were not
> >> +                * looking. Inject the interrupt and carry on.
> >> +                */
> >> +               kvm_timer_inject_irq(vcpu);
> >> +               return;
> >> +       }
> >
> > Does this buy you much? You still have to cope with the timer expiring here
> > anyway.
> 
> It definitely does from a latency point of view. Programming a timer
> that will expire right away, calling the interrupt handler, queuing the
> work queue, waiting for the workqueue to be scheduled and finally
> delivering the interrupt... If we can catch a few of these early (and we
> do), it is worth it.

Ok, interesting. I wasn't sure how often that happened in practice.

> >> +int kvm_timer_init(struct kvm *kvm)
> >> +{
> >> +       if (timecounter && wqueue) {
> >> +               kvm->arch.timer.cntvoff = kvm_phys_timer_read();
> >
> > Shouldn't this be initialised to 0 and then updated on world switch?
> 
> No. You do not want your virtual offset to drift. Otherwise you'll
> observe something like time dilatation, and your clocks will drift.
> Plus, you really want all your vcpus to be synchronized. Allowing them
> to drift apart could be an interesting experience... ;-)

In which case, why do we initialise it to the physical timer in the first
place? Surely the value doesn't matter, as long as everybody agrees on what
it is?

Will

[PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch

[Marc Zyngier]

On 23/11/12 16:30, Will Deacon wrote:
> On Sat, Nov 10, 2012 at 03:46:25PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> Do the necessary save/restore dance for the timers in the world
>> switch code. In the process, allow the guest to read the physical
>> counter, which is useful for its own clock_event_device.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
>> ---
>>  arch/arm/kernel/asm-offsets.c  |    8 ++++++++
>>  arch/arm/kvm/arm.c             |    3 +++
>>  arch/arm/kvm/interrupts_head.S |   41 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 52 insertions(+)
>>
>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
>> index 39b6221..50df318 100644
>> --- a/arch/arm/kernel/asm-offsets.c
>> +++ b/arch/arm/kernel/asm-offsets.c
>> @@ -177,6 +177,14 @@ int main(void)
>>    DEFINE(VGIC_CPU_APR,		offsetof(struct vgic_cpu, vgic_apr));
>>    DEFINE(VGIC_CPU_LR,		offsetof(struct vgic_cpu, vgic_lr));
>>    DEFINE(VGIC_CPU_NR_LR,	offsetof(struct vgic_cpu, nr_lr));
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +  DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
>> +  DEFINE(VCPU_TIMER_CNTV_CVALH,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high));
>> +  DEFINE(VCPU_TIMER_CNTV_CVALL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low));
>> +  DEFINE(KVM_TIMER_CNTVOFF_H,	offsetof(struct kvm, arch.timer.cntvoff32.high));
>> +  DEFINE(KVM_TIMER_CNTVOFF_L,	offsetof(struct kvm, arch.timer.cntvoff32.low));
>> +  DEFINE(KVM_TIMER_ENABLED,	offsetof(struct kvm, arch.timer.enabled));
>> +#endif
>>    DEFINE(KVM_VGIC_VCTRL,	offsetof(struct kvm, arch.vgic.vctrl_base));
>>  #endif
>>    DEFINE(KVM_VTTBR,		offsetof(struct kvm, arch.vttbr));
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 1716f12..8cdef69 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		update_vttbr(vcpu->kvm);
>>  
>>  		kvm_vgic_sync_to_cpu(vcpu);
>> +		kvm_timer_sync_to_cpu(vcpu);
>>  
>>  		local_irq_disable();
>>  
>> @@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  		if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>>  			local_irq_enable();
>> +			kvm_timer_sync_from_cpu(vcpu);
>>  			kvm_vgic_sync_from_cpu(vcpu);
>>  			continue;
>>  		}
>> @@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		 * Back from guest
>>  		 *************************************************************/
>>  
>> +		kvm_timer_sync_from_cpu(vcpu);
>>  		kvm_vgic_sync_from_cpu(vcpu);
>>  
>>  		ret = handle_exit(vcpu, run, ret);
>> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
>> index 0003aab..ece84d1 100644
>> --- a/arch/arm/kvm/interrupts_head.S
>> +++ b/arch/arm/kvm/interrupts_head.S
>> @@ -422,6 +422,25 @@
>>  #define CNTHCTL_PL1PCEN		(1 << 1)
>>  
>>  .macro save_timer_state	vcpup
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +	ldr	r4, [\vcpup, #VCPU_KVM]
>> +	ldr	r2, [r4, #KVM_TIMER_ENABLED]
>> +	cmp	r2, #0
>> +	beq	1f
>> +
>> +	mrc	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
>> +	and	r2, #3
> 
> Why do you need this and (you do the masking on the restore path)?

Probably not necessary. We could just clear the enable below.

> 
>> +	str	r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
>> +	bic	r2, #1			@ Clear ENABLE
>> +	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
>> +	isb
>> +
>> +	mrrc	p15, 3, r2, r3, c14	@ CNTV_CVAL
>> +	str	r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
>> +	str	r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
>> +
>> +1:
>> +#endif
>>  	@ Allow physical timer/counter access for the host
>>  	mrc	p15, 4, r2, c14, c1, 0	@ CNTHCTL
>>  	orr	r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)
>> @@ -435,6 +454,28 @@
>>  	orr	r2, r2, #CNTHCTL_PL1PCTEN
>>  	bic	r2, r2, #CNTHCTL_PL1PCEN
>>  	mcr	p15, 4, r2, c14, c1, 0	@ CNTHCTL
>> +
>> +#ifdef CONFIG_KVM_ARM_TIMER
>> +	ldr	r4, [\vcpup, #VCPU_KVM]
>> +	ldr	r2, [r4, #KVM_TIMER_ENABLED]
>> +	cmp	r2, #0
>> +	beq	1f
>> +
>> +	ldr	r3, [r4, #KVM_TIMER_CNTVOFF_H]
>> +	ldr	r2, [r4, #KVM_TIMER_CNTVOFF_L]
>> +	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
>> +	isb
>> +
>> +	ldr	r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
>> +	ldr	r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
>> +	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
>> +
>> +	ldr	r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
>> +	and	r2, #3
> 
> Please use the long form syntax for these instructions (and r2, r2, #3) as
> toolchains have historically been picky about accepting these when not
> targetting Thumb and it also makes it consistent with the rest of arch/arm/.

OK.

> 
>> +	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
>> +	isb
>> +1:
>> +#endif
> 
> It's pretty horrible to take a macro argument (vcpup) and then open-code all
> the other register numbers without moving the argument somewhere nice. Are
> callers just expected to understand that you clobber {r2,r3,r4} and avoid
> them? If so, why not just use the PCS so that people don't have to learn a
> new convention?

I'm happy to remove this vcpup and stick to r0/r1 (depending on the
path). All macros have to know about the side effects anyway, as we
switch all the registers.

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

[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

[Marc Zyngier]

On 23/11/12 17:00, Will Deacon wrote:
> On Fri, Nov 23, 2012 at 04:52:12PM +0000, Marc Zyngier wrote:
>> On 23/11/12 16:17, Will Deacon wrote:
>>>> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
>>>> index b80256b..7463f5b 100644
>>>> --- a/arch/arm/kvm/reset.c
>>>> +++ b/arch/arm/kvm/reset.c
>>>> @@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
>>>>         .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>>>>  };
>>>>
>>>> +#ifdef CONFIG_KVM_ARM_TIMER
>>>> +static const struct kvm_irq_level a15_virt_timer_ppi = {
>>>> +       { .irq = 27 },  /* irq: A7/A15 specific */
>>>
>>> This should be parameterised by the vCPU type.
>>
>> This is already A15 specific, and assigned in an A15 specific code
>> section below.
> 
> Right, but we can take the interrupt number from the device-tree, like we do
> for the host anyway.

Certainly. I'll update this bit.

>>>> +static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>>>> +{
>>>> +       struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
>>>> +
>>>> +       /*
>>>> +        * We disable the timer in the world switch and let it be
>>>> +        * handled by kvm_timer_sync_from_cpu(). Getting a timer
>>>> +        * interrupt at this point is a sure sign of some major
>>>> +        * breakage.
>>>> +        */
>>>> +       pr_warn("Unexpected interrupt %d on vcpu %p\n", irq, vcpu);
>>>> +       return IRQ_HANDLED;
>>>
>>> IRQ_NONE?
>>
>> I don't think so. We're actually handling the interrupt (admittedly in a
>> very basic way), and as it is a per-cpu interrupt, there will be noone
>> else to take care of it.
> 
> For an SPI, returning IRQ_NONE would (eventually) silence a screaming
> interrupt because the generic IRQ bits would disable it. I'm not sure if that
> applies to PPIs or not but if it does, I'd say that's a good reason to use it.
> 
>>
>>>> +       BUG_ON(timer->armed);
>>>> +
>>>> +       if (cval <= now) {
>>>> +               /*
>>>> +                * Timer has already expired while we were not
>>>> +                * looking. Inject the interrupt and carry on.
>>>> +                */
>>>> +               kvm_timer_inject_irq(vcpu);
>>>> +               return;
>>>> +       }
>>>
>>> Does this buy you much? You still have to cope with the timer expiring here
>>> anyway.
>>
>> It definitely does from a latency point of view. Programming a timer
>> that will expire right away, calling the interrupt handler, queuing the
>> work queue, waiting for the workqueue to be scheduled and finally
>> delivering the interrupt... If we can catch a few of these early (and we
>> do), it is worth it.
> 
> Ok, interesting. I wasn't sure how often that happened in practice.
> 
>>>> +int kvm_timer_init(struct kvm *kvm)
>>>> +{
>>>> +       if (timecounter && wqueue) {
>>>> +               kvm->arch.timer.cntvoff = kvm_phys_timer_read();
>>>
>>> Shouldn't this be initialised to 0 and then updated on world switch?
>>
>> No. You do not want your virtual offset to drift. Otherwise you'll
>> observe something like time dilatation, and your clocks will drift.
>> Plus, you really want all your vcpus to be synchronized. Allowing them
>> to drift apart could be an interesting experience... ;-)
> 
> In which case, why do we initialise it to the physical timer in the first
> place? Surely the value doesn't matter, as long as everybody agrees on what
> it is?

This ensures that the VM gets its virtual counter starting as closely as
possible to 0. As good a convention as any.

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

[PATCH v4 2/5] ARM: KVM: arch_timers: Add minimal infrastructure

[Christoffer Dall]

On Fri, Nov 23, 2012 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Sat, Nov 10, 2012 at 03:46:12PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> Add some very minimal architected timer related infrastructure.
>> For the moment, we just provide empty structures, and enable/disable
>> access to the physical timer across world switch.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
>
> Is it even worth having this patch? It doesn't seem to do much other than
> stub everything out, so I'd be inclined to fold it in with the next one.
>
done,
-Christoffer

[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

[Christoffer Dall]

On Fri, Nov 23, 2012 at 12:11 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 23/11/12 17:00, Will Deacon wrote:
>> On Fri, Nov 23, 2012 at 04:52:12PM +0000, Marc Zyngier wrote:
>>> On 23/11/12 16:17, Will Deacon wrote:
>>>>> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
>>>>> index b80256b..7463f5b 100644
>>>>> --- a/arch/arm/kvm/reset.c
>>>>> +++ b/arch/arm/kvm/reset.c
>>>>> @@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
>>>>>         .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
>>>>>  };
>>>>>
>>>>> +#ifdef CONFIG_KVM_ARM_TIMER
>>>>> +static const struct kvm_irq_level a15_virt_timer_ppi = {
>>>>> +       { .irq = 27 },  /* irq: A7/A15 specific */
>>>>
>>>> This should be parameterised by the vCPU type.
>>>
>>> This is already A15 specific, and assigned in an A15 specific code
>>> section below.
>>
>> Right, but we can take the interrupt number from the device-tree, like we do
>> for the host anyway.
>
> Certainly. I'll update this bit.
>
So we lock ourselves to always only support emulating the same CPU on
the guest as on the host?

(I know we assume this elsewhere and that even the A7 uses the same
IRQ number, but these doesn't necessarily have to be the same in the
future, and there's no need to make such a task more difficult, and we
are not really making things mode stable/simple by taking it from the
DT - are we?)

-Christoffer

[PATCH v4 4/5] ARM: KVM: arch_timers: Add timer world switch

[Christoffer Dall]

On Fri, Nov 23, 2012 at 12:04 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 23/11/12 16:30, Will Deacon wrote:
>> On Sat, Nov 10, 2012 at 03:46:25PM +0000, Christoffer Dall wrote:
>>> From: Marc Zyngier <marc.zyngier@arm.com>
>>>
>>> Do the necessary save/restore dance for the timers in the world
>>> switch code. In the process, allow the guest to read the physical
>>> counter, which is useful for its own clock_event_device.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
>>> ---
>>>  arch/arm/kernel/asm-offsets.c  |    8 ++++++++
>>>  arch/arm/kvm/arm.c             |    3 +++
>>>  arch/arm/kvm/interrupts_head.S |   41 ++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 52 insertions(+)
>>>
>>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
>>> index 39b6221..50df318 100644
>>> --- a/arch/arm/kernel/asm-offsets.c
>>> +++ b/arch/arm/kernel/asm-offsets.c
>>> @@ -177,6 +177,14 @@ int main(void)
>>>    DEFINE(VGIC_CPU_APR,              offsetof(struct vgic_cpu, vgic_apr));
>>>    DEFINE(VGIC_CPU_LR,               offsetof(struct vgic_cpu, vgic_lr));
>>>    DEFINE(VGIC_CPU_NR_LR,    offsetof(struct vgic_cpu, nr_lr));
>>> +#ifdef CONFIG_KVM_ARM_TIMER
>>> +  DEFINE(VCPU_TIMER_CNTV_CTL,       offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
>>> +  DEFINE(VCPU_TIMER_CNTV_CVALH,     offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.high));
>>> +  DEFINE(VCPU_TIMER_CNTV_CVALL,     offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval32.low));
>>> +  DEFINE(KVM_TIMER_CNTVOFF_H,       offsetof(struct kvm, arch.timer.cntvoff32.high));
>>> +  DEFINE(KVM_TIMER_CNTVOFF_L,       offsetof(struct kvm, arch.timer.cntvoff32.low));
>>> +  DEFINE(KVM_TIMER_ENABLED, offsetof(struct kvm, arch.timer.enabled));
>>> +#endif
>>>    DEFINE(KVM_VGIC_VCTRL,    offsetof(struct kvm, arch.vgic.vctrl_base));
>>>  #endif
>>>    DEFINE(KVM_VTTBR,         offsetof(struct kvm, arch.vttbr));
>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>> index 1716f12..8cdef69 100644
>>> --- a/arch/arm/kvm/arm.c
>>> +++ b/arch/arm/kvm/arm.c
>>> @@ -661,6 +661,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>              update_vttbr(vcpu->kvm);
>>>
>>>              kvm_vgic_sync_to_cpu(vcpu);
>>> +            kvm_timer_sync_to_cpu(vcpu);
>>>
>>>              local_irq_disable();
>>>
>>> @@ -674,6 +675,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>
>>>              if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) {
>>>                      local_irq_enable();
>>> +                    kvm_timer_sync_from_cpu(vcpu);
>>>                      kvm_vgic_sync_from_cpu(vcpu);
>>>                      continue;
>>>              }
>>> @@ -713,6 +715,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>               * Back from guest
>>>               *************************************************************/
>>>
>>> +            kvm_timer_sync_from_cpu(vcpu);
>>>              kvm_vgic_sync_from_cpu(vcpu);
>>>
>>>              ret = handle_exit(vcpu, run, ret);
>>> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
>>> index 0003aab..ece84d1 100644
>>> --- a/arch/arm/kvm/interrupts_head.S
>>> +++ b/arch/arm/kvm/interrupts_head.S
>>> @@ -422,6 +422,25 @@
>>>  #define CNTHCTL_PL1PCEN             (1 << 1)
>>>
>>>  .macro save_timer_state     vcpup
>>> +#ifdef CONFIG_KVM_ARM_TIMER
>>> +    ldr     r4, [\vcpup, #VCPU_KVM]
>>> +    ldr     r2, [r4, #KVM_TIMER_ENABLED]
>>> +    cmp     r2, #0
>>> +    beq     1f
>>> +
>>> +    mrc     p15, 0, r2, c14, c3, 1  @ CNTV_CTL
>>> +    and     r2, #3
>>
>> Why do you need this and (you do the masking on the restore path)?
>
> Probably not necessary. We could just clear the enable below.
>
>>
>>> +    str     r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
>>> +    bic     r2, #1                  @ Clear ENABLE
>>> +    mcr     p15, 0, r2, c14, c3, 1  @ CNTV_CTL
>>> +    isb
>>> +
>>> +    mrrc    p15, 3, r2, r3, c14     @ CNTV_CVAL
>>> +    str     r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
>>> +    str     r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
>>> +
>>> +1:
>>> +#endif
>>>      @ Allow physical timer/counter access for the host
>>>      mrc     p15, 4, r2, c14, c1, 0  @ CNTHCTL
>>>      orr     r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)
>>> @@ -435,6 +454,28 @@
>>>      orr     r2, r2, #CNTHCTL_PL1PCTEN
>>>      bic     r2, r2, #CNTHCTL_PL1PCEN
>>>      mcr     p15, 4, r2, c14, c1, 0  @ CNTHCTL
>>> +
>>> +#ifdef CONFIG_KVM_ARM_TIMER
>>> +    ldr     r4, [\vcpup, #VCPU_KVM]
>>> +    ldr     r2, [r4, #KVM_TIMER_ENABLED]
>>> +    cmp     r2, #0
>>> +    beq     1f
>>> +
>>> +    ldr     r3, [r4, #KVM_TIMER_CNTVOFF_H]
>>> +    ldr     r2, [r4, #KVM_TIMER_CNTVOFF_L]
>>> +    mcrr    p15, 4, r2, r3, c14     @ CNTVOFF
>>> +    isb
>>> +
>>> +    ldr     r3, [\vcpup, #VCPU_TIMER_CNTV_CVALH]
>>> +    ldr     r2, [\vcpup, #VCPU_TIMER_CNTV_CVALL]
>>> +    mcrr    p15, 3, r2, r3, c14     @ CNTV_CVAL
>>> +
>>> +    ldr     r2, [\vcpup, #VCPU_TIMER_CNTV_CTL]
>>> +    and     r2, #3
>>
>> Please use the long form syntax for these instructions (and r2, r2, #3) as
>> toolchains have historically been picky about accepting these when not
>> targetting Thumb and it also makes it consistent with the rest of arch/arm/.
>
> OK.
>
>>
>>> +    mcr     p15, 0, r2, c14, c3, 1  @ CNTV_CTL
>>> +    isb
>>> +1:
>>> +#endif
>>
>> It's pretty horrible to take a macro argument (vcpup) and then open-code all
>> the other register numbers without moving the argument somewhere nice. Are
>> callers just expected to understand that you clobber {r2,r3,r4} and avoid
>> them? If so, why not just use the PCS so that people don't have to learn a
>> new convention?
>
> I'm happy to remove this vcpup and stick to r0/r1 (depending on the
> path). All macros have to know about the side effects anyway, as we
> switch all the registers.
>
Using the PCS is not going to help us, because we need a lot of
registers for a lot of these operations and we don't want to push/pop
extra stuff to the stack here - this is in the super critical path.

The nature of this code is not one where you have random callers that
need to work under certain assumptions - you need to scrutinize every
line here.

What we could do is pass all temporary variables to the macros, but it
becomes ugly when they take 5+ temporary regs, and it doesn't make
sense for register save/restore that also makes assumptions about
which register is used for the vcpu pointer.

Alternatively, and more attainable, we could make it a convention that
all the macros in this file take r0 as the vcpu pointer. Actually,
let's use r0 for the cpu pointer all over. Why not.

Let me craft a patch and send that out right away!

-Christoffer

[PATCH v4 5/5] ARM: KVM: arch_timers: Wire the init code and config option

[Christoffer Dall]

On Fri, Nov 23, 2012 at 11:31 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Sat, Nov 10, 2012 at 03:46:32PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> It is now possible to select CONFIG_KVM_ARM_TIMER to enable the
>> KVM architected timer support.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>
>> ---
>>  arch/arm/kvm/Kconfig  |    7 +++++++
>>  arch/arm/kvm/Makefile |    1 +
>>  arch/arm/kvm/arm.c    |   11 +++++++++++
>>  arch/arm/kvm/vgic.c   |    1 +
>>  4 files changed, 20 insertions(+)
>>
>> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
>> index 3c979ce..eaecb9f 100644
>> --- a/arch/arm/kvm/Kconfig
>> +++ b/arch/arm/kvm/Kconfig
>> @@ -58,6 +58,13 @@ config KVM_ARM_VGIC
>>       ---help---
>>         Adds support for a hardware assisted, in-kernel GIC emulation.
>>
>> +config KVM_ARM_TIMER
>> +        bool "KVM support for Architected Timers"
>> +     depends on KVM_ARM_VGIC && ARM_ARCH_TIMER
>> +     select HAVE_KVM_IRQCHIP
>> +     ---help---
>> +       Adds support for the Architected Timers in virtual machines
>> +
>
> This should probably be default y
>
yes, good idea, they all should, like this:

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 60d3d2a..739500b 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -21,6 +21,7 @@ config KVM
 	select PREEMPT_NOTIFIERS
 	select ANON_INODES
 	select KVM_MMIO
+	select KVM_ARM_HOST
 	depends on ARM_VIRT_EXT && ARM_LPAE
 	---help---
 	  Support hosting virtualized guest machines. You will also
@@ -54,6 +55,7 @@ config KVM_ARM_VGIC
         bool "KVM support for Virtual GIC"
 	depends on KVM_ARM_HOST && OF
 	select HAVE_KVM_IRQCHIP
+	default y
 	---help---
 	  Adds support for a hardware assisted, in-kernel GIC emulation.

@@ -61,6 +63,7 @@ config KVM_ARM_TIMER
         bool "KVM support for Architected Timers"
 	depends on KVM_ARM_VGIC && ARM_ARCH_TIMER
 	select HAVE_KVM_IRQCHIP
+	default y
 	---help---
 	  Adds support for the Architected Timers in virtual machines

--

Thanks,
-Christoffer