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

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

hi Markus,

thanks for your review, here is the status of the rework as it will be 
published in next version of the patchset.

On 27.04.21 19:31, Metzger, Markus T wrote:
> Hello Zied,
>
>> 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.
>
>> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
>> index 324f7ef0407..dabf6a14c29 100644
>> --- a/gdb/nat/linux-btrace.c
>> +++ b/gdb/nat/linux-btrace.c
>> @@ -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;
> This is an independent improvement.  You can send this in a separate
> patch, if you want.
>
> We should check the return value of sysconf (), though, and fall back
> to PAGE_SIZE on errors; maybe with a one-time warning that gives error
> details.
[Zied] PAGE_SIZE is not portable on all systems (ubuntu linux on arm for 
e.g) and the software does not compile on target when using it. see 
https://github.com/intel/libipt/issues/11 for example
>> @@ -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,
> Same here.
[Zied] Idem.
>
>
>> +  if (conf->sink != nullptr)
>> +    {
>> +      if (strcmp (conf->sink, "default")!=0)
> Spaces around !=.
[Zied] Done.
>
>
>> +  /* 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));
> All that perf_event setup code looks very similar to PT.  Could we share
> all that?
[Zied] Yes, there is a lot of similarity in getting the file descriptor 
out of the syscall and the mmaped file etc.. the difference is in the 
attributes settings and later on the in setting btrace_tinfo_pt or 
btrace_tinfo_etm etc... which requires passing a lot of parameters. 
Therefore I opted to have two dedicated functions.
>
>
>> @@ -726,8 +885,22 @@ linux_disable_bts (struct btrace_tinfo_bts *tinfo)
>> static enum btrace_error
>> linux_disable_pt (struct btrace_tinfo_pt *tinfo)
>> {
>> -  munmap((void *) tinfo->pt.mem, tinfo->pt.size);
>> -  munmap((void *) tinfo->header, PAGE_SIZE);
>> +  long page_size = sysconf (_SC_PAGESIZE);
>> +  munmap ((void *) tinfo->pt.mem, tinfo->pt.size);
>> +  munmap ((void *) tinfo->header, page_size);
> Maybe it makes sense to store the page size we used for the mmap so
> we don't need to look it up again - and can be sure we use the same value.
[Zied] The page size is a system configuration parameter, to change you 
will need to recompile the kernel. So the value will not change on a 
running system.
>
>
>> @@ -898,6 +1075,194 @@ 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;
> Please declare on first use.
[Zied] Done.
>
>> +
>> +  fseek (file.get (), 0, SEEK_END);
>> +  length = ftell (file.get ());
>> +  fseek (file.get (), 0, SEEK_SET);
>> +
>> +  char *buffer;
> Same here.
[Zied] Done.
>
>> +
>> +  buffer = (char *) xmalloc (length+1);
>> +
>> +  length = fread (buffer, 1, length, file.get ());
>> +  buffer[length]='\0';
>> +  while (--length)
> Please use explicit comparisons against zero.
[Zied] Done.
>
>> +    {
>> +      if ((buffer[length] == ',') || (buffer[length] == '-'))
>> +	{
>> +	  length++;
>> +	  break;
>> +	}
>> +    }
>> +
>> +  int cpu_count;
>> +
>> +  if (sscanf (&buffer[length], "%d", &cpu_count) < 1)
> Please move the sscanf () call outside of the if expression.
[Zied] Done.
>
>> +    error (_("Failed to get cpu count in %s: %s."),
>> +	     buffer, safe_strerror (errno));
>> +
>> +  cpu_count ++;
> You may want to add a comment that the kernel starts enumerating
> cpus at zero.
[Zied] Done.
>> +  return (cpu_count);
>> +}
>> +
>> +/* Check if the ETM is an etmv4.  */
>> +
>> +static bool
>> +cs_etm_is_etmv4 (int cpu)
>> +{
>> +  char filename[PATH_MAX];
>> +  snprintf (filename, PATH_MAX,
>> +	    "/sys/bus/event_source/devices/cs_etm/cpu%d/trcidr/trcidr0", cpu);
>> +  errno = 0;
>> +  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>> +  if (file.get () == nullptr)
>> +    return false;
>> +
>> +  return true;
> Wouldn't other (future) versions re-use the infrastructure?
[Zied] I have no guarantee. For the time being this is how perf 
distinguishes between etmv3 and etmv4.
>
>> +}
>> +
>> +/* Get etm configuration register from sys filesystem.  */
> You use the term sysfs below.  Let's use it here, as well.
[Zied] Done.
>
>
>> +#define CORESIGHT_ETM_PMU_SEED  0x10
>> +
>> +/* Calculate trace_id for this cpu
>> +   to be kept aligned with coresight-pmu.h.  */
> Can we include that header?
[Zied] Unfortunately not, it is part of the kernel and it is not exported.
>
>
>> +/* 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)
> No need for comparing against bools.
[Zied] Done.
>
>
>> +  else
>> +    {
>> +      etm_trace_params->arch_ver = ARCH_V7;
>> +      etm_trace_params->core_profile = profile_CortexA;
> I don't think that's safe to assume that not v4 automatically means v3.
> Can we check for v3 explicitly?
[Zied] Unfortunately not, after searching for other means to 
differentiate it, I used the same solution than perf. I will request the 
kernel drivers developer to add a node in sysfs with the version number. 
I will report this to the kernel driver developer.
>
>
>> +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->cpu_count = get_cpu_count ();
>> +  conf->etm_trace_params = new std::vector<cs_etm_trace_params>;
>> +  for (int i = 0; i < conf->cpu_count; i++)
>> +    {
>> +      fill_etm_trace_params (&etm_trace_params,i);
> Can this throw?  We just allocated memory.

[Zied] Yes, new can throw a std::bad_alloc. If we would like to have a 
graceful continuation of gdb when we fail in new, then we will have to 
catch it. Otherwise we ignore it and we let it crash gdb.

Which behaviour do we want to have here?

||

>
>
>> 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;
>> +
>> +  /* The trace perf event buffer.  */
>> +  struct perf_event_buffer etm;
>> +};
> Maybe we can share that, too.
[Zied] Yes. This should be basically possible, we have to rename "struct 
perf_event_buffer etm/pt"  to "struct perf_event_buffer buffer", and 
change its occurrences. shall we go for it in this patch set or in a 
different one?
>
> regards,
> Markus.
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
-- 

*Zied Guermazi*
founder

Trande UG
Leuschnerstraße 2
69469 Weinheim/Germany

Mobile: +491722645127
mailto:zied.guermazi@trande.de

*Trande UG*
Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209 
- Geschäftsführung: Zied Guermazi

*Confidentiality Note*
This message is intended only for the use of the named recipient(s) and 
may contain confidential and/or privileged information. If you are not 
the intended recipient, please contact the sender and delete the 
message. Any unauthorized use of the information contained in this 
message is prohibited.