Re: [PATCH 23/33] arm_mpam: Allow configuration to be applied and restored during cpu online

Ben Horgan
2025-11-11T17:45:42Z

Hi Jonathan,

On 11/10/25 17:27, Jonathan Cameron wrote:
> On Fri, 7 Nov 2025 12:34:40 +0000
> Ben Horgan <ben.horgan@arm.com> wrote:
>
>> From: James Morse <james.morse@arm.com>
>>
>> When CPUs come online the MSC's original configuration should be restored.
>>
>> Add struct mpam_config to hold the configuration. This has a bitmap of
>> features that were modified. Once the maximum partid is known, allocate
>
> I'm not following 'were modified'. When? Sometime in the past?
> Perhaps "features that have been modified when XXX happens" or
>
> Having read the code I think this is something like "are modified as configuration
> is read".
>
>> a configuration array for each component, and reprogram each RIS
>> configuration from this.
>>
>> CC: Dave Martin <Dave.Martin@arm.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Cc: Shaopeng Tan (Fujitsu) tan.shaopeng@fujitsu.com
>> Cc: Peter Newman peternewman@google.com
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>> Changes since v3:
>> Drop tags
>> Fix component reset, otherwise cpbm wrong and controls not set.
>> Add a cfg_lock to guard configuration of an msc
>
> The use of bitmap_set() for things that aren't unsigned long (arrays) is a bad
> idea. Much better to use GENMASK() to fill those.
>
>> ---
>> drivers/resctrl/mpam_devices.c | 268 ++++++++++++++++++++++++++++++--
>> drivers/resctrl/mpam_internal.h | 27 ++++
>> 2 files changed, 280 insertions(+), 15 deletions(-)

> Why manipulate a u32 with bitmap_set() with a horrible pretend it's an unsigned long cast.
> Instead just do:
> comp->cfg[i].cpbm = GENMASK(cprops->cpbm_wd, 0);
> Which is indeed what bitmap_set will do internally due to an optimization for small bitmaps
> but lets avoid that making one integer pretend to be another of a different length.
>
>
>> + bitmap_set((unsigned long *)&comp->cfg[i].mbw_pbm, 0, cprops->mbw_pbm_bits);
>> + bitmap_set((unsigned long *)&comp->cfg[i].mbw_max, 16 - cprops->bwa_wd, cprops->bwa_wd);
>> + }
>> +}
>

It is a bit nasty. I was trying to keep the concept of setting n bits and avoid
considering explicitly what happens when n is zero.

These bitmap_set() can become:

if (cprops->cpbm_wd)
comp->cfg[i].cpbm = GENMASK(cprops->cpbm_wd - 1, 0);
if (cprops->mbw_pbm_bits)
comp->cfg[i].mbw_pbm = GENMASK(cprops->mbw_pbm_bits - 1, 0);
if (cprops->bwa_wd)
comp->cfg[i].mbw_max = GENMASK(15, 16 - cprops->bwa_wd);

Thanks,

Ben