Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a375b136-c4da-0676-7a67-d2308c012b8b@intel.com>
Date:   Fri, 6 Jan 2023 06:50:45 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 1/2] perf/x86/rapl: Add support for Intel Meteor Lake
Content-Language: en-US
To:     "Zhang, Rui" <rui.zhang@intel.com>, "bp@alien8.de" <bp@alien8.de>
Cc:     "ak@linux.intel.com" <ak@linux.intel.com>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "linux-perf-users@vger.kernel.org" <linux-perf-users@vger.kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "kan.liang@linux.intel.com" <kan.liang@linux.intel.com>,
        "Bityutskiy, Artem" <artem.bityutskiy@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230104145831.25498-1-rui.zhang@intel.com>
 <25d07838-3904-a086-4238-f56c9424b53a@intel.com>
 <ea9186869cca50a21efcc1a9cc4dbe5adcd1784b.camel@intel.com>
 <Y7aejeHDpLlwwYbr@zn.tnic>
 <4cc5cd868b20366fc9d4bf157656e0c295074282.camel@intel.com>
 <Y7f6RhF8FuK9R399@zn.tnic>
 <d6ae35f1e72d4bb8e02f44fe6d9369f1a5f21619.camel@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <d6ae35f1e72d4bb8e02f44fe6d9369f1a5f21619.camel@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/6/23 06:38, Zhang, Rui wrote:
> My original proposal is that, instead of maintaining model lists in a
> series of different drivers, can we use feature flags instead, and
> maintain them in a central place instead of different drivers. say,
> something like
> 
> static const struct x86_cpu_id intel_pm_features[] __initconst = {
>         X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_L,           X86_FEATURE_RAPL | X86_FEATURE_TCC_COOLING),
>         X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,           X86_FEATURE_RAPL | X86_FEATURE_UNCORE_FREQ),
>         ...
>         X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE,           X86_FEATURE_RAPL | X86_FEATURE_TCC_COOLING),
>         X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X,    X86_FEATURE_RAPL | X86_FEATURE_UNCORE_FREQ),
>         ...
>         {},
> };
> And then set the feature flags based on this, and make the drivers test
> the feature flags.

That works if you have very few features.  SKYLAKE_X looks to have on
the order of 15 model-specific features, or at least references in the code.

That means that the

	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, ...

list goes on for 15 features.  It's even worse than that because you'd
*like* to be able to scan up and down the list looking for, say, "all
the CPUs that support RAPL".  But, if you do that, you actually need a
table -- a really wide table -- for *all* the features and a column for
each.

What we have now isn't bad.  The only real way to fix this is to have
the features enumerated *properly*, aka. architecturally.

I get it, Intel doesn't want to dedicate CPUID bits and architecture to
one-offs.  But, at the point that there are a dozen CPU models across
three or four different CPU generations, it's time to revisit it.  Could
you help our colleagues revisit it, please?