On 03/11/2016 15:17, Roman Kagan wrote:
> On Mon, Oct 31, 2016 at 07:31:53PM -0400, Paolo Bonzini wrote:
>>
>>> In commit 108b249c453dd7132599ab6dc7e435a7036c193f, "KVM: x86: introduce
>>> get_kvmclock_ns", a function to obtain the time as would be done by the
>>> guest via kvmclock was introduced and used throughout the code.
>>>
>>> The problem is that it reads the guest TSC value via kvm_read_l1_tsc
>>> which can only be called in vCPU context as it reads VMCS(TSC_OFFSET)
>>> directly (on Intel CPUs). Therefore, when called in kvm_arch_vm_ioctl
>>> for KVM_[GS]ET_CLOCK, it returns bogus values, breaking save/restore.
>>
>> The patch isn't good unfortunately, because only __get_kvmclock_ns
>> can return the pvclock value.
>
> Agreed.
>
>> However, after Luiz's commit a545ab6a0085
>> ("kvm: x86: add tsc_offset field to struct kvm_vcpu_arch", 2016-09-07)
>> the TSC offset is cached outside the VMCS/VMCB
>
> I was tempted to use it but saw it not updated in
> adjust_tsc_offset_* and thought it had some different meaning.
> Apparently that was just a bug which you also fix in your patch by
> making adjust_tsc_offset_guest go through kvm_vcpu_write_tsc_offset.
>
>> and all the complexity in
>> vmx.c and (to a lesser extent) svm.c can be dropped. See the following
>> untested patch...
>>
>
> Consider it
>
> Reviewed-by: Roman Kagan <rkagan@xxxxxxxxxxxxx>
> Tested-by: Roman Kagan <rkagan@xxxxxxxxxxxxx>
Too bad, I've already sent out the pull request so I couldn't include
the tags. But thanks very much for the careful review and for testing!
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
