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

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

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:
>> > 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:
>> 
>> (Would be nicer to check for HV_X64_MSR_REFERENCE_TSC_AVAILABLE.)
> 
> 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 = 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() |

Another minor benefit of zeroing TscSequence before writing data is that
counting always starts at 0.
(The code doesn't handle remapping anyway.)

>                                                   we can't prevent them
> 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 pages"
>>  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 ...)
> 
> 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.