Re: [PATCH v6 05/18] x86,arm,fs/resctrl: Introduce architecture hook to program global kernel mode

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

There is no reference in upstream resctrl but during discussions around
the "multiple controls" support we settled on a new subject prefix of
"arm,x86,fs/resctrl" for patches that touch all these parts.

On 10/1/26 8:10 AM, Babu Moger wrote:
> RESCTRL_GLOBAL_ENABLE_PER_CPU mode enables kernel work to use different
> allocation and monitoring associations than the originating user task.
> 
> The mode is enabled independently on each CPU, so the filesystem must
> program hardware when the mode is activated, when CPUs are added or
> removed, and when it is deactivated. Allocation and monitoring associations
> can be done independently, so the architecture callback takes separate
> control and monitoring options.

Going back and forth through this series a couple of times all these
usages of "allocation and monitoring associations" in different contexts
become a blur and at this point I do not know what it means at all.

> 
> Add resctrl_arch_configure_kmode_global() with an x86 PLZA implementation
> and an empty MPAM stub for ARM systems that do not support this mode.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 38 +++++++++++++++++++++++
>  drivers/resctrl/mpam_resctrl.c            |  7 +++++
>  include/linux/resctrl.h                   | 37 ++++++++++++++++++++++
>  3 files changed, 82 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 62044489b052..a343dc0c71c5 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -137,3 +137,41 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable)
>  
>  	return 0;
>  }
> +
> +static void resctrl_kmode_set_one_amd(void *arg)
> +{
> +	union msr_pqr_plza_assoc *plza = arg;
> +
> +	wrmsrq(MSR_IA32_PQR_PLZA_ASSOC, plza->full);
> +}
> +
> +/*
> + * resctrl_arch_configure_kmode_global - Program Privilege Level Zero
> + *					 Association (PLZA).
> + * @cpu_mask:	CPUs whose PLZA MSR should be updated.
> + * @closid:	CLOSID programmed into MSR_IA32_PQR_PLZA_ASSOC. Always
> + *		written; required even when @assign_ctrl is false.
> + * @assign_ctrl: Whether PLZA should program the kernel-mode CLOSID.

At this point the series seems to switch terms from the "kernel mode" in
previous patches to "kernel-mode" here. Could this all be consistently 
"kernel mode"? 

On a similar note, the series switches between "global kernel mode" (like
in this subject) and "global per-CPU kernel mode" (like in patch #14
subject). 

> + * @rmid:	RMID programmed into MSR_IA32_PQR_PLZA_ASSOC. Always
> + *		written; required even when @assign_mon is false.
> + * @assign_mon:	Whether PLZA should program the kernel-mode RMID.
> + * @enable:	Whether PLZA should program the kernel-mode association.
> + *
> + * Program PLZA on @cpu_mask for %RESCTRL_GLOBAL_ENABLE_PER_CPU. See
> + * union msr_pqr_plza_assoc for the hardware field meanings.
> + */
> +void resctrl_arch_configure_kmode_global(const struct cpumask *cpu_mask,
> +					 u32 closid, bool assign_ctrl,
> +					 u32 rmid, bool assign_mon,
> +					 bool enable)
> +{
> +	union msr_pqr_plza_assoc plza = { 0 };
> +
> +	plza.split.rmid = rmid;
> +	plza.split.rmid_en = assign_mon;
> +	plza.split.closid = closid;
> +	plza.split.closid_en = assign_ctrl;
> +	plza.split.plza_en = enable;
> +
> +	on_each_cpu_mask(cpu_mask, resctrl_kmode_set_one_amd, &plza, 1);
> +}
> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index f2c651e9f6ea..0c8594e98526 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -139,6 +139,13 @@ bool resctrl_arch_get_io_alloc_enabled(struct rdt_resource *r)
>  	return false;
>  }
>  
> +void resctrl_arch_configure_kmode_global(const struct cpumask *cpu_mask,
> +					 u32 closid, bool assign_ctrl,
> +					 u32 rmid, bool assign_mon,
> +					 bool enable)
> +{
> +}
> +
>  void resctrl_arch_pre_mount(void)
>  {
>  }
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f6adff7f4e1f..dcb0beb847b6 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -752,6 +752,43 @@ enum resctrl_kernel_mode {
>  
>  #define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)
>  
> +/**
> + * resctrl_arch_configure_kmode_global() - Program RESCTRL_GLOBAL_ENABLE_PER_CPU
> + *
> + * @cpu_mask:	CPUs whose kernel-mode association should be updated.

This could be made more specific about being online CPUs. For example,
	@cpu_mask: Online CPUs on which to update the association.

> + * @closid:	CLOSID that matches the RMID to program kernel mode. Depending
> + *		on the architecture, the counter may match traffic of both
> + *		@closid and @rmid, or @rmid only. Always provided; a valid
> + *		CLOSID is required even when @assign_ctrl is false.

Above description seems to be a concatenation of different texts. The "counter" text
seems to be copied from resctrl_arch_rmid_read() and does not seem relevant here.
The "CLOSID is required" is copied from the x86 arch helper where it is indeed
required - but this does not apply to resctrl fs?

How about something like:
	@closid: CLOSID of the resource group backing %RESCTRL_GLOBAL_ENABLE_PER_CPU mode.
		 Always provided, even when @assign_ctrl is false.


> + * @assign_ctrl: true to assign @closid for kernel mode; false to inherit
> + *		association from the user-space task.
> + * @rmid:	RMID to program the kernel mode. Some architectures may use
> + *		CLOSID/RMID separately, others will consider them together.
> + *		Always provided; a valid RMID is required even when
> + *		@assign_mon is false.

(similar comment as for @closid)

> + * @assign_mon:	true to assign @rmid for kernel mode; false to inherit
> + *		monitoring association from the user-space task.
> + * @enable:	true to enable kernel mode association on CPUs in @cpu_mask;
> + *		false to disable kernel mode.

"false to disable kernel mode" does not seem to capture all that is required
from architecture. When considering resctrl_kmode_switch(), @enable being false
is only thing resctrl fs does to put CPUs into RESCTRL_INHERIT_USER mode? So when
@enable is false the architecture should disable %RESCTRL_GLOBAL_ENABLE_PER_CPU
*and* enable %RESCTRL_INHERIT_USER?

> + *
> + * This callback programs %RESCTRL_GLOBAL_ENABLE_PER_CPU. It is called in
> + * the following scenarios:
> + * - If the mode is active when user space changes the configuration, it is
> + *   first called to deactivate the mode on all CPUs where it is active.
> + * - When user space activates the mode, it is called with cpu_online_mask.
> + * - When user space adds a CPU to the active mode.
> + * - When user space removes a CPU from the active mode.

This attempt at listing all callsites is proving problematic since it is
already out of date by the end of the series. I would thus suggest to drop
listing the individual call sites and instead focus on writing a clear
contract. Consider, for example, something like (but please verify, especially
for accuracy and correct terms, do not just copy&paste):

	An association starts when %RESCTRL_GLOBAL_ENABLE_PER_CPU is activated
	and ends when the mode is deactivated or user space changes the backing
	resource group, @assign_ctrl or @assign_mon. Every call for one
	association provides the same @closid, @assign_ctrl, @rmid and
	@assign_mon; only @cpu_mask and @enable differ between calls. Before an
	association ends, it is disabled on every CPU where it is enabled.

> + *
> + * Contract between fs and arch: resctrl fs always provides the same
> + * closid, assign_ctrl, rmid, and assign_mon when activating
> + * %RESCTRL_GLOBAL_ENABLE_PER_CPU, during CPU add/remove while the mode is
> + * active, and when deactivating the mode.
> + */
> +void resctrl_arch_configure_kmode_global(const struct cpumask *cpu_mask,
> +					 u32 closid, bool assign_ctrl,
> +					 u32 rmid, bool assign_mon,
> +					 bool enable);
> +
>  extern unsigned int resctrl_rmid_realloc_threshold;
>  extern unsigned int resctrl_rmid_realloc_limit;
>  

Reinette