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

[Jinjie Ruan]

在 2026/10/31 0:38, Ben Horgan 写道:
> 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:

Hi Ben,


This means there are two copies: one for data and one for code.

> 
> Enabling CDP requires at least 2 PARTIDs.

OK.

> 
>> 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, ... .

Thank you! I will fix it.

> 
>> 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.

Will update it.

> 
>>
>> 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?

Modified them together as a whole.

> 
>> changed so a failed enable has no effect.
>>
>> Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
>> ---
>>  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.

Will delete it.

> 
>> +	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.

Will put it back in its original place.

> 
> I think we can just have the check here:
> 
> if (enable && mpam_partid_max < 1)
>   	return -EINVAL;

That is fine.

> 
> 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));
>>  
> 

-- 
Best regards,
Jinjie