On Mon, Jan 14, 2013 at 12:51 PM, Marc Zyngier <marc.zyng...@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.zyng...@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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
