| 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: <119e3ad5-afe1-43e5-8784-dd8f9e387545@amd.com>
Date: Fri, 7 Feb 2025 11:30:31 -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: 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,
sandipan.das@....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 v11 10/23] x86/resctrl: Remove MSR reading of event
configuration value
Hi Reinette,
On 2/5/2025 5:58 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/22/25 12:20 PM, Babu Moger wrote:
>> The event configuration is domain specific and initialized during domain
>> initialization. The values are stored in struct rdt_hw_mon_domain.
>>
>> It is not required to read the configuration register every time user asks
>> for it. Use the value stored in struct rdt_hw_mon_domain instead.
>>
>> Introduce resctrl_arch_mon_event_config_get() and
>> resctrl_arch_mon_event_config_set() to get/set architecture domain specific
>> mbm_total_cfg/mbm_local_cfg values.
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 15 +++++++
>> arch/x86/kernel/cpu/resctrl/monitor.c | 46 +++++++++++++++++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 +++++---------------------
>> 3 files changed, 72 insertions(+), 50 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ab28b9340ee7..cfaea20145d0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -605,6 +605,18 @@ union cpuid_0x10_x_edx {
>> unsigned int full;
>> };
>>
>> +/**
>> + * struct mon_config_info - Monitoring event configuratiin details
>
> Same typo as previous version.
I am really sorry about this.
>
>> + * @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;
>> +};
>> +
>> void rdt_last_cmd_clear(void);
>> void rdt_last_cmd_puts(const char *s);
>> __printf(1, 2)
>> @@ -674,4 +686,7 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable);
>> bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r);
>> void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>> unsigned int mon_event_config_index_get(u32 evtid);
>> +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);
>> #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 8917c7261680..6fe9e610e9a0 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
>>
>> return 0;
>> }
>> +
>> +u32 resctrl_arch_mon_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_mon_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_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;
>> + default:
>> + break;
>> + }
>> +}
>
> This new arch API has sharp corners because of asymmetry of where resctrl
> runs the arch function. I do not think it is required to change this since we
> can only speculate about how this may be used in the future but I do think
> it will be helpful to add comments that highlight:
>
> resctrl_arch_mon_event_config_get() -> May run on CPU that does not belong to domain.
> resctrl_arch_mon_event_config_set() -> Runs on CPU that belongs to domain.
Sure. will do.
>
> ...
>
>> @@ -1683,33 +1653,23 @@ 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};
>
> As discussed in previous version it is unnecessary to explicitly initialize
> the structure if it is fully initialized in the code. This avoids need for
> future cleanups like commit 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
Yes. Need to change it.
thanks
Babu
Powered by blists - more mailing lists