[PATCH v3] x86/time: correctly honor late clearing of TSC related feature flags

[PATCH v3] x86/time: correctly honor late clearing of TSC related feature flags

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

As such clearing of flags may have an impact on the selected rendezvous
function, defer the establishing of a rendezvous function other than
the initial default one (std) until after all APs have been brought up.

But don't allow such feature flags to be cleared during CPU hotplug:
Platform and local system times may have diverged significantly by
then, potentially causing noticeably (even if only temporary) strange
behavior. As we're anyway expecting only sufficiently similar CPUs to
appear during hotplug, this shouldn't be introducing new limitations.

Reported-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Drop original approach entirely - defer everything to
    verify_tsc_reliability(), making for quite a bit smaller a patch.

Note: Considering that tsc_check_writability() checks
      TSC_RELIABLE, it being run before that feature flag has obtained
      its final value seems problematic too. Should we defer that call
      too?

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1638,9 +1638,18 @@ static int __init verify_tsc_reliability
 
             printk("Switched to Platform timer %s TSC\n",
                    freq_string(plt_src.frequency));
+            return 0;
         }
     }
 
+    /*
+     * While with constant-rate TSCs the scale factor can be shared, when TSCs
+     * are not marked as 'reliable', re-sync during rendezvous.
+     */
+    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
+         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
+        time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
+
     return 0;
 }
 __initcall(verify_tsc_reliability);
@@ -1650,14 +1659,6 @@ int __init init_xen_time(void)
 {
     tsc_check_writability();
 
-    /* If we have constant-rate TSCs then scale factor can be shared. */
-    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) )
-    {
-        /* If TSCs are not marked as 'reliable', re-sync during rendezvous. */
-        if ( !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
-            time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
-    }
-
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
     /* NB. get_cmos_time() can take over one second to execute. */




[-- Attachment #2: x86-time-late-feature-disable.patch --]
[-- Type: text/plain, Size: 2385 bytes --]

x86/time: correctly honor late clearing of TSC related feature flags

As such clearing of flags may have an impact on the selected rendezvous
function, defer the establishing of a rendezvous function other than
the initial default one (std) until after all APs have been brought up.

But don't allow such feature flags to be cleared during CPU hotplug:
Platform and local system times may have diverged significantly by
then, potentially causing noticeably (even if only temporary) strange
behavior. As we're anyway expecting only sufficiently similar CPUs to
appear during hotplug, this shouldn't be introducing new limitations.

Reported-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Drop original approach entirely - defer everything to
    verify_tsc_reliability(), making for quite a bit smaller a patch.

Note: Considering that tsc_check_writability() checks
      TSC_RELIABLE, it being run before that feature flag has obtained
      its final value seems problematic too. Should we defer that call
      too?

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1638,9 +1638,18 @@ static int __init verify_tsc_reliability
 
             printk("Switched to Platform timer %s TSC\n",
                    freq_string(plt_src.frequency));
+            return 0;
         }
     }
 
+    /*
+     * While with constant-rate TSCs the scale factor can be shared, when TSCs
+     * are not marked as 'reliable', re-sync during rendezvous.
+     */
+    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
+         !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
+        time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
+
     return 0;
 }
 __initcall(verify_tsc_reliability);
@@ -1650,14 +1659,6 @@ int __init init_xen_time(void)
 {
     tsc_check_writability();
 
-    /* If we have constant-rate TSCs then scale factor can be shared. */
-    if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) )
-    {
-        /* If TSCs are not marked as 'reliable', re-sync during rendezvous. */
-        if ( !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
-            time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
-    }
-
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
     /* NB. get_cmos_time() can take over one second to execute. */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v3] x86/time: correctly honor late clearing of TSC related feature flags

[Andrew Cooper]

On 06/02/17 08:39, Jan Beulich wrote:
> As such clearing of flags may have an impact on the selected rendezvous
> function, defer the establishing of a rendezvous function other than
> the initial default one (std) until after all APs have been brought up.
>
> But don't allow such feature flags to be cleared during CPU hotplug:
> Platform and local system times may have diverged significantly by
> then, potentially causing noticeably (even if only temporary) strange
> behavior. As we're anyway expecting only sufficiently similar CPUs to
> appear during hotplug, this shouldn't be introducing new limitations.
>
> Reported-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v3: Drop original approach entirely - defer everything to
>     verify_tsc_reliability(), making for quite a bit smaller a patch.
>
> Note: Considering that tsc_check_writability() checks
>       TSC_RELIABLE, it being run before that feature flag has obtained
>       its final value seems problematic too. Should we defer that call
>       too?

Do we know which CPUs this applies to?  Might we be safe by being only
64bit these days?

~Andrew

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

Re: [PATCH v3] x86/time: correctly honor late clearing of TSC related feature flags

[Jan Beulich]

>>> On 07.02.17 at 11:27, <andrew.cooper3@citrix.com> wrote:
> On 06/02/17 08:39, Jan Beulich wrote:
>> As such clearing of flags may have an impact on the selected rendezvous
>> function, defer the establishing of a rendezvous function other than
>> the initial default one (std) until after all APs have been brought up.
>>
>> But don't allow such feature flags to be cleared during CPU hotplug:
>> Platform and local system times may have diverged significantly by
>> then, potentially causing noticeably (even if only temporary) strange
>> behavior. As we're anyway expecting only sufficiently similar CPUs to
>> appear during hotplug, this shouldn't be introducing new limitations.
>>
>> Reported-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> v3: Drop original approach entirely - defer everything to
>>     verify_tsc_reliability(), making for quite a bit smaller a patch.
>>
>> Note: Considering that tsc_check_writability() checks
>>       TSC_RELIABLE, it being run before that feature flag has obtained
>>       its final value seems problematic too. Should we defer that call
>>       too?
> 
> Do we know which CPUs this applies to?  Might we be safe by being only
> 64bit these days?

amd.c sets it when ITSC and model != 0x11, which I think includes
64-bit CPUs. intel.c sets it unconditionally when ITSC. However, I
don't see how all this matters, as we clear it as the result of the
TSC warp producing a non-zero tsc_max_warp, and that check is
purely a software thing.

Jan


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

Re: [PATCH v3] x86/time: correctly honor late clearing of TSC related feature flags

[Joao Martins]

On 02/07/2017 10:47 AM, Jan Beulich wrote:
>>>> On 07.02.17 at 11:27, <andrew.cooper3@citrix.com> wrote:
>> On 06/02/17 08:39, Jan Beulich wrote:
>>> As such clearing of flags may have an impact on the selected rendezvous
>>> function, defer the establishing of a rendezvous function other than
>>> the initial default one (std) until after all APs have been brought up.
>>>
>>> But don't allow such feature flags to be cleared during CPU hotplug:
>>> Platform and local system times may have diverged significantly by
>>> then, potentially causing noticeably (even if only temporary) strange
>>> behavior. As we're anyway expecting only sufficiently similar CPUs to
>>> appear during hotplug, this shouldn't be introducing new limitations.
>>>
>>> Reported-by: Joao Martins <joao.m.martins@oracle.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Thanks.
> 
>>> ---
>>> v3: Drop original approach entirely - defer everything to
>>>     verify_tsc_reliability(), making for quite a bit smaller a patch.
>>>
>>> Note: Considering that tsc_check_writability() checks
>>>       TSC_RELIABLE, it being run before that feature flag has obtained
>>>       its final value seems problematic too. Should we defer that call
>>>       too?
>>
>> Do we know which CPUs this applies to?  Might we be safe by being only
>> 64bit these days?
> 
> amd.c sets it when ITSC and model != 0x11, which I think includes
> 64-bit CPUs. intel.c sets it unconditionally when ITSC. However, I
> don't see how all this matters, as we clear it as the result of the
> TSC warp producing a non-zero tsc_max_warp, and that check is
> purely a software thing.

Considering that TSC_RELIABLE bit (or the lack of it) is only truly valid after
tsc reliability checks have been done, then it probably makes sense to move the
writability check after verify_tsc_reliability (or at the end of it) one as you
seem to hint above.

Joao

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