Re: [PATCH v3 4/7] start/stop btrace with coresight etm and collect etm buffer on linux os

[Zied Guermazi <zied.guermazi@trande.de>]

hi Luis

thanks for your review comments. Here is the status of the updates. The 
changes will be published in next version of the patch set.

/Zied


On 01.04.21 14:16, Luis Machado wrote:
> On 3/30/21 11:52 PM, Zied Guermazi wrote:
>>
>> This patch implement the lower layer for starting ad stopping
>> ARM CoreSight tracing on linux targets for arm and aarch64
>>
>> gdb/ChangeLog
>>
>>     * nat/linux-btrace.h (btrace_tinfo_etm): New.
>>     (btrace_target_info): add etm.
>>     * nat/linux-btrace.c (linux_enable_bts): get page size from sysconf.
>>     (linux_enable_pt): get page size from sysconf.
>>     (perf_event_etm_event_type): New.
>>     (perf_event_etm_event_sink): New.
>>     (linux_enable_etm): New.
>>     (linux_enable_btrace): add enabling etm traces.
>>     (linux_disable_bts): get page size from sysconf.
>>     (linux_disable_pt): get page size from sysconf.
>>     (linux_disable_etm): New.
>>     (linux_disable_btrace): add disabling etm traces.
>>     (get_cpu_count): New.
>>     (cs_etm_is_etmv4): New.
>>     (cs_etm_get_register): New.
>>     (coresight_get_trace_id): New.
>>     (fill_etm_trace_params): New.
>>     (linux_fill_btrace_etm_config): New.
>>     (linux_read_etm): New.
>>     (linux_read_btrace): add reading etm traces.
>>     * arm-linux-nat.c (arm_linux_nat_target::enable_btrace): New.
>>     (arm_linux_nat_target::disable_btrace): New.
>>     (arm_linux_nat_target::teardown_btrace): New.
>>     (arm_linux_nat_target::read_btrace): New.
>>     (arm_linux_nat_target::btrace_conf): New.
>>     * aarch64-linux-nat.c (aarch64_linux_nat_target::enable_btrace): 
>> New.
>>     (aarch64_linux_nat_target::disable_btrace): New.
>>     (aarch64_linux_nat_target::teardown_btrace): New.
>>     (aarch64_linux_nat_target::read_btrace): New.
>>     (aarch64_linux_nat_target::btrace_conf): New.
>>
>> ---
>>   gdb/ChangeLog           |  34 ++++
>>   gdb/aarch64-linux-nat.c |  68 +++++++
>>   gdb/arm-linux-nat.c     |  68 +++++++
>>   gdb/nat/linux-btrace.c  | 400 ++++++++++++++++++++++++++++++++++++++--
>>   gdb/nat/linux-btrace.h  |  19 ++
>>   5 files changed, 577 insertions(+), 12 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 4453a3e8185..d22ec8b5976 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,37 @@
>> +2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>> +
>> +    * nat/linux-btrace.h (btrace_tinfo_etm): New.
>> +    (btrace_target_info): add etm.
>> +    * nat/linux-btrace.c (perf_event_read_available): New.
>> +    (linux_enable_bts): get page size from sysconf.
>> +    (linux_enable_pt): get page size from sysconf.
>> +    (perf_event_etm_event_type): New.
>> +    (perf_event_etm_event_sink): New.
>> +    (linux_enable_etm): New.
>> +    (linux_enable_btrace): add enabling etm traces.
>> +    (linux_disable_bts): get page size from sysconf.
>> +    (linux_disable_pt): get page size from sysconf.
>> +    (linux_disable_etm): New.
>> +    (linux_disable_btrace): add disabling etm traces.
>> +    (get_cpu_count): New.
>> +    (cs_etm_is_etmv4): New.
>> +    (cs_etm_get_register): New.
>> +    (coresight_get_trace_id): New.
>> +    (fill_etm_trace_params): New.
>> +    (linux_fill_btrace_etm_config): New.
>> +    (linux_read_etm): New.
>> +    (linux_read_btrace): add reading etm traces.
>> +    * arm-linux-nat.c (arm_linux_nat_target::enable_btrace): New.
>> +    (arm_linux_nat_target::disable_btrace): New.
>> +    (arm_linux_nat_target::teardown_btrace): New.
>> +    (arm_linux_nat_target::read_btrace): New.
>> +    (arm_linux_nat_target::btrace_conf): New.
>> +    * aarch64-linux-nat.c (aarch64_linux_nat_target::enable_btrace): 
>> New.
>> +    (aarch64_linux_nat_target::disable_btrace): New.
>> +    (aarch64_linux_nat_target::teardown_btrace): New.
>> +    (aarch64_linux_nat_target::read_btrace): New.
>> +    (aarch64_linux_nat_target::btrace_conf): New.
>> +
>>   2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>>         * btrace.c (ftrace_remove_last_insn): New.
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 0cae91b569c..f459fdf6f0e 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -35,6 +35,7 @@
>>   #include "nat/aarch64-linux.h"
>>   #include "nat/aarch64-linux-hw-point.h"
>>   #include "nat/aarch64-sve-linux-ptrace.h"
>> +#include "nat/linux-btrace.h"
>>     #include "elf/external.h"
>>   #include "elf/common.h"
>> @@ -84,6 +85,17 @@ class aarch64_linux_nat_target final : public 
>> linux_nat_target
>>     /* Override the GNU/Linux post attach hook.  */
>>     void post_attach (int pid) override;
>>   +  /* Override branch tracing.  */
>> +  struct btrace_target_info *enable_btrace (ptid_t ptid,
>> +                const struct btrace_config *conf) override;
>> +  void disable_btrace (struct btrace_target_info *tinfo) override;
>> +  void teardown_btrace (struct btrace_target_info *tinfo) override;
>> +  enum btrace_error read_btrace (struct btrace_data *data,
>> +                  struct btrace_target_info *btinfo,
>> +                  enum btrace_read_type type) override;
>> +  const struct btrace_config *btrace_conf (const struct 
>> btrace_target_info *)
>> +                override;
>> +
>>     /* These three defer to common nat/ code.  */
>>     void low_new_thread (struct lwp_info *lp) override
>>     { aarch64_linux_new_thread (lp); }
>> @@ -632,6 +644,62 @@ aarch64_linux_nat_target::post_attach (int pid)
>>     linux_nat_target::post_attach (pid);
>>   }
>>   +/* Enable branch tracing.  */
>> +
>> +struct btrace_target_info *
>> +aarch64_linux_nat_target::enable_btrace (ptid_t ptid,
>> +                      const struct btrace_config *conf)
>> +{
>> +  struct btrace_target_info *tinfo = nullptr;
>> +  try
>> +    {
>> +      tinfo = linux_enable_btrace (ptid, conf);
>> +    }
>> +  catch (const gdb_exception_error &exception)
>> +    {
>> +      error (_("Could not enable branch tracing for %s: %s"),
>> +         target_pid_to_str (ptid).c_str (), exception.what ());
>> +    }
>> +
>> +  return tinfo;
>> +}
>> +
>> +/* Disable branch tracing.  */
>> +
>> +void
>> +aarch64_linux_nat_target::disable_btrace (struct btrace_target_info 
>> *tinfo)
>> +{
>> +  enum btrace_error errcode = linux_disable_btrace (tinfo);
>> +
>> +  if (errcode != BTRACE_ERR_NONE)
>> +    error (_("Could not disable branch tracing."));
>> +}
>> +
>> +/* Teardown branch tracing.  */
>> +
>> +void
>> +aarch64_linux_nat_target::teardown_btrace (struct btrace_target_info 
>> *tinfo)
>> +{
>> +  /* Ignore errors.  */
>> +  linux_disable_btrace (tinfo);
>> +}
>> +
>> +enum btrace_error
>> +aarch64_linux_nat_target::read_btrace (struct btrace_data *data,
>> +                       struct btrace_target_info *btinfo,
>> +                       enum btrace_read_type type)
>> +{
>> +  return linux_read_btrace (data, btinfo, type);
>> +}
>> +
>> +/* See to_btrace_conf in target.h.  */
>> +
>> +const struct btrace_config *
>> +aarch64_linux_nat_target::btrace_conf (const struct 
>> btrace_target_info *btinfo)
>> +{
>> +  return linux_btrace_conf (btinfo);
>> +}
>> +
>>   /* Implement the "read_description" target_ops method.  */
>>     const struct target_desc *
>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
>> index 662dade0a12..8a8d4b65776 100644
>> --- a/gdb/arm-linux-nat.c
>> +++ b/gdb/arm-linux-nat.c
>> @@ -39,6 +39,7 @@
>>   #include <sys/procfs.h>
>>     #include "nat/linux-ptrace.h"
>> +#include "nat/linux-btrace.h"
>>   #include "linux-tdep.h"
>>     /* Prototypes for supply_gregset etc.  */
>> @@ -95,6 +96,17 @@ class arm_linux_nat_target final : public 
>> linux_nat_target
>>       const struct target_desc *read_description () override;
>>   +  /* Override branch tracing.  */
>> +  struct btrace_target_info *enable_btrace (ptid_t ptid,
>> +                const struct btrace_config *conf) override;
>> +  void disable_btrace (struct btrace_target_info *tinfo) override;
>> +  void teardown_btrace (struct btrace_target_info *tinfo) override;
>> +  enum btrace_error read_btrace (struct btrace_data *data,
>> +                  struct btrace_target_info *btinfo,
>> +                  enum btrace_read_type type) override;
>> +  const struct btrace_config *btrace_conf (const struct 
>> btrace_target_info *)
>> +                override;
>> +
>>     /* Override linux_nat_target low methods.  */
>>       /* Handle thread creation and exit.  */
>> @@ -1190,6 +1202,62 @@ 
>> arm_linux_nat_target::watchpoint_addr_within_range (CORE_ADDR addr,
>>     return start <= addr && start + length - 1 >= addr;
>>   }
>>   +/* Enable branch tracing.  */
>> +
>> +struct btrace_target_info *
>> +arm_linux_nat_target::enable_btrace (ptid_t ptid,
>> +                     const struct btrace_config *conf)
>> +{
>> +  struct btrace_target_info *tinfo = nullptr;
>> +  try
>> +    {
>> +      tinfo = linux_enable_btrace (ptid, conf);
>> +    }
>> +  catch (const gdb_exception_error &exception)
>> +    {
>> +      error (_("Could not enable branch tracing for %s: %s"),
>> +         target_pid_to_str (ptid).c_str (), exception.what ());
>> +    }
>> +
>> +  return tinfo;
>> +}
>> +
>> +/* Disable branch tracing.  */
>> +
>> +void
>> +arm_linux_nat_target::disable_btrace (struct btrace_target_info *tinfo)
>> +{
>> +  enum btrace_error errcode = linux_disable_btrace (tinfo);
>> +
>> +  if (errcode != BTRACE_ERR_NONE)
>> +    error (_("Could not disable branch tracing."));
>
> Is this error due to not being able to disable branch tracing or not 
> having an active tracing session to disable? If the latter, you may 
> want to rephrase the error message.
[Zied] not having an active tracing session is handled in btrace_disable 
in btrace.c. so it is not being able to disable branch tracing.
>
>> +}
>> +
>> +/* Teardown branch tracing.  */
>> +
>> +void
>> +arm_linux_nat_target::teardown_btrace (struct btrace_target_info 
>> *tinfo)
>> +{
>> +  /* Ignore errors.  */
>> +  linux_disable_btrace (tinfo);
>> +}
>> +
>> +enum btrace_error
>> +arm_linux_nat_target::read_btrace (struct btrace_data *data,
>> +                   struct btrace_target_info *btinfo,
>> +                   enum btrace_read_type type)
>> +{
>> +  return linux_read_btrace (data, btinfo, type);
>> +}
>> +
>> +/* See to_btrace_conf in target.h.  */
>> +
>> +const struct btrace_config *
>> +arm_linux_nat_target::btrace_conf (const struct btrace_target_info 
>> *btinfo)
>> +{
>> +  return linux_btrace_conf (btinfo);
>> +}
>> +
>>   /* Handle thread creation.  We need to copy the breakpoints and 
>> watchpoints
>>      in the parent thread to the child thread.  */
>>   void
>> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
>> index 324f7ef0407..11e2dd96397 100644
>> --- a/gdb/nat/linux-btrace.c
>> +++ b/gdb/nat/linux-btrace.c
>> @@ -32,6 +32,10 @@
>>     #include <sys/syscall.h>
>>   +#if defined (HAVE_LIBOPENCSD_C_API)
>> +#  include <opencsd/ocsd_if_types.h>
>> +#endif
>> +
>>   #if HAVE_LINUX_PERF_EVENT_H && defined(SYS_perf_event_open)
>>   #include <unistd.h>
>>   #include <sys/mman.h>
>> @@ -483,10 +487,11 @@ linux_enable_bts (ptid_t ptid, const struct 
>> btrace_config_bts *conf)
>>     scoped_fd fd (syscall (SYS_perf_event_open, &bts->attr, pid, -1, 
>> -1, 0));
>>     if (fd.get () < 0)
>>       diagnose_perf_event_open_fail ();
>> +  long page_size = sysconf (_SC_PAGESIZE);
>>       /* Convert the requested size in bytes to pages (rounding up).  */
>> -  pages = ((size_t) conf->size / PAGE_SIZE
>> -       + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
>> +  pages = ((size_t) conf->size / page_size
>> +       + ((conf->size % page_size) == 0 ? 0 : 1));
>>     /* We need at least one page.  */
>>     if (pages == 0)
>>       pages = 1;
>> @@ -505,17 +510,17 @@ linux_enable_bts (ptid_t ptid, const struct 
>> btrace_config_bts *conf)
>>         size_t length;
>>         __u64 data_size;
>>   -      data_size = (__u64) pages * PAGE_SIZE;
>> +      data_size = (__u64) pages * page_size;
>>           /* Don't ask for more than we can represent in the 
>> configuration.  */
>>         if ((__u64) UINT_MAX < data_size)
>>       continue;
>>           size = (size_t) data_size;
>> -      length = size + PAGE_SIZE;
>> +      length = size + page_size;
>>           /* Check for overflows.  */
>> -      if ((__u64) length != data_size + PAGE_SIZE)
>> +      if ((__u64) length != data_size + page_size)
>>       continue;
>>           errno = 0;
>> @@ -530,7 +535,7 @@ linux_enable_bts (ptid_t ptid, const struct 
>> btrace_config_bts *conf)
>>       struct perf_event_mmap_page *header = (struct 
>> perf_event_mmap_page *)
>>       data.get ();
>> -  data_offset = PAGE_SIZE;
>> +  data_offset = page_size;
>>     #if defined (PERF_ATTR_SIZE_VER5)
>>     if (offsetof (struct perf_event_mmap_page, data_size) <= 
>> header->size)
>> @@ -613,7 +618,8 @@ linux_enable_pt (ptid_t ptid, const struct 
>> btrace_config_pt *conf)
>>       diagnose_perf_event_open_fail ();
>>       /* Allocate the configuration page. */
>> -  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, 
>> MAP_SHARED,
>> +  long page_size = sysconf (_SC_PAGESIZE);
>> +  scoped_mmap data (nullptr, page_size, PROT_READ | PROT_WRITE, 
>> MAP_SHARED,
>>               fd.get (), 0);
>>     if (data.get () == MAP_FAILED)
>>       error (_("Failed to map trace user page: %s."), safe_strerror 
>> (errno));
>> @@ -624,8 +630,8 @@ linux_enable_pt (ptid_t ptid, const struct 
>> btrace_config_pt *conf)
>>     header->aux_offset = header->data_offset + header->data_size;
>>       /* Convert the requested size in bytes to pages (rounding up).  */
>> -  pages = ((size_t) conf->size / PAGE_SIZE
>> -       + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
>> +  pages = ((size_t) conf->size / page_size
>> +       + ((conf->size % page_size) == 0 ? 0 : 1));
>>     /* We need at least one page.  */
>>     if (pages == 0)
>>       pages = 1;
>> @@ -644,7 +650,7 @@ linux_enable_pt (ptid_t ptid, const struct 
>> btrace_config_pt *conf)
>>         size_t length;
>>         __u64 data_size;
>>   -      data_size = (__u64) pages * PAGE_SIZE;
>> +      data_size = (__u64) pages * page_size;
>>           /* Don't ask for more than we can represent in the 
>> configuration.  */
>>         if ((__u64) UINT_MAX < data_size)
>> @@ -689,6 +695,154 @@ linux_enable_pt (ptid_t ptid, const struct 
>> btrace_config_pt *conf)
>>     #endif /* !defined (PERF_ATTR_SIZE_VER5) */
>>   +/* Determine the etm event type.  */
>> +
>> +static int
>> +perf_event_etm_event_type ()
>> +{
>> +  static const char filename[] = 
>> "/sys/bus/event_source/devices/cs_etm/type";
>> +
>> +  errno = 0;
>> +  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>> +  if (file.get () == nullptr)
>> +    error (_("Failed to open %s: %s."), filename, safe_strerror 
>> (errno));
>> +
>> +  int type, found = fscanf (file.get (), "%d", &type);
>> +  if (found != 1)
>> +    error (_("Failed to read the ETM event type from %s."), filename);
>> +
>> +  return type;
>> +}
>> +
>> +/* Get the sink hash to use in the event attributes.  */
>> +
>> +static unsigned int
>> +perf_event_etm_event_sink (const struct btrace_config_etm *conf)
>> +{
>> +  char filename[PATH_MAX];
>> +
>> +  sprintf (filename, "/sys/bus/event_source/devices/cs_etm/sinks/%s",
>> +       conf->sink);
>
> Instead of hardcoding the filename buffer size, you could use 
> string_printf or snprintf here.
[Zied]  done
>
>> +  errno = 0;
>> +  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>> +  if (file.get () == nullptr)
>> +    error (_("Failed to open %s: %s."), filename, safe_strerror 
>> (errno));
>> +
>> +  unsigned int sink;
>> +  int  found = fscanf (file.get (), "0x%x", &sink);
>> +  if (found != 1)
>> +    error (_("Failed to read the ETM sink from %s."), filename);
>> +
>> +  return sink;
>> +}
>> +
>> +/* Enable arm CoreSight ETM tracing.  */
>> +
>
> arm -> ARM
[Zied] done
>
>> +static struct btrace_target_info *
>> +linux_enable_etm (ptid_t ptid, const struct btrace_config_etm *conf)
>> +{
>> +  struct btrace_tinfo_etm *etm;
>> +  size_t pages;
>> +  int pid, pg;
>> +
>> +  pid = ptid.lwp ();
>> +  if (pid == 0)
>> +    pid = ptid.pid ();
>> +
>> +  gdb::unique_xmalloc_ptr<btrace_target_info> tinfo
>> +    (XCNEW (btrace_target_info));
>> +  tinfo->ptid = ptid;
>> +
>> +  tinfo->conf.format = BTRACE_FORMAT_ETM;
>> +  etm = &tinfo->variant.etm;
>> +
>> +  etm->attr.type = perf_event_etm_event_type ();
>> +  etm->attr.size = sizeof (etm->attr);
>> +
>> +  etm->attr.sample_type = PERF_SAMPLE_CPU;
>> +  etm->attr.read_format = PERF_FORMAT_ID;
>> +  etm->attr.sample_id_all = 1;
>> +  etm->attr.enable_on_exec = 1;
>> +  etm->attr.exclude_kernel = 1;
>> +  etm->attr.exclude_hv = 1;
>> +  etm->attr.exclude_idle = 1;
>> +  if (conf->sink != nullptr)
>> +    {
>> +      if (strcmp (conf->sink, "default")!=0)
>> +    etm->attr.config2 = perf_event_etm_event_sink (conf);
>> +    }
>> +
>> +  errno = 0;
>> +  scoped_fd fd (syscall (SYS_perf_event_open, &etm->attr, pid, -1, 
>> -1, 0));
>> +  if (fd.get () < 0)
>> +    diagnose_perf_event_open_fail ();
>> +
>> +  /* Allocate the configuration page.  */
>> +  long page_size = sysconf (_SC_PAGESIZE);
>> +  scoped_mmap data (nullptr, page_size, PROT_READ | PROT_WRITE, 
>> MAP_SHARED,
>> +            fd.get (), 0);
>> +  if (data.get () == MAP_FAILED)
>> +    error (_("Failed to map trace user page: %s."), safe_strerror 
>> (errno));
>> +
>> +  struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
>> +    data.get ();
>> +
>> +  header->aux_offset = header->data_offset + header->data_size;
>> +  /* Convert the requested size in bytes to pages (rounding up).  */
>> +  pages = ((size_t) conf->size / page_size
>> +       + ((conf->size % page_size) == 0 ? 0 : 1));
>> +  /* We need at least one page.  */
>> +  if (pages == 0)
>> +    pages = 1;
>> +
>> +  /* The buffer size can be requested in powers of two pages. Adjust 
>> PAGES
>> +     to the next power of two.  */
>> +  for (pg = 0; pages != ((size_t) 1 << pg); ++pg)
>> +    if ((pages & ((size_t) 1 << pg)) != 0)
>> +      pages += ((size_t) 1 << pg);
>> +
>> +  /* We try to allocate the requested size.
>> +     If that fails, try to get as much as we can.  */
>> +  scoped_mmap aux;
>> +  for (; pages > 0; pages >>= 1)
>> +    {
>> +      size_t length;
>> +      __u64 data_size;
>> +      data_size = (__u64) pages * page_size;
>> +
>> +      /* Don't ask for more than we can represent in the 
>> configuration.  */
>> +      if ((__u64) UINT_MAX < data_size)
>> +    continue;
>> +
>> +      length = (size_t) data_size;
>> +
>> +      /* Check for overflows.  */
>> +      if ((__u64) length != data_size)
>> +    continue;
>> +
>> +      header->aux_size = data_size;
>> +
>> +      errno = 0;
>> +      aux.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get (),
>> +         header->aux_offset);
>> +      if (aux.get () != MAP_FAILED)
>> +    break;
>> +    }
>> +  if (pages == 0)
>> +    error (_("Failed to map trace buffer: %s."), safe_strerror 
>> (errno));
>> +
>> +  etm->etm.size = aux.size ();
>> +  etm->etm.mem = (const uint8_t *) aux.release ();
>> +  etm->etm.data_head = &header->aux_head;
>> +  etm->etm.last_head = header->aux_tail;
>> +  etm->header = (struct perf_event_mmap_page *) data.release ();
>> +  gdb_assert (etm->header == header);
>> +  etm->file = fd.release ();
>> +
>> +  tinfo->conf.etm.size = (unsigned int) etm->etm.size;
>> +  return tinfo.release ();
>> +}
>> +
>>   /* See linux-btrace.h.  */
>>     struct btrace_target_info *
>> @@ -707,6 +861,10 @@ linux_enable_btrace (ptid_t ptid, const struct 
>> btrace_config *conf)
>>         case BTRACE_FORMAT_PT:
>>         return linux_enable_pt (ptid, &conf->pt);
>> +
>> +    case BTRACE_FORMAT_ETM:
>> +      return linux_enable_etm (ptid, &conf->etm);
>> +
>>       }
>>   }
>>   @@ -715,7 +873,8 @@ linux_enable_btrace (ptid_t ptid, const struct 
>> btrace_config *conf)
>>   static enum btrace_error
>>   linux_disable_bts (struct btrace_tinfo_bts *tinfo)
>>   {
>> -  munmap((void *) tinfo->header, tinfo->bts.size + PAGE_SIZE);
>> +  long page_size = sysconf (_SC_PAGESIZE);
>> +  munmap((void *) tinfo->header, tinfo->bts.size + page_size);
>>     close (tinfo->file);
>>       return BTRACE_ERR_NONE;
>> @@ -726,8 +885,22 @@ linux_disable_bts (struct btrace_tinfo_bts *tinfo)
>>   static enum btrace_error
>>   linux_disable_pt (struct btrace_tinfo_pt *tinfo)
>>   {
>> +  long page_size = sysconf (_SC_PAGESIZE);
>>     munmap((void *) tinfo->pt.mem, tinfo->pt.size);
>> -  munmap((void *) tinfo->header, PAGE_SIZE);
>> +  munmap((void *) tinfo->header, page_size);
>> +  close (tinfo->file);
>> +
>> +  return BTRACE_ERR_NONE;
>> +}
>> +
>> +/* Disable arm CoreSight ETM tracing.  */
>> +
>
> arm -> ARM
[Zied] done
>
>> +static enum btrace_error
>> +linux_disable_etm (struct btrace_tinfo_etm *tinfo)
>> +{
>> +  long page_size = sysconf (_SC_PAGESIZE);
>> +  munmap ((void *) tinfo->etm.mem, tinfo->etm.size);
>> +  munmap ((void *) tinfo->header, page_size);
>>     close (tinfo->file);
>>       return BTRACE_ERR_NONE;
>> @@ -753,6 +926,10 @@ linux_disable_btrace (struct btrace_target_info 
>> *tinfo)
>>       case BTRACE_FORMAT_PT:
>>         errcode = linux_disable_pt (&tinfo->variant.pt);
>>         break;
>> +
>> +    case BTRACE_FORMAT_ETM:
>> +      errcode = linux_disable_etm (&tinfo->variant.etm);
>> +      break;
>>       }
>>       if (errcode == BTRACE_ERR_NONE)
>> @@ -898,6 +1075,197 @@ linux_read_pt (struct btrace_data_pt *btrace,
>>     internal_error (__FILE__, __LINE__, _("Unknown btrace read type."));
>>   }
>>   +/* Return the number of CPUs that are present.  */
>> +
>> +static int
>> +get_cpu_count (void)
>> +{
>> +  static const char filename[] = "/sys/devices/system/cpu/present";
>> +
>> +  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>> +  if (file.get () == nullptr)
>> +    error (_("Failed to open %s: %s."), filename, safe_strerror 
>> (errno));
>> +
>> +  int length;
>> +
>> +  fseek (file.get (), 0, SEEK_END);
>> +  length = ftell (file.get ());
>> +  fseek (file.get (), 0, SEEK_SET);
>> +
>> +  char * buffer;
>
> Spurious space between * and buffer.
[Zied] fixed
>> +
>> +  buffer = (char*) xmalloc (length+1);
>
> space between char and *.
>
>> +
>> +  length = fread (buffer, 1, length, file.get ());
>> +  buffer[length]='\0';
>
> Space between buffer[length] and =, and between = and '\0'.
>
[Zied] done
>> +  while (--length)
>> +    {
>> +      if ((buffer[length] == ',') || (buffer[length] == '-'))
>> +    {
>> +      length++;
>> +      break;
>> +    }
>> +    }
>> +
>> +  int cpu_count;
>> +
>> +  if (sscanf (&buffer[length], "%d", &cpu_count) < 1)
>> +    error (_("Failed to get cpu count in %s: %s."),
>> +         buffer, safe_strerror (errno));
>> +
>> +  cpu_count ++;
>> +  return (cpu_count);
>> +}
>> +
>> +/* Check if the ETM is an etmv4.  */
>> +
>> +static bool
>> +cs_etm_is_etmv4 (int cpu)
>> +{
>> +  char filename[PATH_MAX];
>> +  sprintf (filename,
>> + "/sys/bus/event_source/devices/cs_etm/cpu%d/trcidr/trcidr0",
>> +      cpu);
>
> Same suggestion about not relying on a fixed-size buffer and using 
> string_printf or snprintf.
[Zied] done
>
>> +  errno = 0;
>> +  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>> +  if (file.get () == nullptr)
>> +    return false;
>> +
>> +  return true;
>> +}
>> +
>> +/* Get etm configuration register from sys filesystem.  */
>> +
>> +static uint32_t
>> +cs_etm_get_register (int cpu, const char *path)
>> +{
>> +  char filename[PATH_MAX];
>> +
>> +  /* Get coresight register from sysfs.  */
>> +  sprintf (filename,
>> +      "/sys/bus/event_source/devices/cs_etm/cpu%d/%s",
>> +      cpu, path);
>
> Same here.
[Zied] done
>
>> +  errno = 0;
>> +  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>> +  if (file.get () == nullptr)
>> +    error (_("Failed to open %s: %s."), filename, safe_strerror 
>> (errno));
>> +
>> +  uint32_t val = 0;
>> +
>> +  int  found = fscanf (file.get (), "0x%x", &val);
>> +  if (found != 1)
>> +    error (_("Failed to read coresight register from %s."), filename);
>> +  return val;
>> +}
>> +
>> +#define CORESIGHT_ETM_PMU_SEED  0x10
>> +
>> +/* Calculate trace_id for this cpu
>> +   to be kept aligned with coresight-pmu.h.  */
>> +
>> +static inline int
>> +coresight_get_trace_id (int cpu)
>> +{
>> +  return (CORESIGHT_ETM_PMU_SEED + (cpu * 2));
>> +}
>> +
>> +
>
> Spurious newline.
[Zied] done
>
>> +/* PTMs ETMIDR[11:8] set to b0011.  */
>> +#define ETMIDR_PTM_VERSION 0x00000300
>> +
>> +/* Collect and fill etm trace parameter.  */
>> +
>> +static void
>> +fill_etm_trace_params (struct cs_etm_trace_params *etm_trace_params, 
>> int cpu)
>> +{
>> +  if (cs_etm_is_etmv4 (cpu) == true)
>> +    {
>> +      etm_trace_params->arch_ver = ARCH_V8;
>> +      etm_trace_params->core_profile = profile_CortexA;
>> +      etm_trace_params->protocol = OCSD_PROTOCOL_ETMV4I;
>> +      /* This is the parameter passed in etm->attr.config in the 
>> call to
>> +     perf_event_open remapped according to linux/coresight-pmu.h.  */
>
> Is the indentation of the second line of this comment block off?
[Zied] no. it is fine. 'p' of perf is below/ at the same column than 'T' 
of This
>
>> + etm_trace_params->etmv4.reg_configr = 0;
>> +      etm_trace_params->etmv4.reg_idr0
>> +     = cs_etm_get_register (cpu, "trcidr/trcidr0");
>> +      etm_trace_params->etmv4.reg_idr1
>> +     = cs_etm_get_register (cpu, "trcidr/trcidr1");
>> +      etm_trace_params->etmv4.reg_idr2
>> +     = cs_etm_get_register (cpu, "trcidr/trcidr2");
>> +      etm_trace_params->etmv4.reg_idr8
>> +     = cs_etm_get_register (cpu, "trcidr/trcidr8");
>> +      etm_trace_params->etmv4.reg_traceidr = coresight_get_trace_id 
>> (cpu);
>> +    }
>> +  else
>> +    {
>> +      etm_trace_params->arch_ver = ARCH_V7;
>> +      etm_trace_params->core_profile = profile_CortexA;
>> +      etm_trace_params->protocol
>> +    = (etm_trace_params->etmv3.reg_idr & ETMIDR_PTM_VERSION)
>> +      == ETMIDR_PTM_VERSION ? OCSD_PROTOCOL_PTM : OCSD_PROTOCOL_ETMV3;
>> +      etm_trace_params->etmv3.reg_ccer
>> +     = cs_etm_get_register (cpu, "mgmt/etmccer");
>> +      etm_trace_params->etmv3.reg_ctrl
>> +     = cs_etm_get_register (cpu, "mgmt/etmcr");
>> +      etm_trace_params->etmv3.reg_idr
>> +     = cs_etm_get_register (cpu, "mgmt/etmidr");
>> +      etm_trace_params->etmv3.reg_trc_id
>> +     = cs_etm_get_register (cpu, "traceid");
>> +    }
>> +}
>> +
>> +static void
>> +linux_fill_btrace_etm_config (struct btrace_target_info *tinfo,
>> +                   struct btrace_data_etm_config *conf)
>> +{
>> +
>> +  cs_etm_trace_params etm_trace_params;
>> +  conf->num_cpu = get_cpu_count ();
>> +  conf->etm_trace_params = new std::vector<cs_etm_trace_params>;
>> +  for (int i = 0; i < conf->num_cpu; i++)
>> +    {
>> +      fill_etm_trace_params (&etm_trace_params,i);
>> +      conf->etm_trace_params->push_back (etm_trace_params);
>> +    }
>> +
>> +  conf->etm_decoder_params.formatted = 1;
>> +  conf->etm_decoder_params.fsyncs = 0;
>> +  conf->etm_decoder_params.hsyncs = 0;
>> +  conf->etm_decoder_params.frame_aligned = 0;
>> +  conf->etm_decoder_params.reset_on_4x_sync = 1;
>> +}
>> +
>> +static enum btrace_error
>> +linux_read_etm (struct btrace_data_etm *btrace,
>> +        struct btrace_target_info *tinfo,
>> +        enum btrace_read_type type)
>> +{
>> +  struct perf_event_buffer *etm;
>> +  etm = &tinfo->variant.etm.etm;
>> +
>> +  linux_fill_btrace_etm_config (tinfo, &btrace->config);
>> +
>> +  switch (type)
>> +    {
>> +    case BTRACE_READ_DELTA:
>> +      /* We don't support delta reads.  The data head (i.e. 
>> aux_head) wraps
>> +     around to stay inside the aux buffer.  */
>
> Indentation of the second line of the comment block off?
[Zied] no, it is fine. 'a' of around is below/ at the same column than 
'W' of We
>
>> +      return BTRACE_ERR_NOT_SUPPORTED;
>> +
>> +    case BTRACE_READ_NEW:
>> +      if (!perf_event_new_data (etm))
>> +    return BTRACE_ERR_NONE;
>> +
>> +      /* Fall through.  */
>> +    case BTRACE_READ_ALL:
>> +      perf_event_read_all (etm, &(btrace->data),&(btrace->size));
>> +      return BTRACE_ERR_NONE;
>> +    }
>> +
>> +  internal_error (__FILE__, __LINE__, _("Unkown btrace read type."));
>> +}
>> +
>> +
>>   /* See linux-btrace.h.  */
>>     enum btrace_error
>> @@ -924,6 +1292,14 @@ linux_read_btrace (struct btrace_data *btrace,
>>         btrace->variant.pt.size = 0;
>>           return linux_read_pt (&btrace->variant.pt, tinfo, type);
>> +
>> +    case BTRACE_FORMAT_ETM:
>> +      /* We read btrace in arm CoreSight ETM Trace format.  */
>
> arm -> ARM
[Zied] fixed
>
>> +      btrace->format = BTRACE_FORMAT_ETM;
>> +      btrace->variant.etm.data = NULL;
>> +      btrace->variant.etm.size = 0;
>> +
>> +      return linux_read_etm (&btrace->variant.etm, tinfo, type);
>>       }
>>       internal_error (__FILE__, __LINE__, _("Unkown branch trace 
>> format."));
>> diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
>> index 607182da144..3038d2f45a0 100644
>> --- a/gdb/nat/linux-btrace.h
>> +++ b/gdb/nat/linux-btrace.h
>> @@ -78,6 +78,22 @@ struct btrace_tinfo_pt
>>     /* The trace perf event buffer.  */
>>     struct perf_event_buffer pt;
>>   };
>> +
>> +/* Branch trace target information for ARM CoreSight ETM Trace.  */
>> +struct btrace_tinfo_etm
>> +{
>> +  /* The Linux perf_event configuration for collecting the branch 
>> trace.  */
>> +  struct perf_event_attr attr;
>> +
>> +  /* The perf event file.  */
>> +  int file;
>> +
>> +  /* The perf event configuration page.  */
>> +  volatile struct perf_event_mmap_page *header;
>
> I'm not too familiar with the interface, but does this need to be 
> volatile? Is this something changing this in the background?
[Zied], yes it is a buffer shared ring buffer between kernel space and 
user space. kernel writes to it. We have to set it to volatile to 
prevent any optimization/caching in registers
>
>> +
>> +  /* The trace perf event buffer.  */
>> +  struct perf_event_buffer etm;
>> +};
>>   #endif /* HAVE_LINUX_PERF_EVENT_H */
>>     /* Branch trace target information per thread.  */
>> @@ -98,6 +114,9 @@ struct btrace_target_info
>>         /* CONF.FORMAT == BTRACE_FORMAT_PT.  */
>>       struct btrace_tinfo_pt pt;
>> +
>> +    /* CONF.FORMAT == BTRACE_FORMAT_ETM.  */
>> +    struct btrace_tinfo_etm etm;
>>     } variant;
>>   #endif /* HAVE_LINUX_PERF_EVENT_H */
>>   };
>>
>
> I'm addressing mostly formatting issues for now, as they tend to get 
> in the way of proper code review. And I'm not too familiar with the 
> branch tracing code.