From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 3/3] x86/kvm: implement Hyper-V reference TSC page clock Date: Tue, 26 Apr 2016 15:00:45 +0200 Message-ID: <20160426130045.GB19872@potion> References: <1461258686-28161-1-git-send-email-rkagan@virtuozzo.com> <1461258686-28161-4-git-send-email-rkagan@virtuozzo.com> <20160425205412.GA19872@potion> <20160426090222.GB20425@rkaganb.sw.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE To: Roman Kagan , kvm@vger.kernel.org, Paolo Bonzini , Marcelo Tosatti , "Denis V. Lunev" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43462 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752549AbcDZNAu (ORCPT ); Tue, 26 Apr 2016 09:00:50 -0400 Content-Disposition: inline In-Reply-To: <20160426090222.GB20425@rkaganb.sw.ru> Sender: kvm-owner@vger.kernel.org List-ID: 2016-04-26 12:02+0300, Roman Kagan: > On Mon, Apr 25, 2016 at 10:54:12PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: >> 2016-04-21 20:11+0300, Roman Kagan: >> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> > @@ -797,23 +798,11 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu= *vcpu, u32 msr, u64 data, >> > mark_page_dirty(kvm, gfn); >> > break; >> > } >> > + case HV_X64_MSR_REFERENCE_TSC: >>=20 >> (Would be nicer to check for HV_X64_MSR_REFERENCE_TSC_AVAILABLE.) >=20 > Hmm, interesting point. This is a jugdement call, whether we should > refuse processing this MSR if we didn't announce its support to the > guest in the respective cpuid leaf (I personally don't think so). We > don't do it for a number of other MSRs, if we should then it probably > has to be a separate patch fixing all of them. Ok. >> > hv->hv_tsc_page =3D data; >> > + if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) >> > + kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); >>=20 >> The MSR value is global and will be seen by other VCPUs before we wr= ite >> the page for the first time, which means there is an extremely unlik= ely >> race that could read random data from a guest page and interpret it = as >> time. Initialization before setting hv_tsc_page would be fine. >=20 > KVM_REQ_MASTERCLOCK_UPDATE will make sure the page has valid contents > before returning to the guest. Yes. > before returning to the guest. As for other VCPUs it's up to the gue= st > to synchronize access to the page with this VCPU; One method of synchronization is checking whether the other vcpu alread= y enabled HV_X64_MSR_REFERENCE_TSC by reading the MSR ... the method is not a clear guest error (though people capable of doing it are going to bug) and we'd have this race vcpu0 | vcpu1 hv->hv_tsc_page =3D data; | *guest rdmsr HV_X64_MSR_REFERENCE_TS= C* | data =3D hv->hv_tsc_page; | kvm_x86_ops->run(vcpu); | *guest reads the page* kvm_gen_update_masterclock() | Another minor benefit of zeroing TscSequence before writing data is tha= t counting always starts at 0. (The code doesn't handle remapping anyway.) > we can't prevent th= em > from reading it before we return to the guest. (Yeah, it's not impossible, but we don't want to.) >> (Also, TLFS 4.0b says that the guest can pick any frame in the GPA >> space. The guest could specify a frame that wouldn't be mapped in = KVM >> and the guest would fail for no good reason. HyperV's "overlay pag= es" >> likely don't read or overwrite content of mapped frames either. >> I think it would be safer to create a new mapping for the page ...) >=20 > I've never seen this happen; if this is really possible we'll have to= do > more (e.g. the migration of the contents of this page won't happen > automatically). I'll double-check with the spec, thanks. Thanks, I was reading mainly 8.1.3 GPA Overlay Pages. Guests probably don't utilize that, but all overlay pages would have this bug, so I'm ok with ignoring it for now too.