linux-kernel - Re: [PATCH 23/33] arm_mpam: Allow configuration to be applied and restored during cpu online

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251110172724.00005675@huawei.com>
Date: Mon, 10 Nov 2025 17:27:24 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Ben Horgan <ben.horgan@....com>
CC: <james.morse@....com>, <amitsinght@...vell.com>,
	<baisheng.gao@...soc.com>, <baolin.wang@...ux.alibaba.com>,
	<bobo.shaobowang@...wei.com>, <carl@...amperecomputing.com>,
	<catalin.marinas@....com>, <dakr@...nel.org>, <dave.martin@....com>,
	<david@...hat.com>, <dfustini@...libre.com>, <fenghuay@...dia.com>,
	<gregkh@...uxfoundation.org>, <gshan@...hat.com>, <guohanjun@...wei.com>,
	<jeremy.linton@....com>, <kobak@...dia.com>, <lcherian@...vell.com>,
	<lenb@...nel.org>, <linux-acpi@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<lpieralisi@...nel.org>, <peternewman@...gle.com>, <quic_jiles@...cinc.com>,
	<rafael@...nel.org>, <robh@...nel.org>, <rohit.mathew@....com>,
	<scott@...amperecomputing.com>, <sdonthineni@...dia.com>,
	<sudeep.holla@....com>, <tan.shaopeng@...itsu.com>, <will@...nel.org>,
	<xhao@...ux.alibaba.com>
Subject: Re: [PATCH 23/33] arm_mpam: Allow configuration to be applied and
 restored during cpu online

On Fri, 7 Nov 2025 12:34:40 +0000
Ben Horgan <ben.horgan@....com> wrote:

> From: James Morse <james.morse@....com>
> 
> When CPUs come online the MSC's original configuration should be restored.
> 
> Add struct mpam_config to hold the configuration. This has a bitmap of
> features that were modified. Once the maximum partid is known, allocate

I'm not following 'were modified'.  When?  Sometime in the past?
Perhaps "features that have been modified when XXX happens" or

Having read the code I think this is something like "are modified as configuration
is read".

> a configuration array for each component, and reprogram each RIS
> configuration from this.
> 
> CC: Dave Martin <Dave.Martin@....com>
> Signed-off-by: James Morse <james.morse@....com>
> Cc: Shaopeng Tan (Fujitsu) tan.shaopeng@...itsu.com
> Cc: Peter Newman peternewman@...gle.com
> Signed-off-by: Ben Horgan <ben.horgan@....com>
> ---
> Changes since v3:
> Drop tags
> Fix component reset, otherwise cpbm wrong and controls not set.
> Add a cfg_lock to guard configuration of an msc

The use of bitmap_set() for things that aren't unsigned long (arrays) is a bad
idea. Much better to use GENMASK() to fill those.

> ---
>  drivers/resctrl/mpam_devices.c  | 268 ++++++++++++++++++++++++++++++--
>  drivers/resctrl/mpam_internal.h |  27 ++++
>  2 files changed, 280 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 3a0ad8d93fff..8b0944bdaf28 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c



> @@ -1125,6 +1225,9 @@ static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev)
>  	if (err)
>  		return ERR_PTR(err);
>  	err = devm_mutex_init(dev, &msc->error_irq_lock);
> +	if (err)
> +		return ERR_PTR(err);
Trivial: As in earlier patches. I'd put a blank line here for readability.
> +	err = devm_mutex_init(dev, &msc->cfg_lock);
>  	if (err)
>  		return ERR_PTR(err);
>  	mpam_mon_sel_lock_init(msc);
> @@ -1585,6 +1688,70 @@ static void mpam_unregister_irqs(void)
>  	}
>  }
>  
> +static void __destroy_component_cfg(struct mpam_component *comp)
> +{
> +	add_to_garbage(comp->cfg);
> +}
> +
> +static void mpam_reset_component_cfg(struct mpam_component *comp)
> +{
> +	int i;
> +	struct mpam_props *cprops = &comp->class->props;
> +
> +	mpam_assert_partid_sizes_fixed();
> +
> +	if (!comp->cfg)
> +		return;
> +
> +	for (i = 0; i <= mpam_partid_max; i++) {
> +		comp->cfg[i] = (struct mpam_config) {};
> +		bitmap_fill(comp->cfg[i].features, MPAM_FEATURE_LAST);
> +		bitmap_set((unsigned long *)&comp->cfg[i].cpbm, 0, cprops->cpbm_wd);

Why manipulate a u32 with bitmap_set() with a horrible pretend it's an unsigned long cast.
Instead just do:
		comp->cfg[i].cpbm = GENMASK(cprops->cpbm_wd, 0);
Which is indeed what bitmap_set will do internally due to an optimization for small bitmaps
but lets avoid that making one integer pretend to be another of a different length.


> +		bitmap_set((unsigned long *)&comp->cfg[i].mbw_pbm, 0, cprops->mbw_pbm_bits);
> +		bitmap_set((unsigned long *)&comp->cfg[i].mbw_max, 16 - cprops->bwa_wd, cprops->bwa_wd);
> +	}
> +}