From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id C6C83220F2C; Mon, 29 Sep 2025 17:44:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1759167875; cv=none; b=qwUKW5NWBa/Co1s2T9hl4FVsXN3bABCkryDsukcUX2JRXvQ2SkrgwVESawB9chdTqc/h81ZQpMovtocpB8/KAa0Zt8flFf1eyL4wKh3Okkrqwu+H1xdLTaRBbVHfSPubahkJvYn11zToiJx1weYJC4KmoolUvK3zs5FHCcvr7wQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1759167875; c=relaxed/simple; bh=bd7QHrtsx8kCGIVX1wp5kMD747210eQWt5uNgI8KWeA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=pAFHyjEXF1USqrkPIyfOiC9nqFkjWtoUowwqlI3Y+gwRdETlIXxgIHlAz8VqczBo3+eFtU6Xdcxu40lb5Vc8s0KWQCVCFGLkM4dhpuOfTEg8nhblgV41uxQ3l1mTd7ZSdfT8ELrrou0fdnbMYczqAio8WxFNGUMzN+4Mnb+fYZI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0C286150C; Mon, 29 Sep 2025 10:44:23 -0700 (PDT) Received: from [10.1.197.69] (eglon.cambridge.arm.com [10.1.197.69]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F2F473F59E; Mon, 29 Sep 2025 10:44:25 -0700 (PDT) Message-ID: <31c1cea8-8265-4d86-b9a7-d8f8955405bf@arm.com> Date: Mon, 29 Sep 2025 18:44:25 +0100 Precedence: bulk X-Mailing-List: linux-acpi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 10/29] arm_mpam: Add cpuhp callbacks to probe MSC hardware To: Jonathan Cameron Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, D Scott Phillips OS , carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, baolin.wang@linux.alibaba.com, Jamie Iles , Xin Hao , peternewman@google.com, dfustini@baylibre.com, amitsinght@marvell.com, David Hildenbrand , Dave Martin , Koba Ko , Shanker Donthineni , fenghuay@nvidia.com, baisheng.gao@unisoc.com, Rob Herring , Rohit Mathew , Rafael Wysocki , Len Brown , Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , Catalin Marinas , Will Deacon , Greg Kroah-Hartman , Danilo Krummrich , Lecopzer Chen References: <20250910204309.20751-1-james.morse@arm.com> <20250910204309.20751-11-james.morse@arm.com> <20250911160737.0000492f@huawei.com> Content-Language: en-GB From: James Morse In-Reply-To: <20250911160737.0000492f@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Jonathan, On 11/09/2025 16:07, Jonathan Cameron wrote: > On Wed, 10 Sep 2025 20:42:50 +0000 > James Morse 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 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