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

Willgerodt, Felix via Gdb-patches
2023-08-07T09:11:31Z

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