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

[Ben Horgan <ben.horgan@arm.com>]

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