Received-SPF: pass (google.com: domain of linux-kernel+bounces-168416-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: <5c612f5c-7709-49db-b998-a676b61245e1@intel.com>
Date: Fri, 3 May 2024 16:33:02 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 11/17] x86/resctrl: Introduce mbm_total_cfg and
 mbm_local_cfg
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>
 <46f83accddba3c25a428d1a30f6091913a4f7c73.1711674410.git.babu.moger@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <46f83accddba3c25a428d1a30f6091913a4f7c73.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?eVRiRE9xYWFLU05mYmMwamN2bTgxUy9ZOHl4MVBTak5kL0VteVRTamZSN0JQ?=
 =?utf-8?B?YnB3WVlaZ0pqWE9PYVl1YlFHdHRJWG9HZEZnSHNFRkRjbmdmcUhwY2FwajRG?=
 =?utf-8?B?bEFDTldudXA2YnhjVUJoRkhBSk55SjdSYXVKR0lSUVNkR2lUeHIrdVYvcFVJ?=
 =?utf-8?B?VTA3My9OUi90Yll0M2VTMVFPY1didTZjRmM3eWZ4Q211TDdmeTFmMG4rT2U5?=
 =?utf-8?B?Vy9obk9WOXp1cHZZVlJPbXpRaWdZZHdxb3NHMlFRZkRaUmFUcERicWcyRlpK?=
 =?utf-8?B?THRkcElycHZFTmhUMDRad0lyOHRsSmc3Nkk3b1A1MmJDR2UzTXk2TVJnaGhT?=
 =?utf-8?B?SGxTRjZ5OThhOXRDNUxvWFIwQkg3eTJRUm1ZK3pBUEtJZmVENlduRnNIeldt?=
 =?utf-8?B?OGk2ZVhxY3d3aWpsS1NCZ25PQ3NoTDJXWjIrLzVaNGJYQnc2ODBhclZLMnBy?=
 =?utf-8?B?Q2R5RDRZNE8yVWhjMnpOdjV6LytNdU9VYnhpL1gvWnJ6NytnNUp1VFN3blk0?=
 =?utf-8?B?aFJRN2I2M2pWK21uSDNHR09qWC9lQWJacTBudmhCRzU0d2lKdTN1MDYvR2RK?=
 =?utf-8?B?T1RUQVY1N0YyVFpOSWRnVVo0UHpsbCsyVE82VnpQZVpYVXl0d2pjYkJTUXJT?=
 =?utf-8?B?Wm8xTisrQXNYV2xjdGdRSTRaTFJLQjNXb3p0dUllT2Y4WFI2ODJleFBac2Q3?=
 =?utf-8?B?U3g3WFRuK2NpY29zUjErT3FCWWRtWkRQaTNucmswK3pqWE9hcUJKM0JJNlNz?=
 =?utf-8?B?RzkvWnR4VGRjM0R6VDV1cytrVXI1VHRSZVBaQmVGbkRMK3JERi9JNUc0ZWZo?=
 =?utf-8?B?clZkODRNcmhsWDU0QUo1T01BeUNXd2NzalNmdDNSUEF5VFErRnN6elVmY3ZX?=
 =?utf-8?B?M1hRYWVoN2hJdlhkTkFVRDZTZ213WVhkdm9lNVhzdlFVTWxEdk1IZHlPNzBM?=
 =?utf-8?B?bG9HYXB0S0c2UUV2cWpnaXlLSDdRVEJYcFU1T0krdmFzZUtHQVhJQTl1MUFp?=
 =?utf-8?B?VWtMdHNEUWJzOGs0bGpoZ01odzB1eWNVWDZYOHJpWmM0ay9LVzRDSEs3S2cx?=
 =?utf-8?B?TmZJaDJiYUZGWEQ5TVVad29pWk1wUXcyallkYzNFcElvTUR6dG5SSTVMejUw?=
 =?utf-8?B?cENFbit6SEpvcEw2dVVDdTZhUGpvSk1RelMycHROeEsxMFBhQUVodUVzd1gy?=
 =?utf-8?B?dFh6RHZZUnJNTXNmY2lQeGlKYlBod2gxa0tPdFN6OEZDVCt1dXZmeC9qdTFy?=
 =?utf-8?B?NjFDNlNrSUt2d2V3QjhPOWJ5TGtBQ01BS3ZSU29keXN2Rk5JWFdpZlJTZ3Vz?=
 =?utf-8?B?b2d5Uk5EeGRONDdaSmtLUlJWMHdrT0JlSTR0VzZJRWd3YnVCeFAveGxIWTVs?=
 =?utf-8?B?Zk5raFUwMHZjb1pZUmY3WGFHQ0M3UHUxVDRmTDFlbkFvY09EVHRBYTBBWnNT?=
 =?utf-8?B?ekl1dmpGc2lYa3NhVDg0SVY4Y25RbEpRMjl1NTVHMVhjYkI4VmxGYWRVUUFN?=
 =?utf-8?B?aExHWE10d2VKTVRmRkltUmV1NXhpeitUV1pra0JVQitoVFpSNlNwTUgzSDZD?=
 =?utf-8?B?UERxNCszZlpXL25IbjBjWmVuaC9ZeUZlRnh3S0h2TzVpUXhjM1I4enBQaXVv?=
 =?utf-8?B?WS91WWZkQ1NxUVVBR1QzUVNoOUlldllaUTN4bVlGeXVEKzhVYVhUMEpWK2Nj?=
 =?utf-8?B?ZnVkdm83bGZUTzZHQUhxaEFVVDNaQ1dDQkxJK3pjL0tVbll1dWVXdkljZzlI?=
 =?utf-8?B?aUtraXdmaFpYOTNuRTdiejZ6OTBmK1FyQ0taZGt5ZEhybzI2QUtONndFN1U0?=
 =?utf-8?B?bHN6aU9maEg5d2FMb1VKbGNwZEVhWFJKSHRza2tXWi9hMmd6cVRHUkEyYVNE?=
 =?utf-8?B?ZHJVd0lDd3BwWjZYeENUeFdCNEwxL2ZDSmlSR1M0UnVURmlvMzZhM1FTTFdI?=
 =?utf-8?B?eFNsaUxaajNURzcvdkdoYnkxa2dmYWs2eU1IMENBSlhQY3krR2VJc0I0MlNI?=
 =?utf-8?B?azZaRnEvakpGQ2lmZ2JHbE0wY3RxVHduRWJxUWp4UFlNMUNhbEppU1Fvaytj?=
 =?utf-8?B?R09GazMxeDNUOUpoRXVDN2MxWklHRnUxMk5uY3RxZ3JjeGZPUWpaRG9pV05J?=
 =?utf-8?B?Z3ZndEkwWVNoNUdYVVlCdmQ5V0VpenFwWXpITC94ZnhEa0lEQmNWVnR2ekJy?=
 =?utf-8?B?SEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 09bb9799-2f63-4c37-d17b-08dc6bc96240
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:33:06.1923
 (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: aT/kPETzi7IbVCAGWf0C25TrSNEESeaeG22HeOp4ZWiKB4cKMfXIJJc5ZmzX7loDym8S+7/yAuUzCOXfR7f6G/DMcXj0oWY4j0mmRN8ViAM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB8076
X-OriginatorOrg: intel.com

Hi Babu,

On 3/28/2024 6:06 PM, Babu Moger wrote:
> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
> supported, the bandwidth events can be configured to track specific events.
> The event configuration is domain specific. ABMC (Assignable Bandwidth
> Monitoring Counters) feature needs event configuration information to
> assign RMID to the hardware counter. Currently, this information is not
> available.

hmmm ... "Currently, this information is not available." does not sound
accurate. Perhaps it can be made more specific with something like:
"Event configurations are not stored in resctrl but instead always
read from or written to hardware directly when prompted by user space."
(feel free to improve)

> 
> Save the event configuration information in the rdt_hw_domain, so it can
> be used while for RMID assignment.

"be used while for RMID assignment" -> "be used for RMID assignment"?

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> 
> ---
> v3: Minor changes related to rebase in mbm_config_write_domain.
> 
> v2: No changes.
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
>  arch/x86/kernel/cpu/resctrl/internal.h |  3 +++
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 11 +++++++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 +++++++++++++++-
>  4 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 50e9ec5e547b..ed4f6d49d737 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -555,6 +555,8 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  		return;
>  	}
>  
> +	arch_domain_mbm_evt_config(hw_dom);
> +
>  	list_add_tail_rcu(&d->list, add_pos);
>  
>  	err = resctrl_online_domain(r, d);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 41b06d46ea74..88453c86474b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -385,6 +385,8 @@ struct rdt_hw_domain {
>  	u32				*ctrl_val;
>  	struct arch_mbm_state		*arch_mbm_total;
>  	struct arch_mbm_state		*arch_mbm_local;
> +	u32				mbm_total_cfg;
> +	u32                             mbm_local_cfg;

(please fix tabs/spaces)

>  };
>  
>  static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r)
> @@ -648,6 +650,7 @@ void __check_limbo(struct rdt_domain *d, bool force_free);
>  void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  void __init resctrl_file_fflags_init(const char *config,
>  				     unsigned long fflags);
> +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom);
>  void rdt_staged_configs_clear(void);
>  bool closid_allocated(unsigned int closid);
>  int resctrl_find_cleanest_closid(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 56dc49021540..8677dbf6de43 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1090,3 +1090,14 @@ void __init intel_rdt_mbm_apply_quirk(void)
>  	mbm_cf_rmidthreshold = mbm_cf_table[cf_index].rmidthreshold;
>  	mbm_cf = mbm_cf_table[cf_index].cf;
>  }
> +
> +void arch_domain_mbm_evt_config(struct rdt_hw_domain *hw_dom)
> +{
> +	if (mbm_total_event.configurable)
> +		hw_dom->mbm_total_cfg = MAX_EVT_CONFIG_BITS;
> +
> +	if (mbm_local_event.configurable)
> +		hw_dom->mbm_local_cfg = READS_TO_LOCAL_MEM |
> +					NON_TEMP_WRITE_TO_LOCAL_MEM |
> +					READS_TO_LOCAL_S_MEM;
> +}

Shouldn't the defaults be discovered from hardware?

Reinette