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

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

Hi again James and Zeng,

On 5/8/26 10:37, Ben Horgan wrote:
> 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)

This isn't correct. I missed that MPAM_ARCHITECTURE_V1 is 0x10 (which matches MPAMF_AIDR_ARCH_REV) and not 1.

Thanks,

Ben

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