Re: [PATCH v2 40/45] arm_mpam: Generate a configuration for min controls

Ben Horgan
2026-01-08T15:35:11Z

Hi Jonathan,

On 1/6/26 15:09, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 18:11:42 +0000
> Ben Horgan <ben.horgan@arm.com> wrote:
>
>> From: James Morse <james.morse@arm.com>
>>
>> MPAM supports a minimum and maximum control for memory bandwidth. The
>> purpose of the minimum control is to give priority to tasks that are below
>> their minimum value. Resctrl only provides one value for the bandwidth
>> configuration, which is used for the maximum.
>>
>> The minimum control is always programmed to zero on hardware that supports
>> it.
>>
>> Generate a minimum bandwidth value that is 5% lower than the value provided
>> by resctrl. This means tasks that are not receiving their target bandwidth
>> can be prioritised by the hardware.
>
> I'm not sure this policy make sense in general as there will be bandwidth
> heavy background tasks where we want to make sure they only use up to X%
> of bandwidth, but don't care if they don't get that. I guess maybe those
> are always in the %5 or less category so maybe this policy is fine.
> Ah well, can revisit later I guess as long as we keep this as the default
> for any new controls.

Yeah, I don't think the default can make everyone happy.

>
>>
>> For component reset reuse the same calculation so that the default is a
>> value resctrl can set.
>>
>> CC: Zeng Heng <zengheng4@huawei.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>
> Most, maybe all the other cases have split test from implementation. I'd
> probably do that here just for consistency.
>
> A few things inline.
>
> J
>
>
>
>> ---
>> Add reset_mbw_min
>> Clear min cfg when setting max
>> use mpam_extend_config on component reset
>> ---
>> drivers/resctrl/mpam_devices.c | 76 +++++++++++++++++++++++++++--
>> drivers/resctrl/mpam_internal.h | 3 ++
>> drivers/resctrl/mpam_resctrl.c | 2 +
>> drivers/resctrl/test_mpam_devices.c | 66 +++++++++++++++++++++++++
>> 4 files changed, 142 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index c20b2757b86b..deea46c2a6c3 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -721,6 +721,13 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
>> mpam_set_feature(mpam_feat_mbw_part, props);
>>
>> props->bwa_wd = FIELD_GET(MPAMF_MBW_IDR_BWA_WD, mbw_features);
>> +
>> + /*
>> + * The BWA_WD field can represent 0-63, but the control fields it
>> + * describes have a maximum of 16 bits.
>
> I'm lost to why this only matters now! Feels like that makes sense way earlier
> in the series. Particularly as that detail confused me no end in the bandwidth
> / percent calculation patches.

Fair point, I've moved this earlier.

>
>> + */
>> + props->bwa_wd = min(props->bwa_wd, 16);
>> +
>> if (props->bwa_wd && FIELD_GET(MPAMF_MBW_IDR_HAS_MAX, mbw_features))
>> mpam_set_feature(mpam_feat_mbw_max, props);
>>
>
>> +static void mpam_extend_config(struct mpam_class *class, struct mpam_config *cfg)
>> +{
>> + struct mpam_props *cprops = &class->props;
>> + u16 min, min_hw_granule, delta;
>> + u16 max_hw_value, res0_bits;
>
> Probably makes sense to drag some of these into the scope below.

I've squashed in a change from a later quirk which moves things out of
the 'if' scope and so only min and delta get moved inside.

>
>> +
>> + /*
>> + * MAX and MIN should be set together. If only one is provided,
>> + * generate a configuration for the other. If only one control
>> + * type is supported, the other value will be ignored.
>> + *
>> + * Resctrl can only configure the MAX.
>> + */
>> + if (mpam_has_feature(mpam_feat_mbw_max, cfg) &&
>> + !mpam_has_feature(mpam_feat_mbw_min, cfg)) {
>> + /*
>> + * Calculate the values the 'min' control can hold.
>> + * e.g. on a platform with bwa_wd = 8, min_hw_granule is 0x00ff
>> + * because those bits are RES0. Configurations of this value
>> + * are effectively zero. But configurations need to saturate
>> + * at min_hw_granule on systems with mismatched bwa_wd, where
>> + * the 'less than 0' values are implemented on some MSC, but
>> + * not others.
>> + */
>> + res0_bits = 16 - cprops->bwa_wd;
>> + max_hw_value = ((1 << cprops->bwa_wd) - 1) << res0_bits;
>> + min_hw_granule = ~max_hw_value;
>> +
>> + delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
>> + if (cfg->mbw_max > delta)
>> + min = cfg->mbw_max - delta;
>> + else
>> + min = 0;
>> +
>> + cfg->mbw_min = max(min, min_hw_granule);
>> + mpam_set_feature(mpam_feat_mbw_min, cfg);
>> + }
>> +}
>> +
>>

Thanks,

Ben