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

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

[Jan Beulich]

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

As such clearing of flags may have an impact on the selected rendezvous
function, handle such in a central place.

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 noticably (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>
Tested-by: Dario Faggioli <dario.faggioli@citrix.com>
Tested-by: Joao Martins <joao.m.martins@oracle.com>
---
The resend is mainly to get the discussion going again on what the
alternatives are, if this patch is not acceptable.

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1162,8 +1162,8 @@ static int mwait_idle_cpu_init(struct no
 		}
 
 		if (state > 2 && !boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
-		    !pm_idle_save)
-			setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE);
+		    !pm_idle_save && system_state < SYS_STATE_active)
+			clear_tsc_cap(X86_FEATURE_TSC_RELIABLE);
 
 		cx = dev->states + dev->count;
 		cx->type = state;
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1465,6 +1465,24 @@ static void time_calibration(void *unuse
                      &r, 1);
 }
 
+void __init clear_tsc_cap(unsigned int feature)
+{
+    void (*rendezvous_fn)(void *) = time_calibration_std_rendezvous;
+
+    if ( feature )
+        setup_clear_cpu_cap(feature);
+
+    /* 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) )
+            rendezvous_fn = time_calibration_tsc_rendezvous;
+    }
+
+    time_calibration_rendezvous_fn = rendezvous_fn;
+}
+
 static struct cpu_time_stamp ap_bringup_ref;
 
 void time_latch_stamps(void)
@@ -1611,7 +1629,7 @@ static int __init verify_tsc_reliability
         if ( tsc_max_warp )
         {
             printk("TSC warp detected, disabling TSC_RELIABLE\n");
-            setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE);
+            clear_tsc_cap(X86_FEATURE_TSC_RELIABLE);
         }
         else if ( !strcmp(opt_clocksource, "tsc") &&
                   (try_platform_timer(&plt_tsc) > 0) )
@@ -1650,13 +1668,7 @@ 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;
-    }
+    clear_tsc_cap(0);
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -71,6 +71,7 @@ void force_update_vcpu_system_time(struc
 
 bool clocksource_is_tsc(void);
 int host_tsc_is_safe(void);
+void clear_tsc_cap(unsigned int feature);
 void cpuid_time_leaf(uint32_t sub_idx, uint32_t *eax, uint32_t *ebx,
                      uint32_t *ecx, uint32_t *edx);
 




[-- Attachment #2: x86-time-late-feature-disable.patch --]
[-- Type: text/plain, Size: 3500 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, handle such in a central place.

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 noticably (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>
Tested-by: Dario Faggioli <dario.faggioli@citrix.com>
Tested-by: Joao Martins <joao.m.martins@oracle.com>
---
The resend is mainly to get the discussion going again on what the
alternatives are, if this patch is not acceptable.

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1162,8 +1162,8 @@ static int mwait_idle_cpu_init(struct no
 		}
 
 		if (state > 2 && !boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
-		    !pm_idle_save)
-			setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE);
+		    !pm_idle_save && system_state < SYS_STATE_active)
+			clear_tsc_cap(X86_FEATURE_TSC_RELIABLE);
 
 		cx = dev->states + dev->count;
 		cx->type = state;
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1465,6 +1465,24 @@ static void time_calibration(void *unuse
                      &r, 1);
 }
 
+void __init clear_tsc_cap(unsigned int feature)
+{
+    void (*rendezvous_fn)(void *) = time_calibration_std_rendezvous;
+
+    if ( feature )
+        setup_clear_cpu_cap(feature);
+
+    /* 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) )
+            rendezvous_fn = time_calibration_tsc_rendezvous;
+    }
+
+    time_calibration_rendezvous_fn = rendezvous_fn;
+}
+
 static struct cpu_time_stamp ap_bringup_ref;
 
 void time_latch_stamps(void)
@@ -1611,7 +1629,7 @@ static int __init verify_tsc_reliability
         if ( tsc_max_warp )
         {
             printk("TSC warp detected, disabling TSC_RELIABLE\n");
-            setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE);
+            clear_tsc_cap(X86_FEATURE_TSC_RELIABLE);
         }
         else if ( !strcmp(opt_clocksource, "tsc") &&
                   (try_platform_timer(&plt_tsc) > 0) )
@@ -1650,13 +1668,7 @@ 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;
-    }
+    clear_tsc_cap(0);
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -71,6 +71,7 @@ void force_update_vcpu_system_time(struc
 
 bool clocksource_is_tsc(void);
 int host_tsc_is_safe(void);
+void clear_tsc_cap(unsigned int feature);
 void cpuid_time_leaf(uint32_t sub_idx, uint32_t *eax, uint32_t *ebx,
                      uint32_t *ecx, uint32_t *edx);
 

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

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

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

[Jan Beulich]

>>> On 06.12.16 at 11:51, <JBeulich@suse.com> wrote:
> As such clearing of flags may have an impact on the selected rendezvous
> function, handle such in a central place.
> 
> 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 noticably (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>
> Tested-by: Dario Faggioli <dario.faggioli@citrix.com>
> Tested-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> The resend is mainly to get the discussion going again on what the
> alternatives are, if this patch is not acceptable.

Even if you don't agree with the patch, can we at least revive
the discussion of what alternatives there are?

Jan


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

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

[Andrew Cooper]

On 15/12/16 09:49, Jan Beulich wrote:
>>>> On 06.12.16 at 11:51, <JBeulich@suse.com> wrote:
>> As such clearing of flags may have an impact on the selected rendezvous
>> function, handle such in a central place.
>>
>> 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 noticably (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>
>> Tested-by: Dario Faggioli <dario.faggioli@citrix.com>
>> Tested-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> The resend is mainly to get the discussion going again on what the
>> alternatives are, if this patch is not acceptable.
> Even if you don't agree with the patch, can we at least revive
> the discussion of what alternatives there are?

Sorry - it slipped through the cracks.  I have no issue with the
principle of the patch.

The only problem I have, which we didn't sort out last time, is the
initialisation of rendezvous_fn

It is still my opinion that under no circumstance is it ok for
clear_tsc_cap() to modify time_calibration_rendezvous_fn from
time_calibration_tsc_rendezvous to time_calibration_std_rendezvous,
which can in principle happen because rendezvous_fn doesn't get
initialised from the current time_calibration_rendezvous_fn.

~Andrew

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

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

[Jan Beulich]

>>> On 15.12.16 at 12:04, <andrew.cooper3@citrix.com> wrote:
> On 15/12/16 09:49, Jan Beulich wrote:
>>>>> On 06.12.16 at 11:51, <JBeulich@suse.com> wrote:
>>> As such clearing of flags may have an impact on the selected rendezvous
>>> function, handle such in a central place.
>>>
>>> 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 noticably (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>
>>> Tested-by: Dario Faggioli <dario.faggioli@citrix.com>
>>> Tested-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>> The resend is mainly to get the discussion going again on what the
>>> alternatives are, if this patch is not acceptable.
>> Even if you don't agree with the patch, can we at least revive
>> the discussion of what alternatives there are?
> 
> Sorry - it slipped through the cracks.  I have no issue with the
> principle of the patch.
> 
> The only problem I have, which we didn't sort out last time, is the
> initialisation of rendezvous_fn
> 
> It is still my opinion that under no circumstance is it ok for
> clear_tsc_cap() to modify time_calibration_rendezvous_fn from
> time_calibration_tsc_rendezvous to time_calibration_std_rendezvous,
> which can in principle happen because rendezvous_fn doesn't get
> initialised from the current time_calibration_rendezvous_fn.

I understand that this is your primary reservation against the patch,
yet at the same time this is also the purpose of the patch. If we're
not allowed to change the rendezvous function to what it is supposed
to be given the final set of CPU features we determined, then the
whole patch is pointless. At the time we first set the pointer, we
simply don't know yet what we'll find once we brought up APs. If we
knew, we'd set it differently. Since pre- and post-AP bringup
knowledge will always have the potential to differ, we need to adjust
the function pointer in one direction anyway: Either we set it to std
early, and switch to tsc later, or we allow setting it to tsc early,
reverting to std if need be.

Jan


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

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

[Andrew Cooper]

On 15/12/2016 15:09, Jan Beulich wrote:
>>>> On 15.12.16 at 12:04, <andrew.cooper3@citrix.com> wrote:
>> On 15/12/16 09:49, Jan Beulich wrote:
>>>>>> On 06.12.16 at 11:51, <JBeulich@suse.com> wrote:
>>>> As such clearing of flags may have an impact on the selected rendezvous
>>>> function, handle such in a central place.
>>>>
>>>> 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 noticably (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>
>>>> Tested-by: Dario Faggioli <dario.faggioli@citrix.com>
>>>> Tested-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>> The resend is mainly to get the discussion going again on what the
>>>> alternatives are, if this patch is not acceptable.
>>> Even if you don't agree with the patch, can we at least revive
>>> the discussion of what alternatives there are?
>> Sorry - it slipped through the cracks.  I have no issue with the
>> principle of the patch.
>>
>> The only problem I have, which we didn't sort out last time, is the
>> initialisation of rendezvous_fn
>>
>> It is still my opinion that under no circumstance is it ok for
>> clear_tsc_cap() to modify time_calibration_rendezvous_fn from
>> time_calibration_tsc_rendezvous to time_calibration_std_rendezvous,
>> which can in principle happen because rendezvous_fn doesn't get
>> initialised from the current time_calibration_rendezvous_fn.
> I understand that this is your primary reservation against the patch,
> yet at the same time this is also the purpose of the patch. If we're
> not allowed to change the rendezvous function to what it is supposed
> to be given the final set of CPU features we determined, then the
> whole patch is pointless. At the time we first set the pointer, we
> simply don't know yet what we'll find once we brought up APs. If we
> knew, we'd set it differently. Since pre- and post-AP bringup
> knowledge will always have the potential to differ, we need to adjust
> the function pointer in one direction anyway: Either we set it to std
> early, and switch to tsc later, or we allow setting it to tsc early,
> reverting to std if need be.

Once we have ever had cause to use time_calibration_tsc_rendezvous,
there is no situation where it is safe to switch back to
time_calibration_std_rendezvous, as the choice to use
time_calibration_tsc_rendezvous means the TSCs aren't in sync, and Xen
has no practical mechanism to resync them.  (We could guarantee not to
ever invoke Cstates, including C1E, but this doesn't prevent an SMI
doing that for us.)

time_calibration_rendezvous_fn should start with the best case scenario,
and be gradually made worse by our AP discovery, but should never get
better.

~Andrew

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

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

[Jan Beulich]

>>> On 20.12.16 at 13:35, <andrew.cooper3@citrix.com> wrote:
> Once we have ever had cause to use time_calibration_tsc_rendezvous,
> there is no situation where it is safe to switch back to
> time_calibration_std_rendezvous, as the choice to use
> time_calibration_tsc_rendezvous means the TSCs aren't in sync, and Xen
> has no practical mechanism to resync them.  (We could guarantee not to
> ever invoke Cstates, including C1E, but this doesn't prevent an SMI
> doing that for us.)

Okay, I think I'm finally following you. Yet ...

> time_calibration_rendezvous_fn should start with the best case scenario,
> and be gradually made worse by our AP discovery, but should never get
> better.

... that's already the case with the patch: CONSTANT_TSC can only
be cleared by command line option (i.e. before any of this code runs)
or in tsc_check_writability() (which runs before clear_tsc_cap() gets
invoked first). Hence the possibility of switching back from tsc to std
depends solely on TSC_RELIABLE potentially getting set during/after
AP bringup. Yet that never happens, we only ever clear that flag
(which is part of the purpose of clear_tsc_cap()).

Bottom line - I don't see how you think we may end up switching back
from tsc to std. Perhaps it is then all just a matter of changing a few
names and/or adding a BUG_ON() or panic() to make things more clear
to you and possible other readers?

Jan


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

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

[Andrew Cooper]

On 05/01/17 14:42, Jan Beulich wrote:
>>>> On 20.12.16 at 13:35, <andrew.cooper3@citrix.com> wrote:
>> Once we have ever had cause to use time_calibration_tsc_rendezvous,
>> there is no situation where it is safe to switch back to
>> time_calibration_std_rendezvous, as the choice to use
>> time_calibration_tsc_rendezvous means the TSCs aren't in sync, and Xen
>> has no practical mechanism to resync them.  (We could guarantee not to
>> ever invoke Cstates, including C1E, but this doesn't prevent an SMI
>> doing that for us.)
> Okay, I think I'm finally following you. Yet ...
>
>> time_calibration_rendezvous_fn should start with the best case scenario,
>> and be gradually made worse by our AP discovery, but should never get
>> better.
> ... that's already the case with the patch: CONSTANT_TSC can only
> be cleared by command line option (i.e. before any of this code runs)
> or in tsc_check_writability() (which runs before clear_tsc_cap() gets
> invoked first). Hence the possibility of switching back from tsc to std
> depends solely on TSC_RELIABLE potentially getting set during/after
> AP bringup. Yet that never happens, we only ever clear that flag
> (which is part of the purpose of clear_tsc_cap()).
>
> Bottom line - I don't see how you think we may end up switching back
> from tsc to std. Perhaps it is then all just a matter of changing a few
> names and/or adding a BUG_ON() or panic() to make things more clear
> to you and possible other readers?

My concern is what happens if some code plays with the feature flags,
then re-calls clear_tsc_cap()

With your proposal, this will be unsafe as it loads the wrong rendezvous
function.

With my proposal, there is no amount of playing with feature flags and
recalling clear_tsc_cap() which can end up reverting to a weaker
rendezvous function.

~Andrew

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