linux-kernel - Re: [PATCH v7 4/8] perf jevents: Support more event fields

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: <166d2a93-0d86-b106-e996-d74fb4521aa2@arm.com>
Date:   Wed, 23 Aug 2023 10:12:40 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Jing Zhang <renyu.zj@...ux.alibaba.com>,
        John Garry <john.g.garry@...cle.com>,
        Ian Rogers <irogers@...gle.com>
Cc:     Will Deacon <will@...nel.org>, James Clark <james.clark@....com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, linux-doc@...r.kernel.org,
        Zhuo Song <zhuo.song@...ux.alibaba.com>,
        Shuai Xue <xueshuai@...ux.alibaba.com>
Subject: Re: [PATCH v7 4/8] perf jevents: Support more event fields

On 2023-08-21 09:36, Jing Zhang wrote:
> The previous code assumes an event has either an "event=" or "config"
> field at the beginning. For CMN neither of these may be present, as an
> event is typically "type=xx,eventid=xxx".
> 
> If EventCode and ConfigCode is not added in the alias JSON file, the
> event description will add "event=0" by default. So, even if the event
> field is added "eventid=xxx" and "type=xxx", the CMN events final
> parsing result will be "event=0, eventid=xxx, type=xxx".
> 
> Therefore, when EventCode and ConfigCode are missing in JSON, "event=0"
> is no longer added by default. And add EventIdCode and Type to the event
> field.
> 
> I compared pmu_event.c before and after compiling with JEVENT_ARCH=all,
> they are consistent.
> 
> Signed-off-by: Jing Zhang <renyu.zj@...ux.alibaba.com>
> ---
>   tools/perf/pmu-events/jevents.py | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index f57a8f2..369c8bf 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -275,11 +275,14 @@ class JsonEvent:
>         }
>         return table[unit] if unit in table else f'uncore_{unit.lower()}'
>   
> -    eventcode = 0
> +    eventcode = None
>       if 'EventCode' in jd:
>         eventcode = int(jd['EventCode'].split(',', 1)[0], 0)
>       if 'ExtSel' in jd:
> -      eventcode |= int(jd['ExtSel']) << 8
> +      if eventcode is None:
> +        eventcode = int(jd['ExtSel']) << 8
> +      else:
> +        eventcode |= int(jd['ExtSel']) << 8
>       configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None
>       self.name = jd['EventName'].lower() if 'EventName' in jd else None
>       self.topic = ''
> @@ -317,7 +320,11 @@ class JsonEvent:
>       if precise and self.desc and '(Precise Event)' not in self.desc:
>         extra_desc += ' (Must be precise)' if precise == '2' else (' (Precise '
>                                                                    'event)')
> -    event = f'config={llx(configcode)}' if configcode is not None else f'event={llx(eventcode)}'
> +    event = None
> +    if eventcode is not None:
> +      event = f'event={llx(eventcode)}'
> +    elif configcode is not None:
> +      event = f'config={llx(configcode)}'
>       event_fields = [
>           ('AnyThread', 'any='),
>           ('PortMask', 'ch_mask='),
> @@ -327,10 +334,15 @@ class JsonEvent:
>           ('Invert', 'inv='),
>           ('SampleAfterValue', 'period='),
>           ('UMask', 'umask='),
> +        ('NodeType', 'type='),
> +        ('EventIdCode', 'eventid='),

FWIW, this smells like another brewing scalability problem, given that 
these are entirely driver-specific. Not sure off-hand how feasible it 
might be, but my instinct says that a neat solution would be to encode 
them right in the JSON, e.g.:

	"FormatAttr": { "type": 0x5 }

such that jevents should then only really need to consider whether an 
event is defined in terms of a raw "ConfigCode", one or more 
"FormatAttr"s which it can then parse dynamically, or reasonable special 
cases like "EventCode" (given how "event" is one of the most commonly 
used formats).

Thanks,
Robin.

>       ]
>       for key, value in event_fields:
>         if key in jd and jd[key] != '0':
> -        event += ',' + value + jd[key]
> +        if event:
> +          event += ',' + value + jd[key]
> +        else:
> +          event = value + jd[key]
>       if filter:
>         event += f',{filter}'
>       if msr: