Re: [PATCH v2 21/29] arm_mpam: Probe and reset the rest of the features

James Morse
2025-10-03T18:05:09Z

Hi Jonathan,

On 12/09/2025 14:07, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 20:43:01 +0000
> James Morse <james.morse@arm.com> wrote:
>
>> MPAM supports more features than are going to be exposed to resctrl.
>> For partid other than 0, the reset values of these controls isn't
>> known.
>>
>> Discover the rest of the features so they can be reset to avoid any
>> side effects when resctrl is in use.
>>
>> PARTID narrowing allows MSC/RIS to support less configuration space than
>> is usable. If this feature is found on a class of device we are likely
>> to use, then reduce the partid_max to make it usable. This allows us
>> to map a PARTID to itself.

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

Thanks!

>> @@ -667,10 +676,35 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>> struct mpam_msc *msc = ris->vmsc->msc;
>> struct device *dev = &msc->pdev->dev;
>> struct mpam_props *props = &ris->props;
>> + struct mpam_class *class = ris->vmsc->comp->class;
>>
>> lockdep_assert_held(&msc->probe_lock);
>> lockdep_assert_held(&msc->part_sel_lock);
>>
>> + /* Cache Capacity Partitioning */
>> + if (FIELD_GET(MPAMF_IDR_HAS_CCAP_PART, ris->idr)) {
>> + u32 ccap_features = mpam_read_partsel_reg(msc, CCAP_IDR);
>> +
>> + props->cmax_wd = FIELD_GET(MPAMF_CCAP_IDR_CMAX_WD, ccap_features);
>> + if (props->cmax_wd &&
>> + FIELD_GET(MPAMF_CCAP_IDR_HAS_CMAX_SOFTLIM, ccap_features))
>> + mpam_set_feature(mpam_feat_cmax_softlim, props);
>> +
>> + if (props->cmax_wd &&
>> + !FIELD_GET(MPAMF_CCAP_IDR_NO_CMAX, ccap_features))
>> + mpam_set_feature(mpam_feat_cmax_cmax, props);
>> +
>> + if (props->cmax_wd &&
>> + FIELD_GET(MPAMF_CCAP_IDR_HAS_CMIN, ccap_features))
>> + mpam_set_feature(mpam_feat_cmax_cmin, props);
>> +
>> + props->cassoc_wd = FIELD_GET(MPAMF_CCAP_IDR_CASSOC_WD, ccap_features);
>> +

> Trivial but blank line here feels inconsistent with local style. I'd drop it.

Sure,

>> + if (props->cassoc_wd &&
>> + FIELD_GET(MPAMF_CCAP_IDR_HAS_CASSOC, ccap_features))
>> + mpam_set_feature(mpam_feat_cmax_cassoc, props);
>> + }
>> +
>
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
>> index 17570d9aae9b..326ba9114d70 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -136,25 +136,34 @@ static inline void mpam_mon_sel_lock_init(struct mpam_msc *msc)
>> * When we compact the supported features, we don't care what they are.
>> * Storing them as a bitmap makes life easy.
>> */
>> -typedef u16 mpam_features_t;
>> +typedef u32 mpam_features_t;

> This is strengthening my view that this should just be a DECLARE_BITMAP(MPAM_FEATURE_LAST)
> in the appropriate places.

I don't think this list is going to grow much. But sure.

Most stuff can be churned out, but this makes an utter mess of exposing the value to
debugfs.., I do need that exposed as "why didn't resctrl do what I wanted" is a very
common question - and being able to see what the hardware supports really helps.

Looks like exposing the first 'ulong' is the simplest - it kicks the can this creates
down the road. I experimented with u32_array - but its no better.

Thanks,

James