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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b6fe1fdb-5c28-9a2e-c000-12b26a6222b7@huawei.com>
Date: Fri, 20 Dec 2024 15:45:36 +0800
From: Zeng Heng <zengheng4@...wei.com>
To: Dave Martin <Dave.Martin@....com>
CC: <james.morse@....com>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <jonathan.cameron@...wei.com>,
	<xiexiuqi@...wei.com>, "Wangshaobo (bobo)" <bobo.shaobowang@...wei.com>
Subject: Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 5/5] arm_mpam:
 Adapting the closid/rmid matching determination functions



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@...wei.com>
>> ---
>>   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