diff mbox series

[1/1] Return EINVAL when precise_ip perf events are requested on Arm

Message ID 20200115105855.13395-2-james.clark@arm.com (mailing list archive)
State New, archived
Headers show
Series Return EINVAL when precise_ip perf events are requested on Arm | expand

Commit Message

James Clark Jan. 15, 2020, 10:58 a.m. UTC
ARM PMU events can be delivered with arbitrary skid, and there's
nothing the kernel can do to prevent this. Given that, the PMU
cannot support precise_ip != 0.

Also update comment to state that attr.config field is used to
set the event type rather than event_id which doesn't exist.

Signed-off-by: James Clark <james.clark@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Tan Xiaojun <tanxiaojun@huawei.com>
Cc: Al Grant <al.grant@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/perf/arm_pmu.c          | 3 +++
 include/uapi/linux/perf_event.h | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Will Deacon Jan. 17, 2020, 12:39 p.m. UTC | #1
Hi James,

On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote:
> ARM PMU events can be delivered with arbitrary skid, and there's
> nothing the kernel can do to prevent this. Given that, the PMU
> cannot support precise_ip != 0.
> 
> Also update comment to state that attr.config field is used to
> set the event type rather than event_id which doesn't exist.

"Also..." is usually a good sign that you should split up the patch. In
this case, you're touching a UAPI header with a questionable clarification,
so I'd definitely rather see that handled separately.

> Signed-off-by: James Clark <james.clark@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Tan Xiaojun <tanxiaojun@huawei.com>
> Cc: Al Grant <al.grant@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/perf/arm_pmu.c          | 3 +++
>  include/uapi/linux/perf_event.h | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index df352b334ea7..4ddbdb93b3b6 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event,
>  	u64 config = event->attr.config;
>  	int type = event->attr.type;
>  
> +	if (event->attr.precise)
> +		return -EINVAL;

You're right that this is a user-visible change, and I'm pretty nervous
about it to be honest with you.

Perhaps a better way would be to expose something under sysfs, a bit like
the caps directory for the SPE PMU, which identifies the fields of the attr
structure that the driver does not ignore. I think doing this as an Arm-PMU
specific thing initially would be fine, but it would be even better to have
something where a driver can tell perf core about the parts it responds to
and have this stuff populated automatically. The current design makes it
inevitable that PMU drivers will have issues like the one you point out in
the cover letter.

Thoughts?

Will
Will Deacon Jan. 17, 2020, 1:05 p.m. UTC | #2
> On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote:
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index df352b334ea7..4ddbdb93b3b6 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event,
> >  	u64 config = event->attr.config;
> >  	int type = event->attr.type;
> >  
> > +	if (event->attr.precise)
> > +		return -EINVAL;

Also, does this field even exist? Guessing you mean 'precise_ip', but
then that means this hasn't even seen a compiler :(

Will
James Clark Jan. 17, 2020, 1:11 p.m. UTC | #3
Hi Will,

Yes you're right, I've somehow managed to post a version of the patch
from before I built and tested it.

I will repost it shortly with the other change split out as well.


Thanks
James 

On 17/01/2020 13:05, Will Deacon wrote:
>> On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote:
>>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>>> index df352b334ea7..4ddbdb93b3b6 100644
>>> --- a/drivers/perf/arm_pmu.c
>>> +++ b/drivers/perf/arm_pmu.c
>>> @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event,
>>>  	u64 config = event->attr.config;
>>>  	int type = event->attr.type;
>>>  
>>> +	if (event->attr.precise)
>>> +		return -EINVAL;
> 
> Also, does this field even exist? Guessing you mean 'precise_ip', but
> then that means this hasn't even seen a compiler :(
> 
> Will
>
Peter Zijlstra Jan. 17, 2020, 2:01 p.m. UTC | #4
On Fri, Jan 17, 2020 at 12:39:21PM +0000, Will Deacon wrote:

> Perhaps a better way would be to expose something under sysfs, a bit like
> the caps directory for the SPE PMU, which identifies the fields of the attr
> structure that the driver does not ignore. I think doing this as an Arm-PMU
> specific thing initially would be fine, but it would be even better to have
> something where a driver can tell perf core about the parts it responds to
> and have this stuff populated automatically. The current design makes it
> inevitable that PMU drivers will have issues like the one you point out in
> the cover letter.
> 
> Thoughts?

We have PERF_PMU_CAP_ flags we could extend to that purpose.
James Clark Jan. 17, 2020, 3 p.m. UTC | #5
Hi Peter,

Do you mean something like this?


diff --git a/kernel/events/core.c b/kernel/events/core.c
index 43d1d4945433..f74acd085bea 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10812,6 +10812,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
                goto err_pmu;
        }
 
+       if (event->attr.precise_ip &&
+               !(pmu->capabilities & PERF_PMU_CAP_PRECISE_IP)) {
+               err = -EOPNOTSUPP;
+               goto err_pmu;
+       }
+
        err = exclusive_event_init(event);
        if (err)
                goto err_pmu;


Or should it only be done via sysfs to not break userspace?

If this was done via sysfs would it be possible to express that some events support
some attributes and others don't? It seems like the "caps" folder couldn't be
that fine grained.

On 17/01/2020 14:01, Peter Zijlstra wrote:
> On Fri, Jan 17, 2020 at 12:39:21PM +0000, Will Deacon wrote:
> 
>> Perhaps a better way would be to expose something under sysfs, a bit like
>> the caps directory for the SPE PMU, which identifies the fields of the attr
>> structure that the driver does not ignore. I think doing this as an Arm-PMU
>> specific thing initially would be fine, but it would be even better to have
>> something where a driver can tell perf core about the parts it responds to
>> and have this stuff populated automatically. The current design makes it
>> inevitable that PMU drivers will have issues like the one you point out in
>> the cover letter.
>>
>> Thoughts?
> 
> We have PERF_PMU_CAP_ flags we could extend to that purpose.
>
Peter Zijlstra Jan. 17, 2020, 3:16 p.m. UTC | #6
On Fri, Jan 17, 2020 at 03:00:37PM +0000, James Clark wrote:
> Hi Peter,
> 
> Do you mean something like this?

Yes.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 43d1d4945433..f74acd085bea 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10812,6 +10812,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>                 goto err_pmu;
>         }
>  
> +       if (event->attr.precise_ip &&
> +               !(pmu->capabilities & PERF_PMU_CAP_PRECISE_IP)) {
> +               err = -EOPNOTSUPP;
> +               goto err_pmu;
> +       }
> +
>         err = exclusive_event_init(event);
>         if (err)
>                 goto err_pmu;
> 
> 
> Or should it only be done via sysfs to not break userspace?

So we've added checks like this in the past and gotten away with it. Do
you already know of some userspace that will break due to it?

An alternative approach is adding a sysctl like kernel.perf_nostrict
which would disable this or something, that way 'old' userspace has a
chicken bit.
kernel test robot Jan. 18, 2020, 2:11 p.m. UTC | #7
Hi James,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on arm-soc/for-next]
[also build test ERROR on tip/perf/core linux/master linus/master v5.5-rc6 next-20200117]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/James-Clark/Return-EINVAL-when-precise_ip-perf-events-are-requested-on-Arm/20200116-195500
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers//perf/arm_pmu.c: In function 'armpmu_map_event':
>> drivers//perf/arm_pmu.c:105:18: error: 'struct perf_event_attr' has no member named 'precise'; did you mean 'precise_ip'?
     if (event->attr.precise)
                     ^~~~~~~
                     precise_ip

vim +105 drivers//perf/arm_pmu.c

    92	
    93	int
    94	armpmu_map_event(struct perf_event *event,
    95			 const unsigned (*event_map)[PERF_COUNT_HW_MAX],
    96			 const unsigned (*cache_map)
    97					[PERF_COUNT_HW_CACHE_MAX]
    98					[PERF_COUNT_HW_CACHE_OP_MAX]
    99					[PERF_COUNT_HW_CACHE_RESULT_MAX],
   100			 u32 raw_event_mask)
   101	{
   102		u64 config = event->attr.config;
   103		int type = event->attr.type;
   104	
 > 105		if (event->attr.precise)
   106			return -EINVAL;
   107	
   108		if (type == event->pmu->type)
   109			return armpmu_map_raw_event(raw_event_mask, config);
   110	
   111		switch (type) {
   112		case PERF_TYPE_HARDWARE:
   113			return armpmu_map_hw_event(event_map, config);
   114		case PERF_TYPE_HW_CACHE:
   115			return armpmu_map_cache_event(cache_map, config);
   116		case PERF_TYPE_RAW:
   117			return armpmu_map_raw_event(raw_event_mask, config);
   118		}
   119	
   120		return -ENOENT;
   121	}
   122	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
Mark Rutland Jan. 20, 2020, 3:33 p.m. UTC | #8
On Fri, Jan 17, 2020 at 04:16:58PM +0100, Peter Zijlstra wrote:
> On Fri, Jan 17, 2020 at 03:00:37PM +0000, James Clark wrote:
> > Hi Peter,
> > 
> > Do you mean something like this?
> 
> Yes.
> 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 43d1d4945433..f74acd085bea 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -10812,6 +10812,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
> >                 goto err_pmu;
> >         }
> >  
> > +       if (event->attr.precise_ip &&
> > +               !(pmu->capabilities & PERF_PMU_CAP_PRECISE_IP)) {
> > +               err = -EOPNOTSUPP;
> > +               goto err_pmu;
> > +       }
> > +
> >         err = exclusive_event_init(event);
> >         if (err)
> >                 goto err_pmu;
> > 
> > 
> > Or should it only be done via sysfs to not break userspace?
> 
> So we've added checks like this in the past and gotten away with it. Do
> you already know of some userspace that will break due to it?
> 
> An alternative approach is adding a sysctl like kernel.perf_nostrict
> which would disable this or something, that way 'old' userspace has a
> chicken bit.

Could we allocate a "strict" bit from perf_event_attr::__reserved_1, and
update drivers to expose a whitelist of fields they support?

Then the core could do something like:

	if (attr->strict && !pmu_check_whitelist(pmu, attr))
		return -EOPNOTSUPP;

... and we could also expose the whitelist somewhere in sysfs.

Thanks,
Mark,
diff mbox series

Patch

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index df352b334ea7..4ddbdb93b3b6 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -102,6 +102,9 @@  armpmu_map_event(struct perf_event *event,
 	u64 config = event->attr.config;
 	int type = event->attr.type;
 
+	if (event->attr.precise)
+		return -EINVAL;
+
 	if (type == event->pmu->type)
 		return armpmu_map_raw_event(raw_event_mask, config);
 
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 377d794d3105..3501b2eb168a 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -38,8 +38,8 @@  enum perf_type_id {
 };
 
 /*
- * Generalized performance event event_id types, used by the
- * attr.event_id parameter of the sys_perf_event_open()
+ * Generalized hardware performance event types, used by the
+ * attr.config parameter of the sys_perf_event_open()
  * syscall:
  */
 enum perf_hw_id {