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: <edd08294-df5a-a297-31c8-7b1934c374a0@os.amperecomputing.com>
Date: Fri, 23 Aug 2024 16:58:07 -0700 (PDT)
From: Ilkka Koskinen <ilkka@...amperecomputing.com>
To: Robin Murphy <robin.murphy@....com>
cc: Mark Rutland <mark.rutland@....com>, will@...nel.org, 
    linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
    ilkka@...amperecomputing.com
Subject: Re: [PATCH 3/8] perf/arm-cmn: Ensure dtm_idx is big enough



On Mon, 19 Aug 2024, Robin Murphy wrote:

> On 16/08/2024 11:14 am, Mark Rutland wrote:
>> On Fri, Aug 09, 2024 at 08:15:42PM +0100, Robin Murphy wrote:
>>> While CMN_MAX_DIMENSION was bumped to 12 for CMN-650, that only supports
>>> up to a 10x10 mesh, so bumping dtm_idx to 256 bits at the time worked
>>> out OK in practice. However CMN-700 did finally support up to 144 XPs,
>>> and thus needs a worst-case 288 bits of dtm_idx for an aggregated XP
>>> event on a maxed-out config. Oops.
>>> 
>>> Fixes: 23760a014417 ("perf/arm-cmn: Add CMN-700 support")
>>> Signed-off-by: Robin Murphy <robin.murphy@....com>
>>> ---
>>>   drivers/perf/arm-cmn.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>>> index 0e2e12e2f4fb..c9a2b21a7aec 100644
>>> --- a/drivers/perf/arm-cmn.c
>>> +++ b/drivers/perf/arm-cmn.c
>>> @@ -563,7 +563,7 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, 
>>> int id) {}
>>>     struct arm_cmn_hw_event {
>>>   	struct arm_cmn_node *dn;
>>> -	u64 dtm_idx[4];
>>> +	u64 dtm_idx[5];
>> 
>> Can't we size this based on CMN_MAX_DIMENSION (or CMN_MAX_XPS or
>> CMN_MAX_DTMS), to make that clear?
>
> Fair enough, I did go back and forth a little on that idea, but reached the 
> opposite conclusion that documenting this with the assert to deliberately 
> make it *not* look straightforward was nicer than wrestling with an accurate 
> name for the logical quantity here, which strictly would be something like 
> CMN_MAX_NODES_PER_TYPE_WE_CARE_ABOUT (there can already be up to 256 RN-Fs, 
> but those aren't visible to the PMU).
>
> I'll have another think on that approach - I do concur that the assert isn't 
> *functionally* any better than automatically sizing the array.

I'm ok with the patch but automatic sizing would be nice as there would be 
one less thing to verify when the mesh size is increased again.

Cheers, Ilkka

>
> Thanks,
> Robin.
>
>>  From the desciription in the commit message it sounds like you need 2 *
>> CMN_MAX_XPS bits, i.e.
>>
>> 	#define DTM_IDX_BITS	(2 * CMN_MAX_XPS)
>>
>> 	u64 dtm_idx[DIV_ROUND_UP(DTM_IDX_BITS, 64)];
>> 
>> Mark.
>>
>>>   	s8 dtc_idx[CMN_MAX_DTCS];
>>>   	u8 num_dns;
>>>   	u8 dtm_offset;
>>> -- 
>>> 2.39.2.101.g768bb238c484.dirty
>>> 
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ