> Commit: e735b9db12d76d45f74aee78bd63bbd2f8f480e1
> Author: Yan, Zheng <[email protected]>
> AuthorDate: Thu Sep 4 16:08:26 2014 -0700
> Committer: Ingo Molnar <[email protected]>
> 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
> 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
--
[email protected] -- Speaking for myself only
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
> > > 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
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