From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM:x86: avoid VMCS access from non-vCPU context Date: Thu, 3 Nov 2016 18:10:00 +0100 Message-ID: References: <1477933936-3681-1-git-send-email-rkagan@virtuozzo.com> <1613603670.9850787.1477956713227.JavaMail.zimbra@redhat.com> <20161103141737.GA31710@rkaganb.sw.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit To: Roman Kagan , kvm@vger.kernel.org, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , "Denis V. Lunev" , Luiz Capitulino Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37690 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759115AbcKCRKF (ORCPT ); Thu, 3 Nov 2016 13:10:05 -0400 In-Reply-To: <20161103141737.GA31710@rkaganb.sw.ru> Sender: kvm-owner@vger.kernel.org List-ID: 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 > Tested-by: Roman Kagan 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