Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 5/5] arm_mpam: Adapting the closid/rmid matching determination functions

[Zeng Heng]

On 2024/12/13 0:19, Dave Martin wrote:
> Hi,
>
> On Sat, Dec 07, 2024 at 05:21:36PM +0800, Zeng Heng wrote:
> > According to the previous patches, add the inverse functions for the
> > closid/rmid conversion functions to serve as the conversion functions for
> > reqpartid/pmg. And adapt the matching determination functions
> > resctrl_arch_match_closid() and resctrl_arch_match_rmid() by the inverse
> > functions.
> >
> > For the same reason, when updating the (req)PARTID/PMG pair for a task,
> > the new conversion functions also are used for adaptation.
> >
> > Signed-off-by: Zeng Heng <zengheng4@xxxxxxxxxx>
> > ---
> >   arch/arm64/include/asm/mpam.h              |  6 ++-
> >   drivers/platform/arm64/mpam/mpam_resctrl.c | 63 +++++++++++++++-------
> >   2 files changed, 47 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/mpam.h
> > index e5f385767174..9fc095530671 100644
> > --- a/arch/arm64/include/asm/mpam.h
> > +++ b/arch/arm64/include/asm/mpam.h
> > @@ -93,6 +93,8 @@ static inline u64 mpam_get_regval(struct task_struct *tsk)
> >   #endif
> >   }
> >   
> > +u32 rmid2pmg(u32 rmid);
> > +
> >   static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
> >   {
> >   #ifdef CONFIG_ARM64_MPAM
> > @@ -100,8 +102,8 @@ static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
> >   
> >   	regval &= ~MPAM1_EL1_PMG_D;
> >   	regval &= ~MPAM1_EL1_PMG_I;
> > -	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid);
> > -	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid);
> > +	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid2pmg(rmid));
> > +	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid2pmg(rmid));
>
> Note, this function does not seem to be used; I added a patch in my
> series [1] to get rid of it instead of converting it.

At that time, I was still hesitating whether to directly delete that 
function. Yes, I will skip the adaptation of this function.

> >   
> >   	WRITE_ONCE(task_thread_info(tsk)->mpam_partid_pmg, regval);
> >   #endif
> > diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
>
> [...]
>
> >   void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid, u32 rmid)
> >   {
> > -	BUG_ON(closid > U16_MAX);
> > -	BUG_ON(rmid > U8_MAX);
> > +	u32 reqpartid = closid_rmid2reqpartid(closid, rmid);
> > +	u32 pmg = rmid2pmg(rmid);
> > +	u32 partid_d, partid_i;
> > +
> > +	BUG_ON(reqpartid > U16_MAX);
> > +	BUG_ON(pmg > U8_MAX);
> >   
> >   	if (!cdp_enabled) {
> > -		mpam_set_cpu_defaults(cpu, closid, closid, rmid, rmid);
> > +		mpam_set_cpu_defaults(cpu, reqpartid, reqpartid, pmg, pmg);
> >   	} else {
> >   		/*
> >   		 * When CDP is enabled, resctrl halves the closid range and we
> >   		 * use odd/even partid for one closid.
> >   		 */
> > -		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
> > -		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
> > +		partid_d = resctrl_get_config_index(reqpartid, CDP_DATA);
> > +		partid_i = resctrl_get_config_index(reqpartid, CDP_CODE);
> >   
> > -		mpam_set_cpu_defaults(cpu, partid_d, partid_i, rmid, rmid);
> > +		mpam_set_cpu_defaults(cpu, partid_d, partid_i, pmg, pmg);
>
> Prior to this patch, will the PARTID and/or PMG programmed for a
> control group be different from the PARTID and/or PMG used to program
> the MSCs?
>
> If so, those changes probably need to be in the same patch.

Thanks to point it out. It's reasonable to put setting PARTID/PMG 
operations together with the closid/rmid pair remapping.

> > @@ -289,41 +307,46 @@ void resctrl_arch_sync_cpu_closid_rmid(void *info)
>
> [...]
>
> >   /* The task's pmg is not unique, the partid must be considered too */
> >   bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
> >   {
> >   	u64 regval = mpam_get_regval(tsk);
> > -	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
> > -	u32 tsk_rmid = FIELD_GET(MPAM1_EL1_PMG_D, regval);
> > +	u32 tsk_pmg = FIELD_GET(MPAM1_EL1_PMG_D, regval);
> > +	u32 tsk_partid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
> >   
> >   	if (cdp_enabled)
> > -		tsk_closid >>= 1;
> > +		tsk_partid >>= 1;
> >   
> > -	return (tsk_closid == closid) && (tsk_rmid == rmid);
> > +	return (reqpartid2closid(tsk_partid) == closid) &&
> > +	       (reqpartid_pmg2rmid(tsk_partid, tsk_pmg) == rmid);
>
> Do we actually need the reverse mappings here?
>
> It doesn't really matter which ID namespace is used for the
> comparison, so in my version of this I converted the passed-in closid
> and rmid to PARTID / PMG form and then compared those with tsk's
> values.
>
> (But if I've missed some subtlety here, please let me know!)

Yes, we actually don't need the reverse mappings, and reuse the
closid_rmid2reqpartid() and rmid2pmg() to finish the comparison.

Best regards,
Zeng Heng