| 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
| ||
|
Message-ID: <20090731195705.GA12270@elte.hu>
Date: Fri, 31 Jul 2009 21:57:05 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
John Stultz <johnstul@...ibm.com>
Cc: "H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Wei Chong Tan <wei.chong.tan@...el.com>
Subject: Re: [GIT PULL] Additional x86 fixes for 2.6.31-rc5
* Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Fri, 31 Jul 2009, H. Peter Anvin wrote:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86-fixes-for-linus
> >
> > Tan, Wei Chong (1):
> > x86: SMI workaround for pit_expect_msb
>
> I really think that last one (commit fb34a8ee86) is
> incorrect.
>
> We do _not_ want to do that SMI workaround in the loop,
> and the logic of that patch is broken anyway.
>
> If an SMI kicks in, then it's true that tscp will be off,
> but that's what the error term is there for. If the error
> term is sufficiently small, then we don't care.
>
> Sure, we might want the error term to be even smaller, but
> in no way does it actually invalidate any of the logic -
> the 'tsc' reading is just a guess anyway. Also, I think
> that the real issue isn't even an SMI - but the fact that
> in the very last iteration of the loop, there's no
> serializing instruction _after_ the last 'rdtsc'. So even
> in the absense of SMI's, we do have a situation where the
> cycle counter was read without proper serialization.
>
> So I think the patch does fix something, I just think it's
> done the wrong way.
>
> The last check shouldn't be done the way that commit does
> it. It should be done outside the outer loop, since
> _inside_ the outer loop, we'll be testing that the PIT has
> the right MSB value has the right value in the next
> iteration.
>
> So only the _last_ iteration is special, because that's
> the one that will not check the PIT MSB value any more,
> and because the final 'get_cycles()' isn't serialized.
>
> In other words:
> - I'd like to move the PIT MSB check to after the last
> iteration, rather
> than in every iteration
>
> - I think we should comment on the fact that it's also a
> serializing
> instruction and so 'fences in' the TSC read.
>
> Here's a suggested replacement - BUT NOTE THAT IT'S
> ENTIRELY UNTESTED!
>
> (Ok, so the patch is bigger than it strictly needs to be -
> I hate repeating code, so I made that 'read PIT counter
> twice and compare MSB' be a new helper routine)
>
> Hmm? What do you guys think?
>
> Linus
>
> ---
> arch/x86/kernel/tsc.c | 29 ++++++++++++++++++++++-------
> 1 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 6e1a368..71f4368 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -275,15 +275,20 @@ static unsigned long pit_calibrate_tsc(u32 latch, unsigned long ms, int loopmin)
> * use the TSC value at the transitions to calculate a pretty
> * good value for the TSC frequencty.
> */
> +static inline int pit_verify_msb(unsigned char val)
> +{
> + /* Ignore LSB */
> + inb(0x42);
> + return inb(0x42) == val;
> +}
> +
> static inline int pit_expect_msb(unsigned char val, u64 *tscp, unsigned long *deltap)
> {
> int count;
> u64 tsc = 0;
>
> for (count = 0; count < 50000; count++) {
> - /* Ignore LSB */
> - inb(0x42);
> - if (inb(0x42) != val)
> + if (!pit_verify_msb(val))
> break;
> tsc = get_cycles();
> }
> @@ -336,8 +341,7 @@ static unsigned long quick_pit_calibrate(void)
> * to do that is to just read back the 16-bit counter
> * once from the PIT.
> */
> - inb(0x42);
> - inb(0x42);
> + pit_verify_msb(0);
>
> if (pit_expect_msb(0xff, &tsc, &d1)) {
> for (i = 1; i <= MAX_QUICK_PIT_ITERATIONS; i++) {
> @@ -348,8 +352,19 @@ static unsigned long quick_pit_calibrate(void)
> * Iterate until the error is less than 500 ppm
> */
> delta -= tsc;
> - if (d1+d2 < delta >> 11)
> - goto success;
> + if (d1+d2 >= delta >> 11)
> + continue;
> +
> + /*
> + * Check the PIT one more time to verify that
> + * all TSC reads were stable wrt the PIT.
> + *
> + * This also guarantees serialization of the
> + * last cycle read ('d2') in pit_expect_msb.
> + */
> + if (!pit_verify_msb(0xfe - i))
> + break;
> + goto success;
Oh, this reminds me of a patch i saw from John(?) a couple
of months ago that added a magic final pit_verify_msb()
call, which solved PIT calibration instabilities.
(Or maybe i did that hack when i tried to write an
auto-error-boundary calibrator?)
I dont think we committed it because it was a 'black magic'
hack as per observation and nobody realized the side-effect
of the TSC synchronization which you mention above.
My memories are sketchy but i think the symptom was one
failed PIT loop every 10 bootups, and it was solved by a
patch very similar to yours.
So with a proper changlog and testing i'd very much go for
this on those grounds ago - and if it also solves the
problem observed by Wei Chong Tan that would be perfect.
[ Unfortunately Thomas (who too saw some PIT calibration
weirdnesses IIRC) wont be back for some time. ]
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/
Powered by blists - more mailing lists