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

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

[Dave Jones]

 > Commit:     e735b9db12d76d45f74aee78bd63bbd2f8f480e1
 > Author:     Yan, Zheng <zheng.z.yan@intel.com>
 > AuthorDate: Thu Sep 4 16:08:26 2014 -0700
 > Committer:  Ingo Molnar <mingo@kernel.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

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

[Andi Kleen]

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

Yes the filter MSRs are only 32bit.

> I'm wondering if these should be just wrmsr's instead of wrmsrl's.

Can we just shut up coverity please?

wrmsr() is stupid and error prone and should have never been added as an
interface.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only

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

[Dave Jones]

On Wed, Nov 26, 2014 at 06:14:22AM -0800, Andi Kleen wrote:
 > > 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 ?
 > 
 > Yes the filter MSRs are only 32bit.

thanks for checking.

 > > I'm wondering if these should be just wrmsr's instead of wrmsrl's.
 > 
 > Can we just shut up coverity please?

I can manually dismiss them, I don't think the checker has the
information to determine this for itself.

	Dave

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

[Andi Kleen]

>  > > I'm wondering if these should be just wrmsr's instead of wrmsrl's.
>  > 
>  > Can we just shut up coverity please?
> 
> I can manually dismiss them, I don't think the checker has the
> information to determine this for itself.

I suspect my out of line MSR patchkit would fix the warning
as a side effect, because there would be a function call
inbetween and the function always gets full u64.

https://lkml.org/lkml/2014/10/10/376

So far it hasn't been reviewed. I'll keep reposting.

-Andi

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

[Thomas Gleixner]

On Thu, 27 Nov 2014, Andi Kleen wrote:
> >  > > I'm wondering if these should be just wrmsr's instead of wrmsrl's.
> >  > 
> >  > Can we just shut up coverity please?
> > 
> > I can manually dismiss them, I don't think the checker has the
> > information to determine this for itself.
> 
> I suspect my out of line MSR patchkit would fix the warning
> as a side effect, because there would be a function call
> inbetween and the function always gets full u64.
> 
> https://lkml.org/lkml/2014/10/10/376
> 
> So far it hasn't been reviewed. I'll keep reposting.

And if you do so, can you please add a proper 0/n cover letter which
explains the scope of the series. I'm tired of wading through N
patches to figure out whether this is something I should care of or
not.

Thanks,

	tglx