Re: [PATCH v3 02/12] x86/resctrl: Add data structures and definitions for PLZA configuration

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

Hi Babu,

On 4/30/26 4:24 PM, Babu Moger wrote:
> Privilege Level Zero Association (PLZA) is configured per logical processor
> via MSR_IA32_PQR_PLZA_ASSOC (0xc00003fc). Software must program RMID and
> CLOSID association fields and their enable bits using the layout defined
> for the MSR.
> 
> Define MSR_IA32_PQR_PLZA_ASSOC and the RMID_EN, CLOSID_EN, and PLZA_EN bit
> masks in asm/msr-index.h. Add union msr_pqr_plza_assoc in arch resctrl
> internal.h

Above paragraph captures what can be seen from the patch. Please check entire
series for this since many changelogs in this series verbatim describes the code
changes in patch without helping reader understand why those changes are made.


> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 9dc6b610e4e2..623628d3c643 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1287,10 +1287,17 @@
>  /* - AMD: */
>  #define MSR_IA32_MBA_BW_BASE		0xc0000200
>  #define MSR_IA32_SMBA_BW_BASE		0xc0000280
> +#define MSR_IA32_PQR_PLZA_ASSOC		0xc00003fc
>  #define MSR_IA32_L3_QOS_ABMC_CFG	0xc00003fd
>  #define MSR_IA32_L3_QOS_EXT_CFG		0xc00003ff
>  #define MSR_IA32_EVT_CFG_BASE		0xc0000400
>  
> +/* Lower 32 bits of MSR_IA32_PQR_PLZA_ASSOC */
> +#define RMID_EN				BIT(31)
> +/* Upper 32 bits of MSR_IA32_PQR_PLZA_ASSOC */
> +#define CLOSID_EN			BIT(15)
> +#define PLZA_EN				BIT(31)
> +

This is unexpected. So far resctrl has only defined the MSR numbers in this file, not
the individual fields. This seems a legitimate use of msr-index.h but creates inconsistency
with how the fields of the other resctrl registers are defined. This may be ok so I am
looking past this for now. Since I am not familiar with this use I am looking at other
patterns of this and it seems that the register fields are usually defined right after
the register to make this relationship clear and also use more verbose naming to establish
this relationship ... I do not think such cryptic names should be used without context
in such a global scope. Please compare with how other fields are defined at this scope.

> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e3cfa0c10e92..1c2f87ffb0ea 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -222,6 +222,33 @@ union l3_qos_abmc_cfg {
>  	unsigned long full;
>  };
>  
> +/*
> + * PLZA is programmed by writing to MSR_IA32_PQR_PLZA_ASSOC. Bitfield
> + * layout for MSR_IA32_PQR_PLZA_ASSOC (Privilege Level Zero Association).

These comments are valuable to describe how resctrl should interact with
this register so it would help to be specific and document any and all
constraints.

For example, I seem to remember that all fields except PLZA_EN are required
to be identical on all CPUs. Please document that and any other constraints here.

> + *
> + * @rmid		: The RMID to be configured for PLZA.

What does "to be configured" mean? It seems to imply that when resctrl
writes to @rmid then the setting does not take immediate effect but would
take effect at some future "configure" time?

> + * @reserved1		: Reserved.
> + * @rmid_en		: Associate RMID or not.

Please elaborate ... what is RMID associated with? What does "or not" imply? 
Here it will help to document relationship with MSR_IA32_PQR_ASSOC.

> + * @closid		: The CLOSID to be configured for PLZA.
> + * @reserved2		: Reserved.
> + * @closid_en		: Associate CLOSID or not.

Same comments as for RMID

> + * @reserved3		: Reserved.
> + * @plza_en		: Configure PLZA or not.

plza_en implies "enable" but the comment mentions "configure". Considering
the other fields are "to be configured" there seems to be relationship but
that is not documented at all. For example, if @plza_en is 1 and resctrl modifies
@rmid should resctrl write "1" to @plza_en again to "configure" the new RMID?

Please add specific detail to help understand how best to interact with this
register. 

> + */
> +union msr_pqr_plza_assoc {
> +	struct {
> +		unsigned long rmid	:12,
> +			      reserved1	:19,
> +			      rmid_en	: 1,
> +			      closid	: 4,
> +			      reserved2	:11,
> +			      closid_en	: 1,
> +			      reserved3	:15,
> +			      plza_en	: 1;
> +	} split;
> +	unsigned long full;
> +};
> +
>  void rdt_ctrl_update(void *arg);
>  
>  int rdt_get_l3_mon_config(struct rdt_resource *r);

Reinette