Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH] powerpc/perf: prevent mixed EBB and non-EBB events
From:   Athira Rajeev <atrajeev@linux.vnet.ibm.com>
In-Reply-To: <20210224122116.221120-1-cascardo@canonical.com>
Date:   Fri, 5 Mar 2021 11:20:25 +0530
Cc:     linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <F791C89D-E644-43D3-8229-3618AF7DE2C2@linux.vnet.ibm.com>
References: <20210224122116.221120-1-cascardo@canonical.com>
To:     Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Precedence: bulk


> On 24-Feb-2021, at 5:51 PM, Thadeu Lima de Souza Cascardo =
<cascardo@canonical.com> wrote:
>=20
> EBB events must be under exclusive groups, so there is no mix of EBB =
and
> non-EBB events on the same PMU. This requirement worked fine as perf =
core
> would not allow other pinned events to be scheduled together with =
exclusive
> events.
>=20
> This assumption was broken by commit 1908dc911792 ("perf: Tweak
> perf_event_attr::exclusive semantics").
>=20
> After that, the test cpu_event_pinned_vs_ebb_test started succeeding =
after
> read_events, but worse, the task would not have given access to PMC1, =
so
> when it tried to write to it, it was killed with "illegal =
instruction".
>=20
> Preventing mixed EBB and non-EBB events from being add to the same PMU =
will
> just revert to the previous behavior and the test will succeed.


Hi,

Thanks for checking this. I checked your patch which is fixing =
=E2=80=9Ccheck_excludes=E2=80=9D to make
sure all events must agree on EBB. But in the PMU group constraints, we =
already have check for
EBB events. This is in arch/powerpc/perf/isa207-common.c ( =
isa207_get_constraint function ).

<<>>
mask  |=3D CNST_EBB_VAL(ebb);
value |=3D CNST_EBB_MASK;
<<>>

But the above setting for mask and value is interchanged. We actually =
need to fix here.

Below patch should fix this:

diff --git a/arch/powerpc/perf/isa207-common.c =
b/arch/powerpc/perf/isa207-common.c
index e4f577da33d8..8b5eeb6fb2fb 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -447,8 +447,8 @@ int isa207_get_constraint(u64 event, unsigned long =
*maskp, unsigned long *valp,
         * EBB events are pinned & exclusive, so this should never =
actually
         * hit, but we leave it as a fallback in case.
         */
-       mask  |=3D CNST_EBB_VAL(ebb);
-       value |=3D CNST_EBB_MASK;
+       mask  |=3D CNST_EBB_MASK;
+       value |=3D CNST_EBB_VAL(ebb);
=20
        *maskp =3D mask;
        *valp =3D value;


Can you please try with this patch.

Thanks
Athira


>=20
> Fixes: 1908dc911792 (perf: Tweak perf_event_attr::exclusive semantics)
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
> arch/powerpc/perf/core-book3s.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/powerpc/perf/core-book3s.c =
b/arch/powerpc/perf/core-book3s.c
> index 43599e671d38..d767f7944f85 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1010,9 +1010,25 @@ static int check_excludes(struct perf_event =
**ctrs, unsigned int cflags[],
> 			  int n_prev, int n_new)
> {
> 	int eu =3D 0, ek =3D 0, eh =3D 0;
> +	bool ebb =3D false;
> 	int i, n, first;
> 	struct perf_event *event;
>=20
> +	n =3D n_prev + n_new;
> +	if (n <=3D 1)
> +		return 0;
> +
> +	first =3D 1;
> +	for (i =3D 0; i < n; ++i) {
> +		event =3D ctrs[i];
> +		if (first) {
> +			ebb =3D is_ebb_event(event);
> +			first =3D 0;
> +		} else if (is_ebb_event(event) !=3D ebb) {
> +			return -EAGAIN;
> +		}
> +	}
> +
> 	/*
> 	 * If the PMU we're on supports per event exclude settings then =
we
> 	 * don't need to do any of this logic. NB. This assumes no PMU =
has both
> @@ -1021,10 +1037,6 @@ static int check_excludes(struct perf_event =
**ctrs, unsigned int cflags[],
> 	if (ppmu->flags & PPMU_ARCH_207S)
> 		return 0;
>=20
> -	n =3D n_prev + n_new;
> -	if (n <=3D 1)
> -		return 0;
> -
> 	first =3D 1;
> 	for (i =3D 0; i < n; ++i) {
> 		if (cflags[i] & PPMU_LIMITED_PMC_OK) {
> --=20
> 2.27.0
>=20