| 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: <0ffec8d1-a27f-48b4-997a-8cc057de701e@intel.com>
Date: Fri, 12 Jul 2024 15:10:05 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....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 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.
>
> 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?
> +
> +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?
> + 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.
> + }
> +}
> +
> #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.
> + 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?
> + 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.
> + 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
Powered by blists - more mailing lists