Re: [PATCH 3/3] x86/kvm: implement Hyper-V reference TSC page clock

["Radim Krčmář" <rkrcmar@redhat.com>]

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.