[RFC PATCH] KVM: arm/arm64: Don't let userspace update CNTVOFF once guest is running

On 26.06.2015 06:49, Jan Kiszka wrote:

On 2015-06-25 11:25, Claudio Fontana wrote:

quoted

On 25.06.2015 11:10, Peter Maydell wrote:

quoted

On 25 June 2015 at 09:59, Claudio Fontana [off-list ref] wrote:

quoted

Once the VM is created, I think QEMU should not request kvm to
change the virtual offset of the VM anymore: maybe an unexpected
consequence of QEMU's target-arm/kvm64.c::kvm_arch_put_registers ?

Hmm. In general we assume that we can:
 * stop the VM
 * read all the guest system registers
 * write those values back again
 * restart the VM

if we need to. Is that what's happening here, or are we doing
something odder?

-- PMM

What I guess could be happening by looking at the code in linux

virt/kvm/arm/arch_timer.c::kvm_arm_timer_set_reg

is that QEMU tries to set the KVM_REG_ARM_TIMER_CNT register from exactly the previous value,
but just because of the fact that the set function is called, cntvoff is updated,
since the value provided by the user is apparently assumed to be _relative_ to the physical timer.

This is apparent to me in the code in that function which says:

case KVM_REG_ARM_TIMER_CNT: {
/* ... */
    u64 cntvoff = kvm_phys_timer_read() - value;
/* ... */
}

And this is matched by the corresponding get function kvm_arm_timer_get_reg where it says:

case KVM_REG_ARM_TIMER_CNT:
   return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;

The time difference between when the GET is issued by QEMU and when the PUT is issued then would account for the difference.

QEMU has the concept of write-back levels: KVM_PUT_RUNTIME_STATE,
KVM_PUT_RESET_STATE and KVM_PUT_FULL_STATE. I suspect this registers is
just sorted into the wrong category, thus written as part of the
RUNTIME_STATE. We had such bug patterns during the x86 maturing phase as
well.

Jan

It seems that QEMU target-arm ignores the level parameter to kvm_arch_put_registers completely.

Is it intended?

Actually even the first set of the cntvoff is wrong, although it does not cause any problems.

When the VM is created, KVM already sets the cntvoff, so even putting the state once at RESET is I think wrong: the VM already has a CNTVOFF when it is created, so trying to change it afterwards even once should be avoided I think.

But if the RFC is committed as is to kvm, it's fine by me also.

Ciao

Claudio