[PATCH v5 0/4] KVM/ARM Architected Timers support

[PATCH v5 0/4] KVM/ARM Architected Timers support

[Christoffer Dall]

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

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

Changes since v1-v4:
 - Get virtual IRQ number from DT
 - Simplify access to cntvoff and cntv_cval
 - Remove extraneous bit clearing
 - Abstract timer arming/disarming to improve code readability
 - Context switch CNTKCTL across world-switches
 - Add CPU hotplug notifier

---

Marc Zyngier (4):
      ARM: arch_timers: switch to physical timers if HYP mode is available
      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 |   85 +++++++++++
 arch/arm/include/asm/kvm_asm.h        |    3 
 arch/arm/include/asm/kvm_host.h       |    5 +
 arch/arm/kernel/arch_timer.c          |    7 +
 arch/arm/kernel/asm-offsets.c         |    6 +
 arch/arm/kvm/Kconfig                  |    8 +
 arch/arm/kvm/Makefile                 |    1 
 arch/arm/kvm/arch_timer.c             |  257 +++++++++++++++++++++++++++++++++
 arch/arm/kvm/arm.c                    |   14 ++
 arch/arm/kvm/coproc.c                 |    4 +
 arch/arm/kvm/interrupts.S             |    2 
 arch/arm/kvm/interrupts_head.S        |   93 ++++++++++++
 arch/arm/kvm/vgic.c                   |    1 
 13 files changed, 484 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/include/asm/kvm_arch_timer.h
 create mode 100644 arch/arm/kvm/arch_timer.c

-- 

[PATCH v5 1/4] 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 v5 2/4] ARM: KVM: arch_timers: Add guest timer core support

[Christoffer Dall]

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

Add some the architected timer related infrastructure, and support timer
interrupt injection, which can happen as a resultof 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 |   85 +++++++++++
 arch/arm/include/asm/kvm_host.h       |    5 +
 arch/arm/kvm/arch_timer.c             |  257 +++++++++++++++++++++++++++++++++
 arch/arm/kvm/interrupts.S             |    2 
 arch/arm/kvm/interrupts_head.S        |   31 ++++
 5 files changed, 380 insertions(+)
 create mode 100644 arch/arm/include/asm/kvm_arch_timer.h
 create mode 100644 arch/arm/kvm/arch_timer.c

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..aed1c42
--- /dev/null
+++ b/arch/arm/include/asm/kvm_arch_timer.h
@@ -0,0 +1,85 @@
+/*
+ * 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
+
+#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 */
+	cycle_t			cntvoff;
+#endif
+};
+
+struct arch_timer_cpu {
+#ifdef CONFIG_KVM_ARM_TIMER
+	/* Registers: control register, timer value */
+	u32				cntv_ctl;	/* Saved/restored */
+	cycle_t				cntv_cval;	/* Saved/restored */
+
+	/*
+	 * 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
+};
+
+#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;
+};
+
+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 149d62b..334b81d 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -23,6 +23,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/fpstate.h>
 #include <asm/kvm_decode.h>
+#include <asm/kvm_arch_timer.h>
 
 #define KVM_MAX_VCPUS CONFIG_KVM_ARM_MAX_VCPUS
 #define KVM_USER_MEM_SLOTS 32
@@ -49,6 +50,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.
@@ -99,6 +103,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/arch_timer.c b/arch/arm/kvm/arch_timer.c
new file mode 100644
index 0000000..6cb9aa3
--- /dev/null
+++ b/arch/arm/kvm/arch_timer.c
@@ -0,0 +1,257 @@
+/*
+ * 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/cpu.h>
+#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 struct kvm_irq_level timer_irq = {
+	.level	= 1,
+};
+
+static cycle_t kvm_phys_timer_read(void)
+{
+	return timecounter->cc->read(timecounter->cc);
+}
+
+static bool timer_is_armed(struct arch_timer_cpu *timer)
+{
+	return timer->armed;
+}
+
+/* timer_arm: as in "arm the timer", not as in ARM the company */
+static void timer_arm(struct arch_timer_cpu *timer, u64 ns)
+{
+	timer->armed = true;
+	hrtimer_start(&timer->timer, ktime_add_ns(ktime_get(), ns),
+		      HRTIMER_MODE_ABS);
+}
+
+static void timer_disarm(struct arch_timer_cpu *timer)
+{
+	if (timer_is_armed(timer)) {
+		hrtimer_cancel(&timer->timer);
+		cancel_work_sync(&timer->expired);
+		timer->armed = false;
+	}
+}
+
+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.
+	 */
+	timer_disarm(timer);
+}
+
+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_is_armed(timer));
+
+	if (cval <= now) {
+		/*
+		 * Timer has already expired while we were not
+		 * looking. Inject the interrupt and carry on.
+		 */
+		kvm_timer_inject_irq(vcpu);
+		return;
+	}
+
+	ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
+	timer_arm(timer, ns);
+}
+
+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;
+	timer->irq = &timer_irq;
+}
+
+static void kvm_timer_init_interrupt(void *info)
+{
+	enable_percpu_irq(timer_irq.irq, 0);
+}
+
+
+static int kvm_timer_cpu_notify(struct notifier_block *self,
+				unsigned long action, void *cpu)
+{
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		kvm_timer_init_interrupt(NULL);
+		break;
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
+		disable_percpu_irq(timer_irq.irq);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block kvm_timer_cpu_nb = {
+	.notifier_call = kvm_timer_cpu_notify,
+};
+
+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");
+		err = -EINVAL;
+		goto out;
+	}
+
+	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);
+		goto out;
+	}
+
+	timer_irq.irq = ppi;
+
+	err = register_cpu_notifier(&kvm_timer_cpu_nb);
+	if (err) {
+		kvm_err("Cannot register timer CPU notifier\n");
+		goto out_free;
+	}
+
+	wqueue = create_singlethread_workqueue("kvm_arch_timer");
+	if (!wqueue) {
+		err = -ENOMEM;
+		goto out_free;
+	}
+
+	kvm_info("%s IRQ%d\n", np->name, ppi);
+	on_each_cpu(kvm_timer_init_interrupt, NULL, 1);
+
+	goto out;
+out_free:
+	free_percpu_irq(ppi, kvm_get_running_vcpus());
+out:
+	of_node_put(np);
+	return err;
+}
+
+void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
+{
+	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+
+	timer_disarm(timer);
+}
+
+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;
+}
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 9ff7904..64a3e06 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -95,6 +95,7 @@ ENTRY(__kvm_vcpu_run)
 	save_host_regs
 
 	restore_vgic_state
+	restore_timer_state
 
 	@ Store hardware CP15 state and load guest state
 	read_cp15_state store_to_vcpu = 0
@@ -189,6 +190,7 @@ after_vfp_restore:
 	read_cp15_state store_to_vcpu = 1
 	write_cp15_state read_from_vcpu = 0
 
+	save_timer_state
 	save_vgic_state
 
 	restore_host_regs
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index b4276ed..dde5f8d 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -455,6 +455,37 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 #endif
 .endm
 
+#define CNTHCTL_PL1PCTEN	(1 << 0)
+#define CNTHCTL_PL1PCEN		(1 << 1)
+
+/*
+ * Save the timer state onto the VCPU and allow physical timer/counter access
+ * for the host.
+ *
+ * Assumes vcpu pointer in vcpu reg
+ */
+.macro save_timer_state
+	@ 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
+
+/*
+ * Load the timer state from the VCPU and deny physical timer/counter access
+ * for the host.
+ *
+ * Assumes vcpu pointer in vcpu reg
+ */
+.macro restore_timer_state
+	@ 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
+
 .equ vmentry,	0
 .equ vmexit,	1
 

[PATCH v5 2/4] ARM: KVM: arch_timers: Add guest timer core support

[Will Deacon]

On Tue, Jan 08, 2013 at 06:43:20PM +0000, Christoffer Dall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> Add some the architected timer related infrastructure, and support timer
> interrupt injection, which can happen as a resultof 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

[...]

> +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.
> +        */
> +       timer_disarm(timer);
> +}
> +
> +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_is_armed(timer));
> +
> +       if (cval <= now) {
> +               /*
> +                * Timer has already expired while we were not
> +                * looking. Inject the interrupt and carry on.
> +                */
> +               kvm_timer_inject_irq(vcpu);
> +               return;
> +       }
> +
> +       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
> +       timer_arm(timer, ns);
> +}

Please use flush/sync terminology to match the rest of arch/arm/.

Will

[PATCH v5 2/4] ARM: KVM: arch_timers: Add guest timer core support

[Christoffer Dall]

On Mon, Jan 14, 2013 at 10:18 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 08, 2013 at 06:43:20PM +0000, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> Add some the architected timer related infrastructure, and support timer
>> interrupt injection, which can happen as a resultof 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
>
> [...]
>
>> +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.
>> +        */
>> +       timer_disarm(timer);
>> +}
>> +
>> +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_is_armed(timer));
>> +
>> +       if (cval <= now) {
>> +               /*
>> +                * Timer has already expired while we were not
>> +                * looking. Inject the interrupt and carry on.
>> +                */
>> +               kvm_timer_inject_irq(vcpu);
>> +               return;
>> +       }
>> +
>> +       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
>> +       timer_arm(timer, ns);
>> +}
>
> Please use flush/sync terminology to match the rest of arch/arm/.
>
ok, the following fixes this for both timers and the vgic:

commit 1b68f39459dbc797f6766c103edf2c1053984161
Author: Christoffer Dall <c.dall@virtualopensystems.com>
Date:   Mon Jan 14 14:16:31 2013 -0500

    KVM: ARM: vgic: use sync/flush terminology

    Use sync/flush for saving state to/from CPUs to be consistent with
    other uses in arch/arm.

    Cc: Will Deacon <will.deacon@arm.com>
    Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>

diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
index 5e81e28..f5f270b 100644
--- a/arch/arm/include/asm/kvm_vgic.h
+++ b/arch/arm/include/asm/kvm_vgic.h
@@ -149,8 +149,8 @@ int kvm_vgic_hyp_init(void);
 int kvm_vgic_init(struct kvm *kvm);
 int kvm_vgic_create(struct kvm *kvm);
 int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
-void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
-void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);
+void kvm_vgic_flush(struct kvm_vcpu *vcpu);
+void kvm_vgic_sync(struct kvm_vcpu *vcpu);
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index f986718..7921bc4 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -714,7 +714,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_vgic_flush(vcpu);
 		kvm_timer_flush(vcpu);

 		local_irq_disable();
@@ -730,7 +730,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(vcpu);
-			kvm_vgic_sync_from_cpu(vcpu);
+			kvm_vgic_sync(vcpu);
 			continue;
 		}

@@ -770,7 +770,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu,
struct kvm_run *run)
 		 *************************************************************/

 		kvm_timer_sync(vcpu);
-		kvm_vgic_sync_from_cpu(vcpu);
+		kvm_vgic_sync(vcpu);

 		ret = handle_exit(vcpu, run, ret);
 	}
diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index 7eb94fa..25daa07 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -946,7 +946,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
  * Fill the list registers with pending interrupts before running the
  * guest.
  */
-static void __kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
+static void __kvm_vgic_flush(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
@@ -1009,7 +1009,7 @@ static bool vgic_process_maintenance(struct
kvm_vcpu *vcpu)
 	 * We do not need to take the distributor lock here, since the only
 	 * action we perform is clearing the irq_active_bit for an EOIed
 	 * level interrupt.  There is a potential race with
-	 * the queuing of an interrupt in __kvm_sync_to_cpu(), where we check
+	 * the queuing of an interrupt in __kvm_vgic_flush(), where we check
 	 * if the interrupt is already active. Two possibilities:
 	 *
 	 * - The queuing is occurring on the same vcpu: cannot happen,
@@ -1055,7 +1055,7 @@ static bool vgic_process_maintenance(struct
kvm_vcpu *vcpu)
  * the distributor here (the irq_pending_on_cpu bit is safe to set),
  * so there is no need for taking its lock.
  */
-static void __kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu)
+static void __kvm_vgic_sync(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
@@ -1085,7 +1085,7 @@ static void __kvm_vgic_sync_from_cpu(struct
kvm_vcpu *vcpu)
 		set_bit(vcpu->vcpu_id, &dist->irq_pending_on_cpu);
 }

-void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
+void kvm_vgic_flush(struct kvm_vcpu *vcpu)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

@@ -1093,16 +1093,16 @@ void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
 		return;

 	spin_lock(&dist->lock);
-	__kvm_vgic_sync_to_cpu(vcpu);
+	__kvm_vgic_flush(vcpu);
 	spin_unlock(&dist->lock);
 }

-void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu)
+void kvm_vgic_sync(struct kvm_vcpu *vcpu)
 {
 	if (!irqchip_in_kernel(vcpu->kvm))
 		return;

-	__kvm_vgic_sync_from_cpu(vcpu);
+	__kvm_vgic_sync(vcpu);
 }

 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
--

commit b9e08fdd32b7f3da3ad0b287c4da575b3b6c23d7
Author: Christoffer Dall <c.dall@virtualopensystems.com>
Date:   Mon Jan 14 14:15:31 2013 -0500

    KVM: ARM: timers: use sync/flush terminology

    Use sync/flush for saving state to/from CPUs to be consistent with other
    uses in arch/arm.

    Cc: Will Deacon <will.deacon@arm.com>
    Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com>

diff --git a/arch/arm/include/asm/kvm_arch_timer.h
b/arch/arm/include/asm/kvm_arch_timer.h
index aed1c42..2af5096 100644
--- a/arch/arm/include/asm/kvm_arch_timer.h
+++ b/arch/arm/include/asm/kvm_arch_timer.h
@@ -62,8 +62,8 @@ struct arch_timer_cpu {
 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_flush(struct kvm_vcpu *vcpu);
+void kvm_timer_sync(struct kvm_vcpu *vcpu);
 void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
 #else
 static inline int kvm_timer_hyp_init(void)
diff --git a/arch/arm/kvm/arch_timer.c b/arch/arm/kvm/arch_timer.c
index 6cb9aa3..3f2580f 100644
--- a/arch/arm/kvm/arch_timer.c
+++ b/arch/arm/kvm/arch_timer.c
@@ -101,7 +101,14 @@ static enum hrtimer_restart
kvm_timer_expire(struct hrtimer *hrt)
 	return HRTIMER_NORESTART;
 }

-void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu)
+/**
+ * kvm_timer_flush - prepare to move the virt timer to the cpu
+ * @vcpu: The vcpu pointer
+ *
+ * Disarm any pending soft timers, since the world-switch code will write the
+ * virtual timer state back to the physical CPU.
+ */
+void kvm_timer_flush(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;

@@ -113,7 +120,14 @@ void kvm_timer_sync_to_cpu(struct kvm_vcpu *vcpu)
 	timer_disarm(timer);
 }

-void kvm_timer_sync_from_cpu(struct kvm_vcpu *vcpu)
+/**
+ * kvm_timer_sync - sync timer state from cpu
+ * @vcpu: The vcpu pointer
+ *
+ * Check if the virtual timer was armed and either schedule a corresponding
+ * soft timer or inject directly if already expired.
+ */
+void kvm_timer_sync(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	cycle_t cval, now;
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index fdd4a7c..f986718 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -715,7 +715,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);
+		kvm_timer_flush(vcpu);

 		local_irq_disable();

@@ -729,7 +729,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_timer_sync(vcpu);
 			kvm_vgic_sync_from_cpu(vcpu);
 			continue;
 		}
@@ -769,7 +769,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_timer_sync(vcpu);
 		kvm_vgic_sync_from_cpu(vcpu);

 		ret = handle_exit(vcpu, run, ret);
--

Thanks,
-Christoffer

[PATCH v5 2/4] ARM: KVM: arch_timers: Add guest timer core support

[Marc Zyngier]

On 14/01/13 19:19, Christoffer Dall wrote:
> On Mon, Jan 14, 2013 at 10:18 AM, Will Deacon <will.deacon@arm.com> wrote:
>> On Tue, Jan 08, 2013 at 06:43:20PM +0000, Christoffer Dall wrote:
>>> From: Marc Zyngier <marc.zyngier@arm.com>
>>>
>>> Add some the architected timer related infrastructure, and support timer
>>> interrupt injection, which can happen as a resultof 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
>>
>> [...]
>>
>>> +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.
>>> +        */
>>> +       timer_disarm(timer);
>>> +}
>>> +
>>> +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_is_armed(timer));
>>> +
>>> +       if (cval <= now) {
>>> +               /*
>>> +                * Timer has already expired while we were not
>>> +                * looking. Inject the interrupt and carry on.
>>> +                */
>>> +               kvm_timer_inject_irq(vcpu);
>>> +               return;
>>> +       }
>>> +
>>> +       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
>>> +       timer_arm(timer, ns);
>>> +}
>>
>> Please use flush/sync terminology to match the rest of arch/arm/.
>>
> ok, the following fixes this for both timers and the vgic:
> 
> commit 1b68f39459dbc797f6766c103edf2c1053984161
> Author: Christoffer Dall <c.dall@virtualopensystems.com>
> Date:   Mon Jan 14 14:16:31 2013 -0500
> 
>     KVM: ARM: vgic: use sync/flush terminology
> 
>     Use sync/flush for saving state to/from CPUs to be consistent with
>     other uses in arch/arm.

Sync and flush on their own are pretty inexpressive. Consider changing
it to {flush,sync}_hwstate, so we're consistent with what VFP does.

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

[PATCH v5 2/4] ARM: KVM: arch_timers: Add guest timer core support

[Christoffer Dall]

On Tue, Jan 15, 2013 at 6:07 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 14/01/13 19:19, Christoffer Dall wrote:
>> On Mon, Jan 14, 2013 at 10:18 AM, Will Deacon <will.deacon@arm.com> wrote:
>>> On Tue, Jan 08, 2013 at 06:43:20PM +0000, Christoffer Dall wrote:
>>>> From: Marc Zyngier <marc.zyngier@arm.com>
>>>>
>>>> Add some the architected timer related infrastructure, and support timer
>>>> interrupt injection, which can happen as a resultof 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
>>>
>>> [...]
>>>
>>>> +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.
>>>> +        */
>>>> +       timer_disarm(timer);
>>>> +}
>>>> +
>>>> +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_is_armed(timer));
>>>> +
>>>> +       if (cval <= now) {
>>>> +               /*
>>>> +                * Timer has already expired while we were not
>>>> +                * looking. Inject the interrupt and carry on.
>>>> +                */
>>>> +               kvm_timer_inject_irq(vcpu);
>>>> +               return;
>>>> +       }
>>>> +
>>>> +       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
>>>> +       timer_arm(timer, ns);
>>>> +}
>>>
>>> Please use flush/sync terminology to match the rest of arch/arm/.
>>>
>> ok, the following fixes this for both timers and the vgic:
>>
>> commit 1b68f39459dbc797f6766c103edf2c1053984161
>> Author: Christoffer Dall <c.dall@virtualopensystems.com>
>> Date:   Mon Jan 14 14:16:31 2013 -0500
>>
>>     KVM: ARM: vgic: use sync/flush terminology
>>
>>     Use sync/flush for saving state to/from CPUs to be consistent with
>>     other uses in arch/arm.
>
> Sync and flush on their own are pretty inexpressive. Consider changing
> it to {flush,sync}_hwstate, so we're consistent with what VFP does.
>
sure

[PATCH v5 3/4] 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/include/asm/kvm_asm.h |    3 +-
 arch/arm/kernel/asm-offsets.c  |    6 ++++
 arch/arm/kvm/arm.c             |    3 ++
 arch/arm/kvm/coproc.c          |    4 +++
 arch/arm/kvm/interrupts_head.S |   62 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 58d787b..8a60ed8 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -45,7 +45,8 @@
 #define c13_TID_URW	23	/* Thread ID, User R/W */
 #define c13_TID_URO	24	/* Thread ID, User R/O */
 #define c13_TID_PRIV	25	/* Thread ID, Privileged */
-#define NR_CP15_REGS	26	/* Number of regs (incl. invalid) */
+#define c14_CNTKCTL	26	/* Timer Control Register (PL1) */
+#define NR_CP15_REGS	27	/* Number of regs (incl. invalid) */
 
 #define ARM_EXCEPTION_RESET	  0
 #define ARM_EXCEPTION_UNDEFINED   1
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 17cea2e..5ce738b 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -179,6 +179,12 @@ 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_CVAL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));
+  DEFINE(KVM_TIMER_CNTVOFF,	offsetof(struct kvm, arch.timer.cntvoff));
+  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 ac72a8f..22f39d6 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -690,6 +690,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();
 
@@ -703,6 +704,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;
 		}
@@ -742,6 +744,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/coproc.c b/arch/arm/kvm/coproc.c
index d782638..4ea9a98 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -222,6 +222,10 @@ static const struct coproc_reg cp15_regs[] = {
 			NULL, reset_unknown, c13_TID_URO },
 	{ CRn(13), CRm( 0), Op1( 0), Op2( 4), is32,
 			NULL, reset_unknown, c13_TID_PRIV },
+
+	/* CNTKCTL: swapped by interrupt.S. */
+	{ CRn(14), CRm( 1), Op1( 0), Op2( 0), is32,
+			NULL, reset_val, c14_CNTKCTL, 0x00000000 },
 };
 
 /* Target specific emulation tables */
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index dde5f8d..57cfa84 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -301,6 +301,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	str	r11, [vcpu, #CP15_OFFSET(c6_IFAR)]
 	str	r12, [vcpu, #CP15_OFFSET(c12_VBAR)]
 	.endif
+
+	mrc	p15, 0, r2, c14, c1, 0	@ CNTKCTL
+
+	.if \store_to_vcpu == 0
+	push	{r2}
+	.else
+	str	r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
+	.endif
 .endm
 
 /*
@@ -312,6 +320,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  */
 .macro write_cp15_state read_from_vcpu
 	.if \read_from_vcpu == 0
+	pop	{r2}
+	.else
+	ldr	r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)]
+	.endif
+
+	mcr	p15, 0, r2, c14, c1, 0	@ CNTKCTL
+
+	.if \read_from_vcpu == 0
 	pop	{r2-r12}
 	.else
 	ldr	r2, [vcpu, #CP15_OFFSET(c13_CID)]
@@ -463,8 +479,29 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * for the host.
  *
  * Assumes vcpu pointer in vcpu reg
+ * Clobbers r2-r4
  */
 .macro save_timer_state
+#ifdef CONFIG_KVM_ARM_TIMER
+	ldr	r4, [vcpu, #VCPU_KVM]
+	ldr	r2, [r4, #KVM_TIMER_ENABLED]
+	cmp	r2, #0
+	beq	1f
+
+	mrc	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+	str	r2, [vcpu, #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
+	ldr	r4, =VCPU_TIMER_CNTV_CVAL
+	add	vcpu, vcpu, r4
+	strd	r2, r3, [vcpu]
+	sub	vcpu, vcpu, r4
+
+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)
@@ -476,6 +513,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * for the host.
  *
  * Assumes vcpu pointer in vcpu reg
+ * Clobbers r2-r4
  */
 .macro restore_timer_state
 	@ Disallow physical timer access for the guest
@@ -484,6 +522,30 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	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, [vcpu, #VCPU_KVM]
+	ldr	r2, [r4, #KVM_TIMER_ENABLED]
+	cmp	r2, #0
+	beq	1f
+
+	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
+	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
+	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
+	isb
+
+	ldr	r4, =VCPU_TIMER_CNTV_CVAL
+	add	vcpu, vcpu, r4
+	ldrd	r2, r3, [vcpu]
+	sub	vcpu, vcpu, r4
+	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
+
+	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
+	and	r2, r2, #3
+	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+	isb
+1:
+#endif
 .endm
 
 .equ vmentry,	0

[PATCH v5 3/4] ARM: KVM: arch_timers: Add timer world switch

[Will Deacon]

On Tue, Jan 08, 2013 at 06:43:27PM +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.

[...]

> @@ -476,6 +513,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * for the host.
>   *
>   * Assumes vcpu pointer in vcpu reg
> + * Clobbers r2-r4
>   */
>  .macro restore_timer_state
>  	@ Disallow physical timer access for the guest
> @@ -484,6 +522,30 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	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, [vcpu, #VCPU_KVM]
> +	ldr	r2, [r4, #KVM_TIMER_ENABLED]
> +	cmp	r2, #0
> +	beq	1f
> +
> +	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
> +	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
> +	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
> +	isb
> +
> +	ldr	r4, =VCPU_TIMER_CNTV_CVAL
> +	add	vcpu, vcpu, r4
> +	ldrd	r2, r3, [vcpu]
> +	sub	vcpu, vcpu, r4
> +	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
> +
> +	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
> +	and	r2, r2, #3
> +	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> +	isb

How many of these isbs are actually needed, given that we're going to make
an exception return to the guest? The last one certainly looks redundant and
I can't see the need for ordering CNTVOFF vs CNTV_CVAL. I can see an
argument to putting one *before* CNTV_CTL, but you don't have one there!

Will

[PATCH v5 3/4] ARM: KVM: arch_timers: Add timer world switch

[Marc Zyngier]

On 14/01/13 15:21, Will Deacon wrote:
> On Tue, Jan 08, 2013 at 06:43:27PM +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.
> 
> [...]
> 
>> @@ -476,6 +513,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>>   * for the host.
>>   *
>>   * Assumes vcpu pointer in vcpu reg
>> + * Clobbers r2-r4
>>   */
>>  .macro restore_timer_state
>>  	@ Disallow physical timer access for the guest
>> @@ -484,6 +522,30 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>>  	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, [vcpu, #VCPU_KVM]
>> +	ldr	r2, [r4, #KVM_TIMER_ENABLED]
>> +	cmp	r2, #0
>> +	beq	1f
>> +
>> +	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
>> +	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
>> +	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
>> +	isb
>> +
>> +	ldr	r4, =VCPU_TIMER_CNTV_CVAL
>> +	add	vcpu, vcpu, r4
>> +	ldrd	r2, r3, [vcpu]
>> +	sub	vcpu, vcpu, r4
>> +	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
>> +
>> +	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>> +	and	r2, r2, #3
>> +	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
>> +	isb
> 
> How many of these isbs are actually needed, given that we're going to make
> an exception return to the guest? The last one certainly looks redundant and
> I can't see the need for ordering CNTVOFF vs CNTV_CVAL. I can see an
> argument to putting one *before* CNTV_CTL, but you don't have one there!

CNTVOFF directly influences whether or not CNTV_CVAL will trigger or
not. Maybe I'm just being paranoid and moving the isb after CNTV_CVAL is
enough.

The last one is definitively superfluous.

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

[PATCH v5 3/4] ARM: KVM: arch_timers: Add timer world switch

[Christoffer Dall]

On Mon, Jan 14, 2013 at 12:51 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 14/01/13 15:21, Will Deacon wrote:
>> On Tue, Jan 08, 2013 at 06:43:27PM +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.
>>
>> [...]
>>
>>> @@ -476,6 +513,7 @@ vcpu     .req    r0              @ vcpu pointer always in r0
>>>   * for the host.
>>>   *
>>>   * Assumes vcpu pointer in vcpu reg
>>> + * Clobbers r2-r4
>>>   */
>>>  .macro restore_timer_state
>>>      @ Disallow physical timer access for the guest
>>> @@ -484,6 +522,30 @@ vcpu    .req    r0              @ vcpu pointer always in r0
>>>      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, [vcpu, #VCPU_KVM]
>>> +    ldr     r2, [r4, #KVM_TIMER_ENABLED]
>>> +    cmp     r2, #0
>>> +    beq     1f
>>> +
>>> +    ldr     r2, [r4, #KVM_TIMER_CNTVOFF]
>>> +    ldr     r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
>>> +    mcrr    p15, 4, r2, r3, c14     @ CNTVOFF
>>> +    isb
>>> +
>>> +    ldr     r4, =VCPU_TIMER_CNTV_CVAL
>>> +    add     vcpu, vcpu, r4
>>> +    ldrd    r2, r3, [vcpu]
>>> +    sub     vcpu, vcpu, r4
>>> +    mcrr    p15, 3, r2, r3, c14     @ CNTV_CVAL
>>> +
>>> +    ldr     r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>>> +    and     r2, r2, #3
>>> +    mcr     p15, 0, r2, c14, c3, 1  @ CNTV_CTL
>>> +    isb
>>
>> How many of these isbs are actually needed, given that we're going to make
>> an exception return to the guest? The last one certainly looks redundant and
>> I can't see the need for ordering CNTVOFF vs CNTV_CVAL. I can see an
>> argument to putting one *before* CNTV_CTL, but you don't have one there!
>
> CNTVOFF directly influences whether or not CNTV_CVAL will trigger or
> not. Maybe I'm just being paranoid and moving the isb after CNTV_CVAL is
> enough.
>
> The last one is definitively superfluous.
>

can't we also get rid of the isb on the return path then?

do you agree with this patch:

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 57cfa84..7e6eedf 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -492,7 +492,6 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	str	r2, [vcpu, #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
 	ldr	r4, =VCPU_TIMER_CNTV_CVAL
@@ -532,18 +531,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
 	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
 	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
-	isb

 	ldr	r4, =VCPU_TIMER_CNTV_CVAL
 	add	vcpu, vcpu, r4
 	ldrd	r2, r3, [vcpu]
 	sub	vcpu, vcpu, r4
 	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
+	isb

 	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
 	and	r2, r2, #3
 	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
-	isb
 1:
 #endif
 .endm
--

Thanks,
-Christoffer

[PATCH v5 3/4] ARM: KVM: arch_timers: Add timer world switch

[Will Deacon]

On Mon, Jan 14, 2013 at 10:08:39PM +0000, Christoffer Dall wrote:
> can't we also get rid of the isb on the return path then?
> 
> do you agree with this patch:
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 57cfa84..7e6eedf 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -492,7 +492,6 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	str	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>  	bic	r2, #1			@ Clear ENABLE
>  	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> -	isb

I'd keep this one as it stops speculation of CVAL until the timer is
disabled.

>  	mrrc	p15, 3, r2, r3, c14	@ CNTV_CVAL
>  	ldr	r4, =VCPU_TIMER_CNTV_CVAL
> @@ -532,18 +531,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
>  	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
>  	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
> -	isb
> 
>  	ldr	r4, =VCPU_TIMER_CNTV_CVAL
>  	add	vcpu, vcpu, r4
>  	ldrd	r2, r3, [vcpu]
>  	sub	vcpu, vcpu, r4
>  	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
> +	isb
> 
>  	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>  	and	r2, r2, #3
>  	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> -	isb

Looks ok to me, but I'll let Marc decide as it's his code.

Will

[PATCH v5 3/4] ARM: KVM: arch_timers: Add timer world switch

[Marc Zyngier]

On 14/01/13 22:08, Christoffer Dall wrote:
> On Mon, Jan 14, 2013 at 12:51 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 14/01/13 15:21, Will Deacon wrote:
>>> On Tue, Jan 08, 2013 at 06:43:27PM +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.
>>>
>>> [...]
>>>
>>>> @@ -476,6 +513,7 @@ vcpu     .req    r0              @ vcpu pointer always in r0
>>>>   * for the host.
>>>>   *
>>>>   * Assumes vcpu pointer in vcpu reg
>>>> + * Clobbers r2-r4
>>>>   */
>>>>  .macro restore_timer_state
>>>>      @ Disallow physical timer access for the guest
>>>> @@ -484,6 +522,30 @@ vcpu    .req    r0              @ vcpu pointer always in r0
>>>>      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, [vcpu, #VCPU_KVM]
>>>> +    ldr     r2, [r4, #KVM_TIMER_ENABLED]
>>>> +    cmp     r2, #0
>>>> +    beq     1f
>>>> +
>>>> +    ldr     r2, [r4, #KVM_TIMER_CNTVOFF]
>>>> +    ldr     r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
>>>> +    mcrr    p15, 4, r2, r3, c14     @ CNTVOFF
>>>> +    isb
>>>> +
>>>> +    ldr     r4, =VCPU_TIMER_CNTV_CVAL
>>>> +    add     vcpu, vcpu, r4
>>>> +    ldrd    r2, r3, [vcpu]
>>>> +    sub     vcpu, vcpu, r4
>>>> +    mcrr    p15, 3, r2, r3, c14     @ CNTV_CVAL
>>>> +
>>>> +    ldr     r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>>>> +    and     r2, r2, #3
>>>> +    mcr     p15, 0, r2, c14, c3, 1  @ CNTV_CTL
>>>> +    isb
>>>
>>> How many of these isbs are actually needed, given that we're going to make
>>> an exception return to the guest? The last one certainly looks redundant and
>>> I can't see the need for ordering CNTVOFF vs CNTV_CVAL. I can see an
>>> argument to putting one *before* CNTV_CTL, but you don't have one there!
>>
>> CNTVOFF directly influences whether or not CNTV_CVAL will trigger or
>> not. Maybe I'm just being paranoid and moving the isb after CNTV_CVAL is
>> enough.
>>
>> The last one is definitively superfluous.
>>
> 
> can't we also get rid of the isb on the return path then?
> 
> do you agree with this patch:
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 57cfa84..7e6eedf 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -492,7 +492,6 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	str	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>  	bic	r2, #1			@ Clear ENABLE
>  	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> -	isb

This isb is required. Otherwise you have no guarantee about how recent
the next CNTV_CVAL read is (could be speculated).

>  	mrrc	p15, 3, r2, r3, c14	@ CNTV_CVAL
>  	ldr	r4, =VCPU_TIMER_CNTV_CVAL
> @@ -532,18 +531,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	ldr	r2, [r4, #KVM_TIMER_CNTVOFF]
>  	ldr	r3, [r4, #(KVM_TIMER_CNTVOFF + 4)]
>  	mcrr	p15, 4, r2, r3, c14	@ CNTVOFF
> -	isb
> 
>  	ldr	r4, =VCPU_TIMER_CNTV_CVAL
>  	add	vcpu, vcpu, r4
>  	ldrd	r2, r3, [vcpu]
>  	sub	vcpu, vcpu, r4
>  	mcrr	p15, 3, r2, r3, c14	@ CNTV_CVAL
> +	isb
> 
>  	ldr	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
>  	and	r2, r2, #3
>  	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
> -	isb
>  1:
>  #endif
>  .endm

This part is OK.

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

[PATCH v5 4/4] 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  |    8 ++++++++
 arch/arm/kvm/Makefile |    1 +
 arch/arm/kvm/arm.c    |   11 +++++++++++
 arch/arm/kvm/vgic.c   |    1 +
 4 files changed, 21 insertions(+)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index d32e33f..739500b 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -59,6 +59,14 @@ 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
+	default y
+	---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 3c6620c..43c4cad 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -20,3 +20,4 @@ obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o guest.o mmu.o emulate.o reset.o
 obj-y += coproc.o coproc_a15.o mmio.o decode.o
 obj-$(CONFIG_KVM_ARM_VGIC) += vgic.o
+obj-$(CONFIG_KVM_ARM_TIMER) += arch_timer.o
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 22f39d6..38ad78a 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -287,6 +287,7 @@ int kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 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);
 }
 
@@ -328,6 +329,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 	if (ret)
 		return ret;
 
+	/* Set up the timer */
+	kvm_timer_vcpu_init(vcpu);
+
 	return 0;
 }
 
@@ -1084,6 +1088,13 @@ static int init_hyp_mode(void)
 		vgic_present = true;
 #endif
 
+	/*
+	 * Init HYP architected timer support
+	 */
+	err = kvm_timer_hyp_init();
+	if (err)
+		goto out_free_mappings;
+
 	kvm_info("Hyp mode initialized successfully\n");
 	return 0;
 out_free_vfp:
diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index 083639b..7eb94fa 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -1405,6 +1405,7 @@ int kvm_vgic_init(struct kvm *kvm)
 	for (i = VGIC_NR_PRIVATE_IRQS; 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);