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

[Moger, Babu]

Hi Reinette,

On 6/11/2026 6:40 PM, Reinette Chatre wrote:
> 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.

Sure. Will rewrite the changelog. And will check other patches also.

> > Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> > ---
>
> > 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.

Sure. Will use the names tony suggested.
https://lore.kernel.org/lkml/SJ1PR11MB6083C069F99FAB8A0BEB8518FC182@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

Also will moving the register "MSR_IA32_PQR_PLZA_ASSOC" together with 
BIT definition. It will break the sorting order. Hope that is not a problem.
> > 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.

Sure. Will re-write this whole comments.

Thanks
Babu