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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 25. Juli 2023 15:30
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org;
> simark@simark.ca
> Subject: RE: [PATCH v10 08/10] btrace, linux: Enable ptwrite packets.
> 
> 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.

I was thinking to re-use this function for future features, like event tracing.
I didn't really dig into Linux code, I just felt that if there is a file for it lets read it.

> >+{
> >+  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.

Will do.

> >+
> >+  int found = fscanf (file.get (), "config:%lu", config_bit);
> 
> The format string doesn't match the type.  We should use SCNu64.

Will do.

> >+  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?

Yes, this could mean that the format changed. But it could also
just be a corrupcted files/system.
I don't think we want to dump the content of a possibly corrupt file.


> >+/* 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.

Makes sense, I will do that.

> >+  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?

Good point. Unfortunately I don't see one with the current design.
Currently we just cache the filter functions in python when one is
registered. The configuration isn't exposed to the python layer in
any way.

I am not sure how we could do this. Exposing "maint btrace info"
and parsing that would be one way, not sure if we want to go down
that path.

Thanks,
Felix
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