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

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

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.

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