Re: [PATCH] arm_mpam: Set feature bits on default partition config

[Ben Horgan]

Hi Fenghua,

On 3/1/26 17:18, Fenghua Yu wrote:
> Currently we only set the default values (cpbm, mbw_pbm, mbw_max)
> and do not set the corresponding feature bits. As a result,
> the MBW_MIN/MBW_MAX and CPBM/MBW_PBM blocks in
> mpam_reprogram_ris_partid() does not apply the default config

The current behaviour is the intended behaviour. The values here
are there so that the expected values appear in the resctrl interface.
When an actual reset is done we want to use the hardware default
values and not limit ourselves to what is currently supported by
resctrl. On well-behaved hardware for controls supporting resctrl
these will be equivalent. cpor isn't supported if there are more than 32
bits.


> because they require both rprops and cfg to have the feature.
> The missing feature bits may cause other issues in the future.
> 
> When initializing default per-partition config in
> mpam_reset_component_cfg(), set the feature bits on cfg for
> each resource we program (cpor_part, mbw_part, mbw_max)
> so that "configured" matches the default values.
> 
> Set mpam_feat_cpor_part on cfg whenever we set default cpbm
> (cpbm_wd implies cpor_part).
> 
> Set mpam_feat_mbw_part and mpam_feat_mbw_max on cfg only when the
> class has the feature (mbw_pbm_bits/bwa_wd can be set without
> HAS_PBM/HAS_MAX).
> 
> Fixes: 09b89d2a72f3 ("arm_mpam: Allow configuration to be applied and restored during cpu online")
> Signed-off-by: Fenghua Yu <fenghuay@xxxxxxxxxx>
> ---
> Without the fix, mpam_wa_t241_calc_min_from_max() in the patch:
> https://lore.kernel.org/lkml/20260224175720.2663924-39-ben.horgan@xxxxxxx/
> sees mpam_has_feature(mpam_feat_mbw_max, cfg) as false and always returned 0
> on hardware that supports mbw_max. This causes performance degration
> because of too small mbw_min on T241 platform.

Isn't it only because of the erratum that this makes a difference. When
mbw_max is at it's maximum then when mbw_min=0 or mbw_max at a maximum.
Either the current bandwidth is always higher than mbw_min or it's
always lower than mbw_min. In both cases everything is classified with
the same priority.

This has caused me to look again into the reset code and I've noticed a
different issue. It's not recorded in the component configuration that
it needs to reset. Hence, on unmount if not all cpus are online
mpam_reset_component_locked() might not reset all msc and rely on cpuhp
to finish the reset but it doesn't have the information to do that. For
this reason, I think we should go back to James' original model of
resetting any feature that isn't set in the feature bitmap. (I shouldn't
have suggested we change that!)

> 
>  drivers/resctrl/mpam_devices.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 9182c8fcf003..be4b554104e3 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -2618,12 +2618,20 @@ static void mpam_reset_component_cfg(struct mpam_component *comp)
>  
>  	for (i = 0; i <= mpam_partid_max; i++) {
>  		comp->cfg[i] = (struct mpam_config) {};
> -		if (cprops->cpbm_wd)
> +		if (cprops->cpbm_wd) {
>  			comp->cfg[i].cpbm = GENMASK(cprops->cpbm_wd - 1, 0);
> -		if (cprops->mbw_pbm_bits)
> +			mpam_set_feature(mpam_feat_cpor_part, &comp->cfg[i]);
> +		}
> +		if (cprops->mbw_pbm_bits) {
>  			comp->cfg[i].mbw_pbm = GENMASK(cprops->mbw_pbm_bits - 1, 0);
> -		if (cprops->bwa_wd)
> +			if (mpam_has_feature(mpam_feat_mbw_part, cprops))
> +				mpam_set_feature(mpam_feat_mbw_part, &comp->cfg[i]);
> +		}
> +		if (cprops->bwa_wd) {
>  			comp->cfg[i].mbw_max = GENMASK(15, 16 - cprops->bwa_wd);
> +			if (mpam_has_feature(mpam_feat_mbw_max, cprops))
> +				mpam_set_feature(mpam_feat_mbw_max, &comp->cfg[i]);
> +		}
>  	}
>  }
>  

Thanks,

Ben