From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] perf: Check all MSRs before passing hw check Date: Mon, 18 Mar 2013 10:55:39 +0000 Message-ID: <5146F2AB.9080206@eu.citrix.com> References: <1363350033-16379-1-git-send-email-george.dunlap@eu.citrix.com> <20130318084242.GB17959@gmail.com> <5146EF30.2040306@eu.citrix.com> <20130318105320.GA28486@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130318105320.GA28486@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ingo Molnar Cc: Peter Zijlstra , Konrad Wilk , "x86@kernel.org" , "xen-devel@lists.xen.org" , "H. Peter Anvin" , Thomas Gleixner List-Id: xen-devel@lists.xenproject.org On 18/03/13 10:53, Ingo Molnar wrote: > * George Dunlap wrote: > >> On 18/03/13 08:42, Ingo Molnar wrote: >>> * George Dunlap wrote: >>> >>>> check_hw_exists has a number of checks which go to two exit paths: >>>> msr_fail and bios_fail. Checks classified as msr_fail will cause >>>> check_hw_exists() to return false, causing the PMU not to be used; >>>> bios_fail checks will only cause a warning to be printed, but will >>>> return true. >>>> >>>> The problem is that if there are both msr failures and bios failures, >>>> and the routine hits a bios_fail check first, it will exit early and >>>> return true, not finishing the rest of the msr checks. If those msrs >>>> are in fact broken, it will cause them to be used erroneously. >>>> >>>> This changset causes check_hw_exists() to go through all of the msr >>>> checks, failing and returning false if any of them fail. >>>> >>>> This problem affects kernels as far back as 3.2, and should thus be >>>> considered for backport. >>>> >>>> Signed-off-by: George Dunlap >>>> CC: Konrad Wilk >>>> CC: Thomas Gleixner >>>> CC: "H. Peter Anvin" >>>> CC: x86@kernel.org >>>> --- >>>> arch/x86/kernel/cpu/perf_event.c | 20 ++++++++++---------- >>>> 1 file changed, 10 insertions(+), 10 deletions(-) >>> What is missing is a description of what specific platform this gets >>> triggered on and exactly why. Is some hw feature emulation missing that >>> causes the check to fail? >> Remember, there are two checks failing: the second one is supposed >> to fail and disable the PMU entirely, but it's not getting there >> because when the first one fails, it skips the rest but returns >> "success" anyway. >> >> The warning on the first check is as follows: >> >> [ 8.131985] Performance Events: Broken BIOS detected, complain to >> your hardware vendor.^M >> [ 8.139997] [Firmware Bug]: the BIOS has corrupted hw-PMU >> resources (MSR c0010000 is 530076)^M >> >> c0010000 is the AMD MSR_K7_EVNTSEL0; the check it's failing is: >> if (val & ARCH_PERFMON_EVENTSEL_ENABLE) >> >> So it discovers that one of the performance counters is already >> enabled -- worth a warning, but by itself not worth disabling the >> PMU. This is most likely to be exactly what the warning message >> says: a buggy BIOS that enables perfcounters enabled for some >> reason. >> >> The second check is supposed to detect that the PMU is actually not >> usable -- in my case because it's running virtualized (under Xen). > I got the logic from your original description - what I wanted was for the > specific messages to be included in the patch changelog, plus a > description of what misbehaved before the patch and what behaves better > after the patch - on your specific system. > > In other words, please use the customary changelog style we use in the > kernel: > > " Current code does (A), this has a problem when (B). > We can improve this doing (C), because (D)." Right, got it. Standby for v2. -George