Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin <igor.druzhinin@citrix.com>]

On 15/01/2020 13:23, Roger Pau Monné wrote:
> On Wed, Jan 15, 2020 at 12:36:08PM +0000, Igor Druzhinin wrote:
>> On 15/01/2020 09:47, Roger Pau Monné wrote:
>>> On Tue, Jan 14, 2020 at 07:36:21PM +0000, Igor Druzhinin wrote:
>>>> If ITSC is not available on CPU (e.g if running nested as PV shim)
>>>> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
>>>> all AMD and some old Intel processors. In which case TSC would need to
>>>> be restored on CPU from platform time by Xen upon exiting deep C-states.
>>>>
>>>> As platform time might be behind the last TSC stamp recorded for the
>>>> current CPU, invariant of TSC stamp being always behind local TSC counter
>>>> is violated. This has an effect of get_s_time() going negative resulting
>>>> in eventual system hang or crash.
>>>>
>>>> Fix this issue by updating local TSC stamp along with TSC counter write.
>>>
>>> Thanks! I haven't seen such issue because I've been running the shim
>>> with nomigrate in order to prevent the vTSC overhead.
>>>
>>>>
>>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>>> ---
>>>> This caused reliable hangs of shim domains with multiple vCPUs on all AMD
>>>> systems. The problem got also reproduced on bare-metal by artifically
>>>> masking ITSC feature bit. The proposed fix has been verified for both
>>>> cases.
>>>> ---
>>>>  xen/arch/x86/time.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
>>>> index e79cb4d..f6b26f8 100644
>>>> --- a/xen/arch/x86/time.c
>>>> +++ b/xen/arch/x86/time.c
>>>> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
>>>>  
>>>>  void cstate_restore_tsc(void)
>>>>  {
>>>> +    struct cpu_time *t = &this_cpu(cpu_time);
>>>> +
>>>>      if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>>>>          return;
>>>>  
>>>> -    write_tsc(stime2tsc(read_platform_stime(NULL)));
>>>> +    t->stamp.master_stime = read_platform_stime(NULL);
>>>> +    t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
>>>> +    t->stamp.local_stime = t->stamp.master_stime;
>>>> +
>>>> +    write_tsc(t->stamp.local_tsc);
>>>
>>> In order to avoid the TSC write (and the likely associated vmexit),
>>> could you instead do:
>>>
>>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
>>> t->stamp.local_tsc = rdtsc_ordered();
>>
>> I think in that case RDTSC might return something behind platform time
>> which is not right I guess.
> 
> The TSC and the platform time are completely independent from Xen's
> PoV, you can have a platform time not based on the TSC (ie: PIT, HPET
> or PM), and hence there's no direct relation between both.
> 
> The TSC is used as a way to get an approximate platform time based on
> the last platform time value and the TSC delta between than value and
> the current TSC value, I assume that's done because reading the TSC is
> much cheaper than reading the platform time.
> 
> As long as the platform time and the TSC stamps are both updated at the
> same time it should be fine.

I see your point. I'll test your approach and get back here with the results.

Igor

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