Re: [PATCH v2 2/2] arm_mpam: Update architecture version check for MPAM MSC

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

Hi James and Zeng,

On 5/8/26 04:47, Zeng Heng wrote:
> Hi James,
> 
> On 2026/5/8 10:26, Zeng Heng wrote:
> 
>>> I think its simpler to rule out the unsupported combinations, something like:
>>> | static bool mpam_msc_check_aidr(struct mpam_msc *msc)
>>> | {
>>> |     u32 rev;
>>> |
>>> |     rev = __mpam_read_reg(msc, MPAMF_AIDR) & MPAMF_AIDR_ARCH_REV;
>>> |
>>> |      /*
>>> |      * v0.0 and >v2.x aren't supported, but anything else should be backward
>>> |     * compatible to v0.1 or v1.0.
>>> |     */
>>> |     if (!rev)
>>> |         return false;
>>> |     if (rev & MPAMF_AIDR_ARCH_MAJOR_REV > MPAM_ARCHITECTURE_V1)
>>> |         return false;
>>> |
> 
> Oops, after more complete version number testing, I found there's an
> operator precedence issue here. The correct fix is:
> 
>     if ((rev & MPAMF_AIDR_ARCH_MAJOR_REV) > MPAM_ARCHITECTURE_V1)
>         return false;
> 
> Note that '>' has higher precedence than '&'.

Isn't it better to use FIELD_GET()?  We could also avoid creating MPAMF_AIDR_ARCH_REV and use MPAMF_AIDR_ARCH_MAJOR_REV
and MPAMF_AIDR_ARCH_MINOR_REV directly to stop splitting the logic over two files. mpam_msc_check_aidr() could become:

static bool mpam_msc_check_aidr(struct mpam_msc *msc)
{
	u32 aidr = __mpam_read_reg(msc, MPAMF_AIDR);
	u32 major = FIELD_GET(MPAMF_AIDR_ARCH_MAJOR_REV, aidr);
	u32 minor = FIELD_GET(MPAMF_AIDR_ARCH_MINOR_REV, aidr);

	/*
	 * v0.0 and >v2.x aren't supported, but anything else should be backward
	 * compatible to v0.1 or v1.0.
	 */
	if (!major && !minor)
		return false;
	if (major > MPAM_ARCHITECTURE_V1)
		return false;

	return true;
}


Thanks,

Ben

> 
> 
> With this fix included:
> Tested-by: Zeng Heng <zengheng4@huawei.com>
> 
> 
>>> |     return true;
>>> | }
>>>
>>>> +    if (!mpam_msc_check_aidr(msc)) {
>>>> +        dev_err_once(dev, "MSC does not match MPAM architecture\n");
>>>>           return -EIO;
>>>>       }
>>>
>>> I'd like to keep the 'v1.x' in this message - this should help folk with old stable
>>> kernels running on new hardware work out why the feature isn't available.
>>> (assuming they have some documentation that says v2.0 in it!)
>>>
>>> I've rebased this with the above changes, which I'll post shortly for fixes.
>>>
>>>
>>
>> Agreed. Keep the backward compatibility extension for versions
>> (compatible with v0.x(x>0) and 1.x), and remove the redundant
>> MPAM_ARCHITECTURE_Vx_x macro definitions.
>>
>> I've verified locally that everything works fine.
>>
> 
>