Received-SPF: pass (google.com: domain of linux-kernel+bounces-168405-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ac22b96d-c4bb-42d9-bac8-22b9e032b59e@intel.com>
Date: Fri, 3 May 2024 16:26:04 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth
 Monitoring feature details
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.1711674410.git.babu.moger@amd.com>
 <63134ddf18208bbd11bcec0ec75c33ef6e5635b2.1711674410.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <63134ddf18208bbd11bcec0ec75c33ef6e5635b2.1711674410.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"
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?eW9YWnMyY1BOTlUxaFJxZjFrYnRzY1hVakFKTkJqMERDVWg1eWJXT3A0RUNM?=
 =?utf-8?B?ZWJ2S0o3aGQvNGhrYmlFSnhDMllpbjl5WHhRak9LN0tteG92UVVJZUJWdldj?=
 =?utf-8?B?aklvZlEwNWNubHVLV21YNHNHRTI2YnVHejBSeUpteitRcVRwRFNWY0I4VUVp?=
 =?utf-8?B?cVdMY09uZm1ycEY4aEtNMXpNVE84Zi9SSGdaTzJQK1BTVy9YMS96eVppT01o?=
 =?utf-8?B?OGdRZXpMNzZsak5ua25Pa2xoZnR2c2N0UEJ4b3IwUEtabllac3pjUk16TDM1?=
 =?utf-8?B?N1B3aysvZVFUbzFMS2hCN1ZHN2hPbWhiUEMyL0x1cHBVNlFsY0czSWpUTGs2?=
 =?utf-8?B?dlh1VmZ3V2ZCK1BGOUg1cUhzR2dMdVlaTTA2Tyt4Vm1xMkkraEFvend1cVJP?=
 =?utf-8?B?bjRNRlY3UFJWazR2Uzc2aTFBNnVRblNlWWJZR2FoNmZtTkMzRkNpc25sek4z?=
 =?utf-8?B?QjZZTGNUbG5rMzZnaVFlNlJFc2s3RVVTU05DWmxFSXl4K250UjUrMG8wTGhp?=
 =?utf-8?B?QlNQR1FtaXNjREF5YkJNKzFLVnU2Q0RheVowKy9qZU45VEMzN1o2NDdhSkVh?=
 =?utf-8?B?RTNnU09lY3FVellEUXFaVXF3M3BGNG1lNk9JOHI3ZEtYdnUyUkJ2QUtjWWYy?=
 =?utf-8?B?KzNkZ1RzVDdCZlJlN0JBcUJIMEpENy9QUWtZNjEzWnNXQWJIRVZUSFZwZUJH?=
 =?utf-8?B?UWp4T3p6b2NodHk4a1MyM2hwdjZWZXNFNkFhM2RlRmdzejdlcnVvbnhVVEp0?=
 =?utf-8?B?dXBweHNZa3lIbFZIaXpPaXBjTEk2Qk5GS29ITXE0aGFscGJFSWpuN2Fna1Zk?=
 =?utf-8?B?U1dNR0ZHWk9TYS9NZXYycWN1aFhrSWcrTVhLdmwrZHlKdnh2dkhISUZLMUJS?=
 =?utf-8?B?Y0FnTmJrTlVpTDdzb1IrRWhwaGd4WE1XNlNFV0dySk9TTndVeWYrTDAxb1hD?=
 =?utf-8?B?Vjg0QXA3SjJjQTkvaW0ycFV1N1JtcG96a3V6QWVoN1RrTjBiSC9vbXJqVzE3?=
 =?utf-8?B?S2RUK2lib2dsODQzZThZRmRPMDJPYUtTUWlFb1NsMVNsVGFUUGlzdDM0aElE?=
 =?utf-8?B?M1FlL0g2Q1RqeFFwbEVvbFgwMWNIM2NJVkFvOEY0Y0hwUmYrd3UwZjBIbGhD?=
 =?utf-8?B?VDdHb2NrZ2xXRE5kRFh5SWNmL3Q0TitTbFBaY0F1c3lLdXZ6bXhIcll0SHBC?=
 =?utf-8?B?WFhKa05uVzNwTy9MdlZ6WmJhWno4M3kzdmp0WHNhc25scVBKV05MRWluOTVL?=
 =?utf-8?B?dGRiSEVKNkdKNTgzd2NmNGo3N1RHL3BtNlNMd0U0N1U4NWh3eFZ1clRwVFlK?=
 =?utf-8?B?OHhKKzhpVzFTY1AxZXlWdDh2TzJJVFpFZG5sdXRZRWd2OFkzZXQwVEgvY0FP?=
 =?utf-8?B?QzdaZGcwQVVnbGg5bW9ldWJ3MUd1K3hTTmpQR2E2MVhWSnFTczdNandDaWo0?=
 =?utf-8?B?ckp5Zm96c21UZnFQUTlHT01DMkRqVDZIUVNVLytwRzAyMHR1amxhd3BFMEo0?=
 =?utf-8?B?YkhSNTVzRlAweWdoK1BxWnFTbzkrZlhaaDZnVlF4WWdUTkNrYjlEM1dZQ0ZG?=
 =?utf-8?B?N1BMcHA4c1c0QkorNE16Wm40bSt4NmNvR1phaVVlTDZpS05ydmptVEYrVzBP?=
 =?utf-8?B?aU8wYW16Q3JSYTJ3VUtXL2NGWlBQeVJZK2Y4Q0I4S01qUWJZL2ZBSlpqNzBM?=
 =?utf-8?B?OEFLK1lXOGFDbmZqTURNOEs3anpPNHA2NlVwdGo5eGxXRW1YcXFqUUdYTmVR?=
 =?utf-8?B?ajFaVldVNFM2bnlUTkZtOHk4QkJXSWM5M0FIN0owZVZQa0Z1QS9sajZkem50?=
 =?utf-8?B?cUJOYm85bDVQYjNDOGhXVDlJWTR2eS9ERkJOTHo2WklMYWppdmg5TVo5NGw4?=
 =?utf-8?B?SXh3ZkllRyt0NXd1Tm0vTG1SUmN1ZVhKbEdKNDdzRXdtVVBwaGxWRTFHcndQ?=
 =?utf-8?B?WXl1QVNNdmtkNFd2cjlTM21EeXJNcHgvbkNVdVlPcWVoL0szdS9YakVvMU1h?=
 =?utf-8?B?VDlCSWliMjRUQjRjU0daOGQ2UGpRM2NMaU4wMjV0OURCb1hDWm1SV2RSMEVu?=
 =?utf-8?B?TG8rMEx1T0U5eTJ1aVBjWnZYeUJRVVlpTjByYjFJV2RFWDI1bnpFbmtQWnJu?=
 =?utf-8?B?VmErQjlxeUU4eFBINmRXU29HZTJQUGZYQ1FycERUVDQ1bkdsZVUycHNwbHpY?=
 =?utf-8?B?SUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b70eed23-9608-423f-ff1e-08dc6bc868c6
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 23:26:07.6389
 (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: 2wbsLW6/VGdX0T7nX//xjFwDETZa7xiLdU0xMjyZFd3Qfv+6qa0+Hq3Vf0V4cHOZxPaLblSAqaWAOQguA3GXQBUsdqMwSUWKA5nAYDlDFWc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR11MB8607
X-OriginatorOrg: intel.com

Hi Babu,

On 3/28/2024 6:06 PM, Babu Moger wrote:
> ABMC feature details are reported via CPUID Fn8000_0020_EBX_x5.
> Bits Description
> 15:0 MAX_ABMC Maximum Supported Assignable Bandwidth
>      Monitoring Counter ID + 1
> 
> The 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
> ---
> v3: Removed changes related to mon_features.
>     Moved rdt_cpu_has to core.c and added new function resctrl_arch_has_abmc.
>     Also moved the fields mbm_assign_capable and mbm_assign_cntrs to
>     rdt_resource. (James)
> 
> v2: Changed the field name to mbm_assign_capable from abmc_capable.
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     | 17 +++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  |  3 +++
>  include/linux/resctrl.h                | 12 ++++++++++++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 57a8c6f30dd6..bb82b392cf5d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -740,6 +740,23 @@ bool __init rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +inline bool __init resctrl_arch_has_abmc(struct rdt_resource *r)
> +{
> +	bool ret = rdt_cpu_has(X86_FEATURE_ABMC);
> +	u32 eax, ebx, ecx, edx;
> +
> +	if (ret) {
> +		/*
> +		 * Query CPUID_Fn80000020_EBX_x05 for number of
> +		 * ABMC counters
> +		 */
> +		cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
> +		r->mbm_assign_cntrs = (ebx & 0xFFFF) + 1;
> +	}
> +
> +	return ret;
> +}

It is not clear to me why this function is needed. I went back to
read James' comment and it sounds to me as though he expected it
to be called from non-arch code ... but this is only called
from rdt_get_mon_l3_config() which is very much architecture specific
and will remain in arch/x86 where rdt_cpu_has() will be accessible.

> +
>  static __init bool get_mem_config(void)
>  {
>  	struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_MBA];
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c99f26ebe7a6..c4ae6f3993aa 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -584,6 +584,7 @@ void free_rmid(u32 closid, u32 rmid);
>  int rdt_get_mon_l3_config(struct rdt_resource *r);
>  void __exit rdt_put_mon_l3_config(void);
>  bool __init rdt_cpu_has(int flag);
> +bool __init resctrl_arch_has_abmc(struct rdt_resource *r);
>  void mon_event_count(void *info);
>  int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index c34a35ec0f03..e5938bf53d5a 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1055,6 +1055,9 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  			mbm_local_event.configurable = true;
>  			mbm_config_rftype_init("mbm_local_bytes_config");
>  		}
> +
> +		if (resctrl_arch_has_abmc(r))
> +			r->mbm_assign_capable = ABMC_ASSIGN;
>  	}

This is confusing to me in two ways:
(a) why need different layers of abstraction to initialize r->mbm_assign_capable
    and r->mbm_assign_cntrs? Can they not just be assigned at the same time?
(b) r->mbm_assign_capable is a bool ... but it is assigned an enum? Why is
    this enum needed for this?

>  
>  	l3_mon_evt_init(r);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index a365f67131ec..a1ee9afabff3 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -150,6 +150,14 @@ struct resctrl_membw {
>  struct rdt_parse_data;
>  struct resctrl_schema;
>  
> +/**
> + * enum mbm_assign_type - The type of assignable monitoring.
> + * @ABMC_ASSIGN: Assignable Bandwidth Monitoring Counters.
> + */
> +enum mbm_assign_type {
> +	ABMC_ASSIGN	= 0x01,
> +};
> +

Either the resource is mbm_assign_capable or not ... it is not clear
to me why an enum is needed.

>  /**
>   * struct rdt_resource - attributes of a resctrl resource
>   * @rid:		The index of the resource
> @@ -168,6 +176,8 @@ struct resctrl_schema;
>   * @evt_list:		List of monitoring events
>   * @fflags:		flags to choose base and info files
>   * @cdp_capable:	Is the CDP feature available on this resource
> + * @mbm_assign_capable:	Does system capable of supporting monitor assignment?

"Does system capable" -> "Is system capable"?

> + * @mbm_assign_cntrs:	Maximum number of assignable counters
>   */
>  struct rdt_resource {
>  	int			rid;
> @@ -188,6 +198,8 @@ struct rdt_resource {
>  	struct list_head	evt_list;
>  	unsigned long		fflags;
>  	bool			cdp_capable;
> +	bool			mbm_assign_capable;
> +	u32                     mbm_assign_cntrs;
>  };

Please check tabs vs spaces (in this whole series please).

I'm thinking that a new "MBM specific" struct within
struct rdt_resource will be helpful to clearly separate the MBM related
data. This will be similar to struct resctrl_cache for 
cache allocation and struct resctrl_membw for memory bandwidth
allocation.

>  
>  /**

Reinette