From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] x86: don't write_tsc() non-zero values on CPUs updating only the lower 32 bits Date: Thu, 14 Apr 2011 10:18:18 +0100 Message-ID: References: <4DA6C735020000780003B8C0@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4DA6C735020000780003B8C0@vpn.id2.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: Dan Magenheimer , "xen-devel@lists.xensource.com" , "winston.l.wang" List-Id: xen-devel@lists.xenproject.org On 14/04/2011 09:06, "Jan Beulich" wrote: >> For physically-added CPUs only. Kind of unavoidable, that one: we can only >> try to do our best in that case. And let's face it, that probably affects >> exactly zero production users of Xen/x86 right now. > > That latter part I agree to. > > But what are you afraid of? Probing the TSC write shouldn't do any > harm. You will end up with a BSP TSC value different than what it would have been if you had not probed. Since you write back a (slightly) stale TSC value. Which would increase cross-CPU TSC skew if the platform has done a good sync job at power-on. Now, do I personally think this much matters? Not really, since I believe that direct guest TSC access is a huge non-issue. But it is something that Dan disagreed on, he did a bunch of work on time management, and the code as-is does try quite hard to avoid writing TSC if at all possible. I don't think it's a good idea to change this without feedback from Dan, at least. > Additionally, did you read the comment immediately preceding > the probing code? AMD doesn't guarantee the TSC to be writable at > all. > >>> cstate_restore_tsc() also has no such gating afaics. >> >> It is gated on NONSTOP_TSC which is implied by TSC_RELIABLE. > > Ah, yes. But (I think) not architecturally, only by virtue of how > code is currently structured. If that changes, we'd be back at a > latent (and quite non-obvious) bug. Yeah, if we want to continue to try avoiding write_tsc() on TSC_RELIABLE then we should assert !TSC_RELIABLE on the write_tsc() path in cstate_tsc_restore(). -- Keir