[PATCH MPAM 6.16.rc1] arm_mpam: Enforce to recalculate mbw_min on configuration

[PATCH MPAM 6.16.rc1] arm_mpam: Enforce to recalculate mbw_min on configuration

[Gavin Shan]

mpam_feat_mbw_max is exposed to user by resctrlfs, but mpam_feat_mbw_min
should be recalculated based on the maximal memory bandwidth in every
configuration, which has been missed unfortunately. When a new group is
created, the default and maximal memory bandwidth percentage (100%) is
configured, the configuration instance (struct mpam_config) on the stack
is updated, including the minimal and maximal memory bandwidth. It's
copied to the domain's configuration instance. On next time when user
tries to configure by writing 'schemata', the minimal memory bandwidth
isn't never recalculated because mpam_feat_mbw_min has been seen in the
configuration, which inherits from the domain's instance.

For example, the value of register MPAMCFG_MBW_MIN is never changed no
matter what configuration is set.

  # uname -r
  6.16.0-rc1-gavin
  # mount -tresctrl none /sys/fs/resctrl
  # mkdir -p /sys/fs/resctrl/test
  # cd /sys/fs/resctrl/test
  # echo "MB:1=2" > schemata
  MAPMF_MBW_IDR               00000c07
  MPAMCFG_MBW_MAX             000005ff
  MPAMCFG_MBW_MIN             0000f000
  # echo "MB:1=100" > schemata
  MAPMF_MBW_IDR               00000c07
  MPAMCFG_MBW_MAX             0000ffff
  MPAMCFG_MBW_MIN             0000f000

Fix the issue by enforcing the calculation of the minimal memory bandwidth
in very configuration. With this applied, The register MPAMCFG_MBW_MIN
is updated with the expected value in every configuration.

  # cd /sys/fs/resctrl/test
  # echo "MB:1=2" > schemata
  MAPMF_MBW_IDR               00000c07
  MPAMCFG_MBW_MAX             000005ff
  MPAMCFG_MBW_MIN             00000200
  # echo "MB:1=100" > schemata
  MAPMF_MBW_IDR               00000c07
  MPAMCFG_MBW_MAX             0000ffff
  MPAMCFG_MBW_MIN             0000f000

Fixes: 75f4101bb338 ("arm_mpam: Generate a configuration for min controls")
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
Appliable to James Morse's latest MPAM branch
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git
(branch: mpam/snapshot/v6.16-rc1)
---
 drivers/platform/arm64/mpam/mpam_devices.c | 27 ++++++++++------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index df8730491de2..4845dcb8e601 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -3192,6 +3192,9 @@ static void mpam_extend_config(struct mpam_class *class, struct mpam_config *cfg
 	u16 min, min_hw_granule, delta;
 	u16 max_hw_value, res0_bits;
 
+	if (!mpam_has_feature(mpam_feat_mbw_max, cfg))
+		return;
+
 	/*
 	 * Calculate the values the 'min' control can hold.
 	 * e.g. on a platform with bwa_wd = 8, min_hw_granule is 0x00ff because
@@ -3211,23 +3214,17 @@ static void mpam_extend_config(struct mpam_class *class, struct mpam_config *cfg
 	 *
 	 * Resctrl can only configure the MAX.
 	 */
-	if (mpam_has_feature(mpam_feat_mbw_max, cfg) &&
-	    !mpam_has_feature(mpam_feat_mbw_min, cfg)) {
-		delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
-		if (cfg->mbw_max > delta)
-			min = cfg->mbw_max - delta;
-		else
-			min = 0;
+	delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
+	if (cfg->mbw_max > delta)
+		min = cfg->mbw_max - delta;
+	else
+		min = 0;
 
-		cfg->mbw_min = max(min, min_hw_granule);
-		mpam_set_feature(mpam_feat_mbw_min, cfg);
-	}
+	cfg->mbw_min = max(min, min_hw_granule);
+	mpam_set_feature(mpam_feat_mbw_min, cfg);
+	if (mpam_has_quirk(T241_FORCE_MBW_MIN_TO_ONE, class))
+		cfg->mbw_min = max(cfg->mbw_min, min_hw_granule + 1);
 
-	if (mpam_has_quirk(T241_FORCE_MBW_MIN_TO_ONE, class) &&
-	    cfg->mbw_min <= min_hw_granule) {
-		cfg->mbw_min = min_hw_granule + 1;
-		mpam_set_feature(mpam_feat_mbw_min, cfg);
-	}
 }
 
 /* TODO: split into write_config/sync_config */
-- 
2.49.0

Re: [PATCH MPAM 6.16.rc1] arm_mpam: Enforce to recalculate mbw_min on configuration

[James Morse]

Hi Gavin,

On 26/06/2025 10:52, Gavin Shan wrote:
> mpam_feat_mbw_max is exposed to user by resctrlfs, but mpam_feat_mbw_min
> should be recalculated based on the maximal memory bandwidth in every
> configuration, which has been missed unfortunately. When a new group is
> created, the default and maximal memory bandwidth percentage (100%) is
> configured, the configuration instance (struct mpam_config) on the stack
> is updated, including the minimal and maximal memory bandwidth. It's
> copied to the domain's configuration instance. On next time when user
> tries to configure by writing 'schemata', the minimal memory bandwidth
> isn't never recalculated because mpam_feat_mbw_min has been seen in the
> configuration, which inherits from the domain's instance.
> 
> For example, the value of register MPAMCFG_MBW_MIN is never changed no
> matter what configuration is set.

Thanks for catching this. I think this was introduced by the patch that picked
the component copy of the configuration, modified it - and passed it back into
the driver. That had the additional side effect of changes going missing as the
mpam_update_config() had the same config twice...

I fixed that shortly after rc1, with that fixed I don't think this can happen -
resctrl never sets the 'min', meaning mpam_extent_config() will always regenerate
the value.

This shouldn't happen with the latest version of the tree.
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/log/?h=mpam/snapshot/v6.16-rc4




> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c
b/drivers/platform/arm64/mpam/mpam_devices.c
> index df8730491de2..4845dcb8e601 100644
> --- a/drivers/platform/arm64/mpam/mpam_devices.c
> +++ b/drivers/platform/arm64/mpam/mpam_devices.c
> @@ -3192,6 +3192,9 @@ static void mpam_extend_config(struct mpam_class *class, struct
mpam_config *cfg
>  	u16 min, min_hw_granule, delta;
>  	u16 max_hw_value, res0_bits;
>
> +	if (!mpam_has_feature(mpam_feat_mbw_max, cfg))
> +		return;

N.B. - The T421 quirk needs to ensure the minimum value is never set to 0 - which is what
mpam_reprogram_ris_partid() will do if there is no minimum config set.

The aim of the quirk is that on affected platforms the mbm_min will always be set, and
never be zero in any configuration. Whether it then gets applied to the hardware depends
on whether the hardware has the feature.
This makes it simpler to think about - and easier to test on platforms that aren't
affected by the errata.

I'd prefer not to to try and spot configurations where its going to matter as it will be
hard to debug the workaround going wrong!


> @@ -3211,23 +3214,17 @@ static void mpam_extend_config(struct mpam_class *class, struct
mpam_config *cfg
>  	 *
>  	 * Resctrl can only configure the MAX.
>  	 */
> -	if (mpam_has_feature(mpam_feat_mbw_max, cfg) &&
> -	    !mpam_has_feature(mpam_feat_mbw_min, cfg)) {
> -		delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
> -		if (cfg->mbw_max > delta)
> -			min = cfg->mbw_max - delta;
> -		else
> -			min = 0;
> +	delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
> +	if (cfg->mbw_max > delta)
> +		min = cfg->mbw_max - delta;
> +	else
> +		min = 0;
>
> -		cfg->mbw_min = max(min, min_hw_granule);
> -		mpam_set_feature(mpam_feat_mbw_min, cfg);
> -	}
> +	cfg->mbw_min = max(min, min_hw_granule);

While resctrl doesn't use it - the idea is the mpam_devices interface should support any
mpam control being set. This overwrites what the user asked for.

The devices/resctrl split might look strange - but its so the resctrl ABI implications are
contained to one file, and another user of the mpam_devices interface can be added later.
(KVM in-kernel MSC emulation was one proposal, another came from people who believe they
 need to change the hardware policy more frequently than they can with a syscall ... I
 don't like either of these!)


Thanks,

James

Re: [PATCH MPAM 6.16.rc1] arm_mpam: Enforce to recalculate mbw_min on configuration

[Gavin Shan]

Hi James,

On 7/14/25 8:15 PM, James Morse wrote:
> On 26/06/2025 10:52, Gavin Shan wrote:
>> mpam_feat_mbw_max is exposed to user by resctrlfs, but mpam_feat_mbw_min
>> should be recalculated based on the maximal memory bandwidth in every
>> configuration, which has been missed unfortunately. When a new group is
>> created, the default and maximal memory bandwidth percentage (100%) is
>> configured, the configuration instance (struct mpam_config) on the stack
>> is updated, including the minimal and maximal memory bandwidth. It's
>> copied to the domain's configuration instance. On next time when user
>> tries to configure by writing 'schemata', the minimal memory bandwidth
>> isn't never recalculated because mpam_feat_mbw_min has been seen in the
>> configuration, which inherits from the domain's instance.
>>
>> For example, the value of register MPAMCFG_MBW_MIN is never changed no
>> matter what configuration is set.
> 
> Thanks for catching this. I think this was introduced by the patch that picked
> the component copy of the configuration, modified it - and passed it back into
> the driver. That had the additional side effect of changes going missing as the
> mpam_update_config() had the same config twice...
> 
> I fixed that shortly after rc1, with that fixed I don't think this can happen -
> resctrl never sets the 'min', meaning mpam_extent_config() will always regenerate
> the value.
> 
> This shouldn't happen with the latest version of the tree.
> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/log/?h=mpam/snapshot/v6.16-rc4
> 

The issue still exists in v6.16-rc4 and v6.16-rc5. Here are the dumped registers
on the varied MBW configurations, and MPAMCFG_MBW_MIN doesn't alter.

   # uname -r
   6.16.0-rc5-gavin
   # mount -tresctrl none /sys/fs/resctrl
   # mkdir -p /sys/fs/resctrl/all
   # mkdir -p /sys/fs/resctrl/test
   # cd /sys/fs/resctrl/test

   # echo "MB:1=100" > schemata
   # cat /proc/dump
   MPAMCFG_PART_SEL            00000002
   MAPMF_MBW_IDR               00000c07
   MPAMCFG_MBW_MAX             0000ffff
   MPAMCFG_MBW_MIN             0000f000

   # echo "MB:1=2" > schemata
   # cat /proc/dump
   MPAMCFG_PART_SEL            00000002
   MAPMF_MBW_IDR               00000c07
   MPAMCFG_MBW_MAX             000005ff
   MPAMCFG_MBW_MIN             0000f000

The problem is mpam_feat_mbw_min is always set in 'struct mpam_config'. With this,
cfg->mbw_min won't be recalculated. I guess we probably just need to simply drop
the check to that cfg->mbw_min can be recalculated every time? Could you please
fix it up in place if the resolution looks good to you, I also can post (v2) to
include the changes if you prefer. I don't know how it can be handled since the
code isn't merged to upstream yet.

static void mpam_extend_config(struct mpam_class *class, struct mpam_config *cfg)
{
	:
         /* The mpam_feat_mbw_min needs to be dropped */
	if (mpam_has_feature(mpam_feat_mbw_max, cfg) &&
             !mpam_has_feature(mpam_feat_mbw_min, cfg)) {
                 delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
                 if (cfg->mbw_max > delta)
                         min = cfg->mbw_max - delta;
                 else
                         min = 0;

                 cfg->mbw_min = max(min, min_hw_granule);
                 mpam_set_feature(mpam_feat_mbw_min, cfg);
         }

         if (mpam_has_quirk(T241_FORCE_MBW_MIN_TO_ONE, class) &&
             cfg->mbw_min <= min_hw_granule) {
                 cfg->mbw_min = min_hw_granule + 1;
                 mpam_set_feature(mpam_feat_mbw_min, cfg);
         }
}

> 
>> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c
> b/drivers/platform/arm64/mpam/mpam_devices.c
>> index df8730491de2..4845dcb8e601 100644
>> --- a/drivers/platform/arm64/mpam/mpam_devices.c
>> +++ b/drivers/platform/arm64/mpam/mpam_devices.c
>> @@ -3192,6 +3192,9 @@ static void mpam_extend_config(struct mpam_class *class, struct
> mpam_config *cfg
>>   	u16 min, min_hw_granule, delta;
>>   	u16 max_hw_value, res0_bits;
>>
>> +	if (!mpam_has_feature(mpam_feat_mbw_max, cfg))
>> +		return;
> 
> N.B. - The T421 quirk needs to ensure the minimum value is never set to 0 - which is what
> mpam_reprogram_ris_partid() will do if there is no minimum config set.
> 
> The aim of the quirk is that on affected platforms the mbm_min will always be set, and
> never be zero in any configuration. Whether it then gets applied to the hardware depends
> on whether the hardware has the feature.
> This makes it simpler to think about - and easier to test on platforms that aren't
> affected by the errata.
> 
> I'd prefer not to to try and spot configurations where its going to matter as it will be
> hard to debug the workaround going wrong!
> 

Ok, thanks for the explanation. In that case, we shouldn't bail early. The mpam_feat_mbw_min
check in mpam_extend_config() needs to be dropped, as explained above.

> 
>> @@ -3211,23 +3214,17 @@ static void mpam_extend_config(struct mpam_class *class, struct
> mpam_config *cfg
>>   	 *
>>   	 * Resctrl can only configure the MAX.
>>   	 */
>> -	if (mpam_has_feature(mpam_feat_mbw_max, cfg) &&
>> -	    !mpam_has_feature(mpam_feat_mbw_min, cfg)) {
>> -		delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
>> -		if (cfg->mbw_max > delta)
>> -			min = cfg->mbw_max - delta;
>> -		else
>> -			min = 0;
>> +	delta = ((5 * MPAMCFG_MBW_MAX_MAX) / 100) - 1;
>> +	if (cfg->mbw_max > delta)
>> +		min = cfg->mbw_max - delta;
>> +	else
>> +		min = 0;
>>
>> -		cfg->mbw_min = max(min, min_hw_granule);
>> -		mpam_set_feature(mpam_feat_mbw_min, cfg);
>> -	}
>> +	cfg->mbw_min = max(min, min_hw_granule);
> 
> While resctrl doesn't use it - the idea is the mpam_devices interface should support any
> mpam control being set. This overwrites what the user asked for.
> 
> The devices/resctrl split might look strange - but its so the resctrl ABI implications are
> contained to one file, and another user of the mpam_devices interface can be added later.
> (KVM in-kernel MSC emulation was one proposal, another came from people who believe they
>   need to change the hardware policy more frequently than they can with a syscall ... I
>   don't like either of these!)
> 

hmm, it's intresting to know we can't tolerate the time consumed to finish a syscall.
The MPAM would have static configurations, meaning it's static after the configuration
is given. Could you share who is looking into MPAM virtualization? Actually, David
and I also looked into MPAM virtualization support, but we had everything emulated in
QEMU in the draft design.

Thanks,
Gavin

> 
> Thanks,
> 
> James
>