linux-kernel - Re: perf/x86/intel/uncore: Add Haswell-EP uncore support

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

Date:	Tue, 25 Nov 2014 18:20:50 -0500
From:	Dave Jones <davej@...hat.com>
To:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Cc:	zheng.z.yan@...el.com, ak@...ux.intel.com, peterz@...radead.org,
	eranian@...gle.com
Subject: Re: perf/x86/intel/uncore: Add Haswell-EP uncore support

 > Commit:     e735b9db12d76d45f74aee78bd63bbd2f8f480e1
 > Author:     Yan, Zheng <zheng.z.yan@...el.com>
 > AuthorDate: Thu Sep 4 16:08:26 2014 -0700
 > Committer:  Ingo Molnar <mingo@...nel.org>
 > CommitDate: Wed Sep 24 14:48:21 2014 +0200
 > 
 >     perf/x86/intel/uncore: Add Haswell-EP uncore support

This commit added some code which looks a bit fishy, and got flagged
by coverity in yesterdays scan.

There are a few cases like this :

 > +static void hswep_cbox_enable_event(struct intel_uncore_box *box,
 > +				  struct perf_event *event)
 > +{
 > +	struct hw_perf_event *hwc = &event->hw;
 > +	struct hw_perf_event_extra *reg1 = &hwc->extra_reg;
 > +
 > +	if (reg1->idx != EXTRA_REG_NONE) {
 > +		u64 filter = uncore_shared_reg_config(box, 0);
 > +		wrmsrl(reg1->reg, filter & 0xffffffff);
 > +		wrmsrl(reg1->reg + 1, filter >> 32);
 > +	}
 > +
 > +	wrmsrl(hwc->config_base, hwc->config | SNBEP_PMON_CTL_EN);
 > +}

given the definition of wrmsrl ..

#define wrmsrl(msr, val)	\
       native_write_msr((msr), (u32)((u64)(val)), (u32)((u64)(val) >> 32))

We can see that coverity got upset because it realised that the
code is turned into constant expressions.

result_independent_of_operands: (u64)(filter & 4294967295U) >> 32 is 0
regardless of the values of its operands.
a function call.

result_independent_of_operands: (u64)(filter >> 32) >> 32 is 0
regardless of the values of its operands.

I couldn't quickly decipher which MSRs we're writing to here in the SDM,
but are these really 64-bit wide registers that need 32 bits always set to zero ?
I'm wondering if these should be just wrmsr's instead of wrmsrl's.

If someone more familiar with perf hw can let me know if this is safe,
I'll dismiss the reports in coverity.

thanks,

	Dave

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