| 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: <654d1c0f-47e9-4bbe-8b7e-404dc1c746ee@amd.com>
Date: Tue, 20 Aug 2024 11:19:23 -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 v6 11/22] x86/resctrl: Remove MSR reading of event
configuration value
Hi Reinette,
On 8/16/24 16:36, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/6/24 3:00 PM, Babu Moger wrote:
>> The event configuration is domain specific and initialized during domain
>> initialization. The values is stored in rdt_hw_mon_domain.
>
> "The values is stored in rdt_hw_mon_domain." -> "The values are stored
> in struct rdt_hw_mon_domain."
Sure.
>
>>
>> It is not required to read the configuration register every time user asks
>> for it. Use the value stored in rdt_hw_mon_domain instead.
>
> "rdt_hw_mon_domain" -> "struct rdt_hw_mon_domain"
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. Also, remove unused config value
>> definitions.
>
> hmmm ... while the config values are not used they are now established
> ABI and any other architecture that wants to support configurable events
> will need to follow these definitions. It is thus required to keep them
> documented in the kernel in support of future changes. I
> understand that they are documented in user docs, but could we keep them
> in the kernel code also? Since they are unused they could perhaps be moved
> to comments as a compromise?
How about just keeping them as is? I will just not remove it.
>
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v6: Fixed inconstancy with types. Made all the types to u32 for config
>> value.
>> Removed few rdt_last_cmd_puts as it is not necessary.
>> Removed unused config value definitions.
>> Few more updates to commit message.
>>
>> 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/internal.h | 21 -----
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 104 ++++++++++++++-----------
>> include/linux/resctrl.h | 4 +
>> 3 files changed, 64 insertions(+), 65 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 4d8cc36a8d79..1021227d8c7e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -32,27 +32,6 @@
>> */
>> #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
>> -/* Reads to Local DRAM Memory */
>> -#define READS_TO_LOCAL_MEM BIT(0)
>> -
>> -/* Reads to Remote DRAM Memory */
>> -#define READS_TO_REMOTE_MEM BIT(1)
>> -
>> -/* Non-Temporal Writes to Local Memory */
>> -#define NON_TEMP_WRITE_TO_LOCAL_MEM BIT(2)
>> -
>> -/* Non-Temporal Writes to Remote Memory */
>> -#define NON_TEMP_WRITE_TO_REMOTE_MEM BIT(3)
>> -
>> -/* Reads to Local Memory the system identifies as "Slow Memory" */
>> -#define READS_TO_LOCAL_S_MEM BIT(4)
>> -
>> -/* Reads to Remote Memory the system identifies as "Slow Memory" */
>> -#define READS_TO_REMOTE_S_MEM BIT(5)
>> -
>> -/* Dirty Victims to All Types of Memory */
>> -#define DIRTY_VICTIMS_TO_ALL_MEM BIT(6)
>> -
>> /* Max event bits supported */
>> #define MAX_EVT_CONFIG_BITS GENMASK(6, 0)
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 02afd3442876..0047b4eb0ff5 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1605,10 +1605,57 @@ static int rdtgroup_size_show(struct
>> kernfs_open_file *of,
>> }
>> struct mon_config_info {
>> + struct rdt_mon_domain *d;
>> u32 evtid;
>> u32 mon_config;
>> };
>> +u32 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_INDEX)
>> + 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;
>> + break;
>> + }
>> +}
>> +
>> /**
>> * mon_event_config_index_get - get the hardware index for the
>> * configurable event
>> @@ -1631,33 +1678,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;
>> + u32 val;
>> cpus_read_lock();
>> mutex_lock(&rdtgroup_mutex);
>> @@ -1666,11 +1691,11 @@ 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);
>> + if (val == INVALID_CONFIG_VALUE)
>
> Can this check and the "break" that follows be dropped? val being
> INVALID_CONFIG_VALUE would be a kernel bug and
> resctrl_arch_event_config_get()
> would already have printed the WARN. In this unlikely scenario I find it
> unexpected that mbm_config_show() will return success in this case and the
> below seq_printf() would handle the printing of INVALID_CONFIG_VALUE without
> issue anyway.
Sure. I will drop the check and break.
>
>> + 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");
>
> Reinette
>
>
--
Thanks
Babu Moger
Powered by blists - more mailing lists