linux-kernel - Re: [PATCH] arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU

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] [day] [month] [year] [list]

Message-ID: <20110103144420.GN4739@erda.amd.com>
Date:	Mon, 3 Jan 2011 15:44:20 +0100
From:	Robert Richter <robert.richter@....com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"oprofile-list@...ts.sourceforge.net" 
	<oprofile-list@...ts.sourceforge.net>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	"atswartz@...il.com" <atswartz@...il.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Dan Carpenter <error27@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH] arch/x86/oprofile/op_model_amd.c: perform
 initialisation on a single CPU

Ingo,

I tested your patch and it fixes the bug too.

On 03.01.11 07:03:10, Ingo Molnar wrote:
> 
> * Robert Richter <robert.richter@....com> wrote:
> 
> >  static void init_ibs(void)
> >  {
> >  	ibs_caps = get_ibs_caps();
> 
> this get_ibs_caps() call is percpu too - still it now runs with preempt off.

get_ibs_caps() uses the cpuid instruction and this is percpu. But
mixed silicon support does not allow the use of different cpus with
different cpuid features in one system, so it should be save to use it
with preemption enabled.

Also, since the check uses a combination of boot_cpu_has() and
cpuid_eax(), disabling preemption would not help here. We would have
to pin the init code to the boot cpu then. Suppose a boot cpu with ibs
enabled and a secondary (init) cpu with ibs disabled. This will crash
the system when accessing ibs msrs.

Anyway, I am fine with your change.

> 
> > +		printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n",
> > +		       (unsigned)ibs_caps);
> 
> that cast looks ugly and unnecessary.

Yes, this cast is unnecessary.

> 
> I fixed both in the commit below. (not tested yet)

Thanks for looking at this,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

--
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/