On Tue, Apr 26, 2016 at 03:00:45PM +0200, Radim Krčmář wrote: > 2016-04-26 12:02+0300, Roman Kagan: > > On Mon, Apr 25, 2016 at 10:54:12PM +0200, Radim Krčmář wrote: > >> 2016-04-21 20:11+0300, Roman Kagan: > >> > hv->hv_tsc_page = data; > >> > + if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) > >> > + kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); > >> > >> 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 unlikely > >> race that could read random data from a guest page and interpret it as > >> time. Initialization before setting hv_tsc_page would be fine. > > > > 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 guest > > to synchronize access to the page with this VCPU; > > One method of synchronization is checking whether the other vcpu already > 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 = data; | *guest rdmsr HV_X64_MSR_REFERENCE_TSC* > | data = hv->hv_tsc_page; > | kvm_x86_ops->run(vcpu); > | *guest reads the page* > kvm_gen_update_masterclock() | I can hardly imagine introducing a clocksource to avoid MSR reads, and synchronizing access to it by reads of another MSR ;) > Another minor benefit of zeroing TscSequence before writing data is that > counting always starts at 0. ... which is arguably a minor disadvantage as it would reset the sequence number on migration. That said, I don't really feel strong about it, and I'm OK zeroing the tsc page out if you think it worth while. Roman. -- 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
