On 17/09/15 16:02, Paolo Bonzini wrote:
>
>
> On 17/09/2015 16:46, Marc Zyngier wrote:
>> When running a guest with the architected timer disabled (with QEMU and
>> the kernel_irqchip=off option, for example), it is important to make
>> sure the timer gets turned off. Otherwise, the guest may try to
>> enable it anyway, leading to a screaming HW interrupt.
>>
>> The fix is to unconditionally turn off the virtual timer on guest
>> exit.
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm64/kvm/hyp.S | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 39aa322..60a83e2 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -562,8 +562,6 @@
>> mrs x3, cntv_ctl_el0
>> and x3, x3, #3
>> str w3, [x0, #VCPU_TIMER_CNTV_CTL]
>> - bic x3, x3, #1 // Clear Enable
>> - msr cntv_ctl_el0, x3
>>
>> isb
>>
>> @@ -571,6 +569,9 @@
>> str x3, [x0, #VCPU_TIMER_CNTV_CVAL]
>>
>> 1:
>> + // Disable the virtual timer
>> + msr cntv_ctl_el0, xzr
>> +
>> // Allow physical timer/counter access for the host
>> mrs x2, cnthctl_el2
>> orr x2, x2, #3
>>
>
> It looks like here in restore_timer_state:
>
> ldr w2, [x0, #VCPU_TIMER_CNTV_CTL]
> and x2, x2, #3
> msr cntv_ctl_el0, x2
>
> the "and" would be unnecessary if kvm_arm_timer_set_reg remembered to
> do it. Something like this, which would also make the code more
> consistent between arm and arm64...
>
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 702740d37465..93e322b4d242 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -514,6 +514,7 @@ ARM_BE8(rev r6, r6 )
> beq 1f
>
> mrc p15, 0, r2, c14, c3, 1 @ CNTV_CTL
> + and r2, r2, #3
I don't think we need this. Exposing the ISTATUS bit to the kernel (or
even userspace) is not really a problem (that's actually an interesting
piece of information), and restoring it is not possible since it is
read-only.
We should drop the equivalent 'and' from the arm64 version.
> str r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
> bic r2, #1 @ Clear ENABLE
> mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL
> @@ -566,7 +567,6 @@ ARM_BE8(rev r6, r6 )
> isb
>
> ldr r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
> - and r2, r2, #3
> mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL
> 1:
> .endm
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 10915aaf0b01..bfcd3f3a947b 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -887,7 +887,6 @@ alternative_endif
> isb
>
> ldr w2, [x0, #VCPU_TIMER_CNTV_CTL]
> - and x2, x2, #3
> msr cntv_ctl_el0, x2
> 1:
> .endm
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 98c95f2fcba4..9b03c9f5abbf 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -218,7 +218,7 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
>
> switch (regid) {
> case KVM_REG_ARM_TIMER_CTL:
> - timer->cntv_ctl = value;
> + timer->cntv_ctl = value & (ARCH_TIMER_CTRL_IT_MASK | ARCH_TIMER_CTRL_ENABLE);
> break;
> case KVM_REG_ARM_TIMER_CNT:
> vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
>
Otherwise looks pretty good. Can you send an updated patch?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
