linux-kernel - Re: [PATCH v1 24/31] x86/resctrl: Move get_config

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: <3b31aab6-a264-4ccb-aca3-f75be21741b8@arm.com>
Date: Fri, 14 Jun 2024 14:58:14 +0100
From: James Morse <james.morse@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Dave Martin <Dave.Martin@....com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
 Fenghua Yu <fenghua.yu@...el.com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 H Peter Anvin <hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
 shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>
Subject: Re: [PATCH v1 24/31] x86/resctrl: Move get_config_index() to a header

Hi guys,

On 11/04/2024 18:41, Reinette Chatre wrote:
> On 4/11/2024 7:25 AM, Dave Martin wrote:
>> On Mon, Apr 08, 2024 at 08:25:26PM -0700, Reinette Chatre wrote:
>>> On 3/21/2024 9:50 AM, James Morse wrote:
>>>> get_config_index() is used by the architecture specific code to map a
>>>> CLOSID+type pair to an index in the configuration arrays.
>>>>
>>>> MPAM needs to do this too to preserve the ABI to user-space, there is
>>>> no reason to do it differently.
>>>>
>>>> Move the helper to a header file.

>>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>>> index 3de5bc63ace0..73c111963433 100644
>>>> --- a/include/linux/resctrl.h
>>>> +++ b/include/linux/resctrl.h
>>>> @@ -258,6 +258,21 @@ bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>>>>  void resctrl_arch_mon_event_config_write(void *info);
>>>>  void resctrl_arch_mon_event_config_read(void *info);
>>>>  
>>>> +/* For use by arch code to remap resctrl's smaller CDP CLOSID range */
>>>> +static inline u32 resctrl_get_config_index(u32 closid,
>>>> +					   enum resctrl_conf_type type)
>>>> +{
>>>> +	switch (type) {
>>>> +	default:
>>>> +	case CDP_NONE:
>>>> +		return closid;
>>>> +	case CDP_CODE:
>>>> +			return (closid * 2) + 1;
>>>> +	case CDP_DATA:
>>>> +			return (closid * 2);
>>>> +	}
>>>> +}
>>>
>>> (please check the tabs)
>>
>> Noted.  I also see that redundant parentheses seem spuriously added
>> compared with the original version of this moved code.  I can make a
>> note to drop them if you prefer.
>>
>>> This change is unexpected to me. Could you please elaborate how
>>> MPAM's variant of CDP works?
>>>
>>> Thank you very much.
>>>
>>> Reinette
>>
>> Note: I haven't discussed this specifically with James, so the following
>> is my best guess at the rationale...  With that in mind:
>>
>> For MPAM, CDP isn't a special mode; instead, the PARTIDs for
>> instructions and data are always configured independently in the CPU.
>> If resctrl is not configured for CDP, we simply program the same PARTID
>> value both for instructions and data on task switch.
>>
>> For a given resctrl control group we could pick two random unrelated
>> PARTIDs, but there seems to be no advantage in doing that since resctrl
>> enables cdp globally or not, and we would require more effort to
>> translate resctrl closids to PARTIDs if we didn't pair the IDs up
>> systematically.
>>
>> (See [1], [2] in James' snapshot, which illustrate how he proposes
>> to do it.)
>>
>>
>> So, we may as well stick with the same scheme already established for
>> x86: nothing forces us to do that, but it looks simpler than the
>> alternatives.  I think that's the idea, anyway.
>>
>> Then, if the same scheme is used by multiple arches (and 100% of the
>> arches currently known to resctrl), it probably makes sense to share the
>> definition of the mapping at least as a default for arches that don't
>> have their own different ways of doing it.
>>
>> Does this make sense?
> 
> It does, thank you very much.

Thanks for the summary Dave - spot on!

There are some additional headaches for MPAM to provide the counters when CDP emulation is
enabled: because two CLOSID/PARTID are in use, two counters have to be allocated and read,
and their counters summed. This is all done behind the scenes in the MPAM driver.

MBA gets even more fun - as there is no MBA_CODE/MBA_DATA - but whatever MPAM is using to
back the MBA resource sees two CLOSID/PARTID. The result is the MPAM driver has to replay
configuration changes to the second CLOSID/PARTID. Again, this is done behind the scenes
in the MPAM driver.


(and as we're on this topic: I've had requests to make the counters available for code and
data separately - I don't intend to do this in resctrl as it wouldn't be portable to x86.
I'm looking at doing this with the perf pmu stuff)


Thanks,

James