linux-kernel - Re: [patch] x86: convert rdtscll() to use __native_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081216101551.GA27481@elte.hu>
Date:	Tue, 16 Dec 2008 11:15:51 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Jeremy Fitzhardinge <jeremy@...p.org>
Cc:	Ken Chen <kenchen@...gle.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [patch] x86: convert rdtscll() to use __native_read_tsc


* Jeremy Fitzhardinge <jeremy@...p.org> wrote:

> Ingo Molnar wrote:
>> The reason for the __native_read_tsc() / native_read_tsc() distinction 
>> is and obscure problem with paravirt function pointers. Such 
>> constructs:
>>
>>   ./xen/enlighten.c:	.read_tsc = native_read_tsc,
>>
>> do not always work fine with all versions of gcc, if native_read_tsc() 
>> is a simple static inline (as it should be) - the build would fail with 
>> certain gcc flags.
>
> I don't think that's true.  We rely on taking function pointers of 
> static inlines pretty extensively; native_read_tsc is hardly unique in 
> this respect.  I don't remember seeing any problems of the sort you 
> describe.  (I can well believe this may have been a problem at some 
> point, but not during the pv-ops development timeframe.)

i do remember build and boot failures there - with weird combos of gcc 
options. It's a clear GCC bug. Anyway, we can clean this up and we'll see 
how relevant the failure modes are.

>> Perhaps the real fix is to do this rename as well:
>>
>>     native_read_tsc => native_read_tsc_paravirt
>>   __native_read_tsc => native_read_tsc
>>
>> as this makes the native_read_tsc_paravirt() a pure technical variant, 
>> to be used in paravirt_ops function pointer assignments. People would 
>> thus just use the obvious native_read_tsc() inline function most of the 
>> time and could forget about native_read_tsc_paravirt().
>>
>> Jeremy?
>
> I'm trying to remember the real reason for 
> __native_read_tsc/native_read_tsc.  At least part of it is that 
> __native_read_tsc is used in a vdso, and so *must* be inlined to avoid a 
> bogus call from user to kernel space.  But I don't know why you wouldn't 
> want to inline native_read_tsc everywhere.  I have a feeling it may be a 
> relic from unification - possibly because x86-64 was late to the 
> clocksource party - but I don't remember anything specific.
>
> I think we can probably make do with a single native_read_tsc, so long 
> as its always inlined.

agreed mostly, with this twist: vdso inlining dependencies should be 
expressed explicitly, via:

  native_vread_tsc()

but we can also make native_read_tsc() __always_inline [it's a single 
instruction with basically no preparatory halo around that instruction] 
and document the vdso detail there.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/