Date: Sat, 17 Jul 2010 12:21:08 +0200
From: Borislav Petkov <borislav.petkov@amd.com>
To: "H. Peter Anvin" <hpa@zytor.com>
CC: Michal Schmidt <mschmidt@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Herrmann3, Andreas" <Andreas.Herrmann3@amd.com>,
        Shaohua Li <shaohua.li@intel.com>, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH 1/2] x86: fix keeping track of AMD C1E
Message-ID: <20100717102108.GA2239@aftab>
References: <20100714160704.GA10473@kryptos.osrc.amd.com>
 <20100714232201.00433aa9@hammerfall>
 <20100714233102.0f64614b@hammerfall>
 <4C3FDF43.6030203@zytor.com>
 <20100716063952.GB27546@aftab>
 <4C3F8418.5020803@zytor.com>
 <20100716072244.GA27954@aftab>
 <4C408298.8060004@zytor.com>
 <20100716162517.GF28902@aftab>
 <4C40DD11.4020803@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <4C40DD11.4020803@zytor.com>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2122
Lines: 51

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Fri, Jul 16, 2010 at 06:28:33PM -0400

> On 07/16/2010 09:25 AM, Borislav Petkov wrote:
> > 
> > I don't see the need for two things denoting C1E for the very simple
> > reason: when C1E gets enabled on a machine, one of the bits gets set in
> > the MSR on each core simultaneously. So if one core sees one of the bits
> > set, all the remaining cores are seeing them too. The first core that
> > sees one of the bits in the MSR, sets c1e_detected.
> > 
> > So next time any core does cpu_idle() => c1e_idle(), it switches to
> > timer broadcast on it since it might go into C1E if all the others
> > follow.
> > 
> > Frankly, I can't think of a case where we'd need to two things - I
> > could be missing something. But this workaround is a couple of years
> > old, maybe Thomas might give us more insight into whether there's a
> > particular reason for the cpuid flag and the c1e_detected variable.
> > Thomas?
> > 
> 
> I'm not saying there should be two things (and that's clearly wrong no
> matter what), but rather it's not clear to me that the one thing should
> be a variable.

Why not? Once one of the MSR bits is turned on, it stays on until next
reboot. The variable caches that value which will not change and saves
us the rdmsr everytime a cpu goes idle. So I don't see the need to have
a function instead which checks the MSR everytime, for example.

Btw, I think we should wait with whatever fix we come up until the
merge window so that we have more time to fix any fallout then (which I
don't expect but who knows) instead of rushing this now. We can always
backport it then too.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

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