linux-kernel - Re: [PATCH v2 10/29] arm_mpam: Add cpuhp callbacks to probe MSC hardware

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: <31c1cea8-8265-4d86-b9a7-d8f8955405bf@arm.com>
Date: Mon, 29 Sep 2025 18:44:25 +0100
From: James Morse <james.morse@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com, Rob Herring <robh@...nel.org>,
 Rohit Mathew <rohit.mathew@....com>, Rafael Wysocki <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Hanjun Guo <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>, Lecopzer Chen <lecopzerc@...dia.com>
Subject: Re: [PATCH v2 10/29] arm_mpam: Add cpuhp callbacks to probe MSC
 hardware

Hi Jonathan,

On 11/09/2025 16:07, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:42:50 +0000
> James Morse <james.morse@....com> wrote:
> 
>> Because an MSC can only by accessed from the CPUs in its cpu-affinity
>> set we need to be running on one of those CPUs to probe the MSC
>> hardware.
>>
>> Do this work in the cpuhp callback. Probing the hardware will only
>> happen before MPAM is enabled, walk all the MSCs and probe those we can
>> reach that haven't already been probed as each CPU's online call is made.
>>
>> This adds the low-level MSC register accessors.
>>
>> Once all MSCs reported by the firmware have been probed from a CPU in
>> their respective cpu-affinity set, the probe-time cpuhp callbacks are
>> replaced.  The replacement callbacks will ultimately need to handle
>> save/restore of the runtime MSC state across power transitions, but for
>> now there is nothing to do in them: so do nothing.
>>
>> The architecture's context switch code will be enabled by a static-key,
>> this can be set by mpam_enable(), but must be done from process context,
>> not a cpuhp callback because both take the cpuhp lock.
>> Whenever a new MSC has been probed, the mpam_enable() work is scheduled
>> to test if all the MSCs have been probed. If probing fails, mpam_disable()
>> is scheduled to unregister the cpuhp callbacks and free memory.

> Trivial suggestion inline. Either way
> Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>

Thanks!


>> +/* Before mpam is enabled, try to probe new MSC */
>> +static int mpam_discovery_cpu_online(unsigned int cpu)
>> +{
>> +	int err = 0;
>> +	struct mpam_msc *msc;
>> +	bool new_device_probed = false;
>> +
>> +	guard(srcu)(&mpam_srcu);
>> +	list_for_each_entry_srcu(msc, &mpam_all_msc, all_msc_list,
>> +				 srcu_read_lock_held(&mpam_srcu)) {
>> +		if (!cpumask_test_cpu(cpu, &msc->accessibility))
>> +			continue;
>> +
>> +		mutex_lock(&msc->probe_lock);
>> +		if (!msc->probed)
>> +			err = mpam_msc_hw_probe(msc);
>> +		mutex_unlock(&msc->probe_lock);
>> +
>> +		if (!err)
>> +			new_device_probed = true;
>> +		else
>> +			break;

> Unless this going to get more complex why not
> 
> 		if (err)
> 			break;
> 
> 		new_device_probed = true;

Sure - its been both simpler and more complex in the past!


>> +	}
>> +
>> +	if (new_device_probed && !err)
>> +		schedule_work(&mpam_enable_work);
>> +	if (err) {
>> +		mpam_disable_reason = "error during probing";
>> +		schedule_work(&mpam_broken_work);
>> +	}
>> +
>> +	return err;
>> +}
> 
>> +static void mpam_enable_once(void)
>> +{
>> +	mpam_register_cpuhp_callbacks(mpam_cpu_online, mpam_cpu_offline);
>> +
>> +	pr_info("MPAM enabled\n");

> Feels too noisy given it should be easy enough to tell. pr_dbg() perhaps.

I was aiming for the driver to only print one thing - once all the hardware has been
probed. Once the driver is assembled, this prints the number of PARTID/PMG that were
discovered as the system wide limits.

The reason to print something is that if you see this message, but don't have resctrl
appear in /proc/filesystems - its never going to appear because the resctrl glue code
couldn't find anything it could use. As this isn't an error, so nothing gets printed in
this case.
This is the most common complaint I get - "our platform doesn't look like a Xeon - why
doesn't resctrl work with it?"

It also matters for other requesters, like the SMMU. If they probe after this point, they
can't reduce the PARTID/PMG range - and may get an error and have their MPAM abilities
disabled. Having an entry in the boot log makes this easier to debug.


The alternative would be to keep track of what the driver is up to, and expose that
through debugfs - but information that only exists for debug purposes is likely to be
wrong. It also doesn't help work out what order different drivers tried to probe in.


Thanks,

James