This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


RE: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_T

>>> On 13.05.11 at 09:28, "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
>>  From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx] 
>> Sent: Friday, May 13, 2011 3:14 PM
>> >>> On 13.05.11 at 07:55, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
>> > On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
>> >
>> >> x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
>> >>
>> >> 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
>> >> tsc msr is not writtable on some old platform, which however also
>> >> adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
>> >> The two don't match as tsc writtable-ness has nothing to do with
>> >> whether it's reliable. As long as Xen can use tsc as the time source
>> >> and it's writable, it should be OK to continue using deep cstate with
>> >> tsc save/restore.
>> >
>> > Looks like I just got the assertion the wrong way round, should be
>> > ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
>> No, the assertion is correct imo (since tsc_check_writability() bails 
> immediately
>> when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
> here we may need a definition about what a reliable TSC means here. no tsc 
> skew
> among cpus, or stably incremented on the bus clock? It looks that we have 
> some
> assumption behind various TSC flags, and use them with implicit assumptions.
> Here I can see that tsc_check_writability may disable deep cstate when it's 
> not
> writable, but it doesn't mean that the assertion on X86_FEATURE_TSC_RELIABLE
> is correct since even when this flag is cleared the tsc could still be 
> writable. That
> way the assertion absolutely is wrong.
>> But the problem Kevin reports is exactly what I expected when we discussed
>> the whole change. Nevertheless, simply removing the assertion won't be
>> correct - instead your addition of the early bail out on TSC_RELIABLE is 
>> what 
> I'd
>> now put under question (the comment that goes with it, as we now see, isn't
>> correct).
> I still don't understand why deep cstate must be disabled when TSC is not 
> reliable.
> Also the early bail out doesn't impact my error, since in my case 
> not set but it's simply writable.

My point is that for the assertion to be removed, the early bail in
tsc_check_writability() must be removed too.


Xen-devel mailing list