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

[Paolo Bonzini <pbonzini@redhat.com>]

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.