RE: [PATCH v10 08/10] btrace, linux: Enable ptwrite packets.

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Felix,

>+/* Read config bits.  */
>+
>+static bool
>+linux_read_pt_config_bit (const std::string &feature, uint64_t *config_bit)

This isn't really necessary since the bit resembles the enable bit in the control MSR.
It also doesn't hurt so I'm OK with the change.

>+{
>+  std::string filename
>+      = "/sys/bus/event_source/devices/intel_pt/format/" + feature;
>+  gdb_file_up file = gdb_fopen_cloexec (filename.c_str (), "r");
>+
>+  if (file.get () == nullptr || config_bit == nullptr)
>+    return false;

We could check CONFIG_BIT before opening the file.

>+
>+  int found = fscanf (file.get (), "config:%lu", config_bit);

The format string doesn't match the type.  We should use SCNu64.

>+  if (found != 1)
>+    {
>+      warning (_("Failed to determine config bit from %s."),
>+	       filename.c_str ());

The issue is that the file didn't contain the expected content.
This will probably need some changes in GDB to fix.  Could we
print the actual content and what we expected?


>+/* Check whether the linux target supports Intel Processor Trace PTWRITE.  */
>+
>+static bool
>+linux_supports_ptwrite (uint64_t *config_bit)
>+{
>+  static const char filename[]
>+      = "/sys/bus/event_source/devices/intel_pt/caps/ptwrite";
>+  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
>+
>+  if (file.get () == nullptr)
>+    return false;
>+
>+  int status, found = fscanf (file.get (), "%d", &status);
>+
>+  if (found != 1)
>+    {
>+      warning (_("Failed to determine ptwrite support from %s."), filename);
>+      return false;
>+    }
>+
>+  if (!linux_read_pt_config_bit ("ptw", config_bit))

We should check STATUS before.  If caps indicates that ptwrite isn't
available, I'm not sure format will contain the bit position for enabling
it.

>+  uint64_t config_bit;
>+  if (conf->ptwrite && linux_supports_ptwrite (&config_bit))
>+    {
>+      pt->attr.config |= 1 << config_bit;
>+      tinfo->conf.pt.ptwrite = conf->ptwrite;
>+    }
>+
>   errno = 0;
>   scoped_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
>   if (fd.get () < 0)
>diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>index c93b3d7c8de..ad3160d42c5 100644
>--- a/gdb/record-btrace.c
>+++ b/gdb/record-btrace.c
>@@ -3295,4 +3295,9 @@ to see the actual buffer size."), NULL,
>show_record_pt_buffer_size_value,
>
>   record_btrace_conf.bts.size = 64 * 1024;
>   record_btrace_conf.pt.size = 16 * 1024;
>+#if (LIBIPT_VERSION >= 0x200)
>+  record_btrace_conf.pt.ptwrite = true;
>+#else
>+  record_btrace_conf.pt.ptwrite = false;
>+#endif

Is there a way to get an error when attempting to install a custom
ptwrite filter when either GDB (via libipt) or the kernel does not
support PTWRITE?

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