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

  Enable ptwrite in the PT config, if it is supported by the kernel.
---
 gdb/nat/linux-btrace.c | 58 ++++++++++++++++++++++++++++++++++++++++++
 gdb/record-btrace.c    |  5 ++++
 2 files changed, 63 insertions(+)
  

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
  

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

Hello Felix,

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

We could warn at decode-time when applying the ptwrite filter.
We should have different warning texts for GDB (via libipt) not
supporting ptwrite and the system (kernel or h/w) not supporting it.

Not as good as an error when installing the ptwrite filter but better
than wondering why it isn't working as expected.

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
  

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 8. August 2023 12:13
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; simark@simark.ca
> Subject: RE: [PATCH v10 08/10] btrace, linux: Enable ptwrite packets.
> 
> Hello Felix,
> 
> >> >+#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.
> 
> We could warn at decode-time when applying the ptwrite filter.
> We should have different warning texts for GDB (via libipt) not
> supporting ptwrite and the system (kernel or h/w) not supporting it.
> 
> Not as good as an error when installing the ptwrite filter but better
> than wondering why it isn't working as expected.

Mhm, I don't see this either. We can't warn when we actually would
apply the filter, as we never would get a ptwrite event. As we don't
enable ptwrite in Linux if libipt doesn't support it.  And if libipt doesn't
support it, we wouldn't get to that place in GDB in the first place,
as there is an #ifdef protecting it.

I looked at doing it when installing the extension language callback
for ptwrite in btrace.c, e.g. something similar to this:

  /* Register the ptwrite filter.  */
  if (btinfo->target->conf.pt.ptwrite)
	apply_ext_lang_ptwrite_filter (btinfo);
  else
    warning (_("Can't decode ptwrite events."));

This we do regardless of libipt. But then we would get a warning
regardless of the user trying to use ptwrite or not (with old libipt
versions). Plus this doesn't compile, as struct btrace_target_info
is forward declared, and we probably can't change that as btrace.c
should be target independent. 

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
  

Hello Felix,

>> >> >+#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.
>>
>> We could warn at decode-time when applying the ptwrite filter.
>> We should have different warning texts for GDB (via libipt) not
>> supporting ptwrite and the system (kernel or h/w) not supporting it.
>>
>> Not as good as an error when installing the ptwrite filter but better
>> than wondering why it isn't working as expected.
>
>Mhm, I don't see this either. We can't warn when we actually would
>apply the filter, as we never would get a ptwrite event.

I was referring to the apply_ext_lang_ptwrite_filter() call in ftrace_add_pt().

> As we don't
>enable ptwrite in Linux if libipt doesn't support it.  And if libipt doesn't
>support it, we wouldn't get to that place in GDB in the first place,
>as there is an #ifdef protecting it.
>
>I looked at doing it when installing the extension language callback
>for ptwrite in btrace.c, e.g. something similar to this:
>
>  /* Register the ptwrite filter.  */
>  if (btinfo->target->conf.pt.ptwrite)
>	apply_ext_lang_ptwrite_filter (btinfo);
>  else
>    warning (_("Can't decode ptwrite events."));
>
>This we do regardless of libipt. But then we would get a warning
>regardless of the user trying to use ptwrite or not (with old libipt
>versions). Plus this doesn't compile, as struct btrace_target_info
>is forward declared, and we probably can't change that as btrace.c
>should be target independent.

We'd want the check inside gdbpy_load_ptwrite_filter(), which gets
called by apply_ext_lang_ptwrite_filter() to install the btrace callback.

This is GDB code so we should be able to check the libipt version as
well ask kernel support.

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
  

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Mittwoch, 9. August 2023 11:48
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; simark@simark.ca
> Subject: RE: [PATCH v10 08/10] btrace, linux: Enable ptwrite packets.
> 
> Hello Felix,
> 
> >> >> >+#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.
> >>
> >> We could warn at decode-time when applying the ptwrite filter.
> >> We should have different warning texts for GDB (via libipt) not
> >> supporting ptwrite and the system (kernel or h/w) not supporting it.
> >>
> >> Not as good as an error when installing the ptwrite filter but better
> >> than wondering why it isn't working as expected.
> >
> >Mhm, I don't see this either. We can't warn when we actually would
> >apply the filter, as we never would get a ptwrite event.
> 
> I was referring to the apply_ext_lang_ptwrite_filter() call in ftrace_add_pt().
> 
> > As we don't
> >enable ptwrite in Linux if libipt doesn't support it.  And if libipt doesn't
> >support it, we wouldn't get to that place in GDB in the first place,
> >as there is an #ifdef protecting it.
> >
> >I looked at doing it when installing the extension language callback
> >for ptwrite in btrace.c, e.g. something similar to this:
> >
> >  /* Register the ptwrite filter.  */
> >  if (btinfo->target->conf.pt.ptwrite)
> >	apply_ext_lang_ptwrite_filter (btinfo);
> >  else
> >    warning (_("Can't decode ptwrite events."));
> >
> >This we do regardless of libipt. But then we would get a warning
> >regardless of the user trying to use ptwrite or not (with old libipt
> >versions). Plus this doesn't compile, as struct btrace_target_info
> >is forward declared, and we probably can't change that as btrace.c
> >should be target independent.
> 
> We'd want the check inside gdbpy_load_ptwrite_filter(), which gets
> called by apply_ext_lang_ptwrite_filter() to install the btrace callback.

If I add a similar warning to what I posted above for btrace.c, to
python/py-record-btrace.c:gdbpy_load_ptwrite_filter(), I still can't
compile. I get the following error:

py-record-btrace.c:906:22: error: invalid use of incomplete type ?struct btrace_target_info?
  906 |   if (!btinfo->target->conf.pt.ptwrite)

The solution would be to include nat/linux-btrace.h to the file, or to move
that struct to some other common header, e.g. btrace-common.h.
I don't think either is possible, it is adding linux specific things to common files.
Or is there any other solution that I missed?

Another point is, even if I would put it there, gdbpy_load_ptwrite_filter()
is called regardless of the user having installed a python filter.
But I think we could additionally check that get_ptwrite_filter() didn't return
Py_None to avoid that issue.

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
  

Hello Felix,

>> >I looked at doing it when installing the extension language callback
>> >for ptwrite in btrace.c, e.g. something similar to this:
>> >
>> >  /* Register the ptwrite filter.  */
>> >  if (btinfo->target->conf.pt.ptwrite)
>> >	apply_ext_lang_ptwrite_filter (btinfo);
>> >  else
>> >    warning (_("Can't decode ptwrite events."));
>> >
>> >This we do regardless of libipt. But then we would get a warning
>> >regardless of the user trying to use ptwrite or not (with old libipt
>> >versions). Plus this doesn't compile, as struct btrace_target_info
>> >is forward declared, and we probably can't change that as btrace.c
>> >should be target independent.
>>
>> We'd want the check inside gdbpy_load_ptwrite_filter(), which gets
>> called by apply_ext_lang_ptwrite_filter() to install the btrace callback.
>
>If I add a similar warning to what I posted above for btrace.c, to
>python/py-record-btrace.c:gdbpy_load_ptwrite_filter(), I still can't
>compile. I get the following error:
>
>py-record-btrace.c:906:22: error: invalid use of incomplete type ?struct
>btrace_target_info?
>  906 |   if (!btinfo->target->conf.pt.ptwrite)
>
>The solution would be to include nat/linux-btrace.h to the file, or to move
>that struct to some other common header, e.g. btrace-common.h.
>I don't think either is possible, it is adding linux specific things to common files.
>Or is there any other solution that I missed?
>
>Another point is, even if I would put it there, gdbpy_load_ptwrite_filter()
>is called regardless of the user having installed a python filter.
>But I think we could additionally check that get_ptwrite_filter() didn't return
>Py_None to avoid that issue.

Yes, that's what I meant.  Warn if a ptwrite filter is installed but ptwrite is not
supported.

We cannot use btrace_target_info directly, so we'd need to add another function
to check whether ptwrite is supported/enabled on the target.

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