Re: [PATCH] x86: don't write_tsc() non-zero values on CPUs updating only the lower 32 bits

[Keir Fraser <keir.xen@gmail.com>]

On 14/04/2011 19:33, "Wang, Winston L" <winston.l.wang@intel.com> wrote:

> Jan and Keir,
> 
> Great efforts for turning the test code re-validate the processor with
> (X86_FEATURE_TSC_RELIABLE) set. We know for sure that the broken old
> processors any write TSC will Zero the upper 32 bit. So can we move this test
> code as early as possible, say immediately after the early Processor init code
> where checking CPU id and set X86_FEATURE_TSC_RELIABLE?

For our meaning of TSC_RELIABLE, there's no need for its setting to rely on
TSC 64-bit writability. I think leaving it where it is in init_xen_time() is
probably fine.

 -- Keir

> Thanks,
> 
> Winston,
> 
>> -----Original Message-----
>> From: Keir Fraser [mailto:keir.xen@gmail.com]
>> Sent: Thursday, April 14, 2011 9:48 AM
>> To: Jan Beulich
>> 
>> On 14/04/2011 17:28, "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>> 
>>>> I also simplified the actual writability check itself. I couldn't
>> figure out
>>>> what the benefit of your more complex approach would be. In fact it
>> looked
>>>> like it wouldn't work if bit 32 was set already in the TSC counter,
>> as then
>>>> you would write back an unmodified TSC (and in fact you would detect
>> the
>>>> wrong way round, as you'd see a big delta if the write silently
>> cleared bit
>>>> 32 (and bits 33-63)). And the final write of tsc+4*delta, wasn't
>> sure what
>>>> that was about either! But if you can explain why your test is
>> better I'd be
>>>> happy to use it as you originally wrote it.
>>> 
>>> So you were concerned about getting the TSC slightly off, and now
>>> you flush it to zero, without any attempt to restore the original
>>> value?
>> 
>> Haha, well it doesn't matter too much if we sync TSCs as we bring them
>> online anyway. But I agree it makes sense to try if we are only able to
>> write the lower 32 bits -- we can at least hope the write test happens
>> while
>> TSC counter is a 32-bit value anyway, and at least we've had a best-
>> effort
>> attempt to keep TSCs in sync.
>> 
>>> As to my original test being broken - I don't think that was the case:
>>> The first write used (u32)tsc as the input, so the two writes, if
>>> happening completely, would be certain to be apart by
>>> approximately 1<<32 (or more, depending on what was in the
>>> upper half originally).
>> 
>> Ah yes, I missed the importance of the (u32)tsc write. Fair enough,
>> your way
>> is better. :-)
>> 
>>> The only case not handled was if the TSC
>>> overflowed 64 bits during the process - I considered this case
>>> hypothetical only.
>>> 
>>> The final write of tsc+4*delta was basically an attempt to restore
>>> the value that would have been there if we didn't fiddle with it.
>> 
>> But the write is actually tsc + 4*(s32)(tmp-tsc), and tmp has 1U<<32
>> ORed
>> into it (because it was read after your second write to the TSC.
>> Perhaps we
>> should just write back the full original tsc and call that good enough?
>> 
>>  -- Keir
>> 
>> 
>>> The factor 4 was sort of invented, on the basis that the delta was
>>> between one write and an immediate read back, with there being
>>> a total of four such windows (read->write or write->read). As
>>> one wouldn't get it precise anyway, the number seemed fine to
>>> me, though just writing back the original values probably wouldn't
>>> have been much worse.
>> 
>>> Jan
>>> 
>> 
>