linux-kernel - Re: [PATCH 03/10] coresight: etm-perf: configuring filters from perf core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANLsYkxYhx3afo_ayHDP_r3kAmmMGy_KpuWxbyrsUGuoo4m3xA@mail.gmail.com>
Date:	Thu, 21 Jul 2016 11:12:32 -0600
From:	Mathieu Poirier <mathieu.poirier@...aro.org>
To:	Suzuki K Poulose <Suzuki.Poulose@....com>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: Re: [PATCH 03/10] coresight: etm-perf: configuring filters from perf core

On 21 July 2016 at 09:15, Mathieu Poirier <mathieu.poirier@...aro.org> wrote:
> On 20 July 2016 at 10:07, Suzuki K Poulose <Suzuki.Poulose@....com> wrote:
>> On 18/07/16 20:51, Mathieu Poirier wrote:
>>>
>>> This patch implements the required API needed to access
>>> and retrieve range and start/stop filters from the perf core.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
>>> ---
>>>  drivers/hwtracing/coresight/coresight-etm-perf.c | 146
>>> ++++++++++++++++++++---
>>>  drivers/hwtracing/coresight/coresight-etm-perf.h |  32 +++++
>>>  2 files changed, 162 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> index 78a1bc0013a2..fde7f42149c5 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> @@ -29,6 +29,7 @@
>>>  #include <linux/workqueue.h>
>>>
>>>  #include "coresight-priv.h"
>>> +#include "coresight-etm-perf.h"
>>>
>>>  static struct pmu etm_pmu;
>>>  static bool etm_perf_up;
>>> @@ -83,12 +84,44 @@ static const struct attribute_group
>>> *etm_pmu_attr_groups[] = {
>>>
>>>  static void etm_event_read(struct perf_event *event) {}
>>>
>>> +static int etm_addr_filters_alloc(struct perf_event *event)
>>> +{
>>
>>
>> ...
>>
>>> +       return 0;
>>> +}
>>> +
>>
>>
>>
>>> +
>>>  static int etm_event_init(struct perf_event *event)
>>>  {
>>> +       int ret;
>>> +
>>>         if (event->attr.type != etm_pmu.type)
>>>                 return -ENOENT;
>>>
>>> -       return 0;
>>> +       ret = etm_addr_filters_alloc(event);
>>
>>
>>
>>>  }
>>>
>>>  static void free_event_data(struct work_struct *work)
>>> @@ -456,6 +489,85 @@ static void etm_free_drv_configs(struct perf_event
>>> *event)
>>>         }
>>>  }
>>>
>>> +static int etm_addr_filters_validate(struct list_head *filters)
>>> +{
>>
>>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static void etm_addr_filters_sync(struct perf_event *event)
>>> +{
>>> +       struct perf_addr_filters_head *head =
>>> perf_event_addr_filters(event);
>>> +       unsigned long start, stop, *offs = event->addr_filters_offs;
>>> +       struct etm_filters *filters = event->hw.addr_filters;
>>> +       struct perf_addr_filter *filter;
>>> +       int i = 0;
>>
>>
>> Is it possible to delay the etm_addr_filters_alloc() until this point ?
>> I understand that this function cannot report back failures if we fail
>> to allocate memory. Or may be do a lazy allocation from
>> addr_filters_validate(),
>> when we get the first filter added.
>
> Humm... You want to avoid allocating memory that may never be used if
> filters aren't specified.  Ok, let's do the allocation in
> addr_filters_validate().
>

On second thought we don't have access to the perf event in
addr_filters_validate() so the previous strategy won't work.  And as
you said etm_addr_filters_sync() doesn't return an error code so that
won't work either.  As such I will keep the current code as is.

Mathieu

>>
>> Of course this could be done as a follow up patch to improve things once
>> we get the initial framework in.
>>
>>
>>
>>> +
>>> +       list_for_each_entry(filter, &head->list, entry) {
>>> +               start = filter->offset + offs[i];
>>> +               stop = start + filter->size;
>>> +
>>> +               if (filter->range == 1) {
>>> +                       filters->filter[i].start_addr = start;
>>> +                       filters->filter[i].stop_addr = stop;
>>> +                       filters->filter[i].type = ETM_ADDR_TYPE_RANGE;
>>> +               } else {
>>> +                       if (filter->filter == 1) {
>>> +                               filters->filter[i].start_addr = start;
>>> +                               filters->filter[i].type =
>>> ETM_ADDR_TYPE_START;
>>> +                       } else {
>>> +                               filters->filter[i].stop_addr = stop;
>>> +                               filters->filter[i].type =
>>> ETM_ADDR_TYPE_STOP;
>>> +                       }
>>> +               }
>>> +               i++;
>>> +       }
>>> +
>>> +       filters->nr_filters = i;
>>> +/**
>>> + * struct etm_filters - set of filters for a session
>>> + * @etm_filter:        All the filters for this session.
>>> + * @nr_filters:        Number of filters
>>> + * @ssstatus:  Status of the start/stop logic.
>>> + */
>>> +struct etm_filters {
>>> +       struct etm_filter       filter[ETM_ADDR_CMP_MAX];
>>
>>
>> nit: having the variable renamed to etm_filter will make the code a bit more
>> readable
>> where we populate/validate the filters.
>
> Very well.
>
> Thanks,
> Mathieu
>
>>
>> Otherwise looks good
>>
>> Suzuki