[PATCH v3 00/12] plugins/execlog: add data address match and address range support

[PATCH v3 00/12] plugins/execlog: add data address match and address range support

[Sven Schnelle]

Hi List,

this patchset adds a new -dfilter option and address range matching. With this
execlog can match only a certain range of address for both instruction and
data adresses.

Example usage:

qemu-system-xxx <other options> -d plugin -plugin libexeclog.so,afilter=0x1000-0x2000,dfilter=0x388

This would only log instruction in the address range 0x1000 to 0x2000
and accessing data at address 0x388.

Changes in v3:
- extend plugin api to re-use the existing range parsing infrastructure
- export error report api

Changes in v2:
- rebased on top of latest master

Sven Schnelle (12):
  util/log: convert debug_regions to GList
  util/log: make qemu_set_dfilter_ranges() take a GList
  util/range: move range_list_from_string() to range.c
  util/range: add range_list_free()
  util/range: use append_new_range() in range_list_from_string()
  util/range: split up range_list_from_string()
  util/range: make range_list_from_string() accept a single number
  qemu/range: add range_list_contains() function
  plugins: add API to print errors
  plugins: add range list API
  plugins/execlog: use range list api
  plugins/execlog: add data address match

 contrib/plugins/execlog.c    |  55 +++++++++++--------
 include/qemu/qemu-plugin.h   |  53 ++++++++++++++++++
 include/qemu/range.h         |  24 +++++++++
 plugins/api.c                |  25 +++++++++
 plugins/qemu-plugins.symbols |   4 ++
 util/log.c                   |  84 ++---------------------------
 util/range.c                 | 102 +++++++++++++++++++++++++++++++++++
 7 files changed, 244 insertions(+), 103 deletions(-)

--
2.43.2

[PATCH v3 01/12] util/log: convert debug_regions to GList

[Sven Schnelle]

In preparation of making the parsing part of qemu_set_dfilter_ranges()
available to other users, convert it to return a GList, so the result
can be used with other functions taking a GList of struct Range.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 util/log.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/util/log.c b/util/log.c
index d36c98da0b..779c0689a9 100644
--- a/util/log.c
+++ b/util/log.c
@@ -46,7 +46,7 @@ static __thread Notifier qemu_log_thread_cleanup_notifier;
 
 int qemu_loglevel;
 static bool log_per_thread;
-static GArray *debug_regions;
+static GList *debug_regions;
 
 /* Returns true if qemu_log() will really write somewhere. */
 bool qemu_log_enabled(void)
@@ -368,38 +368,36 @@ bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp)
  */
 bool qemu_log_in_addr_range(uint64_t addr)
 {
-    if (debug_regions) {
-        int i = 0;
-        for (i = 0; i < debug_regions->len; i++) {
-            Range *range = &g_array_index(debug_regions, Range, i);
-            if (range_contains(range, addr)) {
-                return true;
-            }
-        }
-        return false;
-    } else {
+    GList *p;
+
+    if (!debug_regions) {
         return true;
     }
-}
 
+    for (p = g_list_first(debug_regions); p; p = g_list_next(p)) {
+        if (range_contains(p->data, addr)) {
+            return true;
+        }
+    }
+    return false;
+}
 
 void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
 {
     gchar **ranges = g_strsplit(filter_spec, ",", 0);
+    struct Range *range = NULL;
     int i;
 
     if (debug_regions) {
-        g_array_unref(debug_regions);
+        g_list_free_full(debug_regions, g_free);
         debug_regions = NULL;
     }
 
-    debug_regions = g_array_sized_new(FALSE, FALSE,
-                                      sizeof(Range), g_strv_length(ranges));
     for (i = 0; ranges[i]; i++) {
         const char *r = ranges[i];
         const char *range_op, *r2, *e;
         uint64_t r1val, r2val, lob, upb;
-        struct Range range;
+        range = g_new0(struct Range, 1);
 
         range_op = strstr(r, "-");
         r2 = range_op ? range_op + 1 : NULL;
@@ -448,10 +446,12 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
             error_setg(errp, "Invalid range");
             goto out;
         }
-        range_set_bounds(&range, lob, upb);
-        g_array_append_val(debug_regions, range);
+        range_set_bounds(range, lob, upb);
+        debug_regions = g_list_append(debug_regions, range);
+        range = NULL;
     }
 out:
+    g_free(range);
     g_strfreev(ranges);
 }
 
-- 
2.43.2

Re: [PATCH v3 01/12] util/log: convert debug_regions to GList

[Alex Bennée]

Sven Schnelle <svens@stackframe.org> writes:

> In preparation of making the parsing part of qemu_set_dfilter_ranges()
> available to other users, convert it to return a GList, so the result
> can be used with other functions taking a GList of struct Range.

Why do we need to convert it to a Glist? When I originally wrote the
dfilter code I deliberately chose GArray over GList to avoid bouncing
across cache lines during the tight loop that is checking ranges. It's
not like we can't pass GArray's to plugins as well?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 01/12] util/log: convert debug_regions to GList

[Sven Schnelle]

Alex Bennée <alex.bennee@linaro.org> writes:

> Sven Schnelle <svens@stackframe.org> writes:
>
>> In preparation of making the parsing part of qemu_set_dfilter_ranges()
>> available to other users, convert it to return a GList, so the result
>> can be used with other functions taking a GList of struct Range.
>
> Why do we need to convert it to a Glist? When I originally wrote the
> dfilter code I deliberately chose GArray over GList to avoid bouncing
> across cache lines during the tight loop that is checking ranges. It's
> not like we can't pass GArray's to plugins as well?

Good point. I'll change it back to a GArray in the next iteration.

Re: [PATCH v3 01/12] util/log: convert debug_regions to GList

[Richard Henderson]

On 3/4/24 03:13, Sven Schnelle wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
>> Sven Schnelle <svens@stackframe.org> writes:
>>
>>> In preparation of making the parsing part of qemu_set_dfilter_ranges()
>>> available to other users, convert it to return a GList, so the result
>>> can be used with other functions taking a GList of struct Range.
>>
>> Why do we need to convert it to a Glist? When I originally wrote the
>> dfilter code I deliberately chose GArray over GList to avoid bouncing
>> across cache lines during the tight loop that is checking ranges. It's
>> not like we can't pass GArray's to plugins as well?
> 
> Good point. I'll change it back to a GArray in the next iteration.

How many address ranges do you expect to have?
If more than a couple, then perhaps IntervalTree would be better for a balanced binary search.


r~

Re: [PATCH v3 01/12] util/log: convert debug_regions to GList

[Sven Schnelle]

Alex Bennée <alex.bennee@linaro.org> writes:

> Sven Schnelle <svens@stackframe.org> writes:
>
>> In preparation of making the parsing part of qemu_set_dfilter_ranges()
>> available to other users, convert it to return a GList, so the result
>> can be used with other functions taking a GList of struct Range.
>
> Why do we need to convert it to a Glist? When I originally wrote the
> dfilter code I deliberately chose GArray over GList to avoid bouncing
> across cache lines during the tight loop that is checking ranges. It's
> not like we can't pass GArray's to plugins as well?

Looking at the code again, i remember that the reason for choosing GList
was that the other range matching function all take GList's - having
some functions take GArray's, and some not would be odd. So i wonder
whether we should convert all of those functions to GArray? (if
possible, i haven't checked)

What do you think?

Re: [PATCH v3 01/12] util/log: convert debug_regions to GList

[Alex Bennée]

Sven Schnelle <svens@stackframe.org> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Sven Schnelle <svens@stackframe.org> writes:
>>
>>> In preparation of making the parsing part of qemu_set_dfilter_ranges()
>>> available to other users, convert it to return a GList, so the result
>>> can be used with other functions taking a GList of struct Range.
>>
>> Why do we need to convert it to a Glist? When I originally wrote the
>> dfilter code I deliberately chose GArray over GList to avoid bouncing
>> across cache lines during the tight loop that is checking ranges. It's
>> not like we can't pass GArray's to plugins as well?
>
> Looking at the code again, i remember that the reason for choosing GList
> was that the other range matching function all take GList's - having
> some functions take GArray's, and some not would be odd.

Ahh I see. There wasn't intended to be much of a relationship between
the dfilter code and the range code apart from the fact I re-used the
Range typedef to avoid duplication.

> So i wonder
> whether we should convert all of those functions to GArray? (if
> possible, i haven't checked)

I think that would depend on access patterns and flexibility. For the
dfilter there is no particular need for sorting and the principle
operation is a sequence of lookups. For the other use cases keeping a
sorted list and quick insertion might be more useful.

While its tempting to go on a wider re-factoring I would caution against
it so close to softfreeze.

> What do you think?

Lets stick to the dfilter case and worry about wider clean-ups later. As
Richard points out it might be the interval tree makes more sense for
some of these things.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 01/12] util/log: convert debug_regions to GList

[Sven Schnelle]

Alex Bennée <alex.bennee@linaro.org> writes:

>> So i wonder
>> whether we should convert all of those functions to GArray? (if
>> possible, i haven't checked)
>
> I think that would depend on access patterns and flexibility. For the
> dfilter there is no particular need for sorting and the principle
> operation is a sequence of lookups. For the other use cases keeping a
> sorted list and quick insertion might be more useful.
>
> While its tempting to go on a wider re-factoring I would caution against
> it so close to softfreeze.
>
>> What do you think?
>
> Lets stick to the dfilter case and worry about wider clean-ups later. As
> Richard points out it might be the interval tree makes more sense for
> some of these things.

I think i go with the GArray variant for now. I'd guess that -dfilter is
usually only used with one or a few arguments, so using a Interval Tree
is probably not neccessary.

[PATCH v3 02/12] util/log: make qemu_set_dfilter_ranges() take a GList

[Sven Schnelle]

In preparation of making qemu_set_dfilter_ranges() available
to other users, move the code to a new function range_list_from_string()
which takes an additional GList argument.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 util/log.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/util/log.c b/util/log.c
index 779c0689a9..f183ea4e4d 100644
--- a/util/log.c
+++ b/util/log.c
@@ -382,15 +382,16 @@ bool qemu_log_in_addr_range(uint64_t addr)
     return false;
 }
 
-void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
+static void range_list_from_string(GList **out_ranges, const char *filter_spec,
+                                   Error **errp)
 {
     gchar **ranges = g_strsplit(filter_spec, ",", 0);
     struct Range *range = NULL;
     int i;
 
-    if (debug_regions) {
-        g_list_free_full(debug_regions, g_free);
-        debug_regions = NULL;
+    if (*out_ranges) {
+        g_list_free_full(*out_ranges, g_free);
+        *out_ranges = NULL;
     }
 
     for (i = 0; ranges[i]; i++) {
@@ -447,7 +448,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
             goto out;
         }
         range_set_bounds(range, lob, upb);
-        debug_regions = g_list_append(debug_regions, range);
+       *out_ranges = g_list_append(*out_ranges, range);
         range = NULL;
     }
 out:
@@ -455,6 +456,11 @@ out:
     g_strfreev(ranges);
 }
 
+void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
+{
+    range_list_from_string(&debug_regions, filter_spec, errp);
+}
+
 const QEMULogItem qemu_log_items[] = {
     { CPU_LOG_TB_OUT_ASM, "out_asm",
       "show generated host assembly code for each compiled TB" },
-- 
2.43.2

[PATCH v3 03/12] util/range: move range_list_from_string() to range.c

[Sven Schnelle]

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 include/qemu/range.h |  7 ++++
 util/log.c           | 74 ------------------------------------------
 util/range.c         | 76 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 74 deletions(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 205e1da76d..530b0c7db1 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -233,4 +233,11 @@ void range_inverse_array(GList *in_ranges,
                          GList **out_ranges,
                          uint64_t low, uint64_t high);
 
+/*
+ * Parse a comma separated list of address ranges into a @GArray
+ * of @Range entries. On error @errp is set.
+ */
+void range_list_from_string(GList **out_ranges, const char *filter_spec,
+                            Error **errp);
+
 #endif
diff --git a/util/log.c b/util/log.c
index f183ea4e4d..90897ef0f9 100644
--- a/util/log.c
+++ b/util/log.c
@@ -382,80 +382,6 @@ bool qemu_log_in_addr_range(uint64_t addr)
     return false;
 }
 
-static void range_list_from_string(GList **out_ranges, const char *filter_spec,
-                                   Error **errp)
-{
-    gchar **ranges = g_strsplit(filter_spec, ",", 0);
-    struct Range *range = NULL;
-    int i;
-
-    if (*out_ranges) {
-        g_list_free_full(*out_ranges, g_free);
-        *out_ranges = NULL;
-    }
-
-    for (i = 0; ranges[i]; i++) {
-        const char *r = ranges[i];
-        const char *range_op, *r2, *e;
-        uint64_t r1val, r2val, lob, upb;
-        range = g_new0(struct Range, 1);
-
-        range_op = strstr(r, "-");
-        r2 = range_op ? range_op + 1 : NULL;
-        if (!range_op) {
-            range_op = strstr(r, "+");
-            r2 = range_op ? range_op + 1 : NULL;
-        }
-        if (!range_op) {
-            range_op = strstr(r, "..");
-            r2 = range_op ? range_op + 2 : NULL;
-        }
-        if (!range_op) {
-            error_setg(errp, "Bad range specifier");
-            goto out;
-        }
-
-        if (qemu_strtou64(r, &e, 0, &r1val)
-            || e != range_op) {
-            error_setg(errp, "Invalid number to the left of %.*s",
-                       (int)(r2 - range_op), range_op);
-            goto out;
-        }
-        if (qemu_strtou64(r2, NULL, 0, &r2val)) {
-            error_setg(errp, "Invalid number to the right of %.*s",
-                       (int)(r2 - range_op), range_op);
-            goto out;
-        }
-
-        switch (*range_op) {
-        case '+':
-            lob = r1val;
-            upb = r1val + r2val - 1;
-            break;
-        case '-':
-            upb = r1val;
-            lob = r1val - (r2val - 1);
-            break;
-        case '.':
-            lob = r1val;
-            upb = r2val;
-            break;
-        default:
-            g_assert_not_reached();
-        }
-        if (lob > upb) {
-            error_setg(errp, "Invalid range");
-            goto out;
-        }
-        range_set_bounds(range, lob, upb);
-       *out_ranges = g_list_append(*out_ranges, range);
-        range = NULL;
-    }
-out:
-    g_free(range);
-    g_strfreev(ranges);
-}
-
 void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
 {
     range_list_from_string(&debug_regions, filter_spec, errp);
diff --git a/util/range.c b/util/range.c
index f3f40098d5..bd2d0961bd 100644
--- a/util/range.c
+++ b/util/range.c
@@ -19,6 +19,8 @@
 
 #include "qemu/osdep.h"
 #include "qemu/range.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
 
 int range_compare(Range *a, Range *b)
 {
@@ -121,3 +123,77 @@ void range_inverse_array(GList *in, GList **rev,
 exit:
     *rev = out;
 }
+
+void range_list_from_string(GList **out_ranges, const char *filter_spec,
+                            Error **errp)
+{
+    gchar **ranges = g_strsplit(filter_spec, ",", 0);
+    struct Range *range = NULL;
+    int i;
+
+    if (*out_ranges) {
+        g_list_free_full(*out_ranges, g_free);
+        *out_ranges = NULL;
+    }
+
+    for (i = 0; ranges[i]; i++) {
+        const char *r = ranges[i];
+        const char *range_op, *r2, *e;
+        uint64_t r1val, r2val, lob, upb;
+        range = g_new0(struct Range, 1);
+
+        range_op = strstr(r, "-");
+        r2 = range_op ? range_op + 1 : NULL;
+        if (!range_op) {
+            range_op = strstr(r, "+");
+            r2 = range_op ? range_op + 1 : NULL;
+        }
+        if (!range_op) {
+            range_op = strstr(r, "..");
+            r2 = range_op ? range_op + 2 : NULL;
+        }
+        if (!range_op) {
+            error_setg(errp, "Bad range specifier");
+            goto out;
+        }
+
+        if (qemu_strtou64(r, &e, 0, &r1val)
+            || e != range_op) {
+            error_setg(errp, "Invalid number to the left of %.*s",
+                       (int)(r2 - range_op), range_op);
+            goto out;
+        }
+        if (qemu_strtou64(r2, NULL, 0, &r2val)) {
+            error_setg(errp, "Invalid number to the right of %.*s",
+                       (int)(r2 - range_op), range_op);
+            goto out;
+        }
+
+        switch (*range_op) {
+        case '+':
+            lob = r1val;
+            upb = r1val + r2val - 1;
+            break;
+        case '-':
+            upb = r1val;
+            lob = r1val - (r2val - 1);
+            break;
+        case '.':
+            lob = r1val;
+            upb = r2val;
+            break;
+        default:
+            g_assert_not_reached();
+        }
+        if (lob > upb) {
+            error_setg(errp, "Invalid range");
+            goto out;
+        }
+        range_set_bounds(range, lob, upb);
+        *out_ranges = g_list_append(*out_ranges, range);
+        range = NULL;
+    }
+out:
+    g_free(range);
+    g_strfreev(ranges);
+}
-- 
2.43.2

[PATCH v3 04/12] util/range: add range_list_free()

[Sven Schnelle]

Introduce range_list_free(), which takes a GList of ranges
and frees the list and each range.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 include/qemu/range.h | 5 +++++
 util/range.c         | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 530b0c7db1..4ff9799d89 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -240,4 +240,9 @@ void range_inverse_array(GList *in_ranges,
 void range_list_from_string(GList **out_ranges, const char *filter_spec,
                             Error **errp);
 
+/*
+ * Free a list of ranges.
+ */
+void range_list_free(GList *ranges);
+
 #endif
diff --git a/util/range.c b/util/range.c
index bd2d0961bd..7234ab7a53 100644
--- a/util/range.c
+++ b/util/range.c
@@ -197,3 +197,8 @@ out:
     g_free(range);
     g_strfreev(ranges);
 }
+
+void range_list_free(GList *ranges)
+{
+    g_list_free_full(ranges, g_free);
+}
-- 
2.43.2

[PATCH v3 05/12] util/range: use append_new_range() in range_list_from_string()

[Sven Schnelle]

Use append_new_ranges() instead of manually allocating and
filling the new range member.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 util/range.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/util/range.c b/util/range.c
index 7234ab7a53..db535de9a7 100644
--- a/util/range.c
+++ b/util/range.c
@@ -128,7 +128,6 @@ void range_list_from_string(GList **out_ranges, const char *filter_spec,
                             Error **errp)
 {
     gchar **ranges = g_strsplit(filter_spec, ",", 0);
-    struct Range *range = NULL;
     int i;
 
     if (*out_ranges) {
@@ -140,7 +139,6 @@ void range_list_from_string(GList **out_ranges, const char *filter_spec,
         const char *r = ranges[i];
         const char *range_op, *r2, *e;
         uint64_t r1val, r2val, lob, upb;
-        range = g_new0(struct Range, 1);
 
         range_op = strstr(r, "-");
         r2 = range_op ? range_op + 1 : NULL;
@@ -189,12 +187,9 @@ void range_list_from_string(GList **out_ranges, const char *filter_spec,
             error_setg(errp, "Invalid range");
             goto out;
         }
-        range_set_bounds(range, lob, upb);
-        *out_ranges = g_list_append(*out_ranges, range);
-        range = NULL;
+        *out_ranges = append_new_range(*out_ranges, lob, upb);
     }
 out:
-    g_free(range);
     g_strfreev(ranges);
 }
 
-- 
2.43.2

[PATCH v3 06/12] util/range: split up range_list_from_string()

[Sven Schnelle]

Makes the code a bit easier to read and maintain.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 util/range.c | 119 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 70 insertions(+), 49 deletions(-)

diff --git a/util/range.c b/util/range.c
index db535de9a7..8c463995e7 100644
--- a/util/range.c
+++ b/util/range.c
@@ -124,6 +124,74 @@ exit:
     *rev = out;
 }
 
+static const char *split_single_range(const char *r, const char **r2)
+{
+    char *range_op;
+
+    range_op = strstr(r, "-");
+    if (range_op) {
+        *r2 = range_op + 1;
+        return range_op;
+    }
+    range_op = strstr(r, "+");
+    if (range_op) {
+        *r2 = range_op + 1;
+        return range_op;
+    }
+    range_op = strstr(r, "..");
+    if (range_op) {
+        *r2 = range_op + 2;
+        return range_op;
+    }
+    return NULL;
+}
+
+static int parse_single_range(const char *r, Error **errp,
+                              uint64_t *lob, uint64_t *upb)
+{
+    const char *range_op, *r2, *e;
+    uint64_t r1val, r2val;
+
+    range_op = split_single_range(r, &r2);
+    if (!range_op) {
+        error_setg(errp, "Bad range specifier");
+        return 1;
+    }
+    if (qemu_strtou64(r, &e, 0, &r1val)
+        || e != range_op) {
+        error_setg(errp, "Invalid number to the left of %.*s",
+                   (int)(r2 - range_op), range_op);
+        return 1;
+    }
+    if (qemu_strtou64(r2, NULL, 0, &r2val)) {
+        error_setg(errp, "Invalid number to the right of %.*s",
+                   (int)(r2 - range_op), range_op);
+        return 1;
+    }
+
+    switch (*range_op) {
+    case '+':
+        *lob = r1val;
+        *upb = r1val + r2val - 1;
+        break;
+    case '-':
+        *upb = r1val;
+        *lob = r1val - (r2val - 1);
+        break;
+    case '.':
+        *lob = r1val;
+        *upb = r2val;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    if (*lob > *upb) {
+        error_setg(errp, "Invalid range");
+        return 1;
+    }
+    return 0;
+}
+
 void range_list_from_string(GList **out_ranges, const char *filter_spec,
                             Error **errp)
 {
@@ -136,60 +204,13 @@ void range_list_from_string(GList **out_ranges, const char *filter_spec,
     }
 
     for (i = 0; ranges[i]; i++) {
-        const char *r = ranges[i];
-        const char *range_op, *r2, *e;
-        uint64_t r1val, r2val, lob, upb;
-
-        range_op = strstr(r, "-");
-        r2 = range_op ? range_op + 1 : NULL;
-        if (!range_op) {
-            range_op = strstr(r, "+");
-            r2 = range_op ? range_op + 1 : NULL;
-        }
-        if (!range_op) {
-            range_op = strstr(r, "..");
-            r2 = range_op ? range_op + 2 : NULL;
-        }
-        if (!range_op) {
-            error_setg(errp, "Bad range specifier");
-            goto out;
-        }
-
-        if (qemu_strtou64(r, &e, 0, &r1val)
-            || e != range_op) {
-            error_setg(errp, "Invalid number to the left of %.*s",
-                       (int)(r2 - range_op), range_op);
-            goto out;
-        }
-        if (qemu_strtou64(r2, NULL, 0, &r2val)) {
-            error_setg(errp, "Invalid number to the right of %.*s",
-                       (int)(r2 - range_op), range_op);
-            goto out;
-        }
+        uint64_t lob, upb;
 
-        switch (*range_op) {
-        case '+':
-            lob = r1val;
-            upb = r1val + r2val - 1;
+        if (parse_single_range(ranges[i], errp, &lob, &upb)) {
             break;
-        case '-':
-            upb = r1val;
-            lob = r1val - (r2val - 1);
-            break;
-        case '.':
-            lob = r1val;
-            upb = r2val;
-            break;
-        default:
-            g_assert_not_reached();
-        }
-        if (lob > upb) {
-            error_setg(errp, "Invalid range");
-            goto out;
         }
         *out_ranges = append_new_range(*out_ranges, lob, upb);
     }
-out:
     g_strfreev(ranges);
 }
 
-- 
2.43.2

[PATCH v3 07/12] util/range: make range_list_from_string() accept a single number

[Sven Schnelle]

To use range_list_from_string() as a replacement in the execlog
plugin, make it accept single numbers instead of a range. This
might also be useful for the already present debug_ranges filtering.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 util/range.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/util/range.c b/util/range.c
index 8c463995e7..7784c21b12 100644
--- a/util/range.c
+++ b/util/range.c
@@ -154,6 +154,11 @@ static int parse_single_range(const char *r, Error **errp,
 
     range_op = split_single_range(r, &r2);
     if (!range_op) {
+        if (!qemu_strtou64(r, &e, 0, &r1val) && *e == '\0') {
+            *lob = r1val;
+            *upb = r1val;
+            return 0;
+        }
         error_setg(errp, "Bad range specifier");
         return 1;
     }
-- 
2.43.2

[PATCH v3 08/12] qemu/range: add range_list_contains() function

[Sven Schnelle]

Add a small inline function to match an address against
a list of ranges.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 include/qemu/range.h | 12 ++++++++++++
 util/log.c           | 10 +---------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 4ff9799d89..7ef9b3d5fb 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -57,6 +57,18 @@ static inline bool range_contains(const Range *range, uint64_t val)
     return val >= range->lob && val <= range->upb;
 }
 
+static inline bool range_list_contains(GList *ranges, uint64_t val)
+{
+    GList *p;
+
+    for (p = g_list_first(ranges); p; p = g_list_next(p)) {
+        if (range_contains(p->data, val)) {
+            return true;
+        }
+    }
+    return false;
+}
+
 /* Initialize @range to the empty range */
 static inline void range_make_empty(Range *range)
 {
diff --git a/util/log.c b/util/log.c
index 90897ef0f9..032110d700 100644
--- a/util/log.c
+++ b/util/log.c
@@ -368,18 +368,10 @@ bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp)
  */
 bool qemu_log_in_addr_range(uint64_t addr)
 {
-    GList *p;
-
     if (!debug_regions) {
         return true;
     }
-
-    for (p = g_list_first(debug_regions); p; p = g_list_next(p)) {
-        if (range_contains(p->data, addr)) {
-            return true;
-        }
-    }
-    return false;
+    return range_list_contains(debug_regions, addr);
 }
 
 void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp)
-- 
2.43.2

[PATCH v3 09/12] plugins: add API to print errors

[Sven Schnelle]

add qemu_plugin_error_print() which is a wrapper around
error_report_err(). This will be used by
qemu_plugin_parse_filter_ranges() to report parse failures.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 include/qemu/qemu-plugin.h   | 12 ++++++++++++
 plugins/api.c                |  7 +++++++
 plugins/qemu-plugins.symbols |  1 +
 3 files changed, 20 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 45e2ebc8f8..5839feea4d 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -752,5 +752,17 @@ QEMU_PLUGIN_API
 int qemu_plugin_read_register(struct qemu_plugin_register *handle,
                               GByteArray *buf);
 
+typedef struct Error Error;
+
+/**
+ * qemu_plugin_error_print() - print and free error
+ *
+ * @err: a @Error handle
+ *
+ * This function shows and and frees the supplied error.
+ */
+
+QEMU_PLUGIN_API
+void qemu_plugin_error_print(Error *err);
 
 #endif /* QEMU_QEMU_PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index 81f43c9ce8..8fd3a8964a 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -45,6 +45,7 @@
 #include "exec/ram_addr.h"
 #include "disas/disas.h"
 #include "plugin.h"
+#include "qapi/error.h"
 #ifndef CONFIG_USER_ONLY
 #include "qemu/plugin-memory.h"
 #include "hw/boards.h"
@@ -465,3 +466,9 @@ int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
 
     return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg));
 }
+
+void qemu_plugin_error_print(Error *err)
+{
+    error_report_err(err);
+}
+
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 27fe97239b..b142d11e58 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -2,6 +2,7 @@
   qemu_plugin_bool_parse;
   qemu_plugin_end_code;
   qemu_plugin_entry_code;
+  qemu_plugin_error_print;
   qemu_plugin_get_hwaddr;
   qemu_plugin_get_registers;
   qemu_plugin_hwaddr_device_name;
-- 
2.43.2

[PATCH v3 10/12] plugins: add range list API

[Sven Schnelle]

Export range_list_from_string(), range_contains() and range_list_free()
to allow plugins to parse filter ranges and match them to avoid
reimplementing this functionality.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 include/qemu/qemu-plugin.h   | 41 ++++++++++++++++++++++++++++++++++++
 plugins/api.c                | 18 ++++++++++++++++
 plugins/qemu-plugins.symbols |  3 +++
 3 files changed, 62 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 5839feea4d..4910a63d70 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -765,4 +765,45 @@ typedef struct Error Error;
 QEMU_PLUGIN_API
 void qemu_plugin_error_print(Error *err);
 
+typedef GList qemu_plugin_range_list;
+
+/**
+ * qemu_plugin_ranges_from_string() - parse a filter range string
+ *
+ * @out_ranges: a pointer to a @qemu_plugin_range_list pointer
+ * @filter_spec: input string
+ * @errp: @Error string on parse failure
+ *
+ * This function parses a filter specification string and stores the
+ * parsed adress ranges found in @out. On parse failure a @Error pointer
+ * is stored in @errp. The function accepts a comma-separated list
+ * of start and end addresses or single addresses.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_range_list_from_string(qemu_plugin_range_list **out_range,
+                                        const char *filter_spec,
+                                        Error **errp);
+
+/**
+ * qemu_plugin_range_list_contains() - match a value against a list
+ * of ranges
+ *
+ * @ranges: a pointer to a @qemu_plugin_range_list
+ * @val: the value to match
+ *
+ * This function matches @val against the adress range list in @ranges.
+ * On success, true is returned, otherwise false.
+ */
+QEMU_PLUGIN_API
+bool qemu_plugin_range_list_contains(qemu_plugin_range_list *ranges,
+                                     uint64_t val);
+
+/**
+ * qemu_plugin_range_list_free() - free a list of ranges
+ *
+ * @ranges: a pointer to the list to be freed
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_range_list_free(qemu_plugin_range_list *ranges);
+
 #endif /* QEMU_QEMU_PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index 8fd3a8964a..8dbd14c648 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -472,3 +472,21 @@ void qemu_plugin_error_print(Error *err)
     error_report_err(err);
 }
 
+void qemu_plugin_range_list_from_string(qemu_plugin_range_list **out,
+                                        const char *filter_spec,
+                                        Error **errp)
+{
+    return range_list_from_string(out, filter_spec, errp);
+}
+
+bool qemu_plugin_range_list_contains(qemu_plugin_range_list *ranges,
+                                     uint64_t val)
+{
+    return range_list_contains(ranges, val);
+}
+
+void qemu_plugin_range_list_free(qemu_plugin_range_list *ranges)
+{
+    return range_list_free(ranges);
+}
+
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index b142d11e58..472b29fc5f 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -21,6 +21,9 @@
   qemu_plugin_num_vcpus;
   qemu_plugin_outs;
   qemu_plugin_path_to_binary;
+  qemu_plugin_range_list_contains;
+  qemu_plugin_range_list_free;
+  qemu_plugin_range_list_from_string;
   qemu_plugin_read_register;
   qemu_plugin_register_atexit_cb;
   qemu_plugin_register_flush_cb;
-- 
2.43.2

Re: [PATCH v3 10/12] plugins: add range list API

[Bernhard Beschow]

Am 1. März 2024 17:46:07 UTC schrieb Sven Schnelle <svens@stackframe.org>:
>Export range_list_from_string(), range_contains() and range_list_free()
>to allow plugins to parse filter ranges and match them to avoid
>reimplementing this functionality.
>
>Signed-off-by: Sven Schnelle <svens@stackframe.org>
>---
> include/qemu/qemu-plugin.h   | 41 ++++++++++++++++++++++++++++++++++++
> plugins/api.c                | 18 ++++++++++++++++
> plugins/qemu-plugins.symbols |  3 +++
> 3 files changed, 62 insertions(+)
>
>diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>index 5839feea4d..4910a63d70 100644
>--- a/include/qemu/qemu-plugin.h
>+++ b/include/qemu/qemu-plugin.h
>@@ -765,4 +765,45 @@ typedef struct Error Error;
> QEMU_PLUGIN_API
> void qemu_plugin_error_print(Error *err);
> 
>+typedef GList qemu_plugin_range_list;
>+
>+/**
>+ * qemu_plugin_ranges_from_string() - parse a filter range string
>+ *
>+ * @out_ranges: a pointer to a @qemu_plugin_range_list pointer
>+ * @filter_spec: input string
>+ * @errp: @Error string on parse failure
>+ *
>+ * This function parses a filter specification string and stores the
>+ * parsed adress ranges found in @out. On parse failure a @Error pointer
>+ * is stored in @errp. The function accepts a comma-separated list
>+ * of start and end addresses or single addresses.
>+ */
>+QEMU_PLUGIN_API
>+void qemu_plugin_range_list_from_string(qemu_plugin_range_list **out_range,

Nice series in general. One nitpick: When the API docs are generated I get the following error:

    include/qemu/qemu-plugin.h:788: warning: Function parameter or member 'out_range' not described in 'qemu_plugin_range_list_from_string'

This is due to the parameter being documented as "out_ranges" which seems like the more appropriate name given its type. It might also be cleaner to have the same names in the source, too.

Best regards,
Bernhard

>+                                        const char *filter_spec,
>+                                        Error **errp);
>+
>+/**
>+ * qemu_plugin_range_list_contains() - match a value against a list
>+ * of ranges
>+ *
>+ * @ranges: a pointer to a @qemu_plugin_range_list
>+ * @val: the value to match
>+ *
>+ * This function matches @val against the adress range list in @ranges.
>+ * On success, true is returned, otherwise false.
>+ */
>+QEMU_PLUGIN_API
>+bool qemu_plugin_range_list_contains(qemu_plugin_range_list *ranges,
>+                                     uint64_t val);
>+
>+/**
>+ * qemu_plugin_range_list_free() - free a list of ranges
>+ *
>+ * @ranges: a pointer to the list to be freed
>+ */
>+QEMU_PLUGIN_API
>+void qemu_plugin_range_list_free(qemu_plugin_range_list *ranges);
>+
> #endif /* QEMU_QEMU_PLUGIN_H */
>diff --git a/plugins/api.c b/plugins/api.c
>index 8fd3a8964a..8dbd14c648 100644
>--- a/plugins/api.c
>+++ b/plugins/api.c
>@@ -472,3 +472,21 @@ void qemu_plugin_error_print(Error *err)
>     error_report_err(err);
> }
> 
>+void qemu_plugin_range_list_from_string(qemu_plugin_range_list **out,
>+                                        const char *filter_spec,
>+                                        Error **errp)
>+{
>+    return range_list_from_string(out, filter_spec, errp);
>+}
>+
>+bool qemu_plugin_range_list_contains(qemu_plugin_range_list *ranges,
>+                                     uint64_t val)
>+{
>+    return range_list_contains(ranges, val);
>+}
>+
>+void qemu_plugin_range_list_free(qemu_plugin_range_list *ranges)
>+{
>+    return range_list_free(ranges);
>+}
>+
>diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
>index b142d11e58..472b29fc5f 100644
>--- a/plugins/qemu-plugins.symbols
>+++ b/plugins/qemu-plugins.symbols
>@@ -21,6 +21,9 @@
>   qemu_plugin_num_vcpus;
>   qemu_plugin_outs;
>   qemu_plugin_path_to_binary;
>+  qemu_plugin_range_list_contains;
>+  qemu_plugin_range_list_free;
>+  qemu_plugin_range_list_from_string;
>   qemu_plugin_read_register;
>   qemu_plugin_register_atexit_cb;
>   qemu_plugin_register_flush_cb;

[PATCH v3 11/12] plugins/execlog: use range list api

[Sven Schnelle]

Instead of doing its own implementation, use the new range
list API to parse and match address lists.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 contrib/plugins/execlog.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index a1dfd59ab7..c518797893 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -35,7 +35,7 @@ static GArray *cpus;
 static GRWLock expand_array_lock;
 
 static GPtrArray *imatches;
-static GArray *amatches;
+static GList *amatches;
 static GPtrArray *rmatches;
 static bool disas_assist;
 static GMutex add_reg_name_lock;
@@ -215,12 +215,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
         }
 
         if (skip && amatches) {
-            int j;
-            for (j = 0; j < amatches->len && skip; j++) {
-                uint64_t v = g_array_index(amatches, uint64_t, j);
-                if (v == insn_vaddr) {
-                    skip = false;
-                }
+            if (qemu_plugin_range_list_contains(amatches, insn_vaddr)) {
+                skip = false;
             }
         }
 
@@ -398,16 +394,6 @@ static void parse_insn_match(char *match)
     g_ptr_array_add(imatches, g_strdup(match));
 }
 
-static void parse_vaddr_match(char *match)
-{
-    uint64_t v = g_ascii_strtoull(match, NULL, 16);
-
-    if (!amatches) {
-        amatches = g_array_new(false, true, sizeof(uint64_t));
-    }
-    g_array_append_val(amatches, v);
-}
-
 /*
  * We have to wait until vCPUs are started before we can check the
  * patterns find anything.
@@ -440,7 +426,12 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
         if (g_strcmp0(tokens[0], "ifilter") == 0) {
             parse_insn_match(tokens[1]);
         } else if (g_strcmp0(tokens[0], "afilter") == 0) {
-            parse_vaddr_match(tokens[1]);
+            Error *err = NULL;
+            qemu_plugin_range_list_from_string(&amatches, tokens[1], &err);
+            if (err) {
+                qemu_plugin_error_print(err);
+                return -1;
+            }
         } else if (g_strcmp0(tokens[0], "reg") == 0) {
             add_regpat(tokens[1]);
         } else if (g_strcmp0(tokens[0], "rdisas") == 0) {
-- 
2.43.2

[PATCH v3 12/12] plugins/execlog: add data address match

[Sven Schnelle]

Add a match similar to the afilter address match, but for data
addresses. When an address is specified with '-dfilter=0x12345'
only load/stores to/from address 0x12345 are printed. All other
instructions are hidden.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 contrib/plugins/execlog.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index c518797893..c44fd0e593 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -27,6 +27,8 @@ typedef struct CPU {
     GString *last_exec;
     /* Ptr array of Register */
     GPtrArray *registers;
+    /* whether this instruction should be logged */
+    bool log;
 } CPU;
 
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
@@ -36,6 +38,7 @@ static GRWLock expand_array_lock;
 
 static GPtrArray *imatches;
 static GList *amatches;
+static GList *dmatches;
 static GPtrArray *rmatches;
 static bool disas_assist;
 static GMutex add_reg_name_lock;
@@ -62,6 +65,11 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info,
 
     /* Find vCPU in array */
 
+    if (dmatches && !qemu_plugin_range_list_contains(dmatches, vaddr)) {
+        return;
+    }
+    c->log = true;
+
     /* Indicate type of memory access */
     if (qemu_plugin_mem_is_store(info)) {
         g_string_append(s, ", store");
@@ -121,15 +129,17 @@ static void vcpu_insn_exec_with_regs(unsigned int cpu_index, void *udata)
         if (cpu->registers) {
             insn_check_regs(cpu);
         }
-
-        qemu_plugin_outs(cpu->last_exec->str);
-        qemu_plugin_outs("\n");
+        if (cpu->log) {
+            qemu_plugin_outs(cpu->last_exec->str);
+            qemu_plugin_outs("\n");
+        }
     }
 
     /* Store new instruction in cache */
     /* vcpu_mem will add memory access information to last_exec */
     g_string_printf(cpu->last_exec, "%u, ", cpu_index);
     g_string_append(cpu->last_exec, (char *)udata);
+    cpu->log = dmatches ? false : true;
 }
 
 /* Log last instruction while checking registers, ignore next */
@@ -157,7 +167,7 @@ static void vcpu_insn_exec(unsigned int cpu_index, void *udata)
     CPU *cpu = get_cpu(cpu_index);
 
     /* Print previous instruction in cache */
-    if (cpu->last_exec->len) {
+    if (cpu->log && cpu->last_exec->len) {
         qemu_plugin_outs(cpu->last_exec->str);
         qemu_plugin_outs("\n");
     }
@@ -166,6 +176,7 @@ static void vcpu_insn_exec(unsigned int cpu_index, void *udata)
     /* vcpu_mem will add memory access information to last_exec */
     g_string_printf(cpu->last_exec, "%u, ", cpu_index);
     g_string_append(cpu->last_exec, (char *)udata);
+    cpu->log = dmatches ? false : true;
 }
 
 /**
@@ -377,7 +388,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
     g_rw_lock_reader_lock(&expand_array_lock);
     for (i = 0; i < cpus->len; i++) {
         CPU *c = get_cpu(i);
-        if (c->last_exec && c->last_exec->str) {
+        if (c->log && c->last_exec && c->last_exec->str) {
             qemu_plugin_outs(c->last_exec->str);
             qemu_plugin_outs("\n");
         }
@@ -432,6 +443,13 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                 qemu_plugin_error_print(err);
                 return -1;
             }
+        } else if (g_strcmp0(tokens[0], "dfilter") == 0) {
+            Error *err = NULL;
+            qemu_plugin_range_list_from_string(&dmatches, tokens[1], &err);
+            if (err) {
+                qemu_plugin_error_print(err);
+                return -1;
+            }
         } else if (g_strcmp0(tokens[0], "reg") == 0) {
             add_regpat(tokens[1]);
         } else if (g_strcmp0(tokens[0], "rdisas") == 0) {
-- 
2.43.2