[PATCH] perf/amd: Prevent grouping of IBS events

[PATCH] perf/amd: Prevent grouping of IBS events

[Ravi Bangoria]

IBS PMUs can have only one event active at any point in time. Restrict
grouping of multiple IBS events.

Reported-by: Sandipan Das <sandipan.das@amd.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 371014802191..74e664266753 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -247,11 +247,33 @@ int forward_event_to_ibs(struct perf_event *event)
 	return -ENOENT;
 }
 
+/*
+ * Grouping of IBS events is not possible since IBS can have only
+ * one event active at any point in time.
+ */
+static int validate_group(struct perf_event *event)
+{
+	struct perf_event *sibling;
+
+	if (event->group_leader == event)
+		return 0;
+
+	if (event->group_leader->pmu == event->pmu)
+		return -EINVAL;
+
+	for_each_sibling_event(sibling, event->group_leader) {
+		if (sibling->pmu == event->pmu)
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static int perf_ibs_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
 	struct perf_ibs *perf_ibs;
 	u64 max_cnt, config;
+	int ret;
 
 	perf_ibs = get_ibs_pmu(event->attr.type);
 	if (!perf_ibs)
@@ -265,6 +287,10 @@ static int perf_ibs_init(struct perf_event *event)
 	if (config & ~perf_ibs->config_mask)
 		return -EINVAL;
 
+	ret = validate_group(event);
+	if (ret)
+		return ret;
+
 	if (hwc->sample_period) {
 		if (config & perf_ibs->cnt_mask)
 			/* raw max_cnt may not be set */
-- 
2.41.0

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ian Rogers]

On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> IBS PMUs can have only one event active at any point in time. Restrict
> grouping of multiple IBS events.

Thanks Ravi,

can you provide an example/test for this? Should this be a weak group issue?

Thanks,
Ian

> Reported-by: Sandipan Das <sandipan.das@amd.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  arch/x86/events/amd/ibs.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index 371014802191..74e664266753 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -247,11 +247,33 @@ int forward_event_to_ibs(struct perf_event *event)
>         return -ENOENT;
>  }
>
> +/*
> + * Grouping of IBS events is not possible since IBS can have only
> + * one event active at any point in time.
> + */
> +static int validate_group(struct perf_event *event)
> +{
> +       struct perf_event *sibling;
> +
> +       if (event->group_leader == event)
> +               return 0;
> +
> +       if (event->group_leader->pmu == event->pmu)
> +               return -EINVAL;
> +
> +       for_each_sibling_event(sibling, event->group_leader) {
> +               if (sibling->pmu == event->pmu)
> +                       return -EINVAL;
> +       }
> +       return 0;
> +}
> +
>  static int perf_ibs_init(struct perf_event *event)
>  {
>         struct hw_perf_event *hwc = &event->hw;
>         struct perf_ibs *perf_ibs;
>         u64 max_cnt, config;
> +       int ret;
>
>         perf_ibs = get_ibs_pmu(event->attr.type);
>         if (!perf_ibs)
> @@ -265,6 +287,10 @@ static int perf_ibs_init(struct perf_event *event)
>         if (config & ~perf_ibs->config_mask)
>                 return -EINVAL;
>
> +       ret = validate_group(event);
> +       if (ret)
> +               return ret;
> +
>         if (hwc->sample_period) {
>                 if (config & perf_ibs->cnt_mask)
>                         /* raw max_cnt may not be set */
> --
> 2.41.0
>

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ravi Bangoria]

Hi Ian,

On 20-Jun-23 10:14 PM, Ian Rogers wrote:
> On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> IBS PMUs can have only one event active at any point in time. Restrict
>> grouping of multiple IBS events.
> 
> Thanks Ravi,
> 
> can you provide an example/test for this? Should this be a weak group issue?

Before:
  $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
  ^C[ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ]

After:
  $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
  Error:
  AMD IBS may only be available in system-wide/per-cpu mode.
  Try using -a, or -C and workload affinity

The error message is stale and misleading. I have a patch to fix it.
I'll post it separately.

Thanks,
Ravi

[PATCH] perf evsel amd: Fix IBS error message

[Ravi Bangoria]

AMD IBS can do per-process profiling[1] and is no longer restricted to
per-cpu or systemwide only. Remove stale error message.

Before:
  $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
  Error:
  AMD IBS may only be available in system-wide/per-cpu mode.
  Try using -a, or -C and workload affinity

After:
  $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
  Error:
  The sys_perf_event_open() syscall returned with 22 (Invalid
  argument) for event (ibs_op//).
  /bin/dmesg | grep -i perf may provide additional information.

[1] https://git.kernel.org/torvalds/c/30093056f7b2

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 tools/perf/util/evsel.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 356c07f03be6..65b0b70830f0 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
 			return scnprintf(msg, size,
 	"Invalid event (%s) in per-thread mode, enable system wide with '-a'.",
 					evsel__name(evsel));
-		if (is_amd(arch, cpuid)) {
-			if (is_amd_ibs(evsel)) {
-				if (evsel->core.attr.exclude_kernel)
-					return scnprintf(msg, size,
+		if (is_amd(arch, cpuid) && is_amd_ibs(evsel)) {
+			if (evsel->core.attr.exclude_kernel) {
+				return scnprintf(msg, size,
 	"AMD IBS can't exclude kernel events.  Try running at a higher privilege level.");
-				if (!evsel->core.system_wide)
-					return scnprintf(msg, size,
-	"AMD IBS may only be available in system-wide/per-cpu mode.  Try using -a, or -C and workload affinity");
 			}
 		}
 
-- 
2.41.0

Re: [PATCH] perf evsel amd: Fix IBS error message

[Namhyung Kim]

Hi Ravi,

On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> AMD IBS can do per-process profiling[1] and is no longer restricted to
> per-cpu or systemwide only. Remove stale error message.
>
> Before:
>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>   Error:
>   AMD IBS may only be available in system-wide/per-cpu mode.
>   Try using -a, or -C and workload affinity

It was strange that the -C option was given already.

>
> After:
>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>   Error:
>   The sys_perf_event_open() syscall returned with 22 (Invalid
>   argument) for event (ibs_op//).
>   /bin/dmesg | grep -i perf may provide additional information.

It can run newer perf tools on an old kernel but the old error
message seems to be invalid anyway.  So I'm ok with removing it.

>
> [1] https://git.kernel.org/torvalds/c/30093056f7b2
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  tools/perf/util/evsel.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 356c07f03be6..65b0b70830f0 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>                         return scnprintf(msg, size,
>         "Invalid event (%s) in per-thread mode, enable system wide with '-a'.",
>                                         evsel__name(evsel));
> -               if (is_amd(arch, cpuid)) {
> -                       if (is_amd_ibs(evsel)) {
> -                               if (evsel->core.attr.exclude_kernel)
> -                                       return scnprintf(msg, size,
> +               if (is_amd(arch, cpuid) && is_amd_ibs(evsel)) {
> +                       if (evsel->core.attr.exclude_kernel) {
> +                               return scnprintf(msg, size,
>         "AMD IBS can't exclude kernel events.  Try running at a higher privilege level.");

I'm not sure if it's enough.  The IBS PMUs have CAP_NO_EXCLUDE then
it can't exclude user events too, right?

Thanks,
Namhyung


> -                               if (!evsel->core.system_wide)
> -                                       return scnprintf(msg, size,
> -       "AMD IBS may only be available in system-wide/per-cpu mode.  Try using -a, or -C and workload affinity");
>                         }
>                 }
>
> --
> 2.41.0
>

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Namhyung Kim]

On Tue, Jun 20, 2023 at 8:28 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Ian,
>
> On 20-Jun-23 10:14 PM, Ian Rogers wrote:
> > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >>
> >> IBS PMUs can have only one event active at any point in time. Restrict
> >> grouping of multiple IBS events.
> >
> > Thanks Ravi,
> >
> > can you provide an example/test for this? Should this be a weak group issue?
>
> Before:
>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>   ^C[ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ]
>
> After:
>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>   Error:
>   AMD IBS may only be available in system-wide/per-cpu mode.
>   Try using -a, or -C and workload affinity
>
> The error message is stale and misleading. I have a patch to fix it.
> I'll post it separately.

I'm just curious if ibs_fetch and ibs_op can be grouped together.

Thanks,
Namhyung

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ravi Bangoria]

Hi Namhyung,

> I'm just curious if ibs_fetch and ibs_op can be grouped together.

No. Both are independent hardware pmus and thus can not be grouped
together.

Thanks,
Ravi

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ian Rogers]

On Tue, Jun 20, 2023 at 8:27 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Ian,
>
> On 20-Jun-23 10:14 PM, Ian Rogers wrote:
> > On Tue, Jun 20, 2023 at 2:16 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >>
> >> IBS PMUs can have only one event active at any point in time. Restrict
> >> grouping of multiple IBS events.
> >
> > Thanks Ravi,
> >
> > can you provide an example/test for this? Should this be a weak group issue?
>
> Before:
>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>   ^C[ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ]
>
> After:
>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>   Error:
>   AMD IBS may only be available in system-wide/per-cpu mode.
>   Try using -a, or -C and workload affinity
>
> The error message is stale and misleading. I have a patch to fix it.
> I'll post it separately.

Thanks Ravi, so this is a workaround for a PMU driver bug where the
perf_event_open should have failed for the sibling event?

The behavior is somewhat reminiscent of arch_evsel__must_be_in_group:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evsel.c?h=perf-tools-next#n41

Normally software events would be valid in the group, should the code
ignore these?

Thanks,
Ian

> Thanks,
> Ravi

Re: [PATCH] perf evsel amd: Fix IBS error message

[Ravi Bangoria]

Hi Namhyung,

On 22-Jun-23 4:03 AM, Namhyung Kim wrote:
> Hi Ravi,
> 
> On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> AMD IBS can do per-process profiling[1] and is no longer restricted to
>> per-cpu or systemwide only. Remove stale error message.
>>
>> Before:
>>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>>   Error:
>>   AMD IBS may only be available in system-wide/per-cpu mode.
>>   Try using -a, or -C and workload affinity
> 
> It was strange that the -C option was given already.

Hmm. evsel->core.system_wide is bit confusing for me. A Comment on it's
definition says "irrespective of user requested CPUs or threads":

        /*
         * system_wide is for events that need to be on every CPU, irrespective
         * of user requested CPUs or threads. Map propagation will set cpus to
         * this event's own_cpus, whereby they will contribute to evlist
         * all_cpus.
         */
        bool                     system_wide;

So, I guess evsel->core.system_wide doesn't depend on -a / -C.

> 
>>
>> After:
>>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>>   Error:
>>   The sys_perf_event_open() syscall returned with 22 (Invalid
>>   argument) for event (ibs_op//).
>>   /bin/dmesg | grep -i perf may provide additional information.
> 
> It can run newer perf tools on an old kernel but the old error
> message seems to be invalid anyway.  So I'm ok with removing it.

Right.

>>
>> [1] https://git.kernel.org/torvalds/c/30093056f7b2
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
>> ---
>>  tools/perf/util/evsel.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 356c07f03be6..65b0b70830f0 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>>                         return scnprintf(msg, size,
>>         "Invalid event (%s) in per-thread mode, enable system wide with '-a'.",
>>                                         evsel__name(evsel));
>> -               if (is_amd(arch, cpuid)) {
>> -                       if (is_amd_ibs(evsel)) {
>> -                               if (evsel->core.attr.exclude_kernel)
>> -                                       return scnprintf(msg, size,
>> +               if (is_amd(arch, cpuid) && is_amd_ibs(evsel)) {
>> +                       if (evsel->core.attr.exclude_kernel) {
>> +                               return scnprintf(msg, size,
>>         "AMD IBS can't exclude kernel events.  Try running at a higher privilege level.");
> 
> I'm not sure if it's enough.  The IBS PMUs have CAP_NO_EXCLUDE then
> it can't exclude user events too, right?

That's correct. Let me try to fix this whole `if (is_amd())` part properly.

Thanks for your feedback,
Ravi

Re: [PATCH] perf evsel amd: Fix IBS error message

[Namhyung Kim]

On Wed, Jun 21, 2023 at 10:28 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Namhyung,
>
> On 22-Jun-23 4:03 AM, Namhyung Kim wrote:
> > Hi Ravi,
> >
> > On Tue, Jun 20, 2023 at 11:24 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> >>
> >> AMD IBS can do per-process profiling[1] and is no longer restricted to
> >> per-cpu or systemwide only. Remove stale error message.
> >>
> >> Before:
> >>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
> >>   Error:
> >>   AMD IBS may only be available in system-wide/per-cpu mode.
> >>   Try using -a, or -C and workload affinity
> >
> > It was strange that the -C option was given already.
>
> Hmm. evsel->core.system_wide is bit confusing for me. A Comment on it's
> definition says "irrespective of user requested CPUs or threads":
>
>         /*
>          * system_wide is for events that need to be on every CPU, irrespective
>          * of user requested CPUs or threads. Map propagation will set cpus to
>          * this event's own_cpus, whereby they will contribute to evlist
>          * all_cpus.
>          */
>         bool                     system_wide;
>
> So, I guess evsel->core.system_wide doesn't depend on -a / -C.

Right, you shouldn't check this flag.  IIRC It's used by Intel PT
for dummy events to get sideband events from every CPU
regardless of the target.

The proper check would be target__has_cpu() and it seems
the evsel__open_strerror() already checks that.

Thanks,
Namhyung


>
> >
> >>
> >> After:
> >>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
> >>   Error:
> >>   The sys_perf_event_open() syscall returned with 22 (Invalid
> >>   argument) for event (ibs_op//).
> >>   /bin/dmesg | grep -i perf may provide additional information.
> >
> > It can run newer perf tools on an old kernel but the old error
> > message seems to be invalid anyway.  So I'm ok with removing it.
>
> Right.
>
> >>
> >> [1] https://git.kernel.org/torvalds/c/30093056f7b2
> >>
> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> >> ---
> >>  tools/perf/util/evsel.c | 10 +++-------
> >>  1 file changed, 3 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> index 356c07f03be6..65b0b70830f0 100644
> >> --- a/tools/perf/util/evsel.c
> >> +++ b/tools/perf/util/evsel.c
> >> @@ -3092,14 +3092,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
> >>                         return scnprintf(msg, size,
> >>         "Invalid event (%s) in per-thread mode, enable system wide with '-a'.",
> >>                                         evsel__name(evsel));
> >> -               if (is_amd(arch, cpuid)) {
> >> -                       if (is_amd_ibs(evsel)) {
> >> -                               if (evsel->core.attr.exclude_kernel)
> >> -                                       return scnprintf(msg, size,
> >> +               if (is_amd(arch, cpuid) && is_amd_ibs(evsel)) {
> >> +                       if (evsel->core.attr.exclude_kernel) {
> >> +                               return scnprintf(msg, size,
> >>         "AMD IBS can't exclude kernel events.  Try running at a higher privilege level.");
> >
> > I'm not sure if it's enough.  The IBS PMUs have CAP_NO_EXCLUDE then
> > it can't exclude user events too, right?
>
> That's correct. Let me try to fix this whole `if (is_amd())` part properly.
>
> Thanks for your feedback,
> Ravi

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ravi Bangoria]

Hi Ian,

>> Before:
>>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>>   ^C[ perf record: Woken up 1 times to write data ]
>>   [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ]
>>
>> After:
>>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
>>   Error:
>>   AMD IBS may only be available in system-wide/per-cpu mode.
>>   Try using -a, or -C and workload affinity
>>
>> The error message is stale and misleading. I have a patch to fix it.
>> I'll post it separately.
> 
> Thanks Ravi, so this is a workaround for a PMU driver bug where the
> perf_event_open should have failed for the sibling event?

This is not a workaround. This kernel patch fixes PMU driver bug. With
the patch, perf_event_open() will fail for sibling IBS event if either
group leader or any other sibling is of the same IBS pmu. Or did I
misread your comment?

> 
> The behavior is somewhat reminiscent of arch_evsel__must_be_in_group:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evsel.c?h=perf-tools-next#n41
> 
> Normally software events would be valid in the group, should the code
> ignore these?

Grouping of SW and IBS event will continue to work after this patch.

Thanks,
Ravi

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ian Rogers]

On Wed, Jun 21, 2023 at 10:39 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Hi Ian,
>
> >> Before:
> >>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
> >>   ^C[ perf record: Woken up 1 times to write data ]
> >>   [ perf record: Captured and wrote 0.540 MB perf.data (531 samples) ]
> >>
> >> After:
> >>   $ sudo ./perf record -e "{ibs_op//,ibs_op//}" -C 0
> >>   Error:
> >>   AMD IBS may only be available in system-wide/per-cpu mode.
> >>   Try using -a, or -C and workload affinity
> >>
> >> The error message is stale and misleading. I have a patch to fix it.
> >> I'll post it separately.
> >
> > Thanks Ravi, so this is a workaround for a PMU driver bug where the
> > perf_event_open should have failed for the sibling event?
>
> This is not a workaround. This kernel patch fixes PMU driver bug. With
> the patch, perf_event_open() will fail for sibling IBS event if either
> group leader or any other sibling is of the same IBS pmu. Or did I
> misread your comment?
>
> >
> > The behavior is somewhat reminiscent of arch_evsel__must_be_in_group:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evsel.c?h=perf-tools-next#n41
> >
> > Normally software events would be valid in the group, should the code
> > ignore these?
>
> Grouping of SW and IBS event will continue to work after this patch.

Sorry Ravi, I've got my head in the clouds. I was reading this as a
tools patch :-)

Thanks,
Ian

> Thanks,
> Ravi

Re: [PATCH] perf/amd: Prevent grouping of IBS events

[Ravi Bangoria]

> Sorry Ravi, I've got my head in the clouds. I was reading this as a
> tools patch :-)

No worries :). And thanks for taking a look at the patch!

Ravi

[tip: perf/core] perf/amd: Prevent grouping of IBS events

[tip-bot2 for Ravi Bangoria]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     7c2128235eff99b448af8f4b5b2933495bf1a440
Gitweb:        https://git.kernel.org/tip/7c2128235eff99b448af8f4b5b2933495bf1a440
Author:        Ravi Bangoria <ravi.bangoria@amd.com>
AuthorDate:    Tue, 20 Jun 2023 14:46:03 +05:30
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 10 Jul 2023 09:52:34 +02:00

perf/amd: Prevent grouping of IBS events

IBS PMUs can have only one event active at any point in time. Restrict
grouping of multiple IBS events.

Reported-by: Sandipan Das <sandipan.das@amd.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20230620091603.269-1-ravi.bangoria@amd.com
---
 arch/x86/events/amd/ibs.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 3710148..74e6642 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -247,11 +247,33 @@ int forward_event_to_ibs(struct perf_event *event)
 	return -ENOENT;
 }
 
+/*
+ * Grouping of IBS events is not possible since IBS can have only
+ * one event active at any point in time.
+ */
+static int validate_group(struct perf_event *event)
+{
+	struct perf_event *sibling;
+
+	if (event->group_leader == event)
+		return 0;
+
+	if (event->group_leader->pmu == event->pmu)
+		return -EINVAL;
+
+	for_each_sibling_event(sibling, event->group_leader) {
+		if (sibling->pmu == event->pmu)
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static int perf_ibs_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
 	struct perf_ibs *perf_ibs;
 	u64 max_cnt, config;
+	int ret;
 
 	perf_ibs = get_ibs_pmu(event->attr.type);
 	if (!perf_ibs)
@@ -265,6 +287,10 @@ static int perf_ibs_init(struct perf_event *event)
 	if (config & ~perf_ibs->config_mask)
 		return -EINVAL;
 
+	ret = validate_group(event);
+	if (ret)
+		return ret;
+
 	if (hwc->sample_period) {
 		if (config & perf_ibs->cnt_mask)
 			/* raw max_cnt may not be set */