| 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: <ZdaNymyCFAXMn18k@agluck-desk3>
Date: Wed, 21 Feb 2024 15:56:58 -0800
From: Tony Luck <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
James Morse <james.morse@....com>,
"x86@...nel.org" <x86@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Yu, Fenghua" <fenghua.yu@...el.com>,
Ingo Molnar <mingo@...hat.com>, H Peter Anvin <hpa@...or.com>,
Babu Moger <Babu.Moger@....com>,
"shameerali.kolothum.thodi@...wei.com" <shameerali.kolothum.thodi@...wei.com>,
D Scott Phillips OS <scott@...amperecomputing.com>,
"carl@...amperecomputing.com" <carl@...amperecomputing.com>,
"lcherian@...vell.com" <lcherian@...vell.com>,
"bobo.shaobowang@...wei.com" <bobo.shaobowang@...wei.com>,
"tan.shaopeng@...itsu.com" <tan.shaopeng@...itsu.com>,
"baolin.wang@...ux.alibaba.com" <baolin.wang@...ux.alibaba.com>,
Jamie Iles <quic_jiles@...cinc.com>,
Xin Hao <xhao@...ux.alibaba.com>,
"peternewman@...gle.com" <peternewman@...gle.com>,
"dfustini@...libre.com" <dfustini@...libre.com>,
"amitsinght@...vell.com" <amitsinght@...vell.com>,
David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH v2] x86/resctrl: Fix WARN in get_domain_from_cpu()
On Wed, Feb 21, 2024 at 02:59:43PM -0800, Reinette Chatre wrote:
> Hi Tony,
>
> On 2/21/2024 11:31 AM, Tony Luck wrote:
> > reset_all_ctrls() and resctrl_arch_update_domains() use on_each_cpu_mask()
> > to call rdt_ctrl_update() on potentially one CPU from each domain.
> >
> > But this means rdt_ctrl_update() needs to figure out which domain to
> > apply changes to. Doing so requires a search of all domains in a resource,
> > which can only be done safely if cpus_lock is held. Both callers do hold
> > this lock, but there isn't a way for a function called on another CPU
> > via IPI to verify this.
> >
> > Fix by adding the target domain to the msr_param structure and passing an
> > array with CDP_NUM_TYPES entries. Then calling for each domain separately
> > using smp_call_function_single()
>
> This work contains no changes to get_domain_from_cpu(). I expect the WARN
> within it to be removed as intended with [1] and then this work can build
> on that without urgency. As I understand, to support the stated goal of this
> work, I expect get_domain_from_cpu() in the end to not have any WARN or
> IS_ENABLED checks, but just a lockdep_assert_cpus_held().
>
> Do you have different expectations?
Same expectations. Boris should apply the simple fix (delete the WARN
that is giving a false positive) for this current cycle.
If there is support for my patch (with changes/fixes you point out
below), then it could be added in the future and get_domain_from_cpu()
can use lockdep_assert_cpus_held().
> > Change the low level cat_wrmsr(), mba_wrmsr_intel(), and mba_wrmsr_amd()
> > functions to just take a msr_param structure since it contains the
> > rdt_resource and rdt_domain information.
>
> Could moving the rdt_domain into msr_param be done in a separate patch?
I can break it into more pieces if there is enthusiam to apply it.
> ...
>
> > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > index 7997b47743a2..09f6e624f1bb 100644
> > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > @@ -272,22 +272,6 @@ static u32 get_config_index(u32 closid, enum resctrl_conf_type type)
> > }
> > }
> >
> > -static bool apply_config(struct rdt_hw_domain *hw_dom,
> > - struct resctrl_staged_config *cfg, u32 idx,
> > - cpumask_var_t cpu_mask)
> > -{
> > - struct rdt_domain *dom = &hw_dom->d_resctrl;
> > -
> > - if (cfg->new_ctrl != hw_dom->ctrl_val[idx]) {
> > - cpumask_set_cpu(cpumask_any(&dom->cpu_mask), cpu_mask);
> > - hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> > -
> > - return true;
> > - }
> > -
> > - return false;
> > -}
> > -
> > int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_domain *d,
> > u32 closid, enum resctrl_conf_type t, u32 cfg_val)
> > {
> > @@ -304,59 +288,50 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_domain *d,
> > msr_param.res = r;
> > msr_param.low = idx;
> > msr_param.high = idx + 1;
> > - hw_res->msr_update(d, &msr_param, r);
> > + hw_res->msr_update(&msr_param);
> >
>
> Is this missing setting the domain in msr_param?
Indeed yes.
> > return 0;
> > }
> >
> > int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
> > {
> > + struct msr_param msr_param[CDP_NUM_TYPES];
> > struct resctrl_staged_config *cfg;
> > struct rdt_hw_domain *hw_dom;
> > - struct msr_param msr_param;
> > enum resctrl_conf_type t;
> > - cpumask_var_t cpu_mask;
> > struct rdt_domain *d;
> > + bool need_update;
> > + int cpu;
> > u32 idx;
> >
> > /* Walking r->domains, ensure it can't race with cpuhp */
> > lockdep_assert_cpus_held();
> >
> > - if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> > - return -ENOMEM;
> > -
> > - msr_param.res = NULL;
> > + memset(msr_param, 0, sizeof(msr_param));
> > list_for_each_entry(d, &r->domains, list) {
> > hw_dom = resctrl_to_arch_dom(d);
> > + need_update = false;
> > for (t = 0; t < CDP_NUM_TYPES; t++) {
> > cfg = &hw_dom->d_resctrl.staged_config[t];
> > if (!cfg->have_new_ctrl)
> > continue;
> >
> > idx = get_config_index(closid, t);
> > - if (!apply_config(hw_dom, cfg, idx, cpu_mask))
> > + if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
> > continue;
> > -
> > - if (!msr_param.res) {
> > - msr_param.low = idx;
> > - msr_param.high = msr_param.low + 1;
> > - msr_param.res = r;
> > - } else {
> > - msr_param.low = min(msr_param.low, idx);
> > - msr_param.high = max(msr_param.high, idx + 1);
> > - }
> > + hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> > + cpu = cpumask_any(&d->cpu_mask);
> > +
> > + msr_param[t].low = idx;
> > + msr_param[t].high = msr_param[t].low + 1;
> > + msr_param[t].res = r;
> > + msr_param[t].dom = d;
> > + need_update = true;
> > }
> > + if (need_update)
> > + smp_call_function_single(cpu, rdt_ctrl_update, &msr_param, 1);
>
> It is not clear to me why it is needed to pass this additional data. Why not
> just use the original mechanism of letting the low and high of msr_param span the
> multiple indices that need updating? There can still be a "need_update" but it
> can be set when msr_param gets its first data. Any other index needing updating
> can just update low/high and a single msr_param can be used.
For some reason this morning I thought that the domain needed to be
different. It isn't, so keeping the code that just adjusts the range
of MSRs will work just fine.
The "need_update" variable isn't required. I will move the
msr_param.res = NULL;
inside the for each domain loop, and can use non-NULL value to
decide whether to IPI a CPU.
-Tony
Powered by blists - more mailing lists