Re: [RFC PATCH 1/2] Hyper-H reference counter

[Vadim Rozenfeld <vrozenfe@redhat.com>]

On Mon, 2013-05-20 at 10:56 +0200, Paolo Bonzini wrote:
> Il 20/05/2013 10:49, Gleb Natapov ha scritto:
> > On Mon, May 20, 2013 at 10:42:52AM +0200, Paolo Bonzini wrote:
> >> Il 20/05/2013 10:36, Gleb Natapov ha scritto:
> >>> On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote:
> >>>> Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto:
> >>>>> On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote:
> >>>>>> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Yes, I have this check added in the second patch.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>> Move it here please.
> >>>>>>>>>>> OK, will do it.
> >>>>>>>>>
> >>>>>>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this
> >>>>>>>>> patch, and leave it all to the second.
> >>>>>>>>>
> >>>>>>> What for? Could you please elaborate?
> >>>>>
> >>>>> To make code reviewable.  Add one MSR here, the other in the second patch.
> >>>>> removing HV_X64_MSR_REFERENCE_TSC will make this particular patch
> >>>>> completely non-functional.
> >>>>
> >>>> Do you mean Windows guest will BSOD or just that they won't use the
> >>>> reference TSC?  If the latter, it's not a problem.
> >>>>
> >>> I think it is. If reference counter works without TSC we have a bisect
> >>> point for the case when something will going wrong with TSC.
> >>
> >> Isn't that exactly what might happen with this patch only?  Windows will
> >> not use the TSC because it finds invalid values in the TSC page.
> > 
> > Yes, it will use reference counter instead. Exactly what we want for a bisect point.
> > 
> >>                                                                  If it
> >> still uses the reference counter, we have the situation you describe.
> >>
> >>     refcount        TSC page
> >>         Y              Y           <= after patch 2
> >>         Y              N           <= after patch 1
> >>         N              Y           <= impossible
> >>         N              N           <= removing TSC page from this patch?
> >>
> >> Of course if the guest BSODs, it's not possible to split the patches
> >> that way.  Perhaps in that case it's simply better to do a single patch.
> >>
> > I am not sure what you are trying to say. Your option list above shows
> > that there is a value to split patches like they are split now.
> 
> Hmm, we're talking past each other. :)
> 
> I put the "?" because that's what Vadim implied ("it would make this
> particular patch non-functional"), but I don't see why it should be like
> this.  To me, the obvious way of getting the desired bisect point is
> implementing one MSR per patch.  So, moving the REFERENCE_TSC handling
> entirely to patch 2 would still be in the "refcount=Y, TSC page=N" case.
> 
> In any case, this patch needs more comments and a better commit message.
>  Microsoft docs are decent, but there are several non-obvious points in
> how the patches were done, and they need to be documented.
We need specify two partition privileges to activate reference time
enlightenment in HYPERV_CPUID_FEATURES (0x40000003)
AccessPartitionReferenceCounter and AccessPartitionReferenceTsc
otherwise VM will use HPET or PMTimer as a timestamp source.
If we specify AccessPartitionReferenceTsc but don't handle write 
request to HV_X64_MSR_REFERENCE_TSC - the system will fail with 0x78
(PHASE0_EXCEPTION) bugcheck code. If we provide HV_X64_MSR_REFERENCE_TSC
handler but don't initialize sequence to 0 - guest will probably newer
start or will be extremely slow, because in this case scale should also
be initialized. Sequence 0 is a special case, it means use reference
counter, but not TSC, as a time source. It is also a fallback solution
in a case when a VM, which using TSC has been migrated to a host, which
is not equipped with invariant TSC.