Re: [RFC PATCH v2 01/19] arm_mpam: resctrl: Leave CDP off when there are too few PARTIDs

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

Hi Jinjie,

Thanks for this fix!

On 29/10/2026 12:35, Jinjie Ruan wrote:
> Enabling CDP needs a second PARTID for the reserved closid.

I'm not sure what a 'reserved closid' is here but you could just say:

Enabling CDP requires at least 2 PARTIDs.

> If firmware
> reports too few PARTIDs, resctrl_arch_set_cdp_enabled() returns -EINVAL,

Not all systems are firmware backed. This can be:

If there are too few PARTIDs, ... .

> but only after it has recorded CDP as enabled, halved the monitor count
> and turned off the mbw_max MBA control. 


> resctrl then reports an error
> and believes CDP is off, which leaves the two sides out of step and the
> halved monitor count and the missing MBA control are not undone.

I think the 'reports an error part' can be dropped leaving 'resctrl then believes CDP is off...' as
exactly what resctrl does is not important here.

> 
> Check the PARTID limit, and work out the CDP PARTIDs, before anything is

Why work out the CDP PARTIDs so early if they aren't going to be used?

> changed so a failed enable has no effect.
> 
> Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/resctrl/mpam_resctrl.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index 5468367776e2..886f0595d364 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -210,6 +210,18 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
>  	if (enable)
>  		pr_warn("CDP is an expert feature and may cause MPAM to malfunction.\n");
>  
> +	/*
> +	 * Check this before changing any state: if CDP can't be enabled the
> +	 * caller must see CDP left off.
> +	 */

I don't think this comment is needed. Failing early rather than failing late after some side effects
have happened is clearly better.

> +	if (enable) {
> +		if (mpam_partid_max < 1)
> +			return -EINVAL;
> +
> +		partid_d = resctrl_get_config_index(RESCTRL_RESERVED_CLOSID, CDP_DATA);
> +		partid_i = resctrl_get_config_index(RESCTRL_RESERVED_CLOSID, CDP_CODE);

The partid_d/partid_i determination can stay below so it's close to the user.

I think we can just have the check here:

if (enable && mpam_partid_max < 1)
  	return -EINVAL;

Thanks,

Ben

> +	}
> +
>  	/*
>  	 * resctrl_arch_set_cdp_enabled() is only called with enable set to
>  	 * false on error and unmount.
> @@ -238,14 +250,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
>  	if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
>  		mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
>  
> -	if (enable) {
> -		if (mpam_partid_max < 1)
> -			return -EINVAL;
> -
> -		partid_d = resctrl_get_config_index(RESCTRL_RESERVED_CLOSID, CDP_DATA);
> -		partid_i = resctrl_get_config_index(RESCTRL_RESERVED_CLOSID, CDP_CODE);
> -	}
> -
>  	mpam_set_task_partid_pmg(current, partid_d, partid_i, 0, 0);
>  	WRITE_ONCE(arm64_mpam_global_default, mpam_get_regval(current));
>