Messages in this thread | | | From | "Liang, Kan" <> | Subject | RE: [tip:perf/core] perf/x86: Add an MSR PMU driver | Date | Tue, 4 Aug 2015 18:11:28 +0000 |
| |
> > On Tue, Aug 4, 2015 at 8:03 AM, Liang, Kan <kan.liang@intel.com> wrote: > > > >> > + > >> > +enum perf_msr_id { > >> > + PERF_MSR_TSC = 0, > >> > + PERF_MSR_APERF = 1, > >> > + PERF_MSR_MPERF = 2, > >> > + PERF_MSR_PPERF = 3, > >> > + PERF_MSR_SMI = 4, > >> > + > >> > + PERF_MSR_EVENT_MAX, > >> > +}; > >> > + > >> > +struct perf_msr { > >> > + int id; > >> > + u64 msr; > >> > +}; > >> > + > >> > +static struct perf_msr msr[] = { > >> > + { PERF_MSR_TSC, 0 }, > >> > + { PERF_MSR_APERF, MSR_IA32_APERF }, > >> > + { PERF_MSR_MPERF, MSR_IA32_MPERF }, > >> > + { PERF_MSR_PPERF, MSR_PPERF }, > >> > + { PERF_MSR_SMI, MSR_SMI_COUNT }, }; > >> > >> I think this could be easier to work with if it were [PERF_MSR_TSC] = > >> {...}, etc. No big deal, though, until the list gets long. However, > >> it might make fixing the apparent issue below easier... > >> > >> > +static int msr_event_init(struct perf_event *event) { > >> > + u64 cfg = event->attr.config; > >> > >> ... > >> > >> > + event->hw.event_base = msr[cfg].msr; > >> > >> Shouldn't this verify that the fancy enumeration code actually > >> believes that msr[cfg] exists on this system? Otherwise we might have > >> a very short wait until the perf fuzzer oopses this thing :) > >> > > > > I think we already did the check before using msr[cfg]. > > Where? All I see is: > > + if (cfg >= PERF_MSR_EVENT_MAX) > + return -EINVAL;
Yes, we check cfg here. So msr[cfg] should be always available.
| |