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 16:36:38 +0200 Message-ID: <20160426143637.GC19789@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> <20160426130045.GB19872@potion> <20160426141610.GG20425@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]:40446 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751845AbcDZOgl (ORCPT ); Tue, 26 Apr 2016 10:36:41 -0400 Content-Disposition: inline In-Reply-To: <20160426141610.GG20425@rkaganb.sw.ru> Sender: kvm-owner@vger.kernel.org List-ID: 2016-04-26 17:16+0300, Roman Kagan: > On Tue, Apr 26, 2016 at 03:00:45PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: >> 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: >> >> > 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= write >> >> the page for the first time, which means there is an extremely un= likely >> >> 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 conte= nts >> > before returning to the guest. As for other VCPUs it's up to the = guest >> > to synchronize access to the page with this VCPU; >>=20 >> One method of synchronization is checking whether the other vcpu alr= eady >> enabled HV_X64_MSR_REFERENCE_TSC by reading the MSR ... the method i= s >> not a clear guest error (though people capable of doing it are going= to >> bug) and we'd have this race >>=20 >> vcpu0 | vcpu1 >> hv->hv_tsc_page =3D data; | *guest rdmsr HV_X64_MSR_REFERENCE= _TSC* >> | data =3D hv->hv_tsc_page; >> | kvm_x86_ops->run(vcpu); >> | *guest reads the page* >> kvm_gen_update_masterclock() | >=20 > I can hardly imagine introducing a clocksource to avoid MSR reads, an= d > synchronizing access to it by reads of another MSR ;) Yes, any sane guest would definitely use memory for that, but synchronization (=3D letting all VCPUs know where the TSC page is prese= nt) is a boot-time only operation and doesn't ruin the idea. Multiple OSes inside one partitioned VM are harder to synchronize, but luckily no-one does that. :) >> Another minor benefit of zeroing TscSequence before writing data is = that >> counting always starts at 0. >=20 > ... which is arguably a minor disadvantage as it would reset the > sequence number on migration. Migration (=3D save/restore) shouldn't write to the MSR. I can see tha= t other VCPUs might write the same value at/after boot time and expect that the counter wouldn't reset, though ... > That said, I don't really feel strong about it, and I'm OK zeroing th= e > tsc page out if you think it worth while. Nah, it turned out that guests can bug both ways, so let's keep it uninitialized.