Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753399AbbHDSNr (ORCPT ); Tue, 4 Aug 2015 14:13:47 -0400 Received: from mail-la0-f48.google.com ([209.85.215.48]:34410 "EHLO mail-la0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751073AbbHDSNq (ORCPT ); Tue, 4 Aug 2015 14:13:46 -0400 MIME-Version: 1.0 In-Reply-To: <37D7C6CF3E00A74B8858931C1DB2F077018D206A@SHSMSX103.ccr.corp.intel.com> References: <1437407346-31186-1-git-send-email-kan.liang@intel.com> <37D7C6CF3E00A74B8858931C1DB2F077018D16F1@SHSMSX103.ccr.corp.intel.com> <37D7C6CF3E00A74B8858931C1DB2F077018D206A@SHSMSX103.ccr.corp.intel.com> From: Andy Lutomirski Date: Tue, 4 Aug 2015 11:13:25 -0700 Message-ID: Subject: Re: [tip:perf/core] perf/x86: Add an MSR PMU driver To: "Liang, Kan" Cc: Thomas Gleixner , "linux-kernel@vger.kernel.org" , "H. Peter Anvin" , Andrew Lutomirski , Linus Torvalds , Peter Zijlstra , Ingo Molnar , "linux-tip-commits@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2250 Lines: 65 On Tue, Aug 4, 2015 at 11:11 AM, Liang, Kan wrote: > > >> >> On Tue, Aug 4, 2015 at 8:03 AM, Liang, Kan 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. > PERF_MSR_EVENT_MAX is a constant. If I run this thing on an AMD CPU that supports TSC, APERF, MPERF, and nothing else, and someone asks for PPERF, then the check will succeed and we'll oops, right? --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/