Re: [PATCH 06/13] x86/hvm: Scale host TSC when setting/getting guest TSC

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 10/27/2015 09:10 AM, Boris Ostrovsky wrote:
> On 10/27/2015 04:44 AM, Haozhong Zhang wrote:
>> On Thu, Oct 22, 2015 at 08:17:29AM -0600, Jan Beulich wrote:
>>>>>> On 28.09.15 at 09:13, <haozhong.zhang@intel.com> wrote:
>>>> The existing hvm_set_guest_tsc_fixed() and hvm_get_guest_tsc_fixed()
>>>> calculate the guest TSC by adding the TSC offset to the host TSC. When
>>>> the TSC scaling is enabled, the host TSC should be scaled first. This
>>>> patch adds the scaling logic to those two functions.
>>> Just like mentioned for the first twp patches - I'd first of all 
>>> like to
>>> understand why the lack of scaling this wasn't an issue for SVM so
>>> far. What you reads plausible, but assuming that SVM TSC scaling
>>> code was tested, I'm hesitant to apply changes to it without
>>> understanding the details (or at least without SVM maintainers'
>>> consent).
>>>
>> Hi SVM maintainers,
>>
>> Could you help to review this patch 6 as well as patch 2? They intend
>> to fix bugs in SVM TSC ratio code (or code that affects SVM TSC ratio
>> code).
>>
>> The detailed explanations of patch 2 and patch 6 can be found at
>> http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg01490.html 
>>
>> and
>> http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg02843.html 
>>
>> respectively.
>
> I agree with patch 2 (so you can add my Reviewed-by).
>
>
>  but I am not so sure about patch 6 (and 11, together with existing 
> SVM handlers).
>
>
> I don't have latest Intel's manual handy but for SVM the guest TSC 
> value is calculated as scaled host TSC plus *unscaled* VMCB's TSC 
> offset. Is Intel's implementation similar?
>
> Both svm_set_tsc_offset and vmx_set_tsc_offset (as proposed in patch 
> 11) write VMCB/VMCS with guest (i.s. scaled) offset, and that doesn't 
> seem right.

(I seem to have lost Haozhong on my previous email)

No, it is right (or, rather, since TSC offset is a constant it can be 
used as scaled or unscaled, depending on how you want to implement it).

So if you adjust patch 11 (and corresponding SVM code) to take scaling 
out from there this patch would be correct.

Of course I still can't test this since the two machines that I have 
available are running at fairly close frequencies.


-boris



>
> If I am right then I think (1) SVM is broken now and (2) patches 6 and 
> 11 don't fix this brokenness and instead propagate it to VMX.
>
> (I should have thought about this when I last replied to you asking to 
> move scaling out of vmx_set_tsc_offset(). But I re-read this code now 
> and it doesn't make sense to me anymore).
>
> -boris
>
>
>>
>> Thanks,
>> Haozhong
>>
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -388,13 +388,12 @@ void hvm_set_guest_tsc_fixed(struct vcpu *v, 
>>>> u64 guest_tsc, u64 at_tsc)
>>>>           tsc = hvm_get_guest_time_fixed(v, at_tsc);
>>>>           tsc = gtime_to_gtsc(v->domain, tsc);
>>>>       }
>>>> -    else if ( at_tsc )
>>>> -    {
>>>> -        tsc = at_tsc;
>>>> -    }
>>>>       else
>>>>       {
>>>> -        tsc = rdtsc();
>>>> +        tsc = at_tsc ? at_tsc : rdtsc();
>>> In cases like this please prefer the gcc extension allowing the middle
>>> operand of the ?: to be omitted.
>>>
>>> Jan
>>>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel