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

[Claudio Fontana <claudio.fontana@huawei.com>]

On 26.06.2015 06:49, Jan Kiszka wrote:
> On 2015-06-25 11:25, Claudio Fontana wrote:
>> On 25.06.2015 11:10, Peter Maydell wrote:
>>> On 25 June 2015 at 09:59, Claudio Fontana <claudio.fontana@huawei.com> wrote:
>>>> 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