Received-SPF: pass (google.com: domain of linux-kernel+bounces-214166-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <28cd2683-2ab5-4772-8597-6aaf0aec1f0e@intel.com>
Date: Thu, 13 Jun 2024 17:59:43 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 07/19] x86/resctrl: Add support to enable/disable ABMC
 feature
To: Babu Moger <babu.moger@amd.com>, <corbet@lwn.net>, <fenghua.yu@intel.com>,
	<tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
	<dave.hansen@linux.intel.com>
CC: <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>,
	<yanjiewtw@gmail.com>, <kim.phillips@amd.com>, <lukas.bulwahn@gmail.com>,
	<seanjc@google.com>, <jmattson@google.com>, <leitao@debian.org>,
	<jpoimboe@kernel.org>, <rick.p.edgecombe@intel.com>,
	<kirill.shutemov@linux.intel.com>, <jithu.joseph@intel.com>,
	<kai.huang@intel.com>, <kan.liang@linux.intel.com>,
	<daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>,
	<sandipan.das@amd.com>, <ilpo.jarvinen@linux.intel.com>,
	<peternewman@google.com>, <maciej.wieczor-retman@intel.com>,
	<linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<eranian@google.com>, <james.morse@arm.com>
References: <cover.1716552602.git.babu.moger@amd.com>
 <e4edf1794852cff132b17c8f061f24799d9e79ad.1716552602.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <e4edf1794852cff132b17c8f061f24799d9e79ad.1716552602.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VnpnLzdmNmRNRE9WMEhTMGJ6d216UDhIQVBZT3o5NVFUMDVMTE9kN1VpcWJH?=
 =?utf-8?B?RzBRQmZFM2pvMDkrRWMwMjNuMVBCaU4xUkhld2NwSFM0VU15L0xDaUlidTZr?=
 =?utf-8?B?dSsvMkk2NHpEa0JITGw1c09jbDR0MEFOSWFRclM5SFlKUUR2S0JjV1dTUjI3?=
 =?utf-8?B?YUJ3bnoxQ1FIZC9KUnZGdnF5b3JMKzA4MTJsSzNuQmcycmVVMnNKSjdWb1JY?=
 =?utf-8?B?V3YwVHpJelljRDVsdW9NQWlUSXltQWxMTEhQbFgvYjd1T1Ywa0VTZWtZdFBx?=
 =?utf-8?B?cjVNVTBvRGpiMWRCTGRQbXBVR1JLWXYvWnA2WDJJZmNlUlRINU9YaGZJcEN0?=
 =?utf-8?B?UlBsOHhVL2FhYUxDZDRQTlNqVlVZWDA3TXdadFlNbzh1WElMVjgrb3laSXNO?=
 =?utf-8?B?cnBXa2gvM1lENHVBMk5Ua09GalY3MU03TjVldXN5V091NER0aERMR1plTUhR?=
 =?utf-8?B?L1RlWW1Na2NEenVnRk5Well5aVNMRTR2OU9CWFFjVG1ESVN2SmJpYnlJNlAx?=
 =?utf-8?B?M1NvWjFxMG1xVzZiMHZrMFNZZGhNR083cG1QeUFHMnpRUUpBL0MyRkp2aDE0?=
 =?utf-8?B?SEs2WWpMeCtyUkVMVGc5bXU3OGZycW42a0dqUy9pZC9CQU1JcEVqTG5jdlFY?=
 =?utf-8?B?ejB1Q3dkNnlSZzN6YWNIdjJUMWZoZ2c3emhKMGc1S1BXM3BFTzZ4Q1dKZFVV?=
 =?utf-8?B?c1RqbmJudVB6dEdoVzlEZmZQMDczNXJrRnBHNlI0TGNuRmdBMnhOaWdpRCtk?=
 =?utf-8?B?d1kzd1lZLzlSRjdaMVBKTXR6dFlERVJmZ3duNUV6bHZPbFdWc0JKRHhhQ21h?=
 =?utf-8?B?ZUg5MDlCOUVSZWg1NkZkMDk0cHJlcXdLYnJTb0RNVE5hdFJ2NVN3V2hMK1Zq?=
 =?utf-8?B?elZ6M2dzWm5VL2xVQkt4andndyt4U2JpM05HTUREdmd5NkRnNTB5VWpZNmho?=
 =?utf-8?B?TVF1WFpOeFJMZ0RpU3FDenlIOWdYMjVUUTJwZ3VjNHBtU3pIVE9sbDF6ZG1W?=
 =?utf-8?B?N3loQ0pyZnhXUUJ4ekh3SFJrTkRmWnZyNXUyN0VETzhSbnRmV1U1ZkFVMkxS?=
 =?utf-8?B?dEFFUk04RjhoaHhDWEdXVC92MmZZOFBWVmhBT2VwdDRLWGtTQnZiMXJQZkZB?=
 =?utf-8?B?cTBxZzVJOGw3bkJXb1JJZmtkN2pzc2djTjgvLytEb1NsVnVIL2VYRDhKcmt2?=
 =?utf-8?B?QzE3S2k4Q2Zlb2w2QkR4d3dCeVRsdm1QVkdKbW5DVnJyRjRCakFWRjlJOFFy?=
 =?utf-8?B?UWZEODl4YlpMWDVaMGhIOWgydUo3bFR6NFgrNU04M0hyaU5URFMvWnVDd0lN?=
 =?utf-8?B?TUdGZTNEMzN4bFhDWm1GMUxqaEl3bCszMkFCNXpvWU5OOUtYNHlXajFQeGhE?=
 =?utf-8?B?WmNzMzZZV2toWWFQUmFaK0lvQ0pCWUJzanMzcWtMWUNqMW95SS90VGFMaC84?=
 =?utf-8?B?TGNDaEU0YU5RbG9uSWhpcndUNEM3VWJCWmg1TVdaZFNRN3ZzbnZ2RlZNa3pK?=
 =?utf-8?B?alVidkNmVEs4Nmx3L2R4K3oxN2NITjl3ZVJHYXhTWW9GZlFtOWhoTC8rRkhF?=
 =?utf-8?B?dml6WlA5a21ZaGZxbVBqejlFWWU1YVQvVVR4amllTVpSblRjbjY0QSszR0NY?=
 =?utf-8?B?OWcySDQ5NXRQZ3Ezekd6ZllINVlSWnVCS1lBZFM5KzZVcFZYN3h5UjNUOTRJ?=
 =?utf-8?B?RlV6Qm4xSzRMOGUwT2R6cEtNOUI1RkpIa1JNbFFXdzFqbGNvYy9ZWFp3ajVn?=
 =?utf-8?B?T3pSTzFvS1ppSWIyNzZvdWJnbktGRXNsSVRkbStURWFkcDRUaFBReGM0WElo?=
 =?utf-8?B?cVVCbTRWd2duNHVHVlBZS1JtT3FnMjZtRmJkcy9IV1JZYXhvK1hCVmNDMFdt?=
 =?utf-8?B?b0JOclVUOXlDQXJtRGp5SHk4em9IclpKQ1FuWTYzdmxGTVlsQUkyU3JWaGIx?=
 =?utf-8?B?RmVNTXJFQSsxazZVYWM1ckliTEUxYnZ4UFlVS0wveW8xaUZjY1ZBWWtGeng3?=
 =?utf-8?B?bnh0U1dyaDVaODI1WVVBRjljSTZ4MGk0WGtnOXp1V3dlZGxMTmowTjJZK2d2?=
 =?utf-8?B?UnJKWGdwNkR5SWdlMWwrbkxTc0tCTW9JYXpCWWYrYVdnTERyYmRGNS9IQkdJ?=
 =?utf-8?B?Q3JDYXNxMzFNV292eGp3MXpVOGRxVS9OS3NqSnV6Uyt6ZWl6Z0JIWTA4U1lV?=
 =?utf-8?B?bWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 553daf35-1048-4c47-5b3f-08dc8c0d4928
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 00:59:47.0751
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: n+NJ8ZA6eI0VB15qHsvJSWacGZ7o9fnzlXyPf6TLX14ubzGZUbU2ZKI+7bzj355W1xgrpw+YqreD3+k9Uq5BrPeR3sqkIw4ac79LECf9mx0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8567
X-OriginatorOrg: intel.com

Hi Babu,

On 5/24/24 5:23 AM, Babu Moger wrote:
> Add the functionality to enable/disable ABMC feature.
> 
> ABMC feature is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG.
> When the state of ABMC is changed, the MSR needs to be updated on all
> the logical processors in the QOS Domain.
> 
> Hardware counters will reset when ABMC state is changed. Kernel internal
> counters need to be reset to avoid overflow condition in the next update.

Please note that there are two "ABMC" features introduced in this series.
First, there is the "abmc" resctrl fs feature that just happens to have
the same name as AMD's "ABMC" (which may be a good motivation to change
this name). Second, there is the architecture (AMD) specific "ABMC" feature
that is enabled in response to user's request to enable the
resctrl "abmc" feature.

Other architectures need to support resctrl fs "abmc" feature with something
entirely different.

Please consider this distinction during this series because it is often
blurred.

> 
> The ABMC feature details are documented in APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
> Monitoring (ABMC).
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> ---
> v4: Removed resctrl_arch_set_abmc_enabled and resctrl_arch_get_abmc_enabled.
>      Directly calling resctrl_abmc_enable and resctrl_abmc_disable.
>      Renamed couple of functions.
>      resctrl_abmc_msrwrite() -> resctrl_abmc_set_one()
>      resctrl_abmc_setup() -> resctrl_abmc_set_all()
>      Added rdtgroup_mutex lockdep asserts.
>      Updated commit log and code comments.
> 
> v3: No changes.
> 
> v2: Few text changes in commit message.
> ---
>   arch/x86/include/asm/msr-index.h       |  1 +
>   arch/x86/kernel/cpu/resctrl/internal.h |  8 ++++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 62 ++++++++++++++++++++++++++
>   3 files changed, 71 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e022e6eb766c..5f9a0139e98c 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1171,6 +1171,7 @@
>   #define MSR_IA32_MBA_BW_BASE		0xc0000200
>   #define MSR_IA32_SMBA_BW_BASE		0xc0000280
>   #define MSR_IA32_EVT_CFG_BASE		0xc0000400
> +#define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
>   
>   /* MSR_IA32_VMX_MISC bits */
>   #define MSR_IA32_VMX_MISC_INTEL_PT                 (1ULL << 14)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index d566251094b2..fabe40304798 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -97,6 +97,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>   	return cpu;
>   }
>   
> +/* Setting bit 0 in L3_QOS_EXT_CFG enables the ABMC feature */
> +#define ABMC_ENABLE			BIT(0)
> +
>   struct rdt_fs_context {
>   	struct kernfs_fs_context	kfc;
>   	bool				enable_cdpl2;
> @@ -436,6 +439,7 @@ struct rdt_parse_data {
>    * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
>    *			Monitoring Event Configuration (BMEC) is supported.
>    * @cdp_enabled:	CDP state of this resource
> + * @abmc_enabled:	ABMC feature is enabled
>    *
>    * Members of this structure are either private to the architecture
>    * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
> @@ -450,6 +454,7 @@ struct rdt_hw_resource {
>   	unsigned int		mbm_width;
>   	unsigned int		mbm_cfg_mask;
>   	bool			cdp_enabled;
> +	bool			abmc_enabled;
>   };
>   

ok, so here by making "abmc_enabled" a member of struct rdt_hw_resource this is
an architecture specific property. This is reasonable since every architecture
will look different. What is _not_ ok is that this causes the rest of the
series to change resctrl fs to reach into the architecture code. For example,
this work causes mbm_config_show() to now need to peek into struct rdt_hw_resource
to see this value. That is not ok. All of the interactions between this
field and resctrl fs needs to be via arch helpers: resctrl_arch_abmc_enable()/
resctrl_arch_abmc_disable() and resctrl_arch_get_abmc_enabled()/resctrl_arch_set_abmc_enabled().

>   static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r)
> @@ -493,6 +498,9 @@ static inline bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l)
>   
>   int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>   
> +int resctrl_abmc_enable(enum resctrl_res_level l);
> +void resctrl_abmc_disable(enum resctrl_res_level l);

Why do these need enum resctrl_res_level parameter?

> +
>   /*
>    * To return the common struct rdt_resource, which is contained in struct
>    * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ca692712b393..9148d1234ede 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2416,6 +2416,68 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>   	return 0;
>   }
>   
> +static void resctrl_abmc_set_one(void *arg)
> +{
> +	bool *enable = arg;
> +	u64 msrval;
> +
> +	rdmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
> +
> +	if (*enable)
> +		msrval |= ABMC_ENABLE;
> +	else
> +		msrval &= ~ABMC_ENABLE;
> +
> +	wrmsrl(MSR_IA32_L3_QOS_EXT_CFG, msrval);
> +}
> +
> +static int resctrl_abmc_set_all(enum resctrl_res_level l, bool enable)

Should this function and resctrl_abmc_set_one() perhaps have "amd" in the
name just to enforce that this is not filesystem code at all and specific
and unique to AMD.

> +{
> +	struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
> +	struct rdt_domain *d;
> +
> +	/*
> +	 * Update QOS_CFG MSR on all the CPUs associated with the resource

end of sentence needs "."

This comment about the specific register seems more appropriate to
resctrl_abmc_set_one() though. This function is a higher level
enable/disable of the hardware feature.

> +	 * Hardware counters will reset after switching the monotor mode.

monotor -> monitor

> +	 * Reset the internal counters so that it is not considered as
> +	 * an overflow in next update.

For the first time the term "internal counters" is introduced. What does it
mean?

> +	 */
> +	list_for_each_entry(d, &r->domains, list) {
> +		on_each_cpu_mask(&d->cpu_mask, resctrl_abmc_set_one, &enable, 1);
> +		resctrl_arch_reset_rmid_all(r, d);
> +	}
> +
> +	return 0;
> +}
> +
> +int resctrl_abmc_enable(enum resctrl_res_level l)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +	int ret = 0;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	if (!hw_res->abmc_enabled) {
> +		ret = resctrl_abmc_set_all(l, true);
> +		if (!ret)
> +			hw_res->abmc_enabled = true;

This error handling seems useless since resctrl_abmc_set_all() always returns
0 ... perhaps it should return void instead and this error handling dropped?
With that this function can never fail either and it can just return void,
but this is probably not what we want as the architecture call since other
architectures may fail.

> +	}
> +
> +	return ret;
> +}
> +
> +void resctrl_abmc_disable(enum resctrl_res_level l)
> +{
> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	if (hw_res->abmc_enabled) {
> +		resctrl_abmc_set_all(l, false);
> +		hw_res->abmc_enabled = false;
> +	}
> +}
> +
>   /*
>    * We don't allow rdtgroup directories to be created anywhere
>    * except the root directory. Thus when looking for the rdtgroup

Reinette