Re: [PATCH] arm_mpam: Fix MPAMCFG_MBW_PBM register setting

[Gavin Shan]

Hi Fenghua,

On 6/7/26 3:09 PM, Fenghua Yu wrote:
> MPAMCFG_MBW_PBM is written from cfg if cfg has the MBW partition feature.
> It is reset when cfg does not have the MBW partition feature.
>
> But the register handling is reversed. This may cause an incorrect
> register setting. For example, during an MPAM reset, reset_cfg is
> empty (no MBW partition feature set), and cfg->mbw_pbm is 0. Instead of
> resetting MPAMCFG_MBW_PBM to all 1's, the current logic will set it to
> cfg->mbw_pbm, which is 0.
>
> Fix the issue by swapping the if/else branches.
>
> Fixes: a1cb6577f575 ("arm_mpam: Reset when feature configuration bit unset")
> Reported-by: Matt Ochs <mochs@xxxxxxxxxx>
> Signed-off-by: Fenghua Yu <fenghuay@xxxxxxxxxx>
> ---
>   drivers/resctrl/mpam_devices.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

The fix itself looks reasonable to me, but two questions below.

Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>

> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index 4b93e89c2678..d8b0383cee92 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -1570,9 +1570,9 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
>   
>   	if (mpam_has_feature(mpam_feat_mbw_part, rprops)) {
>   		if (mpam_has_feature(mpam_feat_mbw_part, cfg))
> -			mpam_reset_msc_bitmap(msc, MPAMCFG_MBW_PBM, rprops->mbw_pbm_bits);
> -		else
>   			mpam_write_partsel_reg(msc, MBW_PBM, cfg->mbw_pbm);
> +		else
> +			mpam_reset_msc_bitmap(msc, MPAMCFG_MBW_PBM, rprops->mbw_pbm_bits);
>   	}
>   
>   	if (mpam_has_feature(mpam_feat_mbw_min, rprops)) {

Which machine or system where mpam_feat_mbw_part is set on RIS? As I can
remember, this feature isn't available on grace-hopper.

Besides, I don't think this feature is well handled at present because
mpam_config::mbw_pbm is only 32-bits in length, which doesn't match with
the maximal length of the bit map (4096) as documented in the spec.

Thanks,
Gavin