Message-ID: <20250912124937.00004250@huawei.com>
Date: Fri, 12 Sep 2025 12:49:37 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: James Morse <james.morse@....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>, Ben Horgan <ben.horgan@....com>
Subject: Re: [PATCH v2 14/29] arm_mpam: Merge supported features during
 mpam_enable() into mpam_class

On Wed, 10 Sep 2025 20:42:54 +0000
James Morse <james.morse@....com> wrote:

> To make a decision about whether to expose an mpam class as
> a resctrl resource we need to know its overall supported
> features and properties.
> 
> Once we've probed all the resources, we can walk the tree
> and produce overall values by merging the bitmaps. This
> eliminates features that are only supported by some MSC
> that make up a component or class.
> 
> If bitmap properties are mismatched within a component we
> cannot support the mismatched feature.
> 
> Care has to be taken as vMSC may hold mismatched RIS.
> 
> Signed-off-by: James Morse <james.morse@....com>
> Reviewed-by: Ben Horgan <ben.horgan@....com>

A trivial things inline.
Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>

> +/*
> + * Combine two props fields.
> + * If this is for controls that alias the same resource, it is safe to just
> + * copy the values over. If two aliasing controls implement the same scheme
> + * a safe value must be picked.
> + * For non-aliasing controls, these control different resources, and the
> + * resulting safe value must be compatible with both. When merging values in
> + * the tree, all the aliasing resources must be handled first.
> + * On mismatch, parent is modified.
> + */
> +static void __props_mismatch(struct mpam_props *parent,
> +			     struct mpam_props *child, bool alias)
> +{
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_cpor_part, alias)) {
> +		parent->cpbm_wd = child->cpbm_wd;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_cpor_part,
> +				   cpbm_wd, alias)) {
> +		pr_debug("%s cleared cpor_part\n", __func__);
> +		mpam_clear_feature(mpam_feat_cpor_part, &parent->features);
> +		parent->cpbm_wd = 0;
> +	}
> +
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_mbw_part, alias)) {
> +		parent->mbw_pbm_bits = child->mbw_pbm_bits;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_mbw_part,
> +				   mbw_pbm_bits, alias)) {
> +		pr_debug("%s cleared mbw_part\n", __func__);
> +		mpam_clear_feature(mpam_feat_mbw_part, &parent->features);
> +		parent->mbw_pbm_bits = 0;
> +	}
> +
> +	/* bwa_wd is a count of bits, fewer bits means less precision */
> +	if (alias && !mpam_has_bwa_wd_feature(parent) && mpam_has_bwa_wd_feature(child)) {

Seems like an overly long line given other local wrapping.

> +		parent->bwa_wd = child->bwa_wd;
> +	} else if (MISMATCHED_HELPER(parent, child, mpam_has_bwa_wd_feature,
> +				     bwa_wd, alias)) {
> +		pr_debug("%s took the min bwa_wd\n", __func__);
> +		parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd);
> +	}
> +
> +	/* For num properties, take the minimum */
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) {
> +		parent->num_csu_mon = child->num_csu_mon;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_csu,
> +				   num_csu_mon, alias)) {
> +		pr_debug("%s took the min num_csu_mon\n", __func__);
> +		parent->num_csu_mon = min(parent->num_csu_mon, child->num_csu_mon);
> +	}
> +
> +	if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_mbwu, alias)) {
> +		parent->num_mbwu_mon = child->num_mbwu_mon;
> +	} else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_mbwu,
> +				   num_mbwu_mon, alias)) {
> +		pr_debug("%s took the min num_mbwu_mon\n", __func__);
> +		parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon);
> +	}

> +
> +/*
> + * If a vmsc doesn't match class feature/configuration, do the right thing(tm).
> + * For 'num' properties we can just take the minimum.
> + * For properties where the mismatched unused bits would make a difference, we
> + * nobble the class feature, as we can't configure all the resources.
> + * e.g. The L3 cache is composed of two resources with 13 and 17 portion
> + * bitmaps respectively.
> + */
> +static void
> +__class_props_mismatch(struct mpam_class *class, struct mpam_vmsc *vmsc)

I'm not really sure what the __ prefix denotes here.

> +{
> +	struct mpam_props *cprops = &class->props;
> +	struct mpam_props *vprops = &vmsc->props;
> +
> +	lockdep_assert_held(&mpam_list_lock); /* we modify class */
> +
> +	pr_debug("%s: Merging features for class:0x%lx &= vmsc:0x%lx\n",
> +		 dev_name(&vmsc->msc->pdev->dev),
> +		 (long)cprops->features, (long)vprops->features);

According to https://docs.kernel.org/core-api/printk-formats.html
should be fine using %x for u16 values. So why dance through a cast to long?
> +
> +	/* Take the safe value for any common features */
> +	__props_mismatch(cprops, vprops, false);
> +}
> +
> +static void
> +__vmsc_props_mismatch(struct mpam_vmsc *vmsc, struct mpam_msc_ris *ris)
> +{
> +	struct mpam_props *rprops = &ris->props;
> +	struct mpam_props *vprops = &vmsc->props;
> +
> +	lockdep_assert_held(&mpam_list_lock); /* we modify vmsc */
> +
> +	pr_debug("%s: Merging features for vmsc:0x%lx |= ris:0x%lx\n",
> +		 dev_name(&vmsc->msc->pdev->dev),
> +		 (long)vprops->features, (long)rprops->features);

Same as above comment on casts being unnecessary.

> +
> +	/*
> +	 * Merge mismatched features - Copy any features that aren't common,
> +	 * but take the safe value for any common features.
> +	 */
> +	__props_mismatch(vprops, rprops, true);
> +}
> +
> +/*
> + * Copy the first component's first vMSC's properties and features to the
> + * class. __class_props_mismatch() will remove conflicts.
> + * It is not possible to have a class with no components, or a component with
> + * no resources. The vMSC properties have already been built.

If it's not possible do we need the defensive _or_null and error checks?

> + */
> +static void mpam_enable_init_class_features(struct mpam_class *class)
> +{
> +	struct mpam_vmsc *vmsc;
> +	struct mpam_component *comp;
> +
> +	comp = list_first_entry_or_null(&class->components,
> +					struct mpam_component, class_list);
> +	if (WARN_ON(!comp))
> +		return;
> +
> +	vmsc = list_first_entry_or_null(&comp->vmsc,
> +					struct mpam_vmsc, comp_list);
> +	if (WARN_ON(!vmsc))
> +		return;
> +
> +	class->props = vmsc->props;
> +}

> +/*
> + * Merge all the common resource features into class.
> + * vmsc features are bitwise-or'd together, this must be done first.

I'm not sure what 'this' is here - I think it's a missing plural that has
me confused.  Perhaps 'these must be done first.'

> + * Next the class features are the bitwise-and of all the vmsc features.
> + * Other features are the min/max as appropriate.
> + *
> + * To avoid walking the whole tree twice, the class->nrdy_usec property is
> + * updated when working with the vmsc as it is a max(), and doesn't need
> + * initialising first.

Perhaps state that this comment is about what happens in each call of
mpam_enable_merge_vmsc_features()  Or move the comment to that function.

> + */
> +static void mpam_enable_merge_features(struct list_head *all_classes_list)
> +{
> +	struct mpam_class *class;
> +	struct mpam_component *comp;
> +
> +	lockdep_assert_held(&mpam_list_lock);
> +
> +	list_for_each_entry(class, all_classes_list, classes_list) {
> +		list_for_each_entry(comp, &class->components, class_list)
> +			mpam_enable_merge_vmsc_features(comp);
> +
> +		mpam_enable_init_class_features(class);
> +
> +		list_for_each_entry(comp, &class->components, class_list)
> +			mpam_enable_merge_class_features(comp);
> +	}
> +}

Powered by blists - more mailing lists