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

[James Morse <james.morse@arm.com>]

Hi Ben,

On 08/05/2026 10:56, Ben Horgan wrote:
> On 5/8/26 10:37, Ben Horgan wrote:
>> On 5/8/26 04:47, Zeng Heng wrote:
>>> 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 '&'.

(yeah, the compiler winged at me. I should have said I didn't even build the hunk above
 before suggesting it!)


>> Isn't it better to use FIELD_GET()?

Always!


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

Done locally. Thanks!

I also ran over two bugs with v0.1 MSC causing mpam_disable() to be called before
mpam_enable().


Thanks,

James