| 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
| ||
|
Message-ID: <779eaea8-ab16-47b0-b1bd-4dda439ae3d7@amd.com>
Date: Tue, 16 Jul 2024 14:34:39 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
fenghua.yu@...el.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, eranian@...gle.com, james.morse@....com
Subject: Re: [PATCH v5 11/20] x86/resctrl: Remove MSR reading of event
configuration value
Hi Reinette,
On 7/12/24 17:10, Reinette Chatre wrote:
> Hi Babu,
>
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> The event configuration is domain specific and initialized during domain
>> initialization. It is not required to read the configuration register
>> every time user asks for it. Use the value stored in rdt_mon_hw_domain
>
> rdt_mon_hw_domain -> rdt_hw_mon_domain
>
>> instead. Also update the configuration value when user writes it.
>
> Please separate the context/problem/solution clearly.
Sure.
>
>>
>> Introduce resctrl_arch_event_config_get() and
>> resctrl_arch_event_config_set() to get/set architecture domain specific
>> mbm_total_cfg/mbm_local_cfg values.
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v5: Introduced resctrl_arch_event_config_get and
>> resctrl_arch_event_config_get() based on our discussion.
>>
>> https://lore.kernel.org/lkml/68e861f9-245d-4496-a72e-46fc57d19c62@amd.com/
>>
>> v4: New patch.
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 112 +++++++++++++++----------
>> include/linux/resctrl.h | 4 +
>> 2 files changed, 72 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index b2b751741dd8..91c5d45ac367 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1591,10 +1591,59 @@ static int rdtgroup_size_show(struct
>> kernfs_open_file *of,
>> }
>> struct mon_config_info {
>> + struct rdt_mon_domain *d;
>> u32 evtid;
>> u32 mon_config;
>> };
>
> as seen above, mon_config is a u32
>
>> +#define INVALID_CONFIG_VALUE UINT_MAX
>
> So an invalid config value can be U32_MAX?
Sure.
>
>> +
>> +unsigned int resctrl_arch_event_config_get(struct rdt_mon_domain *d,
>> + enum resctrl_event_id eventid)
>> +{
>> + struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +
>> + switch (eventid) {
>> + case QOS_L3_OCCUP_EVENT_ID:
>> + break;
>> + case QOS_L3_MBM_TOTAL_EVENT_ID:
>> + return hw_dom->mbm_total_cfg;
>> + case QOS_L3_MBM_LOCAL_EVENT_ID:
>> + return hw_dom->mbm_local_cfg;
>> + }
>> +
>> + /* Never expect to get here */
>> + WARN_ON_ONCE(1);
>> +
>> + return INVALID_CONFIG_VALUE;
>> +}
>> +
>> +void resctrl_arch_event_config_set(void *info)
>> +{
>> + struct mon_config_info *mon_info = info;
>> + struct rdt_hw_mon_domain *hw_dom;
>> + unsigned int index;
>> +
>> + index = mon_event_config_index_get(mon_info->evtid);
>> + if (index == INVALID_CONFIG_VALUE) {
>
> INVALID_CONFIG_INDEX?
Yes.
>
>> + pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>> + return;
>> + }
>> + wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> +
>> + hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
>> +
>> + switch (mon_info->evtid) {
>> + case QOS_L3_OCCUP_EVENT_ID:
>> + break;
>> + case QOS_L3_MBM_TOTAL_EVENT_ID:
>> + hw_dom->mbm_total_cfg = mon_info->mon_config;
>> + break;
>> + case QOS_L3_MBM_LOCAL_EVENT_ID:
>> + hw_dom->mbm_local_cfg = mon_info->mon_config;
>
> Please add a break here.
Sure.
>
>> + }
>> +}
>> +
>> #define INVALID_CONFIG_INDEX UINT_MAX
>> /**
>> @@ -1619,33 +1668,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 = {0};
>> struct rdt_mon_domain *dom;
>> bool sep = false;
>> + int val;
>> cpus_read_lock();
>> mutex_lock(&rdtgroup_mutex);
>> @@ -1654,11 +1681,13 @@ static int mbm_config_show(struct seq_file *s,
>> struct rdt_resource *r, u32 evtid
>> if (sep)
>> seq_puts(s, ";");
>> - memset(&mon_info, 0, sizeof(struct mon_config_info));
>> - mon_info.evtid = evtid;
>> - mondata_config_read(dom, &mon_info);
>> + val = resctrl_arch_event_config_get(dom, evtid);
>
> There are too many types used interchangeably. The mon_config is a "u32",
> but the new function
> returns "unsigned int", which is then assigned to an "int". Please just
> use one type
> consistently, it is a u32 so resctrl_arch_event_config_get() can return
> u32 and "val" should
> be u32.
>
Sure. Will do.
>> + if (val == INVALID_CONFIG_VALUE) {
>> + rdt_last_cmd_puts("Invalid event configuration\n");
>
> I do not see a reason to print message to user space here. If this error
> is encountered
> then it is a kernel bug and resctrl_arch_event_config_get() would already
> have triggered
> a WARN.
>
> Since this is a "never should happen" scenario I wonder if we can not just
> print
> the INVALID_CONFIG_VALUE to user space?
Ok. Will remove the check and the message.
>
>
>> + break;
>> + }
>> - seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
>> + seq_printf(s, "%d=0x%02x", dom->hdr.id, val);
>> sep = true;
>> }
>> seq_puts(s, "\n");
>> @@ -1689,33 +1718,27 @@ static int mbm_local_bytes_config_show(struct
>> kernfs_open_file *of,
>> return 0;
>> }
>> -static void mon_event_config_write(void *info)
>> -{
>> - struct mon_config_info *mon_info = info;
>> - unsigned int index;
>> -
>> - 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;
>> - }
>> - wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> -}
>> static void mbm_config_write_domain(struct rdt_resource *r,
>> struct rdt_mon_domain *d, u32 evtid, u32 val)
>> {
>> struct mon_config_info mon_info = {0};
>> + int config_val;
>> /*
>> - * Read the current config value first. If both are the same then
>> + * Check the current config value first. If both are the same then
>> * no need to write it again.
>> */
>> - mon_info.evtid = evtid;
>> - mondata_config_read(d, &mon_info);
>> - if (mon_info.mon_config == val)
>> + config_val = resctrl_arch_event_config_get(d, evtid);
>> + if (config_val == INVALID_CONFIG_VALUE) {
>> + rdt_last_cmd_puts("Invalid event configuration\n");
>
> same here about unneeded print to user space. When this is encountered it is
> a kernel bug.
Ok. Will remove the check and the message.
>
>> + return;
>> + }
>> + if (config_val == val)
>> return;
>> + mon_info.d = d;
>> + mon_info.evtid = evtid;
>> mon_info.mon_config = val;
>> /*
>> @@ -1724,7 +1747,8 @@ static void mbm_config_write_domain(struct
>> rdt_resource *r,
>> * are scoped at the domain level. Writing any of these MSRs
>> * on one CPU is observed by all the CPUs in the domain.
>> */
>> - smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_write,
>> + smp_call_function_any(&d->hdr.cpu_mask,
>> + resctrl_arch_event_config_set,
>> &mon_info, 1);
>> /*
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 62f0f002ef41..f017258ebf85 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -352,6 +352,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_event_config_set(void *info);
>> +unsigned int resctrl_arch_event_config_get(struct rdt_mon_domain *d,
>> + enum resctrl_event_id eventid);
>> +
>> extern unsigned int resctrl_rmid_realloc_threshold;
>> extern unsigned int resctrl_rmid_realloc_limit;
>>
>
> Reinette
>
--
Thanks
Babu Moger
Powered by blists - more mailing lists