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.
--
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
