Received-SPF: pass (google.com: domain of linux-kernel+bounces-175007-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <ed0bc0b7-c37a-466b-ba2d-dcb3c5123b85@amd.com>
Date: Thu, 9 May 2024 16:45:43 -0500
User-Agent: Mozilla Thunderbird
Reply-To: babu.moger@amd.com
Subject: Re: [RFC PATCH v3 07/17] x86/resctrl: Add support to enable/disable
 ABMC feature
To: Reinette Chatre <reinette.chatre@intel.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>
 <0db75c94886da62b8da498ef159d8fe27b0b3811.1711674410.git.babu.moger@amd.com>
 <54b1fe8f-13e5-440b-bb36-4100c1d283d0@intel.com>
 <54ec73f9-6c9e-44f4-8ee9-a683bfcee607@amd.com>
 <a873f6ba-904e-4317-b177-5645c3a8526d@intel.com>
Content-Language: en-US
From: "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <a873f6ba-904e-4317-b177-5645c3a8526d@intel.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?MHJ5NFlpRUpTdlpMelZkYVQrVDRmMWd4cEZBYUI1ZnBzQ1cwRG5IMzd5MXo5?=
 =?utf-8?B?ZnN3ZkZZTmdsMWFDSEpvRVEwbFZDeW03VGtxaFF6SEl0SWhLaG1aWlF5b3Ar?=
 =?utf-8?B?NmQ4OVFLS3lPQUE3NVVFVG5JODJQZ2tYNkxUVWFGaGplYWN0L0gyREZHa2p6?=
 =?utf-8?B?OS80aGllNTRIbURlNWxnT1pobkpjM2ZTeldhandkYmpxTDliQ3orNUFNbWxr?=
 =?utf-8?B?djloM2xrL1VFa3R6SXFQaDc4L0xtbXI2dVRjTEpsT0tMNlQvUW1QQ2dKQ3ZP?=
 =?utf-8?B?NlhwRW9zUkJNQmxuaTY0UmNSQ3djWGNENTJjdzVrTTQzQjlScHdvNHlGN25Z?=
 =?utf-8?B?Q1RwZ2JnTVFrSVNTbU1sVXJLS0RVanU2V1l5ME03N3JlYkJTbEVPMzAwSkhn?=
 =?utf-8?B?ajRXNFRpSFJRek5vTlk5alpTK3VZcU9OMzN3Ri93SW5qRDk2QThzamdIZUtL?=
 =?utf-8?B?MmVIRkdYMW9STWh6N0Q3eTlFNHdzcW53VTR1QjdneUhWNGFCdVN6VmxjSjU5?=
 =?utf-8?B?RjB4MGVFZzdEQmZaM3BSV1dWQzNuWDRuTXFpU0JudnlmS1JCcEs0MHN2RTM0?=
 =?utf-8?B?cVhvTGhHTklaQzlBNjJna3hlcEFmRHAyQ3VWNTY2RTROeVZBVng0eFhQbGlH?=
 =?utf-8?B?eDI3WlBFVGlzc1NPblhlbjJnQWdQamJmTVl2SVBpYUd6MU5MOXo4dFhrSGc3?=
 =?utf-8?B?SjJobUQ5VHFDNjdRQ2dUSGhPTUw5Z0NYcmkzUzBaM0tzMW4yTC9VS1hlSFNl?=
 =?utf-8?B?NVVPRkVXUHMrc1lTWlJ2UkxuK3VkaS9uMWVBdnovRThKdDAzNWxEKzdXNTdx?=
 =?utf-8?B?WDM0ekNiRWQzdE1xOVRlSW1qZ1haVU16NCsvZVVHRVlaNXNoSlFzdDRhV1Bo?=
 =?utf-8?B?UXpFMGVXZmpIV2FUR3MxcXI4ZC9qVWtmdUFQeW1rNWROcjV3aUl5bm5IUmRQ?=
 =?utf-8?B?SHlHbVRxalQ3QWtTVlA2Si9KTHVNOHY2QUtKQy8xdlpjbTQ1MG9YdGxIc0ZU?=
 =?utf-8?B?RUpiWXQ2d2FnbjVzR3EwRGprcnh5d0c1ODJpdXpMakkwZkV4bzFzUXE3cjhZ?=
 =?utf-8?B?SG50TlFQTDFrWS8vTm1ac3RvZDRwY215NnlGQXRDNWFPUkxTTW1icHRvSVRD?=
 =?utf-8?B?a1VLSjUzVVdXQXk1YnRIRnB3cVM3YWo0QndUTDJaNmNxTkFibEF3YWZqdmZx?=
 =?utf-8?B?dmpmTzl6dS9zMGsyUGJlZDJyQVY2UHJRbUxxUjc0b01BMWZCMDVNSEFXV0tR?=
 =?utf-8?B?QTJBUmtJYm8xWm9EY0xQWFg3MWtxby9uUm9sYkV2VktYUlh3eHl0aFV5Njl6?=
 =?utf-8?B?MWhiZTZxZTFlaXNaYTJUV1dGSGxtQVVGL2pXWU1uMmJLTHVBdENoSStEU3dS?=
 =?utf-8?B?dFQrNW1IUk9NUnM1YUs5TWVqZ0JxbVFUZTlhZnV1TWNpSTRhMHIxMmIyTUpV?=
 =?utf-8?B?bm5lbVJpVytPaDdpYUdKaWJmc21wWFpXSFB5QVNKcTh2eUVPQWhML0dSUE9v?=
 =?utf-8?B?Q2tKNURPYS9rUFYwL2M3UXV0UmVxTWowayt0aXdSdldQSmxqcUZCdjFtR0Jx?=
 =?utf-8?B?MkErYlA0dVZDRHZXSHhwdjI0NWljRnpCMUNSbDIwTDFlMkNxY0RHNmxKNUdp?=
 =?utf-8?B?OHFuNGxHS0N1Qnp2cFB4K2xMd2Qva1dpYmREZzJ1a0tZeGsxUWs1amhWcjMr?=
 =?utf-8?B?ZVEvaW9ET3VlZkZFYWVSZ0RuM2xob2ErY3Ywb3VxS0FaSEgwN21scVNJOHdZ?=
 =?utf-8?B?UmREZU5wcG1lT2RETUZBT1g1RDlLbmtVY2hxeGIwMVBmTVF4U0dFQ3FaenQ4?=
 =?utf-8?B?N04vMTJmOFpyeURwM3BOaTB1QTNqTHFHQ21FbERHb1I3YlBMWkJxZi9hbytH?=
 =?utf-8?B?UXVubmd5NEtPQjRUT0FyOVVmU0RQdmZ1d3RWdnhLcUZzcGd2dkFrQ3EwV1U0?=
 =?utf-8?B?Tnc0a3RsVVhkb1liMU1nV3Y2cS9ZSUI0SllZYVBRUUVMakNDN0FRSVFQQzM4?=
 =?utf-8?B?MUVFRDdaSWorMDZsQ0E4NGZuV2VkNUJHV0JyS2YwYm9BY2ZNb0VGRGkwN25H?=
 =?utf-8?B?dnJUREoxWnU4aXRja0YrOXE1OFAzM3FnS3NLRm5JTUd3THg2dGt3T0tmZkVY?=
 =?utf-8?Q?eAcM=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 44d22b57-85c9-4546-8d84-08dc70716399
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2024 21:45:48.5943
 (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: 3z/yXAwUTuCQM410Z9WEKJz9KYNYU2hAl0xaedzJSpvaUw2ldl41AZZ70hPb0Bjk
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB7320

Hi Reinette,

On 5/7/24 15:32, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/7/2024 12:12 PM, Moger, Babu wrote:
>> On 5/3/24 18:30, Reinette Chatre wrote:
>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
> 
> ...
> 
>>>>  /* 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 722388621403..8238ee437369 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -96,6 +96,9 @@ cpumask_any_housekeeping(const struct cpumask *mask, int exclude_cpu)
>>>>  	return cpu;
>>>>  }
>>>>  
>>>> +/* ABMC ENABLE */
>>>
>>> Can this comment be made more useful?
>>
>> How about?
>> /* Setting bit 0 in L3_QOS_EXT_CFG enables ABMC features */
> 
> Regarding "ABMC features" - are there several features connected to
> "ABMC"?
> 

No. It should have been "ABMC feature". Will correct it.

>>
>> Or I can remove it totally.
>>
>>>
> 
> ...
> 
>>>> +int resctrl_arch_set_abmc_enabled(enum resctrl_res_level l, bool enable)
>>>> +{
>>>> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>>> +
>>>> +	if (!hw_res->r_resctrl.mbm_assign_capable)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (enable)
>>>> +		return resctrl_abmc_enable(l);
>>>> +
>>>> +	resctrl_abmc_disable(l);
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Why is resctrl_arch_set_abmc_enabled() necessary? It seem to add an unnecessary
>>> layer of abstraction.
>>>
>>
>> I feel it is better to keep it that way. It is consistent with definition
>> of resctrl_arch_set_cdp_enabled. It handles both enable and disable.
>> Otherwise we have add those checks from the caller.
> 
> Caller needs to know anyway whether to provide "true" or "false" to this function ...
> caller may as well just call the appropriate _enable()/_disable() variant, no?

Yes. We can call resctrl_abmc_enable() and resctrl_abmc_disable() directly.
Thanks
Babu Moger