KVM/ARM Architected Timers support

From: Marc Zyngier <[email protected]>

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

arch/arm/kvm/arch_timer.c new file +29 -0

+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) {
+               /*

Quoted 11 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

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

[...]

Quoted 33 lines

(quoted diff) +33 -0

+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);

Review

Quoted 6 lines

(quoted diff) +6 -0

+               return;
+       }
+
+       ns = cyclecounter_cyc2ns(timecounter->cc, cval - now);
+       timer_arm(timer, ns);
+}

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

Quoted 48 lines · Will Deacon

Quoted 11 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

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

[...]

Quoted 33 lines

(quoted diff) +33 -0

+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);

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+               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 <[email protected]>
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.

Quoted 64 lines · Christoffer Dall

Quoted 48 lines · Will Deacon

Quoted 11 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

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

[...]

Quoted 33 lines

(quoted diff) +33 -0

+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);

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+               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 <[email protected]>
Date:   Mon Jan 14 14:16:31 2013 -0500

Quoted 4 lines

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.

From: Marc Zyngier <[email protected]>

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.

arch/arm/kvm/interrupts_head.S +13 -0

@@ -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
+

Quoted 5 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

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.

[...]

Quoted 31 lines

arch/arm/kvm/interrupts_head.S +20 -0

@@ -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]

arch/arm/kvm/interrupts_head.S vcpu .req r0 @ vcpu pointer always in r0 · line 546

Quoted 3 lines

arch/arm/kvm/interrupts_head.S +3 -0

+	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!

Quoted 40 lines · Will Deacon

Quoted 5 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

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.

[...]

Quoted 31 lines

(quoted diff) +20 -0

@@ -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]

Quoted 8 lines

Review vcpu .req r0 @ vcpu pointer always in r0 · line 546

Quoted 3 lines

(quoted diff) +3 -0

+    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.

arch/arm/kvm/interrupts_head.S +0 -1

 	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

Quoted 50 lines · Marc Zyngier

Quoted 40 lines · Will Deacon

Quoted 5 lines · Christoffer Dall

From: Marc Zyngier <[email protected]>

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.

[...]

Quoted 31 lines

(quoted diff) +20 -0

@@ -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]

Quoted 8 lines

Review vcpu .req r0 @ vcpu pointer always in r0 · line 546

Quoted 3 lines

(quoted diff) +3 -0

+    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!

Quoted 6 lines

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:

arch/arm/kvm/interrupts_head.S +0 -0

index 57cfa84..7e6eedf 100644

arch/arm/kvm/interrupts_head.S +0 -1

 	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