From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nx174.node01.secure-mailgate.com (nx174.node01.secure-mailgate.com [89.22.108.174]) by sourceware.org (Postfix) with ESMTPS id 024923858C2D for ; Thu, 12 May 2022 22:52:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 024923858C2D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=trande.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=trande.de Received: from host202.checkdomain.de ([185.137.168.148]) by node01.secure-mailgate.com with esmtps (TLSv1.2:AES128-GCM-SHA256:128) (Exim 4.92) (envelope-from ) id 1npHfe-001lNz-NK; Fri, 13 May 2022 00:52:37 +0200 X-SecureMailgate-Identity: zied.guermazi@trande.de;host202.checkdomain.de Received: from [192.168.178.42] (dynamic-077-189-076-183.77.189.pool.telefonica.de [77.189.76.183]) (Authenticated sender: zied.guermazi@trande.de) by host202.checkdomain.de (Postfix) with ESMTPSA id 46CEA280666; Fri, 13 May 2022 00:52:29 +0200 (CEST) X-SecureMailgate-Identity: zied.guermazi@trande.de;host202.checkdomain.de Message-ID: <761d9b7e-09f3-7c00-1f8f-df9af83c5aba@trande.de> Date: Fri, 13 May 2022 00:52:28 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH v6 4/7] start/stop btrace with coresight etm and collect etm buffer on linux os Content-Language: en-US To: "Metzger, Markus T" , "gdb-patches@sourceware.org" References: <20210531213307.275079-1-zied.guermazi@trande.de> <20210531213307.275079-5-zied.guermazi@trande.de> From: Zied Guermazi In-Reply-To: X-PPP-Message-ID: <20220512225229.792613.32206@host202.checkdomain.de> X-PPP-Vhost: trande.de X-Originating-IP: 185.137.168.148 X-SecureMailgate-Domain: host202.checkdomain.de X-SecureMailgate-Username: 185.137.168.148 Authentication-Results: secure-mailgate.com; auth=pass smtp.auth=185.137.168.148@host202.checkdomain.de X-SecureMailgate-Outgoing-Class: ham X-SecureMailgate-Outgoing-Evidence: SB/global_tokens (4.43997331972e-05) X-Recommended-Action: accept X-Filter-ID: Pt3MvcO5N4iKaDQ5O6lkdGlMVN6RH8bjRMzItlySaT93PBcdFJPkvK3NaJIy+Sa3PUtbdvnXkggZ 3YnVId/Y5jcf0yeVQAvfjHznO7+bT5xepKJyYLZISh0ScmnJxtOQL++T0fzDkcbraJCnPSgcQrqW ghmgrvbuYBMbgPFqhEjKF590ee3aGyyzch48U9RYPumU89c8OaxcWLmp7ynpEvCCPLPw4J/aBvYJ R9AMpwzasE0yYSwoSJtDBJTXEF44+qKA9WNimSzzMEEnsLmJnD3bQmEqUddg4pDcHxi62gxzSOZO aYOlkYc6lCBjGxXRtrdIeJGUu23VykQIijZdTM9OO12tKvW9/EX5OZ0Bv15y8oFzlZDzSFMZMCEJ KqX3DU5uCP5D/CUxYBzL65HHiH6YFrhxVw5i30CZMe0+JKoB15eMIME6iZU0/33rc4pTBldNoc4/ ugpbufbubK+x4vA/QHsSr98zh4ylHHSOvF4EizIlMC06U8aFKFbkglvNo7UbuWzwvM5eRjzlfUYD MWFgOK8gW+hazh84oBeMtGGs+REr5it4yuwWmXN5FmKajBUnmlmmmmIH8Dg80WcRp+fC5fGvBaRW nWe6WFte+h1HBQQL7n9+OTZ37LZfI/sW30YZMgUX6Z4ThlOFxTu4LUsF4GHuFWly2YtZO0AGnGQ3 T2YxzxB8woiuExubdcsml481x58OeqpgxEwDRkEDKwPt8rNcco5Lt6ku63DkksyWUQhsiRV7ToHt 3HSONnNjcxw3qqhc+N6cuEg4XWh5FqxOllGagQ5/czOCWuwRvLCUCv2x3P3WX8VSN7UvG2eDTwdA +dI0eDMIjJmTs4l5VJ/ZEFdgfl/gg4TaPUbxShQ6dhCmw4vcPhYpg9v2GJCqP7g/v82w1PskcK86 cc+ng92f5yi6xPn6AUYBw5o0nqUyS+Z2X0Qnz8P/HVUOy0aiB40jT4zRzMvG8ro2P5IOJvGI+E6N r4fFIgfK7iZZ5HXrNwwZsWS6qXq44OKONj1oPAlbDjazCbhs7qBpykynMviyWk7Z3swtSg+6+Puj 0aVAO2qOWilKQPYZioij5z0qlZbBsJm9lVGRRgNQHKD9w1KE4fbZJGBsdarW5yIwOgNQrOGchSlR E8ACmTzaJRNOhLZUadD3AfXfai9XShp+2Qswr0cL6lRAEDJHTf5jN3xeYdFqPdXjTfBL2sIJSlUc o7WGTBDiXkmPjxliCYSRIoW+QAhvxzwVkMP27kFl8HgxL7hrJSk60SF3F6RYOYr2 X-Report-Abuse-To: spam@node04.secure-mailgate.com X-Spam-Status: No, score=1.9 required=5.0 tests=BAYES_00, BODY_8BITS, FOREIGN_BODY1, HTML_MESSAGE, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Level: * X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 May 2022 22:52:47 -0000 Hello Markus, thanks for your feedback, below are the reworking comments. /Zied On 23.06.21 10:00, 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 > The patch looks good overall. There are a few style nits and I'd ask you to > split the PAGE_SIZE changes into a separate patch as they are unrelated. > > Then, there's the discussion about sharing perf_event buffer mapping. > I pointed out which parts I believe can be shared. > > >> +/* 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); >> +} > There's some inconsistency in comments on functions ranging from no comment > over referring to the original target struct, to an own comment. [Zied] I will align the comments. please notice that the same applies to x86-linux-nat.c (it was a copy-paste from it) > > >> @@ -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); > Please split those PAGE_SIZE changes into a separate patch. This is unrelated > to what this patch is doing. > > Note that PAGE_SIZE was unsigned whereas sysconf () returns a signed integer. > I'd expect compilers to require proper casting. [Zied] done > > >> +/* Enable ARM CoreSight ETM tracing. */ >> + >> +static struct btrace_target_info * >> +linux_enable_etm (ptid_t ptid, const struct btrace_config_etm *conf) >> +{ > [...] >> + etm->attr.sample_type = PERF_SAMPLE_CPU; >> + etm->attr.read_format = PERF_FORMAT_ID; >> + etm->attr.sample_id_all = 1; > You enable sampling. Wouldn't you need to mmap the data buffer, as well? [Zied] it is not needed for current implementation. removed. > > > This ... > >> + 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); > ... can be shared with btrace_enable_pt () by introducing some > > perf_event_open_aux (struct perf_event_buffer *, const struct perf_event_attr *) > > helper. > > And if you indeed need to mmap the data buffer, as well, we can share that with > btrace_enable_bts (), although we'd need some more restructuring to leave > perf_event_open to the caller and just allocate the data and aux buffers using > two helpers - they would again look very similar but need to touch a different > set of fields in the header, so I'd keep those separate. > > btrace_enable_foo () would then become > { > perf_event_open () > perf_event_mmap_data () > perf_event_mmap_aux () /* not for bts */ > } > [Zied] I like the idea, there are two aspects that we need to consider to bring it to the mainstream code: - 1: interface and scope definition static scoped_fd perf_event_open ( const struct perf_event_attr *event_attributes,  const int pid ) {   errno = 0;   scoped_fd fd (syscall (SYS_perf_event_open, event_attributes, pid, -1, -1, 0));   if (fd.get () < 0)     diagnose_perf_event_open_fail ();   return fd; } this function will only open the file descriptor and return it static scoped_mmap data perf_event_mmap_data (const scoped_fd fd, size_t size, size_t page_size, int offset) {  //alternative 1 just create it and return it   /* Allocate the configuration page. */   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));   return data;  //alternative 2, create it it and make sure that we resize it to the highest possible power of 2 supported by the system /* Convert the requested size in bytes to pages (rounding up). */   pages = size / page_size        + (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 data;   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;       size = (size_t) data_size;       length = size + page_size;       /* Check for overflows.  */       if ((__u64) length != data_size + page_size)     continue;       errno = 0;       /* The number of pages we request needs to be a power of two.  */       data.reset (nullptr, length, PROT_READ, MAP_SHARED, fd.get (), offset);       if (data.get () != MAP_FAILED)     break;     }   if (pages == 0)     error (_("Failed to map trace buffer: %s."), safe_strerror (errno)); } static scoped_mmap perf_event_mmap_aux (const scoped_fd fd, size_t size, size_t page_size, int offset) { //idem, the function is similar to previous one it is only the offset in data.reset call that changes } so both perf_event_mmap_data perf_event_mmap_aux can be in fact reduced to one function if we give size_t size, size_t page_size, int offset as parameters. 2- how to bring this change to the mainstream. here we have two alternatives : either I do this restructuring for etm only and then you take care of using the helper functions for bts and pt or I commit it as it is and then I issue a patch for this point only for bts PT and ETM. What do you prefer? >> + length = fread (buffer, 1, length, file.get ()); >> + buffer[length]='\0'; > Spaces around =. [Zied] done. > >> + while ((--length) != 0) >> + { >> + if ((buffer[length] == ',') || (buffer[length] == '-')) >> + { >> + length++; >> + break; >> + } >> + } >> + >> + int cpu_count; >> + int found = sscanf (&buffer[length], "%d", &cpu_count); >> + if (found < 1) >> + error (_("Failed to get cpu count in %s: %s."), >> + buffer, safe_strerror (errno)); >> + >> + cpu_count ++; >> + return (cpu_count); > No need for (). [Zied] done. > > >> + char filename[PATH_MAX]; >> + snprintf (filename, PATH_MAX, > sizeof (filename) [Zied] done. > >> + char filename[PATH_MAX]; >> + >> + /* Get coresight register from sysfs. */ >> + snprintf (filename, PATH_MAX, > sizeof (filename) [Zied] done > >> + "/sys/bus/event_source/devices/cs_etm/cpu%d/%s", cpu, path); >> + 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; >> +} > Empty line before return? I'd also remove the empty line between the > declaration of val and the call to fscanf (). > > There are several very similar functions in this patch and each is structured > differently: > > +perf_event_etm_event_type () > +{ > [...] > + > + 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_cpu_count (void) > +{ > [...] > + > + int cpu_count; > + int found = sscanf (&buffer[length], "%d", &cpu_count); > + if (found < 1) > + error (_("Failed to get cpu count in %s: %s."), > + buffer, safe_strerror (errno)); > + > + cpu_count ++; > + return (cpu_count); > > +perf_event_etm_event_sink (const struct btrace_config_etm *conf) > +{ > [...] > + > + 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; > > +cs_etm_get_register (int cpu, const char *path) > +{ > [...] > + > + 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; > [Zied] existing code in this file has an empty line before the last return everywhere. shall I stick to this convention? eliminating the empty lines before returns will bring inconsistency in the file. which coding convention do we have to apply here? >> + >> +#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)); > In patch 3, you wrote > > + /* Trace id for this thread. > + On a linux system, trace_id is assigned per cpu. The kernel copies > + the traces of each thread in a dedicated ring buffer. By this, > + traces belonging to different threads are de-multiplexed. > + On an RTOS system, especially when routing the traces outside of the SoC, > + the OS has no other mean for de-multiplexing the traces than > + the trace_id. The hardware (ETM IP) reserves 7 bits for the trace_id. > + On linux system trace id is not needed, set it to 0xFF to ignore it > + during parsing. */ > + uint8_t trace_id; > > Should this function return uint8_t and check that the ID is 7 bit max? [Zied] done, reserved and not allowed values are also generating a warning now. > > >> +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 explicit checks on bool. [Zied] done. > > >> +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; > Please declare at initialization time. [Zied] pushed forwards before the for loop > >> + conf->cpu_count = get_cpu_count (); >> + conf->etm_trace_params = new std::vector; >> + for (int i = 0; i < conf->cpu_count; i++) >> + { >> + fill_etm_trace_params (&etm_trace_params,i); >> + conf->etm_trace_params->push_back (etm_trace_params); >> + } > We need to avoid leaking the vector when fill_etm_trace_params () throws. > > > >> +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; > Please combine. No forward declarations anymore. The old code was written > when GDB was still C. [Zied] done. > > > regards, > markus. > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0,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 GmbH Leuschnerstraße 2 69469 Weinheim/Germany Mobile: +491722645127 mailto:zied.guermazi@trande.de *Trande GmbH* 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.