Re: [Xen-devel] [PATCH 5/8] x86/time: correctly honor late clearing of TSC related feature flags

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.08.16 at 21:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/07/16 16:53, Jan Beulich wrote:
>>>>> On 04.07.16 at 17:39, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 20/06/16 16:20, Jan Beulich wrote:
>>>>>>> On 20.06.16 at 16:32, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> On 15/06/16 11:28, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/time.c
>>>>>> +++ b/xen/arch/x86/time.c
>>>>>> @@ -1358,6 +1358,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;
>>>>> This should read time_calibration_rendezvous_fn rather than assuming
>>>>> time_calibration_std_rendezvous.
>>>>>
>>>>> Otherwise, there is a risk that it could be reset back to
>>>>> time_calibration_std_rendezvous.
>>>> But that's the purpose: We may need to switch back.
>>> Under what circumstances could we ever move from re-syncing back to not
>>> re-syncing?
>> verify_tsc_reliability() may result in X86_FEATURE_TSC_RELIABLE
>> getting cleared. That's an initcall, which means it runs after
>> init_xen_time(), and hence after the rendezvous function got
>> established initially.
> 
> Right, but that isn't important.
> 
> There will never be a case where, once TSC_RELIABLE is cleared, it is
> safe to revert back to std_rendezvous, even if TSC_RELIABLE is
> subsequently re-set.

You've got this backwards: TSC_RELIABLE may get _cleared_ late.
Nothing can ever set it late, due to the use of setup_clear_cpu_cap().
Reverting back to time_calibration_std_rendezvous() would only be
possible if CONSTANT_TSC got cleared late, ...

> Therefore, this function must never accidentally revert
> time_calibration_rendezvous_fn from time_calibration_tsc_rendezvous back
> to time_calibration_std_rendezvous.

... yet I can't see what would be wrong with that especially when
that still happened at boot time. Or else how would it be safe to
switch the other direction? (If neither switch was safe, then all we
could do upon finding the TSC unreliable in verify_tsc_reliability()
would be to panic().)

Jan


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