2016-04-26 17:16+0300, Roman Kagan: > 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. 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 ;) Yes, any sane guest would definitely use memory for that, but synchronization (= letting all VCPUs know where the TSC page is present) 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. > > ... which is arguably a minor disadvantage as it would reset the > sequence number on migration. Migration (= save/restore) shouldn't write to the MSR. I can see that 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 the > 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. -- 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
