On Mon, Jan 14, 2013 at 12:51 PM, Marc Zyngier [off-list ref] wrote:

quoted

On 14/01/13 15:21, Will Deacon wrote:

quoted

On Tue, Jan 08, 2013 at 06:43:27PM +0000, Christoffer Dall wrote:

quoted

From: Marc Zyngier <redacted>

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

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