Re: [PATCH v2 14/29] arm_mpam: Merge supported features during mpam_enable() into mpam_class

James Morse
2025-09-29T17:45:20Z

Hi Jonathan,

On 12/09/2025 12:49, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:42:54 +0000
> James Morse <james.morse@arm.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.

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

Thanks!

>> +/*
>> + * 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.

Fixed.

>> + 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__);

These __func__ arguments need to go as pr_fmt has this covered.

>> + 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.

Just that its the innards of something and needs comprehending as part of its caller.

>> +{
>> + 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?

To isolate it from a subsequent change that makes that field a u32, the existence of which
means one day it'll be a u64. If it ever gets bigger than unsigned-long, it'll need to be
a bitmap array, which would need this code to change. Until then doing, it like this makes
changes to the size less churny.

>> +
>> + /* 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.

I expect to have to change this in the future.

As these two debug messages have a dev to hand, they should probably use dev_debug()
instead of manually printing the dev_name().

>> +
>> + /*
>> + * 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?

This is just paranoia. I've removed it.

>> + */
>> +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.'

The bitwise-or. It's singular because its done for each class/component at a time.

The reason is so that mpam_enable_init_class_features() see's all the features in the
first vmsc, not a subset of them. It's because vmsc hold non-overlapping features, which
is all to support a platform that has two RIS with different types of control that do the
same thing. (made up example:) memory min/max on ingress and memory portions on egress.
Both are memory bandwidth for the same hardware block, but for whatever structural reason,
it gets exposed as separate RIS.

Rephrased as:
| * vmsc features are bitwise-or'd together as the first step so that
| * mpam_enable_init_class_features() can initialise the class with a
| * representive set of features.

>> + * 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.

Sure. The comment is to try and stop people 'fixing' it to only loop over
class->components once. That'll work on 99% of platforms, but discard all the
controls on a few strange ones.

>> + */
>> +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);
>> + }
>> +}
>
>

Thanks,

James