[PATCH 0/2] perf cs-etm: Track exception level fixups

[PATCH 0/2] perf cs-etm: Track exception level fixups

[James Clark]

A couple of changes related to edge cases since commit 8d3031d39fe8
("perf cs-etm: Track exception level").

I think the second one is low risk seeing as any path requiring a thread
leading up to adding to the histogram would already have been crashing.
Maybe the thread check could also be added to hist_entry_iter__add()
although other users of it don't seem to have the same issue, and there
is another use of al.thread above in builtin-report.c so it's probably
ok where I've added it.

Applies to perf-tools-next/perf-tools-next (929ff679b69)

James Clark (2):
  perf cs-etm: Handle per-thread mode on EL1 host kernel case
  perf report: Don't add to histogram when there is no thread found

 tools/perf/builtin-report.c |  3 +++
 tools/perf/util/cs-etm.c    | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)


base-commit: 929ff679b694f0f9656aec38b3a7d5c440c5ca24
-- 
2.34.1

[PATCH 1/2] perf cs-etm: Handle per-thread mode on EL1 host kernel case

[James Clark]

In per-thread mode there are no context packets so no way to determine
which type of context packets exist. But because it's only possible to
trace host processes in per-thread mode without context packets then
assume host in this case.

This fixes the per-thread test case failures when running on nVHE:

  98: Check Arm CoreSight trace data recording and synthesized samples:
  --- start ---
  ...
  Recording trace with '-e cs_etm/timestamp=0/ --per-thread'
  Looking at perf.data file for dumping branch samples:
  CoreSight basic testing with '-e cs_etm/timestamp=0/ --per-thread': FAIL
  Recording trace with '-e cs_etm/timestamp=1/ --per-thread'
  Looking at perf.data file for dumping branch samples:
  CoreSight basic testing with '-e cs_etm/timestamp=1/ --per-thread': FAIL
  ...

Fixes: 8d3031d39fe8 ("perf cs-etm: Track exception level")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/cs-etm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 1419b40dfbe8..85821cc5650e 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -900,10 +900,17 @@ static struct machine *cs_etm__get_machine(struct cs_etm_queue *etmq,
 
 	/*
 	 * For any virtualisation based on nVHE (e.g. pKVM), or host kernels
-	 * running at EL1 assume everything is the host.
+	 * running at EL1, or no context IDs (per-thread mode) assume everything
+	 * is the host.
 	 */
-	if (pid_fmt == CS_ETM_PIDFMT_CTXTID)
+	switch (pid_fmt) {
+	case CS_ETM_PIDFMT_CTXTID:
+	case CS_ETM_PIDFMT_NONE:
 		return &etmq->etm->session->machines.host;
+	case CS_ETM_PIDFMT_CTXTID2:
+	default:
+		break;
+	}
 
 	/*
 	 * Not perfect, but otherwise assume anything in EL1 is the default
-- 
2.34.1

[PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[James Clark]

thread__find_map() chooses to exit without assigning a thread to the
addr_location in some scenarios, for example when there are samples from
a guest and perf_guest == false. This results in a segfault when adding
to the histogram because it uses unguarded accesses to the thread member
of the addr_location.

Fix it by exiting early if no thread is set. This fixes the referenced
commit when using perf report with Coresight but probably isn't
exclusive to that case.

Fixes: 8d3031d39fe8 ("perf cs-etm: Track exception level")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/builtin-report.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index dcedfe00f04d..1a2caa4ce5c3 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -293,6 +293,9 @@ static int process_sample_event(struct perf_tool *tool,
 		goto out_put;
 	}
 
+	if (!al.thread)
+		goto out_put;
+
 	if (rep->stitch_lbr)
 		thread__set_lbr_stitch_enable(al.thread, true);
 
-- 
2.34.1

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[Namhyung Kim]

On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> thread__find_map() chooses to exit without assigning a thread to the
> addr_location in some scenarios, for example when there are samples from
> a guest and perf_guest == false. This results in a segfault when adding
> to the histogram because it uses unguarded accesses to the thread member
> of the addr_location.

Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
init/exit/copy functions") that introduced the change, I'm not sure if
it's the intend behavior.

It might change maps and map, but not thread.  Then I think no reason
to not set the al->thread at the beginning.

How about this?  Ian?
(I guess we can get rid of the duplicate 'al->map = NULL' part)

Thanks,
Namhyung


---8<---
 
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 3860b0c74829..4cbb092e0684 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -581,15 +581,14 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 	maps__zput(al->maps);
 	map__zput(al->map);
 	thread__zput(al->thread);
+	al->thread = thread__get(thread);
 
 	al->addr = addr;
 	al->cpumode = cpumode;
 	al->filtered = 0;
 
-	if (machine == NULL) {
-		al->map = NULL;
+	if (machine == NULL)
 		return NULL;
-	}
 
 	if (cpumode == PERF_RECORD_MISC_KERNEL && perf_host) {
 		al->level = 'k';
@@ -605,7 +604,6 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 		al->level = 'u';
 	} else {
 		al->level = 'H';
-		al->map = NULL;
 
 		if ((cpumode == PERF_RECORD_MISC_GUEST_USER ||
 			cpumode == PERF_RECORD_MISC_GUEST_KERNEL) &&
@@ -619,7 +617,6 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 		return NULL;
 	}
 	al->maps = maps__get(maps);
-	al->thread = thread__get(thread);
 	al->map = map__get(maps__find(maps, al->addr));
 	if (al->map != NULL) {
 		/*

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[Ian Rogers]

On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> > thread__find_map() chooses to exit without assigning a thread to the
> > addr_location in some scenarios, for example when there are samples from
> > a guest and perf_guest == false. This results in a segfault when adding
> > to the histogram because it uses unguarded accesses to the thread member
> > of the addr_location.
>
> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
> init/exit/copy functions") that introduced the change, I'm not sure if
> it's the intend behavior.
>
> It might change maps and map, but not thread.  Then I think no reason
> to not set the al->thread at the beginning.
>
> How about this?  Ian?
> (I guess we can get rid of the duplicate 'al->map = NULL' part)

It seemed strange that we were failing to find a map (the function's
purpose) but then populating the address_location. The change below
brings back that somewhat odd behavior. I'm okay with reverting to the
old behavior, clearly there were users relying on it. We should
probably also copy maps and not just thread, as that was the previous
behavior.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> ---8<---
>
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 3860b0c74829..4cbb092e0684 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -581,15 +581,14 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>         maps__zput(al->maps);
>         map__zput(al->map);
>         thread__zput(al->thread);
> +       al->thread = thread__get(thread);
>
>         al->addr = addr;
>         al->cpumode = cpumode;
>         al->filtered = 0;
>
> -       if (machine == NULL) {
> -               al->map = NULL;
> +       if (machine == NULL)
>                 return NULL;
> -       }
>
>         if (cpumode == PERF_RECORD_MISC_KERNEL && perf_host) {
>                 al->level = 'k';
> @@ -605,7 +604,6 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>                 al->level = 'u';
>         } else {
>                 al->level = 'H';
> -               al->map = NULL;
>
>                 if ((cpumode == PERF_RECORD_MISC_GUEST_USER ||
>                         cpumode == PERF_RECORD_MISC_GUEST_KERNEL) &&
> @@ -619,7 +617,6 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>                 return NULL;
>         }
>         al->maps = maps__get(maps);
> -       al->thread = thread__get(thread);
>         al->map = map__get(maps__find(maps, al->addr));
>         if (al->map != NULL) {
>                 /*

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[Namhyung Kim]

On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> > > thread__find_map() chooses to exit without assigning a thread to the
> > > addr_location in some scenarios, for example when there are samples from
> > > a guest and perf_guest == false. This results in a segfault when adding
> > > to the histogram because it uses unguarded accesses to the thread member
> > > of the addr_location.
> >
> > Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
> > init/exit/copy functions") that introduced the change, I'm not sure if
> > it's the intend behavior.
> >
> > It might change maps and map, but not thread.  Then I think no reason
> > to not set the al->thread at the beginning.
> >
> > How about this?  Ian?
> > (I guess we can get rid of the duplicate 'al->map = NULL' part)
>
> It seemed strange that we were failing to find a map (the function's
> purpose) but then populating the address_location. The change below
> brings back that somewhat odd behavior. I'm okay with reverting to the
> old behavior, clearly there were users relying on it. We should
> probably also copy maps and not just thread, as that was the previous
> behavior.

Probably.  But it used to support samples without maps and I think
that's why it ignores the return value of thread__find_map().  So
we can expect al.map is NULL and maybe fine to leave it for now.

As machine__resolve() returns -1 if it gets no thread, we should set
al.thread when it returns 0.

Can I get your Acked-by?

Thanks,
Namhyung

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[Ian Rogers]

On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> > > > thread__find_map() chooses to exit without assigning a thread to the
> > > > addr_location in some scenarios, for example when there are samples from
> > > > a guest and perf_guest == false. This results in a segfault when adding
> > > > to the histogram because it uses unguarded accesses to the thread member
> > > > of the addr_location.
> > >
> > > Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
> > > init/exit/copy functions") that introduced the change, I'm not sure if
> > > it's the intend behavior.
> > >
> > > It might change maps and map, but not thread.  Then I think no reason
> > > to not set the al->thread at the beginning.
> > >
> > > How about this?  Ian?
> > > (I guess we can get rid of the duplicate 'al->map = NULL' part)
> >
> > It seemed strange that we were failing to find a map (the function's
> > purpose) but then populating the address_location. The change below
> > brings back that somewhat odd behavior. I'm okay with reverting to the
> > old behavior, clearly there were users relying on it. We should
> > probably also copy maps and not just thread, as that was the previous
> > behavior.
>
> Probably.  But it used to support samples without maps and I think
> that's why it ignores the return value of thread__find_map().  So
> we can expect al.map is NULL and maybe fine to leave it for now.
>
> As machine__resolve() returns -1 if it gets no thread, we should set
> al.thread when it returns 0.
>
> Can I get your Acked-by?

Yep:
Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Thanks,
> Namhyung

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[James Clark]

On 27/06/2023 18:19, Ian Rogers wrote:
> On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote:
>>>
>>> On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>>>
>>>> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
>>>>> thread__find_map() chooses to exit without assigning a thread to the
>>>>> addr_location in some scenarios, for example when there are samples from
>>>>> a guest and perf_guest == false. This results in a segfault when adding
>>>>> to the histogram because it uses unguarded accesses to the thread member
>>>>> of the addr_location.
>>>>
>>>> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
>>>> init/exit/copy functions") that introduced the change, I'm not sure if
>>>> it's the intend behavior.
>>>>
>>>> It might change maps and map, but not thread.  Then I think no reason
>>>> to not set the al->thread at the beginning.
>>>>
>>>> How about this?  Ian?
>>>> (I guess we can get rid of the duplicate 'al->map = NULL' part)
>>>
>>> It seemed strange that we were failing to find a map (the function's
>>> purpose) but then populating the address_location. The change below
>>> brings back that somewhat odd behavior. I'm okay with reverting to the
>>> old behavior, clearly there were users relying on it. We should
>>> probably also copy maps and not just thread, as that was the previous
>>> behavior.
>>
>> Probably.  But it used to support samples without maps and I think
>> that's why it ignores the return value of thread__find_map().  So
>> we can expect al.map is NULL and maybe fine to leave it for now.
>>
>> As machine__resolve() returns -1 if it gets no thread, we should set
>> al.thread when it returns 0.
>>
>> Can I get your Acked-by?
> 
> Yep:
> Acked-by: Ian Rogers <irogers@google.com>

Looks good to me too. Should I resend the set with this change instead
of my one?

> 
> Thanks,
> Ian
> 
>> Thanks,
>> Namhyung

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[Namhyung Kim]

On Wed, Jun 28, 2023 at 3:34 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 27/06/2023 18:19, Ian Rogers wrote:
> > On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >>
> >> On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote:
> >>>
> >>> On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >>>>
> >>>> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> >>>>> thread__find_map() chooses to exit without assigning a thread to the
> >>>>> addr_location in some scenarios, for example when there are samples from
> >>>>> a guest and perf_guest == false. This results in a segfault when adding
> >>>>> to the histogram because it uses unguarded accesses to the thread member
> >>>>> of the addr_location.
> >>>>
> >>>> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
> >>>> init/exit/copy functions") that introduced the change, I'm not sure if
> >>>> it's the intend behavior.
> >>>>
> >>>> It might change maps and map, but not thread.  Then I think no reason
> >>>> to not set the al->thread at the beginning.
> >>>>
> >>>> How about this?  Ian?
> >>>> (I guess we can get rid of the duplicate 'al->map = NULL' part)
> >>>
> >>> It seemed strange that we were failing to find a map (the function's
> >>> purpose) but then populating the address_location. The change below
> >>> brings back that somewhat odd behavior. I'm okay with reverting to the
> >>> old behavior, clearly there were users relying on it. We should
> >>> probably also copy maps and not just thread, as that was the previous
> >>> behavior.
> >>
> >> Probably.  But it used to support samples without maps and I think
> >> that's why it ignores the return value of thread__find_map().  So
> >> we can expect al.map is NULL and maybe fine to leave it for now.
> >>
> >> As machine__resolve() returns -1 if it gets no thread, we should set
> >> al.thread when it returns 0.
> >>
> >> Can I get your Acked-by?
> >
> > Yep:
> > Acked-by: Ian Rogers <irogers@google.com>
>
> Looks good to me too. Should I resend the set with this change instead
> of my one?

No, I can take care of that.  I'll take this as your Acked-by. :)

Thanks,
Namhyung

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[Namhyung Kim]

On Wed, Jun 28, 2023 at 1:06 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Jun 28, 2023 at 3:34 AM James Clark <james.clark@arm.com> wrote:
> >
> >
> >
> > On 27/06/2023 18:19, Ian Rogers wrote:
> > > On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >>
> > >> On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote:
> > >>>
> > >>> On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >>>>
> > >>>> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
> > >>>>> thread__find_map() chooses to exit without assigning a thread to the
> > >>>>> addr_location in some scenarios, for example when there are samples from
> > >>>>> a guest and perf_guest == false. This results in a segfault when adding
> > >>>>> to the histogram because it uses unguarded accesses to the thread member
> > >>>>> of the addr_location.
> > >>>>
> > >>>> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
> > >>>> init/exit/copy functions") that introduced the change, I'm not sure if
> > >>>> it's the intend behavior.
> > >>>>
> > >>>> It might change maps and map, but not thread.  Then I think no reason
> > >>>> to not set the al->thread at the beginning.
> > >>>>
> > >>>> How about this?  Ian?
> > >>>> (I guess we can get rid of the duplicate 'al->map = NULL' part)
> > >>>
> > >>> It seemed strange that we were failing to find a map (the function's
> > >>> purpose) but then populating the address_location. The change below
> > >>> brings back that somewhat odd behavior. I'm okay with reverting to the
> > >>> old behavior, clearly there were users relying on it. We should
> > >>> probably also copy maps and not just thread, as that was the previous
> > >>> behavior.
> > >>
> > >> Probably.  But it used to support samples without maps and I think
> > >> that's why it ignores the return value of thread__find_map().  So
> > >> we can expect al.map is NULL and maybe fine to leave it for now.
> > >>
> > >> As machine__resolve() returns -1 if it gets no thread, we should set
> > >> al.thread when it returns 0.
> > >>
> > >> Can I get your Acked-by?
> > >
> > > Yep:
> > > Acked-by: Ian Rogers <irogers@google.com>
> >
> > Looks good to me too. Should I resend the set with this change instead
> > of my one?
>
> No, I can take care of that.  I'll take this as your Acked-by. :)

This part is applied to perf-tools-next, thanks!

Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found

[James Clark]

On 30/06/2023 22:02, Namhyung Kim wrote:
> On Wed, Jun 28, 2023 at 1:06 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> On Wed, Jun 28, 2023 at 3:34 AM James Clark <james.clark@arm.com> wrote:
>>>
>>>
>>>
>>> On 27/06/2023 18:19, Ian Rogers wrote:
>>>> On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim <namhyung@kernel.org> wrote:
>>>>>
>>>>> On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote:
>>>>>>
>>>>>> On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>>>>>>
>>>>>>> On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote:
>>>>>>>> thread__find_map() chooses to exit without assigning a thread to the
>>>>>>>> addr_location in some scenarios, for example when there are samples from
>>>>>>>> a guest and perf_guest == false. This results in a segfault when adding
>>>>>>>> to the histogram because it uses unguarded accesses to the thread member
>>>>>>>> of the addr_location.
>>>>>>>
>>>>>>> Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add
>>>>>>> init/exit/copy functions") that introduced the change, I'm not sure if
>>>>>>> it's the intend behavior.
>>>>>>>
>>>>>>> It might change maps and map, but not thread.  Then I think no reason
>>>>>>> to not set the al->thread at the beginning.
>>>>>>>
>>>>>>> How about this?  Ian?
>>>>>>> (I guess we can get rid of the duplicate 'al->map = NULL' part)
>>>>>>
>>>>>> It seemed strange that we were failing to find a map (the function's
>>>>>> purpose) but then populating the address_location. The change below
>>>>>> brings back that somewhat odd behavior. I'm okay with reverting to the
>>>>>> old behavior, clearly there were users relying on it. We should
>>>>>> probably also copy maps and not just thread, as that was the previous
>>>>>> behavior.
>>>>>
>>>>> Probably.  But it used to support samples without maps and I think
>>>>> that's why it ignores the return value of thread__find_map().  So
>>>>> we can expect al.map is NULL and maybe fine to leave it for now.
>>>>>
>>>>> As machine__resolve() returns -1 if it gets no thread, we should set
>>>>> al.thread when it returns 0.
>>>>>
>>>>> Can I get your Acked-by?
>>>>
>>>> Yep:
>>>> Acked-by: Ian Rogers <irogers@google.com>
>>>
>>> Looks good to me too. Should I resend the set with this change instead
>>> of my one?
>>
>> No, I can take care of that.  I'll take this as your Acked-by. :)
> 
> This part is applied to perf-tools-next, thanks!

Thanks Namhyung