| 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: <b78a01b4-3583-4689-a894-96dab5dfb9fd@intel.com>
Date: Mon, 18 Nov 2024 11:43:48 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <tglx@...utronix.de>,
<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <x86@...nel.org>, <hpa@...or.com>,
<thuth@...hat.com>, <paulmck@...nel.org>, <rostedt@...dmis.org>,
<akpm@...ux-foundation.org>, <xiongwei.song@...driver.com>,
<pawan.kumar.gupta@...ux.intel.com>, <daniel.sneddon@...ux.intel.com>,
<perry.yuan@....com>, <sandipan.das@....com>, <kai.huang@...el.com>,
<xiaoyao.li@...el.com>, <seanjc@...gle.com>, <jithu.joseph@...el.com>,
<brijesh.singh@....com>, <xin3.li@...el.com>, <ebiggers@...gle.com>,
<andrew.cooper3@...rix.com>, <mario.limonciello@....com>,
<james.morse@....com>, <tan.shaopeng@...itsu.com>, <tony.luck@...el.com>,
<vikas.shivappa@...ux.intel.com>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <peternewman@...gle.com>,
<maciej.wieczor-retman@...el.com>, <eranian@...gle.com>,
<jpoimboe@...nel.org>, <thomas.lendacky@....com>
Subject: Re: [PATCH v9 24/26] x86/resctrl: Update assignments on event
configuration changes
Hi Babu,
On 10/29/24 4:21 PM, Babu Moger wrote:
> Users can modify the configuration of assignable events. Whenever the
> event configuration is updated, MBM assignments must be revised across
> all monitor groups within the impacted domains.
Please revisit the "Changelog" section in
Documentation/process/maintainer-tip.rst
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> v9: Again patch changed completely based on the comment.
> https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@intel.com/
> Introduced resctrl_mon_event_config_set to handle IPI.
> But sending another IPI inside IPI causes problem. Kernel reports SMP
> warning. So, introduced resctrl_arch_update_cntr() to send the command directly.
I see ... the WARN is because there is a check whether IRQs are disabled before
the check whether the function can be run locally.
>
> v8: Patch changed completely.
> Updated the assignment on same IPI as the event is updated.
> Could not do the way we discussed in the thread.
> https://lore.kernel.org/lkml/f77737ac-d3f6-3e4b-3565-564f79c86ca8@amd.com/
> Needed to figure out event type to update the configuration.
>
> v7: New patch to update the assignments. Missed it earlier.
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 86 +++++++++++++++++++++++---
> include/linux/resctrl.h | 3 +-
> 2 files changed, 79 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5b8bb8bd913c..7646d67ea10e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1710,6 +1710,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
> }
>
> struct mon_config_info {
> + struct rdt_resource *r;
> struct rdt_mon_domain *d;
> u32 evtid;
> u32 mon_config;
> @@ -1735,26 +1736,28 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
> return INVALID_CONFIG_VALUE;
> }
>
> -void resctrl_arch_mon_event_config_set(void *info)
> +void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
> + enum resctrl_event_id eventid, u32 val)
> {
> - 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);
> + index = mon_event_config_index_get(eventid);
> if (index == INVALID_CONFIG_INDEX)
> return;
>
> - wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
> + wrmsr(MSR_IA32_EVT_CFG_BASE + index, val, 0);
>
> - hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
> + hw_dom = resctrl_to_arch_mon_dom(d);
>
> - switch (mon_info->evtid) {
> + switch (eventid) {
> case QOS_L3_MBM_TOTAL_EVENT_ID:
> - hw_dom->mbm_total_cfg = mon_info->mon_config;
> + hw_dom->mbm_total_cfg = val;
> break;
> case QOS_L3_MBM_LOCAL_EVENT_ID:
> - hw_dom->mbm_local_cfg = mon_info->mon_config;
> + hw_dom->mbm_local_cfg = val;
> + break;
> + default:
> break;
> }
> }
> @@ -1826,6 +1829,70 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
> return 0;
> }
>
> +static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_index(int cntr_id, unsigned int index)
> +{
> + struct rdtgroup *prgrp, *crgrp;
> +
> + /* Check if the cntr_id is associated to the event type updated */
> + list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> + if (prgrp->mon.cntr_id[index] == cntr_id)
> + return prgrp;
> +
> + list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
> + if (crgrp->mon.cntr_id[index] == cntr_id)
> + return crgrp;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> + enum resctrl_event_id evtid, u32 rmid,
> + u32 closid, u32 cntr_id, u32 val)
> +{
> + union l3_qos_abmc_cfg abmc_cfg = { 0 };
> +
> + abmc_cfg.split.cfg_en = 1;
> + abmc_cfg.split.cntr_en = 1;
> + abmc_cfg.split.cntr_id = cntr_id;
> + abmc_cfg.split.bw_src = rmid;
> + abmc_cfg.split.bw_type = val;
> +
> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
Is it needed to create an almost duplicate function? What if instead
only resctrl_arch_config_cntr() exists and it uses parameter to decide
whether to call resctrl_abmc_config_one_amd() directly or via
smp_call_function_any()? I think that should help to make clear how
the code flows.
Also note that this is an almost identical arch callback with no
error return. I expect that building on existing resctrl_arch_config_cntr()
will make things easier to understand.
> +}
> +
> +static void resctrl_mon_event_config_set(void *info)
> +{
> + struct mon_config_info *mon_info = info;
> + struct rdt_mon_domain *d = mon_info->d;
> + struct rdt_resource *r = mon_info->r;
Note that local variable r is created here while the function is inconsistent by
switching between using r and mon_info->r.
> + struct rdtgroup *rdtgrp;
> + unsigned int index;
> + u32 cntr_id;
> +
> + resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
> +
> + if (!resctrl_arch_mbm_cntr_assign_enabled(r))
> + return;
> +
> + index = mon_event_config_index_get(mon_info->evtid);
This is an AMD arch specific helper to know which offset of an MSR to use. It should
not be used directly in resctrl fs code, this is what MBM_EVENT_ARRAY_INDEX was created for.
Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used,
within rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name.
> + if (index == INVALID_CONFIG_INDEX)
> + return;
> +
> + for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> + if (test_bit(cntr_id, d->mbm_cntr_map)) {
> + rdtgrp = rdtgroup_find_grp_by_cntr_id_index(cntr_id, index);
> + if (rdtgrp)
> + resctrl_arch_update_cntr(mon_info->r, d,
> + mon_info->evtid,
> + rdtgrp->mon.rmid,
> + rdtgrp->closid,
> + cntr_id,
> + mon_info->mon_config);
> + }
> + }
> +}
Could you please add some function comments to explain the flow here? For example,
what should reader consider if there is to rdtgroup found?
Reinette
Powered by blists - more mailing lists