Re: [PATCH] KVM:x86: avoid VMCS access from non-vCPU context

[Paolo Bonzini <pbonzini@redhat.com>]

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@virtuozzo.com>
> Tested-by: Roman Kagan <rkagan@virtuozzo.com>

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