Question about patch "svm: fix incorrect TSC scaling"

Question about patch "svm: fix incorrect TSC scaling"

[Dongli Zhang]

Hi Haozhong and AMD SVM maintainers,

I am writing to ask about below patch as I am not familiar with AMD tsc
scaling:

 commit 11eeca65126e51f03a883907751d5ccbe4f35aa3
 Author: Haozhong Zhang <haozhong.zhang@intel.com>
 Date:   Tue Dec 8 09:46:12 2015 +0100

     svm: fix incorrect TSC scaling
 
     SVM TSC ratio is incorrectly used in the current
     svm_get_tsc_offset(). This patch replaces the scaling logic in
     svm_get_tsc_offset() with a correct implementation.
 
     Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
     Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


The below equation is used in the above patch:

-------------------------
host_tsc * ratio * 2^-32.
-------------------------


While the below incorrect equation (indeed code) was used before above patch:

---------------------------------------------------------------
offset = (((host_tsc >> 32U) * (ratio >> 32U)) << 32U) +
         (host_tsc & 0xffffffffULL) * (ratio & 0xffffffffULL);
return guest_tsc - offset;
---------------------------------------------------------------

Can I summarize the above code as:

tsc_h * mult * 2^32 + tsc_l * frag or

tsc_h * int * 2^32 + tsc_l * frag ?


Would you please help confirm why the equation is replaced? Or would you please
help and let me know the chapter/page of AMD reference manual showing the
correct equation?

Thank you very much!

Dongli Zhang

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Question about patch "svm: fix incorrect TSC scaling"

[Jan Beulich]

>>> On 28.05.18 at 14:25, <dongli.zhang@oracle.com> wrote:
> I am writing to ask about below patch as I am not familiar with AMD tsc
> scaling:
> 
>  commit 11eeca65126e51f03a883907751d5ccbe4f35aa3
>  Author: Haozhong Zhang <haozhong.zhang@intel.com>
>  Date:   Tue Dec 8 09:46:12 2015 +0100
> 
>      svm: fix incorrect TSC scaling
>  
>      SVM TSC ratio is incorrectly used in the current
>      svm_get_tsc_offset(). This patch replaces the scaling logic in
>      svm_get_tsc_offset() with a correct implementation.
>  
>      Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>      Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> 
> The below equation is used in the above patch:
> 
> -------------------------
> host_tsc * ratio * 2^-32.
> -------------------------
> 
> 
> While the below incorrect equation (indeed code) was used before above 
> patch:
> 
> ---------------------------------------------------------------
> offset = (((host_tsc >> 32U) * (ratio >> 32U)) << 32U) +
>          (host_tsc & 0xffffffffULL) * (ratio & 0xffffffffULL);
> return guest_tsc - offset;
> ---------------------------------------------------------------
> 
> Can I summarize the above code as:
> 
> tsc_h * mult * 2^32 + tsc_l * frag or
> 
> tsc_h * int * 2^32 + tsc_l * frag ?
> 
> 
> Would you please help confirm why the equation is replaced? Or would you please
> help and let me know the chapter/page of AMD reference manual showing the
> correct equation?

This has nothing to do with AMD's doc, but only with maths: To split
a multiplication x * y when considering x = xh + xl (high and low parts)
and y = yh + yl you get x * y = xh * yh + xh * yl + xl * yh + xl * xl.
Width constraints make it unnecessary to calculate the xh * yh part
here. The original expression, however, calculated something rather
strange instead (only the xl * yl part was actually correct).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Question about patch "svm: fix incorrect TSC scaling"

[Dongli Zhang]

On 05/29/2018 05:56 PM, Jan Beulich wrote:
>>>> On 28.05.18 at 14:25, <dongli.zhang@oracle.com> wrote:
>> I am writing to ask about below patch as I am not familiar with AMD tsc
>> scaling:
>>
>>  commit 11eeca65126e51f03a883907751d5ccbe4f35aa3
>>  Author: Haozhong Zhang <haozhong.zhang@intel.com>
>>  Date:   Tue Dec 8 09:46:12 2015 +0100
>>
>>      svm: fix incorrect TSC scaling
>>  
>>      SVM TSC ratio is incorrectly used in the current
>>      svm_get_tsc_offset(). This patch replaces the scaling logic in
>>      svm_get_tsc_offset() with a correct implementation.
>>  
>>      Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>      Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>>
>> The below equation is used in the above patch:
>>
>> -------------------------
>> host_tsc * ratio * 2^-32.
>> -------------------------
>>
>>
>> While the below incorrect equation (indeed code) was used before above 
>> patch:
>>
>> ---------------------------------------------------------------
>> offset = (((host_tsc >> 32U) * (ratio >> 32U)) << 32U) +
>>          (host_tsc & 0xffffffffULL) * (ratio & 0xffffffffULL);
>> return guest_tsc - offset;
>> ---------------------------------------------------------------
>>
>> Can I summarize the above code as:
>>
>> tsc_h * mult * 2^32 + tsc_l * frag or
>>
>> tsc_h * int * 2^32 + tsc_l * frag ?
>>
>>
>> Would you please help confirm why the equation is replaced? Or would you please
>> help and let me know the chapter/page of AMD reference manual showing the
>> correct equation?
> 
> This has nothing to do with AMD's doc, but only with maths: To split
> a multiplication x * y when considering x = xh + xl (high and low parts)
> and y = yh + yl you get x * y = xh * yh + xh * yl + xl * yh + xl * xl.
> Width constraints make it unnecessary to calculate the xh * yh part
> here. The original expression, however, calculated something rather
> strange instead (only the xl * yl part was actually correct).

Thank you very much for the explanation!

Now it is clear to me.

Dongli Zhang

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Question about patch "svm: fix incorrect TSC scaling"

[Dongli Zhang]

On 05/29/2018 06:23 PM, Dongli Zhang wrote:
> 
> 
> On 05/29/2018 05:56 PM, Jan Beulich wrote:
>>>>> On 28.05.18 at 14:25, <dongli.zhang@oracle.com> wrote:
>>> I am writing to ask about below patch as I am not familiar with AMD tsc
>>> scaling:
>>>
>>>  commit 11eeca65126e51f03a883907751d5ccbe4f35aa3
>>>  Author: Haozhong Zhang <haozhong.zhang@intel.com>
>>>  Date:   Tue Dec 8 09:46:12 2015 +0100
>>>
>>>      svm: fix incorrect TSC scaling
>>>  
>>>      SVM TSC ratio is incorrectly used in the current
>>>      svm_get_tsc_offset(). This patch replaces the scaling logic in
>>>      svm_get_tsc_offset() with a correct implementation.
>>>  
>>>      Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>>      Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>
>>>
>>> The below equation is used in the above patch:
>>>
>>> -------------------------
>>> host_tsc * ratio * 2^-32.
>>> -------------------------

Indeed, I have an extra question that why 2^-32 is involved here?

Is it because we would like to ignore the frag part and only consider the
leading integer part? Why not just use "host_tsc * ratio"?

Thank you very much!

Dongli Zhang


>>>
>>>
>>> While the below incorrect equation (indeed code) was used before above 
>>> patch:
>>>
>>> ---------------------------------------------------------------
>>> offset = (((host_tsc >> 32U) * (ratio >> 32U)) << 32U) +
>>>          (host_tsc & 0xffffffffULL) * (ratio & 0xffffffffULL);
>>> return guest_tsc - offset;
>>> ---------------------------------------------------------------
>>>
>>> Can I summarize the above code as:
>>>
>>> tsc_h * mult * 2^32 + tsc_l * frag or
>>>
>>> tsc_h * int * 2^32 + tsc_l * frag ?
>>>
>>>
>>> Would you please help confirm why the equation is replaced? Or would you please
>>> help and let me know the chapter/page of AMD reference manual showing the
>>> correct equation?
>>
>> This has nothing to do with AMD's doc, but only with maths: To split
>> a multiplication x * y when considering x = xh + xl (high and low parts)
>> and y = yh + yl you get x * y = xh * yh + xh * yl + xl * yh + xl * xl.
>> Width constraints make it unnecessary to calculate the xh * yh part
>> here. The original expression, however, calculated something rather
>> strange instead (only the xl * yl part was actually correct).
> 
> Thank you very much for the explanation!
> 
> Now it is clear to me.
> 
> Dongli Zhang
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Question about patch "svm: fix incorrect TSC scaling"

[Jan Beulich]

>>> On 29.05.18 at 12:31, <dongli.zhang@oracle.com> wrote:

> 
> On 05/29/2018 06:23 PM, Dongli Zhang wrote:
>> 
>> 
>> On 05/29/2018 05:56 PM, Jan Beulich wrote:
>>>>>> On 28.05.18 at 14:25, <dongli.zhang@oracle.com> wrote:
>>>> I am writing to ask about below patch as I am not familiar with AMD tsc
>>>> scaling:
>>>>
>>>>  commit 11eeca65126e51f03a883907751d5ccbe4f35aa3
>>>>  Author: Haozhong Zhang <haozhong.zhang@intel.com>
>>>>  Date:   Tue Dec 8 09:46:12 2015 +0100
>>>>
>>>>      svm: fix incorrect TSC scaling
>>>>  
>>>>      SVM TSC ratio is incorrectly used in the current
>>>>      svm_get_tsc_offset(). This patch replaces the scaling logic in
>>>>      svm_get_tsc_offset() with a correct implementation.
>>>>  
>>>>      Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>>>      Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>
>>>>
>>>> The below equation is used in the above patch:
>>>>
>>>> -------------------------
>>>> host_tsc * ratio * 2^-32.
>>>> -------------------------
> 
> Indeed, I have an extra question that why 2^-32 is involved here?
> 
> Is it because we would like to ignore the frag part and only consider the
> leading integer part? Why not just use "host_tsc * ratio"?

Quoting the PM: "The TSC Ratio MSR specifies the TSCRatio value as a
fixed-point binary number in 8.32 format, ..." IOW this is to convert
ratio from its presentation (uint64_t) to its purpose (a fixed point value
with 8 [really 32] bits representing the integer part and 32 bits of
fraction). We very certainly don't want to ignore the fractional part.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel