linux-kernel - Re: [PATCH v10 11/24] x86/resctrl: Remove MSR reading of event configuration value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d1b79835-f24b-4a37-9e11-60a9834ad98b@amd.com>
Date: Fri, 20 Dec 2024 10:09:56 -0600
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....com>, corbet@....net, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 tony.luck@...el.com, peternewman@...gle.com
Cc: fenghua.yu@...el.com, x86@...nel.org, hpa@...or.com, paulmck@...nel.org,
 akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
 xiongwei.song@...driver.com, pawan.kumar.gupta@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, jpoimboe@...nel.org, perry.yuan@....com,
 kai.huang@...el.com, xiaoyao.li@...el.com, seanjc@...gle.com,
 xin3.li@...el.com, andrew.cooper3@...rix.com, ebiggers@...gle.com,
 mario.limonciello@....com, james.morse@....com, tan.shaopeng@...itsu.com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 maciej.wieczor-retman@...el.com, eranian@...gle.com
Subject: Re: [PATCH v10 11/24] x86/resctrl: Remove MSR reading of event
 configuration value

Hi Reinette,

On 12/19/2024 4:12 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/12/24 12:15 PM, Babu Moger wrote:
> 
>> @@ -1604,33 +1645,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>   	}
>>   }
>>   
>> -static void mon_event_config_read(void *info)
>> -{
>> -	struct mon_config_info *mon_info = info;
>> -	unsigned int index;
>> -	u64 msrval;
>> -
>> -	index = mon_event_config_index_get(mon_info->evtid);
>> -	if (index == INVALID_CONFIG_INDEX) {
>> -		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>> -		return;
>> -	}
>> -	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>> -
>> -	/* Report only the valid event configuration bits */
>> -	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>> -}
>> -
>> -static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
>> -{
>> -	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
>> -}
>> -
>>   static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
>>   {
>> -	struct mon_config_info mon_info;
>>   	struct rdt_mon_domain *dom;
>>   	bool sep = false;
>> +	u32 val;
> 
> Could this variable name be more descriptive? For example, mon_config, or config_val as
> used in mbm_config_write_domain()?

Will change it to config_val.

> 
> ...
> 
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index f11d6fdfd977..c8ab3d7a0dab 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -118,6 +118,18 @@ struct rdt_mon_domain {
>>   	int				cqm_work_cpu;
>>   };
>>   
>> +/**
>> + * struct mon_config_info - Monitoring event configuratiin details
> 
> configuratiin -> configuration

Sure.

> 
> ... but actually, the motivation for moving this struct here was
> to make it available for an arch to interpret the data passed
> via resctrl_arch_mon_event_config_set(). This patch passes data
> in this struct but a later patch modifies
> resctrl_arch_mon_event_config_set() to not use struct anymore ...
> and then leaves struct mon_config_info here.
> 
> Even so, considering Boris's preference this is no longer needed.

ok. I will move the "struct mon_config_info" definition where it is 
used(rdtgroup.c).

> 
>> + * @d:			Domain for the event
>> + * @evtid:		Event type
>> + * @mon_config:		Event configuration value
>> + */
>> +struct mon_config_info {
>> +	struct rdt_mon_domain *d;
>> +	enum resctrl_event_id evtid;
>> +	u32 mon_config;
>> +};
>> +
>>   /**
>>    * struct resctrl_cache - Cache allocation related data
>>    * @cbm_len:		Length of the cache bit mask
>> @@ -352,6 +364,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
>>    */
>>   void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
>>   
>> +void resctrl_arch_mon_event_config_set(void *info);
>> +u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
>> +				      enum resctrl_event_id eventid);
>> +
> 
> Please move to internal header file instead and consider this for
> all changes to include/linux/resctrl.h

Sure. Thanks

Babu