| 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: <6faaa78e-d269-4391-bb42-5e9b936734f9@intel.com>
Date: Fri, 11 Apr 2025 13:49:16 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <tony.luck@...el.com>,
<peternewman@...gle.com>
CC: <corbet@....net>, <tglx@...utronix.de>, <mingo@...hat.com>,
<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
<hpa@...or.com>, <paulmck@...nel.org>, <akpm@...ux-foundation.org>,
<thuth@...hat.com>, <rostedt@...dmis.org>, <ardb@...nel.org>,
<gregkh@...uxfoundation.org>, <daniel.sneddon@...ux.intel.com>,
<jpoimboe@...nel.org>, <alexandre.chartre@...cle.com>,
<pawan.kumar.gupta@...ux.intel.com>, <thomas.lendacky@....com>,
<perry.yuan@....com>, <seanjc@...gle.com>, <kai.huang@...el.com>,
<xiaoyao.li@...el.com>, <kan.liang@...ux.intel.com>, <xin3.li@...el.com>,
<ebiggers@...gle.com>, <xin@...or.com>, <sohil.mehta@...el.com>,
<andrew.cooper3@...rix.com>, <mario.limonciello@....com>,
<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<maciej.wieczor-retman@...el.com>, <eranian@...gle.com>
Subject: Re: [PATCH v12 01/26] x86/resctrl: Introduce mbm_total_cfg and
mbm_local_cfg in struct rdt_hw_mon_domain
Hi Babu,
On 4/3/25 5:18 PM, Babu Moger wrote:
> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
> supported, the bandwidth events can be configured to track specific
> events. The event configuration is domain specific. Event configurations
> are not stored in resctrl but instead always read from or written to
> hardware directly when prompted by user space.
Why is this a problem?
>
> Read the event configuration from the hardware during domain
> initialization and store the configuration value in the rdt_hw_mon_domain
> structure for later use when the user requests to display it.
Why is this required?
This series is about adding support for ABMC while this appears to be
an optimization for BMEC. Even more, as I see it, this optimization makes
resctrl support of ABMC and BMEC confusing (more below).
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> v12: Fixed the conflicts due to recent merge.
> This patch is for BMEC and there is no dependancy on ABMC feature.
Why still do it?
> Moved it earlier.
>
> v11: Resolved minor conflicts due to code displacement. Actual code didnt
> change.
>
> v10: Conflicts due to code displacement. Actual code didnt change.
>
> v9: Added Reviewed-by tag. No other changes.
>
> v8: Renamed resctrl_mbm_evt_config_init() to arch_mbm_evt_config_init()
> Minor commit message update.
>
> v7: Fixed initializing INVALID_CONFIG_VALUE to mbm_local_cfg in case of error.
>
> v6: Renamed resctrl_arch_mbm_evt_config -> resctrl_mbm_evt_config_init
> Initialized value to INVALID_CONFIG_VALUE if it is not configurable.
> Minor commit message update.
>
> v5: Exported mon_event_config_index_get.
> Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.
>
> v4: Read the configuration information from the hardware to initialize.
> Added few commit messages.
> Fixed the tab spaces.
>
> v3: Minor changes related to rebase in mbm_config_write_domain.
>
> v2: No changes.
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 2 ++
> arch/x86/kernel/cpu/resctrl/internal.h | 9 +++++++++
> arch/x86/kernel/cpu/resctrl/monitor.c | 26 ++++++++++++++++++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +-
> 4 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index cf29681d01e0..a28de257168f 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -558,6 +558,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> return;
> }
>
> + arch_mbm_evt_config_init(hw_dom);
> +
> list_add_tail_rcu(&d->hdr.list, add_pos);
>
> err = resctrl_online_mon_domain(r, d);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c44c5b496355..9846153aa48f 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -32,6 +32,9 @@
> */
> #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
>
> +#define INVALID_CONFIG_VALUE U32_MAX
> +#define INVALID_CONFIG_INDEX UINT_MAX
> +
> /**
> * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
> * aren't marked nohz_full
> @@ -335,6 +338,8 @@ struct rdt_hw_ctrl_domain {
> * @d_resctrl: Properties exposed to the resctrl file system
> * @arch_mbm_total: arch private state for MBM total bandwidth
> * @arch_mbm_local: arch private state for MBM local bandwidth
> + * @mbm_total_cfg: MBM total bandwidth configuration
> + * @mbm_local_cfg: MBM local bandwidth configuration
> *
> * Members of this structure are accessed via helpers that provide abstraction.
> */
> @@ -342,6 +347,8 @@ struct rdt_hw_mon_domain {
> struct rdt_mon_domain d_resctrl;
> struct arch_mbm_state *arch_mbm_total;
> struct arch_mbm_state *arch_mbm_local;
> + u32 mbm_total_cfg;
> + u32 mbm_local_cfg;
> };
This introduces an architecture managed per-domain event configuration while
the rest of the series introduces a resctrl fs managed global event configuration.
I see this as the start of a source for confusion about how events are configured since
there is no further connection between this per-domain event configuration maintained
by the architecture and the global event configuration maintained by resctrl fs.
>
> static inline struct rdt_hw_ctrl_domain *resctrl_to_arch_ctrl_dom(struct rdt_ctrl_domain *r)
> @@ -504,6 +511,8 @@ void resctrl_file_fflags_init(const char *config, unsigned long fflags);
> void rdt_staged_configs_clear(void);
> bool closid_allocated(unsigned int closid);
> int resctrl_find_cleanest_closid(void);
> +void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
> +unsigned int mon_event_config_index_get(u32 evtid);
>
> #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
> int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index a93ed7d2a160..abd337fbd01d 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1284,6 +1284,32 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
> return 0;
> }
>
> +void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom)
> +{
> + unsigned int index;
> + u64 msrval;
> +
> + /*
> + * Read the configuration registers QOS_EVT_CFG_n, where <n> is
> + * the BMEC event number (EvtID).
> + */
> + if (mbm_total_event.configurable) {
Please keep an eye on where things are going in the arch/fs split.
mbm_total_event is private to resctrl fs and arch code cannot reach into it.
There is the arch helper resctrl_arch_is_evt_configurable() but I also
think that this helper needs to be reconsidered in the light of ABMC.
Overall I think this ABMC support needs to consider what already exists
for BMEC support and ensure that both are supported coherently. For example,
when a monitor domain has a "MBM local bandwidth configuration" then it should
be obvious what that means.
Reinette
Powered by blists - more mailing lists