Re: [PATCH] powerpc/perf: prevent mixed EBB and non-EBB events

[Athira Rajeev]

> On 05-Mar-2021, at 11:20 AM, Athira Rajeev <atrajeev@xxxxxxxxxxxxxxxxxx> wrote:
> 
> 
> 
>> On 24-Feb-2021, at 5:51 PM, Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxx> wrote:
>> 
>> 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.
>> 
>> This assumption was broken by commit 1908dc911792 ("perf: Tweak
>> perf_event_attr::exclusive semantics").
>> 
>> 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".
>> 
>> 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 “check_excludes” 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  |= CNST_EBB_VAL(ebb);
> value |= CNST_EBB_MASK;
> <<>>
> 
> But the above setting for mask and value is interchanged. We actually need to fix here.
> 

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=237669

Thanks
Athira

> 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  |= CNST_EBB_VAL(ebb);
> -       value |= CNST_EBB_MASK;
> +       mask  |= CNST_EBB_MASK;
> +       value |= CNST_EBB_VAL(ebb);
> 
>        *maskp = mask;
>        *valp = value;
> 
> 
> Can you please try with this patch.
> 
> Thanks
> Athira
> 
> 
>> 
>> Fixes: 1908dc911792 (perf: Tweak perf_event_attr::exclusive semantics)
>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxx>
>> ---
>> arch/powerpc/perf/core-book3s.c | 20 ++++++++++++++++----
>> 1 file changed, 16 insertions(+), 4 deletions(-)
>> 
>> 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 = 0, ek = 0, eh = 0;
>> +	bool ebb = false;
>> 	int i, n, first;
>> 	struct perf_event *event;
>> 
>> +	n = n_prev + n_new;
>> +	if (n <= 1)
>> +		return 0;
>> +
>> +	first = 1;
>> +	for (i = 0; i < n; ++i) {
>> +		event = ctrs[i];
>> +		if (first) {
>> +			ebb = is_ebb_event(event);
>> +			first = 0;
>> +		} else if (is_ebb_event(event) != 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;
>> 
>> -	n = n_prev + n_new;
>> -	if (n <= 1)
>> -		return 0;
>> -
>> 	first = 1;
>> 	for (i = 0; i < n; ++i) {
>> 		if (cflags[i] & PPMU_LIMITED_PMC_OK) {
>> -- 
>> 2.27.0