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
Subject: Re: [PATCH] powerpc/perf: prevent mixed EBB and non-EBB events
From:   Athira Rajeev <atrajeev@linux.vnet.ibm.com>
In-Reply-To: <F791C89D-E644-43D3-8229-3618AF7DE2C2@linux.vnet.ibm.com>
Date:   Tue, 6 Apr 2021 21:51:21 +0530
Cc:     Peter Zijlstra <peterz@infradead.org>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        linux-kernel@vger.kernel.org
Message-Id: <0D709DAB-1C5A-4526-9BAB-11BE27E8DBCB@linux.vnet.ibm.com>
References: <20210224122116.221120-1-cascardo@canonical.com>
 <F791C89D-E644-43D3-8229-3618AF7DE2C2@linux.vnet.ibm.com>
To:     Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk


> On 05-Mar-2021, at 11:20 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> =
wrote:
>=20
>=20
>=20
>> On 24-Feb-2021, at 5:51 PM, Thadeu Lima de Souza Cascardo <cascardo@cano=
nical.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 exclus=
ive
>> 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 aft=
er
>> 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 w=
ill
>> just revert to the previous behavior and the test will succeed.
>=20
>=20
> Hi,
>=20
> Thanks for checking this. I checked your patch which is fixing =E2=80=9Cc=
heck_excludes=E2=80=9D to make
> sure all events must agree on EBB. But in the PMU group constraints, we a=
lready have check for
> EBB events. This is in arch/powerpc/perf/isa207-common.c ( isa207_get_con=
straint function ).
>=20
> <<>>
> mask  |=3D CNST_EBB_VAL(ebb);
> value |=3D CNST_EBB_MASK;
> <<>>
>=20
> But the above setting for mask and value is interchanged. We actually nee=
d to fix here.
>=20

Hi,

I have sent a patch for fixing this EBB mask/value setting.
This is the link to patch:

powerpc/perf: Fix PMU constraint check for EBB events
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=3D237669

Thanks
Athira

> Below patch should fix this:
>=20
> 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 *m=
askp, 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;
>=20
>=20
> Can you please try with this patch.
>=20
> Thanks
> Athira
>=20
>=20
>>=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-bo=
ok3s.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 **ctr=
s, 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 **ctr=
s, 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