Re: [RFC PATCH 01/38] arm64: mpam: Context switch the MPAM registers

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

Hi Fenghua,

On 12/5/25 23:53, Fenghua Yu wrote:
> Hi, James,
> 
> On 12/5/25 13:58, James Morse wrote:
[...]
>>
>> Resctrl allows a per-cpu 'default' value to be set, this overrides the
>> values when scheduling a task in the default control-group, which has
>> PARTID 0. The way 'code data prioritisation' gets emulated means the
>> register value for the default group needs to be a variable.
>>
[...]
>> diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/
>> mpam.h
>> new file mode 100644
>> index 000000000000..86a55176f884
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/mpam.h
[...]
>> +/*
>> + * The value of the MPAM0_EL1 sysreg when a task is in resctrl's
>> default group.
>> + * This is used by the context switch code to use the resctrl CPU
>> property
>> + * instead. The value is modified when CDP is enabled/disabled by
>> mounting
>> + * the resctrl filesystem.
>> + */
>> +extern u64 arm64_mpam_global_default;
>> +
>> +/*
>> + * The resctrl filesystem writes to the partid/pmg values for threads
>> and CPUs,
>> + * which may race with reads in __mpam_sched_in(). Ensure only one of
>> the old
>> + * or new values are used. Particular care should be taken with the
>> pmg field
>> + * as __mpam_sched_in() may read a partid and pmg that don't match,
>> causing
>> + * this value to be stored with cache allocations, despite being
>> considered
>> + * 'free' by resctrl.
>> + *
>> + * A value in struct thread_info is used instead of struct
>> task_struct as the
>> + * cpu's u64 register format is used, but struct task_struct has two
>> u32'.
>> + */
>> +static inline u64 mpam_get_regval(struct task_struct *tsk)
>> +{
>> +#ifdef CONFIG_ARM64_MPAM
>> +    return READ_ONCE(task_thread_info(tsk)->mpam_partid_pmg);
>> +#else
>> +    return 0;
>> +#endif
>> +}
>> +
>> +static inline void mpam_thread_switch(struct task_struct *tsk)
>> +{
>> +    u64 oldregval;
>> +    int cpu = smp_processor_id();
>> +    u64 regval = mpam_get_regval(tsk);
>> +
>> +    if (!IS_ENABLED(CONFIG_ARM64_MPAM) ||
>> +        !static_branch_likely(&mpam_enabled))
>> +        return;
>> +
>> +    if (regval == READ_ONCE(arm64_mpam_global_default))
> Why is this check needed? We need to read arm64_mpam_default in any
> case, right?

If a task is in the default resctrl group then the per-cpu resctrl
configuration determines the mpam configuration. The way this is dealt
with here is that for a task in the default group mpam_partid_pmg is set
to arm64_mpam_global_default. When mpam_partid_pmg is
arm64_mpam_global_default we know the task is in the default group,
otherwise it would have a different partid, and so we consider the
per-cpu configuration, arm64_mpam_default. When the task is not in the
default resctrl group then we can just consider the per-task configuration.

As mentioned in the commit message, this is complicated by code data
prioritisation (cdp) and the value of arm64_mpam_global_default, depends
on whether this is enabled or not. See resctrl_arch_set_cdp_enabled()
added in a later patch. I'm not sure of a way to make this clearer in
the code while keeping the arch and drivers/resctrl patches separate.

> +        regval = READ_ONCE(per_cpu(arm64_mpam_default,
> cpu));
>> +
>> +    oldregval = READ_ONCE(per_cpu(arm64_mpam_current, cpu));
>> +    if (oldregval == regval)
>> +        return;
>> +
>> +    write_sysreg_s(regval, SYS_MPAM1_EL1);
>> +    isb();
>> +
>> +    /* Synchronising the EL0 write is left until the ERET to EL0 */
>> +    write_sysreg_s(regval, SYS_MPAM0_EL1);
>> +
>> +    WRITE_ONCE(per_cpu(arm64_mpam_current, cpu), regval);
>> +}
>> +#endif /* __ASM__MPAM_H */
> 
> [SNIP]
> Thanks.
> 
> -Fenghua
> 
Thanks,

Ben