Received-SPF: pass (google.com: domain of linux-kernel+bounces-18431-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <964d9751-0b9a-4394-9b04-95d693ac9518@amd.com>
Date: Fri, 5 Jan 2024 18:13:16 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v3 2/2] x86/resctrl: Remove hard-coded memory bandwidth
 event configuration
Content-Language: en-US
To: Reinette Chatre <reinette.chatre@intel.com>,
 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, seanjc@google.com,
 kim.phillips@amd.com, jmattson@google.com, ilpo.jarvinen@linux.intel.com,
 jithu.joseph@intel.com, kan.liang@linux.intel.com, nikunj@amd.com,
 daniel.sneddon@linux.intel.com, pbonzini@redhat.com,
 rick.p.edgecombe@intel.com, rppt@kernel.org,
 maciej.wieczor-retman@intel.com, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, eranian@google.com, peternewman@google.com,
 dhagiani@amd.com
References: <20231201005720.235639-1-babu.moger@amd.com>
 <20240104212130.209490-2-babu.moger@amd.com>
 <e25ef699-7b6c-4731-b62a-39a90dd888e7@intel.com>
From: "Moger, Babu" <bmoger@amd.com>
In-Reply-To: <e25ef699-7b6c-4731-b62a-39a90dd888e7@intel.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?b3dxeE91WlhVRHdXck1Gcnk4UjFHTkZWZGdLaGYxc1ZaUCtwOHJ3YklhY3dO?=
 =?utf-8?B?aGhYTTh0d3FlNlpNYnpCKzE0Mlp4RGsrNmVzYWdha2NEbXpyWk9VTFBMSE9U?=
 =?utf-8?B?T2FPNkhsanJCRUJ6b1RmNTBGRVpFZDNlVTdDWVpJdlRXdWxER1Yrc08rYitu?=
 =?utf-8?B?ODlodkoyN3Q0NlRJVmZ3VEtSOXNsNlVsNmVnSmcwc0EzMStheXRSRERPdVI3?=
 =?utf-8?B?YXpnWDJvSnJCZzBseXRuWk1SMmJQQjh6TTczeFpwNXdMRG44akJZTkZaRTZH?=
 =?utf-8?B?Y09MSjA2YUcrYit2VkxZa3ByQk5IQ28xZ0E2V3hYQjdBdmc0aHRsL0dxRG5n?=
 =?utf-8?B?MC9sbjYrNG5ja0k4WFhEOHhTT2xvOGdmeWFGUEV1dDQzL1hRWlBzYjlackEz?=
 =?utf-8?B?c0FNOFRPdHp2ZkxOZXl0UXFjelJwOWdLaXVCS2REZ1JpVnU0VUhNS3Vwb0lS?=
 =?utf-8?B?YzFEL3h6VEROcUpsWUs2cVFZSXE0RU5lMFN4ZHpWbXFBaExwNHJUTWFQaDJS?=
 =?utf-8?B?T3RDY01oZnlNVnhaaHlhRUg2NDJ3eEl3cGdSSXd3dS8rYlBzc2JVN1Fody9H?=
 =?utf-8?B?aEVCR0t1VVRGelU3am43QUh5RnJPS3NlNHljajNGVTZXWGxFYXNIQlovcHNL?=
 =?utf-8?B?WWI1K1dUT2c0eENZOXNkck1PZ3BFMHpEVlVrYllVc3lSSG05OVFUeHpHUmls?=
 =?utf-8?B?UERIL1lWY2VnUm9Vc1pXZWpESDRVTnFONWZFQS90ajR4dDZSWXBORFprWElu?=
 =?utf-8?B?ejl5WWY1WlU3emdTOEI3c05qV3gyUFl3UXh5aXhGV3A5SDRkV04ycEs1VjNB?=
 =?utf-8?B?bWVFVGhuR3ZyZm9TK1hub282cDJvSFUwc0tMeTJWMGJFYVFySGQ4eGorWTJ4?=
 =?utf-8?B?Vysvd24xSzMwL0ZqWUlQcUVQOHNVbWNJK2JQSFJSY3pZN2JENTRpQUZnUE9W?=
 =?utf-8?B?R3NoMUdZTHFpVW5mT0h1MUFwZ010bE8yOVhRVUFnZ1hJU25Oa1RuV2NmRFdi?=
 =?utf-8?B?cVRoV3p1UUxYb3RDNzB3dDAzSmFTMVhxRUJ2V29Rdit0a3hBd0ZFcUhDUEFm?=
 =?utf-8?B?TER0Z1VqcjZlWHlvUG5iMFhabHI5cVBJeE1HMjF6Q20xbTZsNHlLMW5qU2RO?=
 =?utf-8?B?UGJLSC9xR2RKeHJNWDVwWVRLWGxkYU1ZRWQ0NXByVTM2b0pMQkVnUEVGdFdY?=
 =?utf-8?B?b3BiaWNlaXhnUDZkVHRWTi9oR2tQM2EyWUFDNDFZdFowczJCaHgxZ05vZVN5?=
 =?utf-8?B?L2FSWEptQ0phRURBQWRwTHlUOEJJRk9QUHZRRkFkT2FvQzhKeTZiRlZPSG1h?=
 =?utf-8?B?SFlDekt3WGxiZTFQQTRqTXZkNmhwcXJFWDMrTXNIMUZGb0ZvdTJ4VmlmK0Zy?=
 =?utf-8?B?a3hHcFE0Q25MRnhhUnZ4REptWEozdkc3WlJhT0lNMjd4Y0dKTTVveDNwVUNM?=
 =?utf-8?B?Q1NpTkJyZ1pTNjdobXpnSng4bDJCTVY0OGlSbUpsejA4aWZpWm5EN0dLalRB?=
 =?utf-8?B?OFlSRk1rdmg5RzNtMlRGQjFOTGEvUXk4aUlJN2l0dERTb3M5NVdLY29SM3dM?=
 =?utf-8?B?bDc4QUlScDR0aW5NMDJjZ04yYmxyRHh3cmNPQlMrdXlRejBEYXpYRTVWQkhs?=
 =?utf-8?B?eW4wWEZweUxIR25JdGdrd2tLYlJpVFFMd0tRNU1SY1dXR3NQNWtPRkZYTXV5?=
 =?utf-8?B?eU1ic0p0R3QvN0lrd09wTmM2d2VKWDl6TlRkblRGQXEzTmd2a2MzSmg5SXVa?=
 =?utf-8?B?NzR1WWZtMVFNYUNPbkJxblVXdkRyM01FVzVLeCtORTZHcnZ1WVdOa3NXbEFV?=
 =?utf-8?B?QjJkS2FNdDVsM3pMM0hiSWVoLzJ0QXVYWlFRbGlTb2JzQ0pnY0ptRHBVUEVW?=
 =?utf-8?B?aFh1T1BMeDU4QUZ6eWxrVk9LUzhYcUdVNWVpTUxzQ2sxQVZpa0Fsb1FPcGND?=
 =?utf-8?B?cWxsZ2htWTBPNHpTbnc0TVZUeGtHSDdrNzBHaEhTNTl2LzZwSVcrdkpwK2JT?=
 =?utf-8?B?VkFzbDVxa0NITi94dlpJRnh2QWRVNVRvZXBlU25RVnB1dEllSjBrRkRqV1V4?=
 =?utf-8?B?Njh3SEpmc1hjWUN1TVlCUnRLLzVYUWV5QWk2WlhUOEhjcGQ1YUgwMi9CU25L?=
 =?utf-8?Q?XBgLGLijdox4jUOV5Y87VWAZS?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c8a743a4-7a91-409c-d837-08dc0e4c4a84
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jan 2024 00:13:21.2504
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jgKccsiCMWYUbcNx/OLHyoElaPb+BIjaiqJpQO97PV72KVMOHmTH2HrEgPB6IODQ
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5324

Hi Reinette,

On 1/5/2024 3:18 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/4/2024 1:21 PM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured. The maximum supported
>> bandwidth bitmask can be determined by following CPUID command.
>>
>> CPUID_Fn80000020_ECX_x03 [Platform QoS Monitoring Bandwidth Event
>> Configuration] Read-only. Reset: 0000_007Fh.
>> Bits    Description
>> 31:7    Reserved
>>   6:0    Identifies the bandwidth sources that can be tracked.
>>
>> The bandwidth sources can change with the processor generations.
>> Remove the hard-coded value and detect using CPUID command. Also,
> I do not think "Remove the hard-coded value" is accurate anymore.

Will change it to.

"Read the supported bandwidth sources using CPUID command. Also,"

Also I need to update the subject line.

>
>> print the valid bitmask when the user tries to configure invalid value.
>>
>> The CPUID details are documentation in the PPR listed below [1].
> "are documentation" -> "are documented"
Sure.
>
>> [1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
>> 11h B1 - 55901 Rev 0.25.
>>
>> Fixes: dc2a3e857981 ("x86/resctrl: Add interface to read mbm_total_bytes_config")
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Same comment about "Link:" as for patch 1/2.
Sure.
>
>> ---
>> v3: Changed the event_mask name to mbm_cfg_mask. Added comments about the field.
>>      Reverted the masking of event configuration to original code.
>>      Few minor comment changes.
>>
>> v2: Earlier sent as a part of ABMC feature.
>>      https://lore.kernel.org/lkml/20231201005720.235639-1-babu.moger@amd.com/
>>      But this is not related to ABMC. Sending it separate now.
>>      Removed the global resctrl_max_evt_bitmask. Added event_mask as part of
>>      the resource.
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h | 3 +++
>>   arch/x86/kernel/cpu/resctrl/monitor.c  | 6 ++++++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 ++++--
>>   3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index d2979748fae4..e3dc35a00a19 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -394,6 +394,8 @@ struct rdt_parse_data {
>>    * @msr_update:		Function pointer to update QOS MSRs
>>    * @mon_scale:		cqm counter * mon_scale = occupancy in bytes
>>    * @mbm_width:		Monitor width, to detect and correct for overflow.
>> + * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
>> + *			Monitoring Event Configuration (BMEC) is supported.
>>    * @cdp_enabled:	CDP state of this resource
>>    *
>>    * Members of this structure are either private to the architecture
>> @@ -408,6 +410,7 @@ struct rdt_hw_resource {
>>   				 struct rdt_resource *r);
>>   	unsigned int		mon_scale;
>>   	unsigned int		mbm_width;
>> +	unsigned int		mbm_cfg_mask;
>>   	bool			cdp_enabled;
>>   };
>>   
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index f136ac046851..acca577e2b06 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -813,6 +813,12 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>   		return ret;
>>   
>>   	if (rdt_cpu_has(X86_FEATURE_BMEC)) {
>> +		u32 eax, ebx, ecx, edx;
>> +
>> +		/* Detect list of bandwidth sources that can be tracked */
>> +		cpuid_count(0x80000020, 3, &eax, &ebx, &ecx, &edx);
>> +		hw_res->mbm_cfg_mask = ecx & MAX_EVT_CONFIG_BITS;
>> +
>>   		if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
>>   			mbm_total_event.configurable = true;
>>   			mbm_config_rftype_init("mbm_total_bytes_config");
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 69a1de92384a..5b5a8f0ffb2f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1617,12 +1617,14 @@ static void mon_event_config_write(void *info)
>>   static int mbm_config_write_domain(struct rdt_resource *r,
>>   				   struct rdt_domain *d, u32 evtid, u32 val)
> Not specific to this patch, but since the valid mask is per resource I do not think
> it is necessary to check user provided value for every domain. The user provided value
> can be checked earlier and only once in mon_config_write() before iterating over all
> domains to write the value.
Yes. Agree.
>
>>   {
>> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>   	struct mon_config_info mon_info = {0};
>>   	int ret = 0;
>>   
>>   	/* mon_config cannot be more than the supported set of events */
>> -	if (val > MAX_EVT_CONFIG_BITS) {
>> -		rdt_last_cmd_puts("Invalid event configuration\n");
>> +	if ((val & hw_res->mbm_cfg_mask) != val) {
>> +		rdt_last_cmd_printf("Invalid input: The maximum valid bitmask is 0x%02x\n",
>> +				    hw_res->mbm_cfg_mask);
> I think keeping "Invalid event configuration" is useful to create a detailed message of:
> "Invalid event configuration: maximum valid bitmask is 0x%02x"

Sure.

Thanks

Babu