Received-SPF: pass (google.com: domain of linux-kernel+bounces-73728-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: <a3651bc5-55ba-44bf-aef7-503350c49760@amd.com>
Date: Tue, 20 Feb 2024 15:27:55 -0600
User-Agent: Mozilla Thunderbird
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v2 04/17] x86/resctrl: Detect Assignable Bandwidth
 Monitoring feature details
Content-Language: en-US
To: James Morse <james.morse@arm.com>, corbet@lwn.net, fenghua.yu@intel.com,
 reinette.chatre@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
References: <20231201005720.235639-1-babu.moger@amd.com>
 <cover.1705688538.git.babu.moger@amd.com>
 <bba869886410265ffac0250ba2a495e3e7576dfa.1705688539.git.babu.moger@amd.com>
 <50bd1168-5461-43de-9e64-cbe0019d571d@arm.com>
From: "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <50bd1168-5461-43de-9e64-cbe0019d571d@arm.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?MkRFL2tBcHkrVEZsZm5MbEtlVnJyditVejI1T04xZXFnb1RjMmpZU3YyT1ZH?=
 =?utf-8?B?YmkzcGsxVUtQbWFONVY2MnhrbFcwcmtDd1l1V3A2T3prZTBIZnJ1ZUt2UnJO?=
 =?utf-8?B?UEpWd3RUL0dhdHZtd251QmJJY0xIVHJTOGZUWnBNZFV5NWYwVFB0TEwyajNC?=
 =?utf-8?B?YkRCNWNQRVNTQmNIOUF4Rm9KUDRpNGR4NkRtTXZTbkVqa0J0bGFFWUhialNy?=
 =?utf-8?B?ZVhXWk5tYmJCakVLUVo2OTE2M0NRU243QnBTTk9VVG1MaHppRGErVERRemxu?=
 =?utf-8?B?QmZQVmVSb0dscVlQcUtOU0w3ZnlLNnJlRURleG9kN25UMGF6TENOK3NwR01q?=
 =?utf-8?B?dnJDeCtQaS9YMEtnbW9Yay82UFhmcFlmNDJCVDVDbktVeVl6Tmtpc3kxWmFy?=
 =?utf-8?B?RDFVbkowOVZUcWFpeks2UWJhZ29kMVlTOUxPRzRmU3ZYMnA3RDNDcGFEQ3Jl?=
 =?utf-8?B?V0k3V1VqRiszMFZCS2ZlM0RSMzNrNVdyeU5jNk1wbUF0NkhFSXFZekQwa2Rw?=
 =?utf-8?B?RjBXc3VQMWJBTlF1SEY2Mm9TdHZMUkVPaDFzZ0M4MUxobjNpbllFZXY4dTRj?=
 =?utf-8?B?REV1d3lpSUxpbDdOSnNKaW1VR21VbCtWRXM3WEJMOXdXa3NSQjcwUjFUN1M1?=
 =?utf-8?B?dHZBV2RnMlZ5a1V3QmRYVzFvYWRjRllUWXAyWTdsbmxWU2JMSGU5OU42aEpk?=
 =?utf-8?B?ZmRnZmhPWk50amV3KzVVK1VuNnQ3eG5kZGgrTHR4UDhkWEpHQTJmczI2cHdx?=
 =?utf-8?B?VVhhZWRlSVZyejBUSjdDZ1d2bXJRUnRlejUzV0FtNEQzNGN3Wm1HUGl6cVQ4?=
 =?utf-8?B?NU16Vi80ekRCMmNRZE9yZFJ1UmJCdmpsUnBQck4xWWFhOExoZEp2YVJrK3lM?=
 =?utf-8?B?RmE4dytyYko1Qi90ZHcvWk9TZG1SMHBsdzJnY29IcjdHZTNnVnphRzNEaGdp?=
 =?utf-8?B?c2dkK293SzdaTlRLYUpIOFdvdjg2UjZhSDMvbmEwbmMyTUI5OEo3Wk1pNUNI?=
 =?utf-8?B?U0xsKzhjV2Vha1gydzJNZnVEcXJ4WmVGdFpxb0dRVGx1L0RySnhUcWk3T2Fi?=
 =?utf-8?B?OHBWSjZ0OU05b3FKUm9LVGMrLys2dFdWVWVhVXpjRkVESWpNbTc2cG96RTgv?=
 =?utf-8?B?V3N3QkppdGV0RGQvU1ovV2diTjc0SUR0Vy81QUt6enhqemRDZU1ZaWFVVmdI?=
 =?utf-8?B?Y1hvbzJiZWpEQTNCOGVlb0YyUnRXUFVsRGhYdnhRc0lQUFVuTlFaMDcwTTlQ?=
 =?utf-8?B?UTFML1B6QnhUNis2MXZuK0RWUUhzM1lFN3FXL2NubUdyemNXVUI0ZWkzVWFU?=
 =?utf-8?B?OFkxeHA4N1piZThJTlRuT2QyRTNZMEhEUWppUHpUUDFNV25tRDU1K2xxQ2M2?=
 =?utf-8?B?K3NpVDZHTGpTdURia3FaNnM4bXpWQk5FaUxkQi9sOG44Z0kwY0RiWHo2a0Ny?=
 =?utf-8?B?R1NBN1ZjdHVqU2Nwem9vQkczMjk0Z0VabkYwTzBMMmMwNHpHVW10UU9tR1BT?=
 =?utf-8?B?YzN1RDlmVEhxc05wcjYyYm5takVGRDhBSk5vMUNvU2Z5ay8rdWNQOHF3S3hL?=
 =?utf-8?B?U2s5KzdDNlFleWxETEZZTlh0SnJkSkRrSFpnb1B5Vyt6YVhvN3ZiWklQeDFU?=
 =?utf-8?B?TUpTZzhYVnorUzZsWXN6MWpUQVJHMWlEdjV3MUs2K0xON25PYmF6SzZzcUZ4?=
 =?utf-8?B?Y29mekxRU1UwM1ZXZ0pNNW5kYnpCY0RXL0N0ZFdQVm9YUk55RlJtV2crSEZU?=
 =?utf-8?B?RSt2Sk5yRU42OXBPTmFPRk5Dc1B3dUxMTy9tcVRIOXlaZjRMajJXLytBRWFk?=
 =?utf-8?B?YUVrTFNqellKU2pva1hrSTY5VmxDZFJUNklqWVJ5d01kUTgyaDdzemJCUnBt?=
 =?utf-8?B?cUVtVHZSSVYzakZldWFEcnZNYWozUDRxMzMwdXYybGRCS1dQM1JaMzNtcCs5?=
 =?utf-8?B?Sk9hdWt6MHNBVFY0bjZmMkY1ZUNodkdSU1JhQk5QeDZQdU8xNVZFYzVoLzdC?=
 =?utf-8?B?WGdGR0F4MDUxSktQUjQ0TExUOWFjUWJBMnBzTHFBRmRQSkwxRXZPSzlINHNB?=
 =?utf-8?B?Mnc4MTA0alhROExtMEEvUFhBeERXaS9NRmlCMVE0U3JPSjhJVFJZZ1IrLzdP?=
 =?utf-8?Q?SKvU=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88ee007b-b2c2-4781-9b10-08dc325ad05f
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 21:28:00.7357
 (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: Y9eYRQ2aH1N5L/3fX0xICHGciRHzcETJ3QduYt3QXz+nWAJNW3lpFvFUlvZtCCU8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7524

Hi James,

On 2/20/24 11:56, James Morse wrote:
> Hi Babu,
> 
> On 19/01/2024 18:22, 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
>>
>> Detect the feature details and update
>> /sys/fs/resctrl/info/L3_MON/mon_features.
>>
>> If the system supports Assignable Bandwidth Monitoring Counters (ABMC),
>> the output will have additional text.
>>  $ cat /sys/fs/resctrl/info/L3_MON/mon_features
>>    mbm_assign_capable
>>
>> 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).
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 4efe2d6a9eb7..f40ee271a5c7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -303,6 +303,17 @@ static void rdt_get_cdp_l2_config(void)
>>  	rdt_get_cdp_config(RDT_RESOURCE_L2);
>>  }
>>  
>> +static void rdt_get_abmc_cfg(struct rdt_resource *r)
>> +{
>> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>> +	u32 eax, ebx, ecx, edx;
>> +
>> +	r->mbm_assign_capable = true;
>> +	/* Query CPUID_Fn80000020_EBX_x05 for number of ABMC counters */
>> +	cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
> 
>> +	hw_res->mbm_assignable_counters = (ebx & 0xFFFF) + 1;
> 
> Please put the mbm_assignable_counters field in struct rdt_resource. The filesystem code
> needs to access this to allocate/free counters and report how many are available.
> After all this gets split and the filesystem code moves to /fs/, the rdt_hw_resrouce
> structure is inaccessible to the filesystem code.

Ok. Sure.

> 
> 
>> +}
>> +
>>  static void
>>  mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m, struct rdt_resource *r)
>>  {
>> @@ -815,6 +826,12 @@ static __init bool get_rdt_alloc_resources(void)
>>  	if (get_slow_mem_config())
>>  		ret = true;
>>  
>> +	if (rdt_cpu_has(X86_FEATURE_ABMC)) {
>> +		r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +		rdt_get_abmc_cfg(r);
> 
>> +		ret = true;
> 
> Does it make sense to report rdt_alloc_capable if the SoC has ABMC, but nothing that can
> be configured?

Good catch. Will move it to get_rdt_mon_resources().
> 
> This code would probably make more sense in the get_rdt_mon_resources().

Sure.
> 
> 
>> +	}
>> +
>>  	return ret;
>>  }
>>  
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index a4f1aa15f0a2..01eb0522b42b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -391,6 +391,8 @@ struct rdt_parse_data {
>>   *			resctrl_arch_get_num_closid() to avoid confusion
>>   *			with struct resctrl_schema's property of the same name,
>>   *			which has been corrected for features like CDP.
> 
>> + * @mbm_assignable_counters:
>> + *			Maximum number of assignable ABMC counters
> 
> As above, please move this to struct rdt_resource. The 'hw' version becomes private to the
> arch code after the move to /fs//
> 
> 
>>   * @msr_base:		Base MSR address for CBMs
>>   * @msr_update:		Function pointer to update QOS MSRs
>>   * @mon_scale:		cqm counter * mon_scale = occupancy in bytes
> 
> 
> Thanks,
> 
> James
> 

-- 
Thanks
Babu Moger