Re: [PATCH] arm_mpam: Fix MPAMCFG_MBW_PBM register setting

[Ben Horgan]

Hi Fenghua,

Thanks for the fix!

On 6/9/26 03:06, Fenghua Yu wrote:
> Hi, Gavin,
> 
> On 6/7/26 21:46, Gavin Shan wrote:
>> 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>
> 
> Thank you for reviewing the patch!
> 
>>
>>> 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.
> 
> Neither Grace nor Vera supports this feature.
> 
>>
>> 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.
> > Right. The current code only can support up to 32 bits of PBM bitmap. If PBM
> bitmp length is bigger than 32 bits, it's broken. I guess we will need to handle
> this feature properly when hardware supports more than 32 bits of PBM bitmap.

mpam_config::mbw_pbm shouldn't have made it into the driver as nothing ever sets
it. Otherwise, there is no attempt to make use of the MBW_PBM other than to
reset it to an appropriate value, which this patch fixes. I guess this is a
lesson for me on the perils of dead code.

> 
> Ditto for CPBM bitmap.

If the CPBM bitmap has more than 32 bits, as checked by
cache_has_usable_cpor(), then we don't attempt to configure it other than to
reset. If there is hardware with more than 32 bits we'd first consider extending
this to 64bits.

> 
> This patch currently only fixes the regression issue introduced in commit
> a1cb6577f575 regardless size of PBM bitmap.

Looks good to me.

Reviewed-by: Ben Horgan <ben.horgan@xxxxxxx>

Thanks,

Ben

> 
> Thanks.
> 
> -Fenghua