[PATCH v3 0/8] Add additional plugin API functions to read and write memory and registers

[PATCH v3 0/8] Add additional plugin API functions to read and write memory and registers

[Rowan Hart]

This patch series adds several new API functions focused on enabling use
cases around reading and writing guest memory from QEMU plugins. To support
these new APIs, some utility functionality around retrieving information about
address spaces is added as well.

The new qemu_plugin_write_register utilizes gdb_write_register, which is now
declared in gdbstub.h for this purpose instead of being static.

qemu_plugin_write_memory_vaddr utilizes cpu_memory_rw_debug much the same as
the existing read_memory_vaddr function does.

The read and write_hwaddr functions are the most different. These functions
use address_space_rw, which works well in most cases. There is an important
caveat that for writes, the page being written will be set dirty by the
write operation. This dirty setting requires locking the page range,
which can contend with an already held lock in page_collection_lock
when called in a tb translate callback with a write to the instruction
memory in the tb. The doc comments warn against doing this, and it's unlikely
anyone would want to do this.

I've also added two test plugins: one that implements a simple hypercall
interface that guest code can use to communicate with the plugin in a
structured way with a test to ensure that this hypercall works and writing
virtual memory works. And one that implements a simple patch utility to patch
memory at runtime. The test for the second plugin ensures the patch applies
successfully to instruction memory, and can use both hw and vaddr methods.

novafacing (8):
  Expose gdb_write_register function to consumers of gdbstub
  Add register write API
  Add address space API
  Add memory virtual address write API
  Add memory hardware address read/write API
  Add patcher plugin and test
  Add hypercalls plugin and test
  Update plugin version and add notes

 gdbstub/gdbstub.c                             |   2 +-
 include/exec/gdbstub.h                        |  14 +
 include/qemu/plugin.h                         |   6 +
 include/qemu/qemu-plugin.h                    | 217 ++++++-
 plugins/api.c                                 | 213 ++++++-
 tests/tcg/Makefile.target                     |   2 +
 tests/tcg/plugins/hypercalls.c                | 552 ++++++++++++++++++
 tests/tcg/plugins/meson.build                 |   2 +-
 tests/tcg/plugins/patch.c                     | 324 ++++++++++
 tests/tcg/x86_64/Makefile.softmmu-target      |  36 +-
 tests/tcg/x86_64/system/hypercalls-target.c   |  45 ++
 tests/tcg/x86_64/system/patch-target.c        |  32 +
 .../tcg/x86_64/system/validate-hypercalls.py  |  40 ++
 tests/tcg/x86_64/system/validate-patch.py     |  39 ++
 14 files changed, 1501 insertions(+), 23 deletions(-)
 create mode 100644 tests/tcg/plugins/hypercalls.c
 create mode 100644 tests/tcg/plugins/patch.c
 create mode 100644 tests/tcg/x86_64/system/hypercalls-target.c
 create mode 100644 tests/tcg/x86_64/system/patch-target.c
 create mode 100755 tests/tcg/x86_64/system/validate-hypercalls.py
 create mode 100755 tests/tcg/x86_64/system/validate-patch.py

-- 
2.49.0

[PATCH v3 1/8] Expose gdb_write_register function to consumers of gdbstub

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 gdbstub/gdbstub.c      |  2 +-
 include/exec/gdbstub.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 565f6b33a9..5846e481be 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -534,7 +534,7 @@ int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
     return 0;
 }
 
-static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
+int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
 {
     GDBRegisterState *r;
 
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 0675b0b646..a16c0051ce 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -124,6 +124,20 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname);
  */
 int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 
+/**
+ * gdb_write_register() - Write a register associated with a CPU.
+ * @cpu: The CPU associated with the register.
+ * @buf: The buffer that the register contents will be set to.
+ * @reg: The register's number returned by gdb_find_feature_register().
+ *
+ * The size of @buf must be at least the size of the register being
+ * written.
+ *
+ * Return: The number of written bytes, or 0 if an error occurred (for
+ * example, an unknown register was provided).
+ */
+int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg);
+
 /**
  * typedef GDBRegDesc - a register description from gdbstub
  */
-- 
2.49.0

[PATCH v3 2/8] Add register write API

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 include/qemu/qemu-plugin.h | 57 +++++++++++++++++++++++++-------------
 plugins/api.c              | 26 ++++++++++++-----
 2 files changed, 56 insertions(+), 27 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 3a850aa216..68c8632fd7 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -254,9 +254,6 @@ typedef struct {
  * @QEMU_PLUGIN_CB_NO_REGS: callback does not access the CPU's regs
  * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
  * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
- *
- * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change
- * system register state.
  */
 enum qemu_plugin_cb_flags {
     QEMU_PLUGIN_CB_NO_REGS,
@@ -871,7 +868,8 @@ struct qemu_plugin_register;
 /**
  * typedef qemu_plugin_reg_descriptor - register descriptions
  *
- * @handle: opaque handle for retrieving value with qemu_plugin_read_register
+ * @handle: opaque handle for retrieving value with qemu_plugin_read_register or
+ *          writing value with qemu_plugin_write_register
  * @name: register name
  * @feature: optional feature descriptor, can be NULL
  */
@@ -893,6 +891,41 @@ typedef struct {
 QEMU_PLUGIN_API
 GArray *qemu_plugin_get_registers(void);
 
+/**
+ * qemu_plugin_read_register() - read register for current vCPU
+ *
+ * @handle: a @qemu_plugin_reg_handle handle
+ * @buf: A GByteArray for the data owned by the plugin
+ *
+ * This function is only available in a context that register read access is
+ * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
+ *
+ * Returns the size of the read register. The content of @buf is in target byte
+ * order. On failure returns -1.
+ */
+QEMU_PLUGIN_API
+int qemu_plugin_read_register(struct qemu_plugin_register *handle,
+                              GByteArray *buf);
+
+/**
+ * qemu_plugin_write_register() - write register for current vCPU
+ *
+ * @handle: a @qemu_plugin_reg_handle handle
+ * @buf: A GByteArray for the data owned by the plugin
+ *
+ * This function is only available in a context that register write access is
+ * explicitly requested via the QEMU_PLUGIN_CB_W_REGS flag.
+ *
+ * The size of @buf must be at least the size of the requested register.
+ * Attempting to write a register with @buf smaller than the register size
+ * will result in a crash or other undesired behavior.
+ *
+ * Returns the number of bytes written. On failure returns 0.
+ */
+QEMU_PLUGIN_API
+int qemu_plugin_write_register(struct qemu_plugin_register *handle,
+                              GByteArray *buf);
+
 /**
  * qemu_plugin_read_memory_vaddr() - read from memory using a virtual address
  *
@@ -915,22 +948,6 @@ QEMU_PLUGIN_API
 bool qemu_plugin_read_memory_vaddr(uint64_t addr,
                                    GByteArray *data, size_t len);
 
-/**
- * qemu_plugin_read_register() - read register for current vCPU
- *
- * @handle: a @qemu_plugin_reg_handle handle
- * @buf: A GByteArray for the data owned by the plugin
- *
- * This function is only available in a context that register read access is
- * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
- *
- * Returns the size of the read register. The content of @buf is in target byte
- * order. On failure returns -1.
- */
-QEMU_PLUGIN_API
-int qemu_plugin_read_register(struct qemu_plugin_register *handle,
-                              GByteArray *buf);
-
 /**
  * qemu_plugin_scoreboard_new() - alloc a new scoreboard
  *
diff --git a/plugins/api.c b/plugins/api.c
index 3c9d4832e9..79b2dc20b8 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -433,6 +433,25 @@ GArray *qemu_plugin_get_registers(void)
     return create_register_handles(regs);
 }
 
+int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
+{
+    g_assert(current_cpu);
+
+    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
+}
+
+int qemu_plugin_write_register(struct qemu_plugin_register *reg, 
+                               GByteArray *buf)
+{
+    g_assert(current_cpu);
+
+    if (buf->len == 0) {
+        return 0;
+    }
+
+    return gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1);
+}
+
 bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data, size_t len)
 {
     g_assert(current_cpu);
@@ -453,13 +472,6 @@ bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data, size_t len)
     return true;
 }
 
-int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
-{
-    g_assert(current_cpu);
-
-    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
-}
-
 struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
 {
     return plugin_scoreboard_new(element_size);
-- 
2.49.0

[PATCH v3 3/8] Add address space API

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 include/qemu/plugin.h      |  6 +++
 include/qemu/qemu-plugin.h | 45 ++++++++++++++++++++++
 plugins/api.c              | 79 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 9726a9ebf3..38439a37fa 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -139,6 +139,12 @@ struct qemu_plugin_tb {
     GArray *cbs;
 };
 
+/* Internal context for address space information */
+struct qemu_plugin_address_space_info {
+    CPUState *cpu;
+    GPtrArray *names;
+};
+
 /**
  * struct CPUPluginState - per-CPU state for plugins
  * @event_mask: plugin event bitmap. Modified only via async work.
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 68c8632fd7..1380f7d441 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -926,6 +926,51 @@ QEMU_PLUGIN_API
 int qemu_plugin_write_register(struct qemu_plugin_register *handle,
                               GByteArray *buf);
 
+/** struct qemu_plugin_address_space_info - Opaque handle for space info */
+struct qemu_plugin_address_space_info;
+
+/**
+ * qemu_plugin_get_current_vcpu_address_spaces() - get a list of address spaces
+ * for the current vCPU
+ *
+ * This function should be called in vCPU context, i.e. from a vCPU, translation
+ * block, or operation callback.
+ *
+ * This function is only valid for softmmu targets.
+ *
+ * Returns an opaque qemu_plugin_address_space* handle that is only valid for
+ * the duration of the callback. The caller is not responsible for freeing the
+ * result.
+ */
+QEMU_PLUGIN_API
+struct qemu_plugin_address_space_info*
+qemu_plugin_get_current_vcpu_address_spaces(void);
+
+/**
+ * qemu_plugin_n_address_spaces() - get the number of address spaces
+ *
+ * @info: opaque handle to address space information
+ *
+ * Returns the number of address spaces, or -1 if the handle is invalid.
+ */
+QEMU_PLUGIN_API
+int qemu_plugin_n_address_spaces(struct qemu_plugin_address_space_info *info);
+
+/**
+ * qemu_plugin_address_space_name() - get the name of an address space
+ *
+ * @info: opaque handle to address space information
+ * @idx: index of the address space
+ *
+ * Returns the name of the address space, or NULL if the handle is invalid. The
+ * caller is responsible for freeing the result.
+ *
+ */
+QEMU_PLUGIN_API
+const char*
+qemu_plugin_address_space_name(struct qemu_plugin_address_space_info *info,
+                               unsigned int idx);
+
 /**
  * qemu_plugin_read_memory_vaddr() - read from memory using a virtual address
  *
diff --git a/plugins/api.c b/plugins/api.c
index 79b2dc20b8..d1cc6ff86e 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -39,6 +39,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/plugin.h"
 #include "qemu/log.h"
+#include "system/memory.h"
 #include "tcg/tcg.h"
 #include "exec/gdbstub.h"
 #include "exec/target_page.h"
@@ -452,6 +453,84 @@ int qemu_plugin_write_register(struct qemu_plugin_register *reg,
     return gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1);
 }
 
+#ifdef CONFIG_SOFTMMU
+static __thread struct qemu_plugin_address_space_info address_space_info = {
+    NULL, NULL
+};
+static void free_g_string_and_data(gpointer data)
+{
+    g_string_free(data, true);
+}
+#endif
+
+struct qemu_plugin_address_space_info*
+qemu_plugin_get_current_vcpu_address_spaces(void)
+{
+#ifdef CONFIG_SOFTMMU
+    CPUState *cpu = current_cpu;
+
+    if (address_space_info.names == NULL) {
+        address_space_info.cpu = NULL;
+        address_space_info.names = g_ptr_array_new();
+        g_ptr_array_set_free_func(address_space_info.names,
+                                  free_g_string_and_data);
+    }
+
+    g_ptr_array_set_size(address_space_info.names, 0);
+
+    for (size_t i = 0; i < cpu->cpu_ases_count; i++) {
+        AddressSpace *as = cpu_get_address_space(cpu, i);
+
+        if (as == NULL || as->name == NULL) {
+            return NULL;
+        }
+
+        g_ptr_array_add(address_space_info.names,
+                        g_string_new(as->name));
+    }
+
+    address_space_info.cpu = cpu;
+
+    return &address_space_info;
+#else
+    return NULL;
+#endif
+}
+
+int qemu_plugin_n_address_spaces(struct qemu_plugin_address_space_info *info)
+{
+#ifdef CONFIG_SOFTMMU
+    if (info->cpu != current_cpu) {
+        address_space_info.cpu = NULL;
+        g_ptr_array_set_size(address_space_info.names, 0);
+        return -1;
+    }
+
+    return info->names->len;
+#else
+    return -1;
+#endif
+}
+
+const char *
+qemu_plugin_address_space_name(struct qemu_plugin_address_space_info *info,
+                               unsigned int idx)
+{
+#ifdef CONFIG_SOFTMMU
+    if (info->cpu != current_cpu) {
+        address_space_info.cpu = NULL;
+        g_ptr_array_set_size(address_space_info.names, 0);
+        return NULL;
+    }
+
+    if (idx < info->names->len) {
+        GString *name = g_ptr_array_index(info->names, idx);
+        return g_strdup(name->str);
+    }
+#endif
+    return NULL;
+}
+
 bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data, size_t len)
 {
     g_assert(current_cpu);
-- 
2.49.0

[PATCH v3 4/8] Add memory virtual address write API

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 include/qemu/qemu-plugin.h | 21 +++++++++++++++++++++
 plugins/api.c              | 18 ++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 1380f7d441..eff8430b4a 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -993,6 +993,27 @@ QEMU_PLUGIN_API
 bool qemu_plugin_read_memory_vaddr(uint64_t addr,
                                    GByteArray *data, size_t len);
 
+/**
+ * qemu_plugin_write_memory_vaddr() - write to memory using a virtual address
+ *
+ * @addr: A virtual address to write to
+ * @data: A byte array containing the data to write
+ *
+ * The contents of @data will be written to memory starting at the virtual
+ * address @addr.
+ *
+ * This function does not guarantee consistency of writes, nor does it ensure
+ * that pending writes are flushed either before or after the write takes place,
+ * so callers should take care to only call this function in vCPU context (i.e.
+ * in callbacks) and avoid depending on the existence of data written using this
+ * function which may be overwritten afterward.
+ *
+ * Returns true on success and false on failure.
+ */
+QEMU_PLUGIN_API
+bool qemu_plugin_write_memory_vaddr(uint64_t addr,
+                                   GByteArray *data);
+
 /**
  * qemu_plugin_scoreboard_new() - alloc a new scoreboard
  *
diff --git a/plugins/api.c b/plugins/api.c
index d1cc6ff86e..19c10bb39e 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -551,6 +551,24 @@ bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data, size_t len)
     return true;
 }
 
+bool qemu_plugin_write_memory_vaddr(uint64_t addr, GByteArray *data)
+{
+    g_assert(current_cpu);
+
+    if (data->len == 0) {
+        return false;
+    }
+
+    int result = cpu_memory_rw_debug(current_cpu, addr, data->data,
+                                     data->len, true);
+
+    if (result < 0) {
+        return false;
+    }
+
+    return true;
+}
+
 struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
 {
     return plugin_scoreboard_new(element_size);
-- 
2.49.0

[PATCH v3 5/8] Add memory hardware address read/write API

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 include/qemu/qemu-plugin.h |  96 +++++++++++++++++++++++++++++++++++
 plugins/api.c              | 100 +++++++++++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index eff8430b4a..d4f229abd9 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -1014,6 +1014,102 @@ QEMU_PLUGIN_API
 bool qemu_plugin_write_memory_vaddr(uint64_t addr,
                                    GByteArray *data);
 
+/**
+ * enum qemu_plugin_hwaddr_operation_result - result of a memory operation
+ *
+ * @QEMU_PLUGIN_HWADDR_OPERATION_OK: hwaddr operation succeeded
+ * @QEMU_PLUGIN_HWADDR_OPERATION_ERROR: unexpected error occurred
+ * @QEMU_PLUGIN_HWADDR_OPERATION_DEVICE_ERROR: error in memory device
+ * @QEMU_PLUGIN_HWADDR_OPERATION_ACCESS_DENIED: permission error
+ * @QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS: address was invalid
+ * @QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS_SPACE: invalid address space
+ */
+enum qemu_plugin_hwaddr_operation_result {
+    QEMU_PLUGIN_HWADDR_OPERATION_OK,
+    QEMU_PLUGIN_HWADDR_OPERATION_ERROR,
+    QEMU_PLUGIN_HWADDR_OPERATION_DEVICE_ERROR,
+    QEMU_PLUGIN_HWADDR_OPERATION_ACCESS_DENIED,
+    QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS,
+    QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS_SPACE,
+};
+
+/**
+ * qemu_plugin_read_memory_hwaddr() - read from memory using a hardware address
+ *
+ * @as_idx: The index of the address space to read from
+ * @addr: A physical address to read from
+ * @data: A byte array to store data into
+ * @len: The number of bytes to read, starting from @addr
+ *
+ * @len bytes of data is read starting at @addr and stored into @data. If @data
+ * is not large enough to hold @len bytes, it will be expanded to the necessary
+ * size, reallocating if necessary. @len must be greater than 0.
+ *
+ * This function does not ensure writes are flushed prior to reading, so
+ * callers should take care when calling this function in plugin callbacks to
+ * avoid attempting to read data which may not yet be written and should use
+ * the memory callback API instead.
+ *
+ * This function is only valid for softmmu targets.
+ *
+ * Returns a qemu_plugin_hwaddr_operation_result indicating the result of the
+ * operation.
+ */
+QEMU_PLUGIN_API
+enum qemu_plugin_hwaddr_operation_result
+qemu_plugin_read_memory_hwaddr(unsigned int as_idx, uint64_t addr,
+                                    GByteArray *data, size_t len);
+
+/**
+ * qemu_plugin_write_memory_hwaddr() - write to memory using a hardware address
+ *
+ * @as_idx: The index of the address space to write to
+ * @addr: A physical address to write to
+ * @data: A byte array containing the data to write
+ *
+ * The contents of @data will be written to memory starting at the hardware
+ * address @addr.
+ *
+ * This function does not guarantee consistency of writes, nor does it ensure
+ * that pending writes are flushed either before or after the write takes place,
+ * so callers should take care when calling this function in plugin callbacks to
+ * avoid depending on the existence of data written using this function which
+ * may be overwritten afterward. In addition, this function requires that the
+ * pages containing the address are not locked. Practically, this means that you
+ * should not write instruction memory in a current translation block inside a
+ * callback registered with qemu_plugin_register_vcpu_tb_trans_cb.
+ *
+ * You can, for example, write instruction memory in a current translation block
+ * in a callback registered with qemu_plugin_register_vcpu_tb_exec_cb, although
+ * be aware that the write will not be flushed until after the translation block
+ * has finished executing.  In general, this function should be used to write
+ * data memory or to patch code at a known address, not in a current translation
+ * block.
+ *
+ * This function is only valid for softmmu targets.
+ *
+ * Returns a qemu_plugin_hwaddr_operation_result indicating the result of the
+ * operation.
+ */
+QEMU_PLUGIN_API
+enum qemu_plugin_hwaddr_operation_result
+qemu_plugin_write_memory_hwaddr(unsigned int as_idx, uint64_t addr,
+                                     GByteArray *data);
+
+/**
+ * qemu_plugin_translate_vaddr() - translate a virtual address to a physical one
+ *
+ * @vaddr: virtual address to translate
+ * @hwaddr: pointer to store the physical address
+ *
+ * This function is only valid in vCPU context (i.e. in callbacks) and is only
+ * valid for softmmu targets.
+ *
+ * Returns true on success and false on failure.
+ */
+QEMU_PLUGIN_API
+bool qemu_plugin_translate_vaddr(uint64_t vaddr, uint64_t *hwaddr);
+
 /**
  * qemu_plugin_scoreboard_new() - alloc a new scoreboard
  *
diff --git a/plugins/api.c b/plugins/api.c
index 19c10bb39e..5983768783 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -569,6 +569,106 @@ bool qemu_plugin_write_memory_vaddr(uint64_t addr, GByteArray *data)
     return true;
 }
 
+enum qemu_plugin_hwaddr_operation_result
+qemu_plugin_read_memory_hwaddr(unsigned int as_idx, hwaddr addr,
+                               GByteArray *data, size_t len)
+{
+#ifdef CONFIG_SOFTMMU
+    CPUState *cpu = current_cpu;
+
+    if (len == 0 || as_idx >= cpu->cpu_ases_count) {
+        return QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS_SPACE;
+    }
+
+    g_byte_array_set_size(data, len);
+
+    AddressSpace *as = cpu_get_address_space(cpu, as_idx);
+
+    if (as == NULL) {
+        return QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS_SPACE;
+    }
+
+    MemTxResult res = address_space_rw(as, addr,
+                                       MEMTXATTRS_UNSPECIFIED, data->data,
+                                       data->len, false);
+
+    switch (res) {
+    case MEMTX_OK:
+        return QEMU_PLUGIN_HWADDR_OPERATION_OK;
+    case MEMTX_ERROR:
+        return QEMU_PLUGIN_HWADDR_OPERATION_DEVICE_ERROR;
+    case MEMTX_DECODE_ERROR:
+        return QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS;
+    case MEMTX_ACCESS_ERROR:
+        return QEMU_PLUGIN_HWADDR_OPERATION_ACCESS_DENIED;
+    default:
+        return QEMU_PLUGIN_HWADDR_OPERATION_ERROR;
+    }
+#else
+    return QEMU_PLUGIN_HWADDR_OPERATION_ERROR;
+#endif
+}
+
+enum qemu_plugin_hwaddr_operation_result
+qemu_plugin_write_memory_hwaddr(unsigned int as_idx, hwaddr addr, 
+                                GByteArray *data)
+{
+#ifdef CONFIG_SOFTMMU
+    CPUState *cpu = current_cpu;
+
+    if (data->len == 0 || as_idx >= cpu->cpu_ases_count) {
+        return QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS_SPACE;
+    }
+
+    AddressSpace *as = cpu_get_address_space(cpu, as_idx);
+
+    if (as == NULL) {
+        return QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS_SPACE;
+    }
+
+    qemu_plugin_outs("Got cpu address space...\n");
+
+    MemTxResult res = address_space_rw(as, addr,
+                                       MEMTXATTRS_UNSPECIFIED, data->data,
+                                       data->len, true);
+    switch (res) {
+    case MEMTX_OK:
+        return QEMU_PLUGIN_HWADDR_OPERATION_OK;
+    case MEMTX_ERROR:
+        return QEMU_PLUGIN_HWADDR_OPERATION_DEVICE_ERROR;
+    case MEMTX_DECODE_ERROR:
+        return QEMU_PLUGIN_HWADDR_OPERATION_INVALID_ADDRESS;
+    case MEMTX_ACCESS_ERROR:
+        return QEMU_PLUGIN_HWADDR_OPERATION_ACCESS_DENIED;
+    default:
+        return QEMU_PLUGIN_HWADDR_OPERATION_ERROR;
+    }
+#else
+    return QEMU_PLUGIN_HWADDR_OPERATION_ERROR;
+#endif
+}
+
+bool qemu_plugin_translate_vaddr(uint64_t vaddr, uint64_t *hwaddr)
+{
+#ifdef CONFIG_SOFTMMU
+    g_assert(current_cpu);
+
+    CPUState *cpu = current_cpu;
+
+    uint64_t res = cpu_get_phys_page_debug(cpu, vaddr);
+
+    if (res == (uint64_t)-1) {
+        return false;
+    }
+
+    *hwaddr = res | (vaddr & ~TARGET_PAGE_MASK);
+
+    return true;
+#else
+    return false;
+#endif
+}
+
 struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
 {
     return plugin_scoreboard_new(element_size);
-- 
2.49.0

[PATCH v3 6/8] Add patcher plugin and test

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 tests/tcg/Makefile.target                 |   1 +
 tests/tcg/plugins/meson.build             |   2 +-
 tests/tcg/plugins/patch.c                 | 324 ++++++++++++++++++++++
 tests/tcg/x86_64/Makefile.softmmu-target  |  32 ++-
 tests/tcg/x86_64/system/patch-target.c    |  32 +++
 tests/tcg/x86_64/system/validate-patch.py |  39 +++
 6 files changed, 424 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/plugins/patch.c
 create mode 100644 tests/tcg/x86_64/system/patch-target.c
 create mode 100755 tests/tcg/x86_64/system/validate-patch.py

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 95ff76ea44..4b709a9d18 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -176,6 +176,7 @@ RUN_TESTS+=$(EXTRA_RUNS)
 # Some plugins need additional arguments above the default to fully
 # exercise things. We can define them on a per-test basis here.
 run-plugin-%-with-libmem.so: PLUGIN_ARGS=$(COMMA)inline=true
+run-plugin-%-with-libpatch.so: PLUGIN_ARGS=$(COMMA)target=ffffffff$(COMMA)patch=00000000
 
 ifeq ($(filter %-softmmu, $(TARGET)),)
 run-%: %
diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
index 41f02f2c7f..163042e601 100644
--- a/tests/tcg/plugins/meson.build
+++ b/tests/tcg/plugins/meson.build
@@ -1,6 +1,6 @@
 t = []
 if get_option('plugins')
-  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall']
+  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall', 'patch']
     if host_os == 'windows'
       t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
                         include_directories: '../../../include/qemu',
diff --git a/tests/tcg/plugins/patch.c b/tests/tcg/plugins/patch.c
new file mode 100644
index 0000000000..6f2bc3688e
--- /dev/null
+++ b/tests/tcg/plugins/patch.c
@@ -0,0 +1,324 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (C) 2025, Rowan Hart <rowanbhart@gmail.com>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ *
+ * This plugin patches instructions matching a pattern to a different
+ * instruction as they execute
+ *
+ */
+
+#include "glib.h"
+#include "glibconfig.h"
+
+#include <qemu-plugin.h>
+#include <string.h>
+#include <stdio.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+static bool use_hwaddr;
+static bool debug_insns;
+static GByteArray *target_data;
+static GByteArray *patch_data;
+
+/**
+ * Parse a string of hexadecimal digits into a GByteArray. The string must be
+ * even length
+ */ 
+static GByteArray *str_to_bytes(const char *str)
+{
+    GByteArray *bytes = g_byte_array_new();
+    char byte[3] = {0};
+    size_t len = strlen(str);
+    guint8 value = 0;
+
+    if (len % 2 != 0) {
+        g_byte_array_free(bytes, true);
+        return NULL;
+    }
+
+    for (size_t i = 0; i < len; i += 2) {
+        byte[0] = str[i];
+        byte[1] = str[i + 1];
+        value = (guint8)g_ascii_strtoull(byte, NULL, 16);
+        g_byte_array_append(bytes, &value, 1);
+    }
+
+    return bytes;
+}
+
+static void patch_hwaddr(unsigned int vcpu_index, void *userdata)
+{
+    uint64_t addr = (uint64_t)userdata;
+    GString *str = g_string_new(NULL);
+    g_string_printf(str, "patching: @0x%"
+                    PRIx64 "\n",
+                    addr);
+    qemu_plugin_outs(str->str);
+    g_string_free(str, true);
+
+    struct qemu_plugin_address_space_info *info =
+        qemu_plugin_get_current_vcpu_address_spaces();
+
+    if (!info) {
+        qemu_plugin_outs("Failed to get address spaces\n");
+        return;
+    }
+
+    qemu_plugin_outs("Got address spaces\n");
+
+    int n_address_spaces = qemu_plugin_n_address_spaces(info);
+    unsigned int memory_idx;
+
+    for (memory_idx = 0; memory_idx < n_address_spaces; memory_idx++) {
+        const char *name = qemu_plugin_address_space_name(info, memory_idx);
+        if (!g_strcmp0(name, "cpu-memory-0")) {
+            break;
+        }
+    }
+
+    if (memory_idx >= n_address_spaces) {
+        qemu_plugin_outs("No matching address space\n");
+        return;
+    }
+
+    qemu_plugin_outs("Got address space cpu-memory-0\n");
+
+    qemu_plugin_outs("Writing memory (hwaddr)...\n");
+    enum qemu_plugin_hwaddr_operation_result result =
+        qemu_plugin_write_memory_hwaddr(memory_idx, addr, patch_data);
+
+
+    if (result != QEMU_PLUGIN_HWADDR_OPERATION_OK) {
+        GString *errmsg = g_string_new(NULL);
+        g_string_printf(errmsg, "Failed to write memory: %d\n", result);
+        qemu_plugin_outs(errmsg->str);
+        g_string_free(errmsg, true);
+        return;
+    }
+
+    GByteArray *read_data = g_byte_array_new();
+
+    result = qemu_plugin_read_memory_hwaddr(memory_idx, addr, read_data, patch_data->len);
+
+    qemu_plugin_outs("Reading memory...\n");
+
+    if (result != QEMU_PLUGIN_HWADDR_OPERATION_OK) {
+        GString *errmsg = g_string_new(NULL);
+        g_string_printf(errmsg, "Failed to read memory: %d\n", result);
+        qemu_plugin_outs(errmsg->str);
+        g_string_free(errmsg, true);
+        return;
+    }
+
+    if (memcmp(patch_data->data, read_data->data, patch_data->len) != 0) {
+        qemu_plugin_outs("Failed to read back written data\n");
+    }
+
+    qemu_plugin_outs("Success!\n");
+
+    return;
+}
+
+static void patch_vaddr(unsigned int vcpu_index, void *userdata)
+{
+    uint64_t addr = (uint64_t)userdata;
+    GString *str = g_string_new(NULL);
+    g_string_printf(str, "patching: @0x%" 
+                    PRIx64 "\n",
+                    addr);
+    qemu_plugin_outs(str->str);
+    g_string_free(str, true);
+
+    qemu_plugin_outs("Writing memory (vaddr)...\n");
+
+    if (!qemu_plugin_write_memory_vaddr(addr, patch_data)) {
+        qemu_plugin_outs("Failed to write memory\n");
+        return;
+    }
+
+    qemu_plugin_outs("Reading memory (vaddr)...\n");
+
+
+    GByteArray *read_data = g_byte_array_new();
+
+    if (!qemu_plugin_read_memory_vaddr(addr, read_data, patch_data->len)) {
+        qemu_plugin_outs("Failed to read memory\n");
+        return;
+    }
+
+    if (memcmp(patch_data->data, read_data->data, patch_data->len) != 0) {
+        qemu_plugin_outs("Failed to read back written data\n");
+    }
+
+    qemu_plugin_outs("Success!\n");
+
+    return;
+}
+
+static void debug_disas(unsigned int vcpu_index, void *userdata)
+{
+    GString *debug_info = (GString *)userdata;
+    qemu_plugin_outs(debug_info->str);
+}
+
+static void debug_print_newline(unsigned int vcpu_index, void *userdata)
+{
+    qemu_plugin_outs("\n");
+}
+
+/*
+ * Callback on translation of a translation block.
+ */
+static void vcpu_tb_trans_cb(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    uint64_t addr = 0;
+    GByteArray *insn_data = g_byte_array_new();
+    for (size_t i = 0; i < qemu_plugin_tb_n_insns(tb); i++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+
+        if (use_hwaddr) {
+            uint64_t vaddr = qemu_plugin_insn_vaddr(insn);
+            if (!qemu_plugin_translate_vaddr(vaddr, &addr)) {
+                qemu_plugin_outs("Failed to translate vaddr\n");
+                continue;
+            }
+        } else {
+            addr = qemu_plugin_insn_vaddr(insn);
+        }
+
+        g_byte_array_set_size(insn_data, qemu_plugin_insn_size(insn));
+        qemu_plugin_insn_data(insn, insn_data->data, insn_data->len);
+
+        if (insn_data->len >= target_data->len &&
+            !memcmp(insn_data->data, target_data->data,
+                    MIN(target_data->len, insn_data->len))) {
+            if (use_hwaddr) {
+                qemu_plugin_register_vcpu_tb_exec_cb(tb, patch_hwaddr,
+                                                     QEMU_PLUGIN_CB_NO_REGS,
+                                                     (void *)addr);
+            } else {
+                qemu_plugin_register_vcpu_tb_exec_cb(tb, patch_vaddr,
+                                                     QEMU_PLUGIN_CB_NO_REGS,
+                                                     (void *)addr);
+            }
+        }
+    }
+    for (size_t i = 0; i < qemu_plugin_tb_n_insns(tb); i++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+        uint64_t vaddr = qemu_plugin_insn_vaddr(insn);
+        uint64_t hwaddr = (uint64_t)qemu_plugin_insn_haddr(insn);
+        uint64_t translated_hwaddr = 0;
+        if (!qemu_plugin_translate_vaddr(vaddr, &translated_hwaddr)) {
+            qemu_plugin_outs("Failed to translate vaddr\n");
+            continue;
+        }
+        char *disas = qemu_plugin_insn_disas(insn);
+        GString *str = g_string_new(NULL);
+        g_string_printf(str,
+                        "vaddr: 0x%" PRIx64 " hwaddr: 0x%" PRIx64
+                        " translated: 0x%" PRIx64 " : %s\n",
+                        vaddr, hwaddr, translated_hwaddr, disas);
+        g_free(disas);
+        if (debug_insns) {
+            qemu_plugin_register_vcpu_insn_exec_cb(insn, debug_disas,
+                                                   QEMU_PLUGIN_CB_NO_REGS,
+                                                   str);
+        }
+
+    }
+
+    if (debug_insns) {
+        qemu_plugin_register_vcpu_tb_exec_cb(tb, debug_print_newline,
+                                             QEMU_PLUGIN_CB_NO_REGS,
+                                             NULL);
+    }
+
+    g_byte_array_free(insn_data, true);
+}
+
+static void usage(void)
+{
+    fprintf(stderr, "Usage: <lib>,target=<target>,patch=<patch>"
+            "[,use_hwaddr=<use_hwaddr>]"
+            "[,debug_insns=<debug_insns>]\n");
+}
+
+/*
+ * Called when the plugin is installed
+ */
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info, int argc,
+                                           char **argv)
+{
+
+    use_hwaddr = true;
+    debug_insns = false;
+    target_data = NULL;
+    patch_data = NULL;
+
+    if (argc > 4) {
+        usage();
+        return -1;
+    }
+
+    for (size_t i = 0; i < argc; i++) {
+        char *opt = argv[i];
+        g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
+        if (g_strcmp0(tokens[0], "use_hwaddr") == 0) {
+            if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &use_hwaddr)) {
+                fprintf(stderr,
+                        "Failed to parse boolean argument use_hwaddr\n");
+                return -1;
+            }
+        } else if (g_strcmp0(tokens[0], "debug_insns") == 0) {
+            if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &debug_insns)) {
+                fprintf(stderr,
+                        "Failed to parse boolean argument debug_insns\n");
+                return -1;
+            }
+        } else if (g_strcmp0(tokens[0], "target") == 0) {
+            target_data = str_to_bytes(tokens[1]);
+            if (!target_data) {
+                fprintf(stderr,
+                         "Failed to parse target bytes.\n");
+                return -1;
+            }
+        } else if (g_strcmp0(tokens[0], "patch") == 0) {
+            patch_data = str_to_bytes(tokens[1]);
+            if (!patch_data) {
+                fprintf(stderr, "Failed to parse patch bytes.\n");
+                return -1;
+            }
+        } else {
+            fprintf(stderr, "Unknown argument: %s\n", tokens[0]);
+            usage();
+            return -1;
+        }
+    }
+
+    if (!target_data) {
+        fprintf(stderr, "target argument is required\n");
+        usage();
+        return -1;
+    }
+
+    if (!patch_data) {
+        fprintf(stderr, "patch argument is required\n");
+        usage();
+        return -1;
+    }
+
+    if (target_data->len != patch_data->len) {
+        fprintf(stderr, "Target and patch data must be the same length\n");
+        return -1;
+    }
+
+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans_cb);
+
+    return 0;
+}
diff --git a/tests/tcg/x86_64/Makefile.softmmu-target b/tests/tcg/x86_64/Makefile.softmmu-target
index ef6bcb4dc7..8d3a067c33 100644
--- a/tests/tcg/x86_64/Makefile.softmmu-target
+++ b/tests/tcg/x86_64/Makefile.softmmu-target
@@ -7,18 +7,27 @@
 #
 
 I386_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/i386/system
-X64_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/x86_64/system
+X86_64_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/x86_64/system
 
 # These objects provide the basic boot code and helper functions for all tests
 CRT_OBJS=boot.o
 
-CRT_PATH=$(X64_SYSTEM_SRC)
-LINK_SCRIPT=$(X64_SYSTEM_SRC)/kernel.ld
+X86_64_TEST_C_SRCS=$(wildcard $(X86_64_SYSTEM_SRC)/*.c)
+X86_64_TEST_S_SRCS=
+
+X86_64_C_TESTS = $(patsubst $(X86_64_SYSTEM_SRC)/%.c, %, $(X86_64_TEST_C_SRCS))
+X86_64_S_TESTS = $(patsubst $(X86_64_SYSTEM_SRC)/%.S, %, $(X86_64_TEST_S_SRCS))
+
+X86_64_TESTS = $(X86_64_C_TESTS)
+X86_64_TESTS += $(X86_64_S_TESTS)
+
+CRT_PATH=$(X86_64_SYSTEM_SRC)
+LINK_SCRIPT=$(X86_64_SYSTEM_SRC)/kernel.ld
 LDFLAGS=-Wl,-T$(LINK_SCRIPT) -Wl,-melf_x86_64
 CFLAGS+=-nostdlib -ggdb -O0 $(MINILIB_INC)
 LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc
 
-TESTS+=$(MULTIARCH_TESTS)
+TESTS+=$(X86_64_TESTS) $(MULTIARCH_TESTS)
 EXTRA_RUNS+=$(MULTIARCH_RUNS)
 
 # building head blobs
@@ -27,11 +36,24 @@ EXTRA_RUNS+=$(MULTIARCH_RUNS)
 %.o: $(CRT_PATH)/%.S
 	$(CC) $(CFLAGS) $(EXTRA_CFLAGS) -Wa,--noexecstack -c $< -o $@
 
-# Build and link the tests
+# Build and link the multiarch tests
 %: %.c $(LINK_SCRIPT) $(CRT_OBJS) $(MINILIB_OBJS)
 	$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
 
+# Build and link the arch tests
+%: $(X86_64_SYSTEM_SRC)/%.c $(LINK_SCRIPT) $(CRT_OBJS) $(MINILIB_OBJS)
+	$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
+
 memory: CFLAGS+=-DCHECK_UNALIGNED=1
+patch-target: CFLAGS+=-O0
 
 # Running
 QEMU_OPTS+=-device isa-debugcon,chardev=output -device isa-debug-exit,iobase=0xf4,iosize=0x4 -kernel
+
+# Add patch-target to ADDITIONAL_PLUGINS_TESTS
+ADDITIONAL_PLUGINS_TESTS += patch-target
+
+run-plugin-patch-target-with-libpatch.so:		\
+	PLUGIN_ARGS=$(COMMA)target=ffc0$(COMMA)patch=9090$(COMMA)use_hwaddr=true$(COMMA)debug_insns=false
+run-plugin-patch-target-with-libpatch.so:		\
+	CHECK_PLUGIN_OUTPUT_COMMAND=$(X86_64_SYSTEM_SRC)/validate-patch.py $@.out
\ No newline at end of file
diff --git a/tests/tcg/x86_64/system/patch-target.c b/tests/tcg/x86_64/system/patch-target.c
new file mode 100644
index 0000000000..671987a873
--- /dev/null
+++ b/tests/tcg/x86_64/system/patch-target.c
@@ -0,0 +1,32 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (C) 2025, Rowan Hart <rowanbhart@gmail.com>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ *
+ * This test target increments a value 100 times. The patcher converts the
+ * inc instruction to a nop, so it only increments the value once.
+ *
+ */
+#include <minilib.h>
+
+int main(void)
+{
+    ml_printf("Running test...\n");
+#if defined(__x86_64__)
+    ml_printf("Testing insn memory read/write...\n");
+    unsigned int x = 0;
+    for (int i = 0; i < 100; i++) {
+        asm volatile (
+            "inc %[x]"
+            : [x] "+a" (x)
+        );
+    }
+    ml_printf("Value: %d\n", x);
+#else
+    #error "This test is only valid for x86_64 architecture."
+#endif
+    return 0;
+}
diff --git a/tests/tcg/x86_64/system/validate-patch.py b/tests/tcg/x86_64/system/validate-patch.py
new file mode 100755
index 0000000000..700950eae5
--- /dev/null
+++ b/tests/tcg/x86_64/system/validate-patch.py
@@ -0,0 +1,39 @@
+#!/usr/bin/env python3
+#
+# validate-patch.py: check the patch applies
+#
+# This program takes two inputs:
+#   - the plugin output
+#   - the binary output
+#
+# Copyright (C) 2024
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import sys
+from argparse import ArgumentParser
+
+def main() -> None:
+    """
+    Process the arguments, injest the program and plugin out and
+    verify they match up and report if they do not.
+    """
+    parser = ArgumentParser(description="Validate patch")
+    parser.add_argument('test_output',
+                        help="The output from the test itself")
+    parser.add_argument('plugin_output',
+                        help="The output from plugin")
+    args = parser.parse_args()
+
+    with open(args.test_output, 'r') as f:
+        test_data = f.read()
+    with open(args.plugin_output, 'r') as f:
+        plugin_data = f.read()
+    if "Value: 1" in test_data:
+        sys.exit(0)
+    else:
+        sys.exit(1)
+
+if __name__ == "__main__":
+    main()
+
-- 
2.49.0

[PATCH v3 7/8] Add hypercalls plugin and test

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: novafacing <rowanbhart@gmail.com>
Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 tests/tcg/Makefile.target                     |   1 +
 tests/tcg/plugins/hypercalls.c                | 552 ++++++++++++++++++
 tests/tcg/plugins/meson.build                 |   2 +-
 tests/tcg/x86_64/Makefile.softmmu-target      |   6 +-
 tests/tcg/x86_64/system/hypercalls-target.c   |  45 ++
 .../tcg/x86_64/system/validate-hypercalls.py  |  40 ++
 6 files changed, 644 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/plugins/hypercalls.c
 create mode 100644 tests/tcg/x86_64/system/hypercalls-target.c
 create mode 100755 tests/tcg/x86_64/system/validate-hypercalls.py

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 4b709a9d18..5ac9638102 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -177,6 +177,7 @@ RUN_TESTS+=$(EXTRA_RUNS)
 # exercise things. We can define them on a per-test basis here.
 run-plugin-%-with-libmem.so: PLUGIN_ARGS=$(COMMA)inline=true
 run-plugin-%-with-libpatch.so: PLUGIN_ARGS=$(COMMA)target=ffffffff$(COMMA)patch=00000000
+run-plugin-%-with-libhypercalls.so: PLUGIN_ARGS=$(COMMA)ignore_unsupported=true
 
 ifeq ($(filter %-softmmu, $(TARGET)),)
 run-%: %
diff --git a/tests/tcg/plugins/hypercalls.c b/tests/tcg/plugins/hypercalls.c
new file mode 100644
index 0000000000..ece2716ae8
--- /dev/null
+++ b/tests/tcg/plugins/hypercalls.c
@@ -0,0 +1,552 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (C) 2024, Rowan Hart <rowanbhart@gmail.com>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ *
+ * This plugin implements a simple hypercall interface for guests (both system
+ * and user mode) to call certain operations from the host.
+ */
+#include "glib.h"
+#include "glibconfig.h"
+#include <assert.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+#define AARCH64_N_HYPERCALL_INSNS  (28)
+#define AARCH64_HYPERCALL_INSN_LEN (4)
+#define AARCH64_HYPERCALL_MAX (AARCH64_N_HYPERCALL_INSNS)
+#define ARM_N_HYPERCALL_INSNS  (12)
+#define ARM_HYPERCALL_INSN_LEN (4)
+#define ARM_HYPERCALL_MAX (ARM_N_HYPERCALL_INSNS)
+#define X86_HYPERCALL_INSN_LEN (2)
+#define X86_HYPERCALL_VALUE_BASE (0x4711)
+#define X86_HYPERCALL_MAX (0x10000)
+#define N_HYPERCALL_ARGS (4)
+
+static bool ignore_unsupported;
+
+static struct qemu_plugin_register *get_register(const char *name);
+static uint64_t byte_array_to_uint64(GByteArray *buf);
+
+enum HypercallInsnType {
+    CONSTANT,
+    CALLBACK,
+};
+
+
+/*
+ * Checks an instruction and returns its hypercall number, if it is
+ * a hypercall instruction, or -1 if it is not. Called at execution
+ * time.
+ */
+typedef int32_t (*hypercall_nr_cb)(GByteArray *);
+
+/*
+ * Checks an instruction and returns whether it is a hypercall, or -1 if it is
+ * not. Called at execution time.
+ */
+typedef bool (*is_hypercall_cb)(GByteArray *);
+
+/*
+ * Specifies a Hypercall for an architecture:
+ *
+ * - Architecture name
+ * - Whether it is enabled
+ * - The hypercall instruction
+ * - The register names to pass the hypercall # and args
+ */
+struct HypercallSpec {
+    const bool enabled;
+    const char *name;
+    const bool le;
+    const char *args[N_HYPERCALL_ARGS];
+    const hypercall_nr_cb hypercall_nr_cb;
+    const is_hypercall_cb is_hypercall_cb;
+};
+
+static int32_t aarch64_hypercall_nr_cb(GByteArray *insn)
+{
+    if (insn->len != AARCH64_HYPERCALL_INSN_LEN) {
+        return -1;
+    }
+
+    static const uint8_t
+    hypercall_insns[AARCH64_N_HYPERCALL_INSNS][AARCH64_HYPERCALL_INSN_LEN] = {
+        { 0xaa, 0x4, 0x0, 0x84 },
+        { 0xaa, 0x5, 0x0, 0xa5 },
+        { 0xaa, 0x6, 0x0, 0xc6 },
+        { 0xaa, 0x7, 0x0, 0xe7 },
+        { 0xaa, 0x8, 0x1, 0x8 },
+        { 0xaa, 0x9, 0x1, 0x29 },
+        { 0xaa, 0xa, 0x1, 0x4a },
+        { 0xaa, 0xb, 0x1, 0x6b },
+        { 0xaa, 0xc, 0x1, 0x8c },
+        { 0xaa, 0xd, 0x1, 0xad },
+        { 0xaa, 0xe, 0x1, 0xce },
+        { 0xaa, 0xf, 0x1, 0xef },
+        { 0xaa, 0x10, 0x2, 0x10 },
+        { 0xaa, 0x11, 0x2, 0x31 },
+        { 0xaa, 0x12, 0x2, 0x52 },
+        { 0xaa, 0x13, 0x2, 0x73 },
+        { 0xaa, 0x14, 0x2, 0x94 },
+        { 0xaa, 0x15, 0x2, 0xb5 },
+        { 0xaa, 0x16, 0x2, 0xd6 },
+        { 0xaa, 0x17, 0x2, 0xf7 },
+        { 0xaa, 0x18, 0x3, 0x18 },
+        { 0xaa, 0x19, 0x3, 0x39 },
+        { 0xaa, 0x1a, 0x3, 0x5a },
+        { 0xaa, 0x1b, 0x3, 0x7b },
+        { 0xaa, 0x1c, 0x3, 0x9c },
+        { 0xaa, 0x1d, 0x3, 0xbd },
+        { 0xaa, 0x1e, 0x3, 0xde },
+        { 0xaa, 0x1f, 0x3, 0xff },
+    };
+
+    for (int32_t i = 0; i < AARCH64_N_HYPERCALL_INSNS; i++) {
+        if (!memcmp(hypercall_insns[i], insn->data, insn->len)) {
+            return i;
+        }
+    }
+    return -1;
+}
+
+static bool aarch64_is_hypercall_cb(GByteArray *insn)
+{
+    return aarch64_hypercall_nr_cb(insn) < 0;
+}
+
+
+static int32_t aarch64_be_hypercall_nr_cb(GByteArray *insn)
+{
+    if (insn->len != AARCH64_HYPERCALL_INSN_LEN) {
+        return -1;
+    }
+
+    static const uint8_t
+    hypercall_insns[AARCH64_N_HYPERCALL_INSNS][AARCH64_HYPERCALL_INSN_LEN] = {
+        {0x84, 0x0, 0x4, 0xaa},
+        {0xa5, 0x0, 0x5, 0xaa},
+        {0xc6, 0x0, 0x6, 0xaa},
+        {0xe7, 0x0, 0x7, 0xaa},
+        {0x8, 0x1, 0x8, 0xaa},
+        {0x29, 0x1, 0x9, 0xaa},
+        {0x4a, 0x1, 0xa, 0xaa},
+        {0x6b, 0x1, 0xb, 0xaa},
+        {0x8c, 0x1, 0xc, 0xaa},
+        {0xad, 0x1, 0xd, 0xaa},
+        {0xce, 0x1, 0xe, 0xaa},
+        {0xef, 0x1, 0xf, 0xaa},
+        {0x10, 0x2, 0x10, 0xaa},
+        {0x31, 0x2, 0x11, 0xaa},
+        {0x52, 0x2, 0x12, 0xaa},
+        {0x73, 0x2, 0x13, 0xaa},
+        {0x94, 0x2, 0x14, 0xaa},
+        {0xb5, 0x2, 0x15, 0xaa},
+        {0xd6, 0x2, 0x16, 0xaa},
+        {0xf7, 0x2, 0x17, 0xaa},
+        {0x18, 0x3, 0x18, 0xaa},
+        {0x39, 0x3, 0x19, 0xaa},
+        {0x5a, 0x3, 0x1a, 0xaa},
+        {0x7b, 0x3, 0x1b, 0xaa},
+        {0x9c, 0x3, 0x1c, 0xaa},
+        {0xbd, 0x3, 0x1d, 0xaa},
+        {0xde, 0x3, 0x1e, 0xaa},
+        {0xff, 0x3, 0x1f, 0xaa},
+    };
+
+    for (int32_t i = 0; i < AARCH64_N_HYPERCALL_INSNS; i++) {
+        if (!memcmp(hypercall_insns[i], insn->data, insn->len)) {
+            return i;
+        }
+    }
+    return -1;
+}
+
+static bool aarch64_be_is_hypercall_cb(GByteArray *insn)
+{
+    return aarch64_be_hypercall_nr_cb(insn) < 0;
+}
+
+
+static int32_t arm_hypercall_nr_cb(GByteArray *insn)
+{
+    if (insn->len != ARM_HYPERCALL_INSN_LEN) {
+        return -1;
+    }
+
+    static const uint8_t
+    hypercall_insns[ARM_N_HYPERCALL_INSNS][ARM_HYPERCALL_INSN_LEN] = {
+        { 0xe1, 0x84, 0x40, 0x4 },
+        { 0xe1, 0x85, 0x50, 0x5 },
+        { 0xe1, 0x86, 0x60, 0x6 },
+        { 0xe1, 0x87, 0x70, 0x7 },
+        { 0xe1, 0x88, 0x80, 0x8 },
+        { 0xe1, 0x89, 0x90, 0x9 },
+        { 0xe1, 0x8a, 0xa0, 0xa },
+        { 0xe1, 0x8b, 0xb0, 0xb },
+        { 0xe1, 0x8c, 0xc0, 0xc },
+        { 0xe1, 0x8d, 0xd0, 0xd },
+        { 0xe1, 0x8e, 0xe0, 0xe },
+        { 0xe1, 0x8f, 0xf0, 0xf },
+    };
+
+    for (int32_t i = 0; i < ARM_N_HYPERCALL_INSNS; i++) {
+        if (!memcmp(hypercall_insns[i], insn->data, insn->len)) {
+            return i;
+        }
+    }
+    return -1;
+}
+
+static bool arm_is_hypercall_cb(GByteArray *insn)
+{
+    return arm_hypercall_nr_cb(insn) < 0;
+}
+
+static int32_t arm_be_hypercall_nr_cb(GByteArray *insn)
+{
+    if (insn->len != ARM_HYPERCALL_INSN_LEN) {
+        return -1;
+    }
+
+    static const uint8_t
+    hypercall_insns[ARM_N_HYPERCALL_INSNS][ARM_HYPERCALL_INSN_LEN] = {
+        {0x4, 0x40, 0x84, 0xe1},
+        {0x5, 0x50, 0x85, 0xe1},
+        {0x6, 0x60, 0x86, 0xe1},
+        {0x7, 0x70, 0x87, 0xe1},
+        {0x8, 0x80, 0x88, 0xe1},
+        {0x9, 0x90, 0x89, 0xe1},
+        {0xa, 0xa0, 0x8a, 0xe1},
+        {0xb, 0xb0, 0x8b, 0xe1},
+        {0xc, 0xc0, 0x8c, 0xe1},
+        {0xd, 0xd0, 0x8d, 0xe1},
+        {0xe, 0xe0, 0x8e, 0xe1},
+        {0xf, 0xf0, 0x8f, 0xe1},
+    };
+
+    for (int32_t i = 0; i < ARM_N_HYPERCALL_INSNS; i++) {
+        if (!memcmp(hypercall_insns[i], insn->data, insn->len)) {
+            return i;
+        }
+    }
+    return -1;
+}
+
+static bool arm_be_is_hypercall_cb(GByteArray *insn)
+{
+    return arm_be_hypercall_nr_cb(insn) < 0;
+}
+
+static int32_t x86_64_hypercall_nr_cb(GByteArray *insn)
+{
+    if (insn->len != X86_HYPERCALL_INSN_LEN) {
+        return -1;
+    }
+
+    uint8_t cpuid[] = { 0x0f, 0xa2 };
+    if (!memcmp(cpuid, insn->data, insn->len)) {
+        GByteArray *reg = g_byte_array_new();
+        qemu_plugin_read_register(get_register("rax"), reg);
+        uint64_t value = byte_array_to_uint64(reg);
+        g_byte_array_free(reg, true);
+        
+
+        if (!(value & X86_HYPERCALL_VALUE_BASE)) {
+            return -1;
+        }
+
+        value = (value >> 16) & 0xffff;
+
+        if (value >= X86_HYPERCALL_MAX) {
+            return -1;
+        }
+
+        return (int32_t)value;
+    }
+
+    return -1;
+}
+
+static bool x86_64_is_hypercall_cb(GByteArray *insn)
+{
+    if (insn->len != X86_HYPERCALL_INSN_LEN) {
+        return false;
+    }
+
+    uint8_t cpuid[] = { 0x0f, 0xa2 };
+    if (!memcmp(cpuid, insn->data, insn->len)) {
+        return true;
+    }
+
+    return false;
+}
+
+static int32_t i386_hypercall_nr_cb(GByteArray *insn)
+{
+    if (insn->len != X86_HYPERCALL_INSN_LEN) {
+        return -1;
+    }
+
+    uint8_t cpuid[] = { 0x0f, 0xa2 };
+    if (!memcmp(cpuid, insn->data, insn->len)) {
+        GByteArray *reg = g_byte_array_new();
+        qemu_plugin_read_register(get_register("eax"), reg);
+        uint64_t value = byte_array_to_uint64(reg);
+        g_byte_array_free(reg, true);
+
+        if (!(value & X86_HYPERCALL_VALUE_BASE)) {
+            return -1;
+        }
+
+        value = (value >> 16) & 0xffff;
+
+        if (value >= X86_HYPERCALL_MAX) {
+            return -1;
+        }
+        return (int32_t)value;
+    }
+
+    return -1;
+
+}
+
+static bool i386_is_hypercall_cb(GByteArray *insn)
+{
+    if (insn->len != X86_HYPERCALL_INSN_LEN) {
+        return false;
+    }
+
+    uint8_t cpuid[] = { 0x0f, 0xa2 };
+    if (!memcmp(cpuid, insn->data, insn->len)) {
+        return true;
+    }
+
+    return false;
+
+}
+
+static const struct HypercallSpec *hypercall_spec;
+
+static const struct HypercallSpec hypercall_specs[] = {
+    { true, "aarch64", true, {
+            "x0", "x1", "x2", "x3",
+        }, aarch64_hypercall_nr_cb, aarch64_is_hypercall_cb
+    },
+    { true, "aarch64_be", false,  {
+            "x0", "x1", "x2", "x3",
+        }, aarch64_be_hypercall_nr_cb, aarch64_be_is_hypercall_cb
+    },
+    { true, "arm", true,  {
+            "r0", "r1", "r2", "r3",
+        }, arm_hypercall_nr_cb, arm_is_hypercall_cb
+    },
+    { true, "armeb", false,  {
+            "r0", "r1", "r2", "r3"
+        }, arm_be_hypercall_nr_cb, arm_be_is_hypercall_cb
+    },
+    { true, "i386", true, {
+            "edi", "esi", "edx", "ecx"
+        }, i386_hypercall_nr_cb, i386_is_hypercall_cb
+    },
+    { true, "x86_64", true, {
+            "rdi", "rsi", "rdx", "rcx"
+
+        }, x86_64_hypercall_nr_cb, x86_64_is_hypercall_cb
+    },
+    { false, NULL, .le = false,  {NULL, NULL, NULL, NULL}, NULL},
+};
+
+static GArray *hypercall_insns;
+
+/*
+ * Returns a handle to a register with a given name, or NULL if there is no
+ * such register.
+ */
+static struct qemu_plugin_register *get_register(const char *name)
+{
+    GArray *registers = qemu_plugin_get_registers();
+
+    struct qemu_plugin_register *handle = NULL;
+
+    qemu_plugin_reg_descriptor *reg_descriptors =
+        (qemu_plugin_reg_descriptor *)registers->data;
+
+    for (size_t i = 0; i < registers->len; i++) {
+        if (!strcmp(reg_descriptors[i].name, name)) {
+            handle = reg_descriptors[i].handle;
+        }
+    }
+
+    g_array_free(registers, true);
+
+    return handle;
+}
+
+/*
+ * Transforms a byte array with at most 8 entries into a uint64_t
+ * depending on the target machine's endianness.
+ */
+static uint64_t byte_array_to_uint64(GByteArray *buf)
+{
+    uint64_t value = 0;
+    if (hypercall_spec->le) {
+        for (int i = 0; i < buf->len && i < sizeof(uint64_t); i++) {
+            value |= ((uint64_t)buf->data[i]) << (i * 8);
+        }
+    } else {
+        for (int i = 0; i < buf->len && i < sizeof(uint64_t); i++) {
+            value |= ((uint64_t)buf->data[i]) << ((buf->len - 1 - i) * 8);
+        }
+    }
+    return value;
+}
+
+/*
+ * Handle a "hypercall" instruction, which has some special meaning for this
+ * plugin.
+ */
+static void hypercall(unsigned int vcpu_index, void *userdata)
+{
+    GByteArray *insn_data = (GByteArray *)userdata;
+    int32_t hypercall_nr = hypercall_spec->hypercall_nr_cb(insn_data);
+
+    if (hypercall_nr < 0) {
+        return;
+    }
+
+    uint64_t args[N_HYPERCALL_ARGS] = {0};
+    GByteArray *buf = g_byte_array_new();
+    for (size_t i = 0; i < N_HYPERCALL_ARGS; i++) {
+        g_byte_array_set_size(buf, 0);
+        struct qemu_plugin_register *reg = 
+            get_register(hypercall_spec->args[i]);
+        qemu_plugin_read_register(reg, buf);
+        args[i] = byte_array_to_uint64(buf);
+    }
+    g_byte_array_free(buf, true);
+
+    switch (hypercall_nr) {
+    /*
+     * The write hypercall (#0x01) tells the plugin to write random bytes
+     * of a given size into the memory of the emulated system at a particular
+     * vaddr
+     */
+    case 1: {
+        GByteArray *data = g_byte_array_new();
+        g_byte_array_set_size(data, args[1]);
+        for (uint64_t i = 0; i < args[1]; i++) {
+            data->data[i] = (uint8_t)g_random_int();
+        }
+        qemu_plugin_write_memory_vaddr(args[0], data);
+        break;
+    }
+    default:
+        break;
+    }
+}
+
+/*
+ * Callback on translation of a translation block.
+ */
+static void vcpu_tb_trans_cb(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    for (size_t i = 0; i < qemu_plugin_tb_n_insns(tb); i++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+        GByteArray *insn_data = g_byte_array_new();
+        size_t insn_len = qemu_plugin_insn_size(insn);
+        g_byte_array_set_size(insn_data, insn_len);
+        qemu_plugin_insn_data(insn, insn_data->data, insn_data->len);
+
+        if (hypercall_spec->is_hypercall_cb(insn_data)) {
+            g_array_append_val(hypercall_insns, insn_data);
+            qemu_plugin_register_vcpu_insn_exec_cb(insn, hypercall,
+                                                   QEMU_PLUGIN_CB_R_REGS,
+                                                   (void *)insn_data);
+        } else {
+            g_byte_array_free(insn_data, true);
+        }
+
+    }
+}
+
+static void atexit_cb(qemu_plugin_id_t id, void *userdata)
+{
+    for (size_t i = 0; i < hypercall_insns->len; i++) {
+        g_byte_array_free(g_array_index(hypercall_insns, GByteArray *, i),
+                          true);
+    }
+
+    g_array_free(hypercall_insns, true);
+}
+
+static void usage(void)
+{
+    fprintf(stderr,
+            "Usage: <lib>,[ignore_unsupported=<ignore_unsupported>]");
+}
+
+/*
+ * Called when the plugin is installed
+ */
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info, int argc,
+                                           char **argv)
+{
+    if (argc > 1) {
+        usage();
+        return -1;
+    }
+
+    for (size_t i = 0; i < argc; i++) {
+        char *opt = argv[i];
+        g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
+        if (g_strcmp0(tokens[0], "ignore_unsupported") == 0) {
+            if (!qemu_plugin_bool_parse(tokens[0],
+                                    tokens[1], &ignore_unsupported)) {
+                fprintf(stderr, "Failed to parse boolean argument ignore_unsupported\n");
+                return -1;
+            }
+        } else {
+            fprintf(stderr, "Unknown argument: %s\n", tokens[0]);
+            usage();
+            return -1;
+        }
+    }
+
+
+    hypercall_spec = &hypercall_specs[0];
+
+    while (hypercall_spec->name != NULL) {
+        if (!strcmp(hypercall_spec->name, info->target_name)) {
+            break;
+        }
+        hypercall_spec++;
+    }
+
+    if (hypercall_spec->name == NULL || !hypercall_spec->enabled) {
+        qemu_plugin_outs("Error: no hypercall spec.");
+        if (ignore_unsupported) {
+            return 0;
+        }
+        return -1;
+    }
+
+    hypercall_insns = g_array_new(true, true, sizeof(GByteArray *));
+
+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans_cb);
+    qemu_plugin_register_atexit_cb(id, atexit_cb, NULL);
+
+    return 0;
+}
diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
index 163042e601..909bf3005a 100644
--- a/tests/tcg/plugins/meson.build
+++ b/tests/tcg/plugins/meson.build
@@ -1,6 +1,6 @@
 t = []
 if get_option('plugins')
-  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall', 'patch']
+  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall', 'hypercalls', 'patch']
     if host_os == 'windows'
       t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
                         include_directories: '../../../include/qemu',
diff --git a/tests/tcg/x86_64/Makefile.softmmu-target b/tests/tcg/x86_64/Makefile.softmmu-target
index 8d3a067c33..8cb2a19461 100644
--- a/tests/tcg/x86_64/Makefile.softmmu-target
+++ b/tests/tcg/x86_64/Makefile.softmmu-target
@@ -46,14 +46,18 @@ EXTRA_RUNS+=$(MULTIARCH_RUNS)
 
 memory: CFLAGS+=-DCHECK_UNALIGNED=1
 patch-target: CFLAGS+=-O0
+hypercalls-target: CFLAGS+=-O0
 
 # Running
 QEMU_OPTS+=-device isa-debugcon,chardev=output -device isa-debug-exit,iobase=0xf4,iosize=0x4 -kernel
 
 # Add patch-target to ADDITIONAL_PLUGINS_TESTS
 ADDITIONAL_PLUGINS_TESTS += patch-target
+ADDITIONAL_PLUGINS_TESTS += hypercalls-target
 
 run-plugin-patch-target-with-libpatch.so:		\
 	PLUGIN_ARGS=$(COMMA)target=ffc0$(COMMA)patch=9090$(COMMA)use_hwaddr=true$(COMMA)debug_insns=false
 run-plugin-patch-target-with-libpatch.so:		\
-	CHECK_PLUGIN_OUTPUT_COMMAND=$(X86_64_SYSTEM_SRC)/validate-patch.py $@.out
\ No newline at end of file
+	CHECK_PLUGIN_OUTPUT_COMMAND=$(X86_64_SYSTEM_SRC)/validate-patch.py $@.out
+run-plugin-hypercalls-target-with-libhypercalls.so:		\
+	CHECK_PLUGIN_OUTPUT_COMMAND=$(X86_64_SYSTEM_SRC)/validate-hypercalls.py $@.out
diff --git a/tests/tcg/x86_64/system/hypercalls-target.c b/tests/tcg/x86_64/system/hypercalls-target.c
new file mode 100644
index 0000000000..643d489e9c
--- /dev/null
+++ b/tests/tcg/x86_64/system/hypercalls-target.c
@@ -0,0 +1,45 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (C) 2025, Rowan Hart <rowanbhart@gmail.com>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ *
+ * This test target invokes a hypercall to write the value 0x1337 to a
+ * variable.
+ *
+ */
+#include <stddef.h>
+#include <stdint.h>
+#include <minilib.h>
+
+#define _hypercall(num, arg0, arg1, arg2, arg3)                      \
+    unsigned int a __attribute__((unused)) = 0;                     \
+    unsigned int b __attribute__((unused)) = 0;                     \
+    unsigned int c __attribute__((unused)) = 0;                     \
+    unsigned int d __attribute__((unused)) = 0;                     \
+    __asm__ __volatile__("cpuid\n\t"                                \
+                         : "=a"(a), "=b"(b), "=c"(c), "=d"(d)       \
+                         : "a"(num), "D"(arg0), "S"(arg1), \
+                           "d"(arg2), "c"(arg3));
+
+#define hypercall(num, arg0, arg1, arg2, arg3) \
+    { \
+        unsigned int __num = 0x4711 | (num << 16); \
+        _hypercall(__num, arg0, arg1, arg2, arg3); \
+    }
+
+int main(void)
+{
+    uint16_t value = 0;
+
+    for (size_t i = 0; i < 1000000; i++) {
+        hypercall(1, &value, sizeof(value), 0, 0);
+        if (value == 0x1337) {
+            ml_printf("Victory!\n");
+            return 0;
+        }
+    }
+    return 0;
+}
diff --git a/tests/tcg/x86_64/system/validate-hypercalls.py b/tests/tcg/x86_64/system/validate-hypercalls.py
new file mode 100755
index 0000000000..6e7c980706
--- /dev/null
+++ b/tests/tcg/x86_64/system/validate-hypercalls.py
@@ -0,0 +1,40 @@
+#!/usr/bin/env python3
+#
+# validate-patch.py: check the patch applies
+#
+# This program takes two inputs:
+#   - the plugin output
+#   - the binary output
+#
+# Copyright (C) 2024
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import sys
+from argparse import ArgumentParser
+
+def main() -> None:
+    """
+    Process the arguments, injest the program and plugin out and
+    verify they match up and report if they do not.
+    """
+    parser = ArgumentParser(description="Validate patch")
+    parser.add_argument('test_output',
+                        help="The output from the test itself")
+    parser.add_argument('plugin_output',
+                        help="The output from plugin")
+    args = parser.parse_args()
+
+    with open(args.test_output, 'r') as f:
+        test_data = f.read()
+    with open(args.plugin_output, 'r') as f:
+        plugin_data = f.read()
+
+    if "Victory" in test_data:
+        sys.exit(0)
+    else:
+        sys.exit(1)
+
+if __name__ == "__main__":
+    main()
+
-- 
2.49.0

[PATCH v3 8/8] Update plugin version and add notes

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
---
 include/qemu/qemu-plugin.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index d4f229abd9..4cf2955560 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -65,11 +65,21 @@ typedef uint64_t qemu_plugin_id_t;
  *
  * version 4:
  * - added qemu_plugin_read_memory_vaddr
+ *
+ * version 5:
+ * - added qemu_plugin_write_memory_vaddr
+ * - added qemu_plugin_read_memory_hwaddr
+ * - added qemu_plugin_write_memory_hwaddr
+ * - added qemu_plugin_write_register
+ * - added qemu_plugin_get_current_vcpu_address_spaces
+ * - added qemu_plugin_n_address_spaces
+ * - added qemu_plugin_address_space_name
+ * - added qemu_plugin_translate_vaddr
  */
 
 extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
-#define QEMU_PLUGIN_VERSION 4
+#define QEMU_PLUGIN_VERSION 5
 
 /**
  * struct qemu_info_t - system information for plugins
-- 
2.49.0

Re: [PATCH v3 1/8] Expose gdb_write_register function to consumers of gdbstub

[Pierrick Bouvier]

On 5/21/25 2:43 AM, Rowan Hart wrote:
> From: novafacing <rowanbhart@gmail.com>
> 
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>   gdbstub/gdbstub.c      |  2 +-
>   include/exec/gdbstub.h | 14 ++++++++++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v3 2/8] Add register write API

[Pierrick Bouvier]

On 5/21/25 2:43 AM, Rowan Hart wrote:
> From: novafacing <rowanbhart@gmail.com>
> 
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>   include/qemu/qemu-plugin.h | 57 +++++++++++++++++++++++++-------------
>   plugins/api.c              | 26 ++++++++++++-----
>   2 files changed, 56 insertions(+), 27 deletions(-)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v3 4/8] Add memory virtual address write API

[Pierrick Bouvier]

On 5/21/25 2:43 AM, Rowan Hart wrote:
> From: novafacing <rowanbhart@gmail.com>
> 
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>   include/qemu/qemu-plugin.h | 21 +++++++++++++++++++++
>   plugins/api.c              | 18 ++++++++++++++++++
>   2 files changed, 39 insertions(+)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Pierrick Bouvier]

On 5/21/25 2:43 AM, Rowan Hart wrote:
> From: novafacing <rowanbhart@gmail.com>
> 
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>   include/qemu/qemu-plugin.h |  96 +++++++++++++++++++++++++++++++++++
>   plugins/api.c              | 100 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 196 insertions(+)

Reading this patch, and patch 3 (Add address space API), I am not sure 
AddressSpace is something we want to leak in plugins interface.
It is a concept *very* internal to QEMU, and not reflecting directly 
something concerning the emulated architecture (it is related, but not 
officially described for it).

The same way qemu_plugin_write_memory_vaddr is only valid in the current 
page table setup, we could assume the same for current address space, 
and return an error if memory is not mapped with current AS.
Eventually, we could read/write a given hwaddr in all existing address 
spaces (starting with current mapped one), if it makes sense to do this, 
which I'm not sure about.

What are your thoughts on this?

qemu_plugin_translate_vaddr is fine for me.

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Rowan Hart]

Well, first I just noticed that I left a debug print in this function! 
So I'll fix that.

> Reading this patch, and patch 3 (Add address space API), I am not sure 
> AddressSpace is something we want to leak in plugins interface.
> It is a concept *very* internal to QEMU, and not reflecting directly 
> something concerning the emulated architecture (it is related, but not 
> officially described for it).
>
> The same way qemu_plugin_write_memory_vaddr is only valid in the 
> current page table setup, we could assume the same for current address 
> space, and return an error if memory is not mapped with current AS.
> Eventually, we could read/write a given hwaddr in all existing address 
> spaces (starting with current mapped one), if it makes sense to do 
> this, which I'm not sure about.
>
> What are your thoughts on this? 

I definitely see the arguments for not exposing it even as an opaque 
struct, internality not withstanding it also adds some complexity for 
plugin authors.

My thought with exposing it is kind of twofold. First, there are 
specific address spaces like the secure address space on ARM or I/O 
memory on x86 that plugins might like to access and I think it's easiest 
to facilitate that if we just let them choose which one they want to r/w 
to. Second, I wanted to lean towards being less restrictive now to avoid 
having to go back and remove restrictions later since even though it's 
very internal, it doesn't seem very likely to change.

That said, if you think it's more trouble than it's worth I'm totally 
fine with just defaulting to writing to the current AS (or to 
cpu-memory, whichever's more reasonable). Your call, just let me know 
which way you think is best for v4 :)

> qemu_plugin_translate_vaddr is fine for me. 
I did have a question about this -- one of the test plugins prints out 
vaddr, haddr from qemu_plugin_insn_haddr, and the translated haddr from 
qemu_plugin_translate_vaddr. When running with direct memory mappings in 
a system test, the vaddr = translated haddr, which is correct, but the 
haddr from qemu_plugin_insn_haddr was incorrect (it was 0x7f....f<actual 
address>). Is this expected behavior?

Thanks for the feedback!

Re: [PATCH v3 1/8] Expose gdb_write_register function to consumers of gdbstub

[Manos Pitsidianakis]

On Wed, May 21, 2025 at 12:45 PM Rowan Hart <rowanbhart@gmail.com> wrote:
>
> From: novafacing <rowanbhart@gmail.com>
>
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>  gdbstub/gdbstub.c      |  2 +-
>  include/exec/gdbstub.h | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 565f6b33a9..5846e481be 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -534,7 +534,7 @@ int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
>      return 0;
>  }
>
> -static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
> +int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>  {
>      GDBRegisterState *r;
>
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 0675b0b646..a16c0051ce 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -124,6 +124,20 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname);
>   */
>  int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>
> +/**
> + * gdb_write_register() - Write a register associated with a CPU.
> + * @cpu: The CPU associated with the register.
> + * @buf: The buffer that the register contents will be set to.
> + * @reg: The register's number returned by gdb_find_feature_register().
> + *
> + * The size of @buf must be at least the size of the register being
> + * written.
> + *
> + * Return: The number of written bytes, or 0 if an error occurred (for
> + * example, an unknown register was provided).
> + */
> +int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg);
> +
>  /**
>   * typedef GDBRegDesc - a register description from gdbstub
>   */
> --
> 2.49.0
>
>

FYI there's another patch in the list that does this
https://lore.kernel.org/qemu-devel/20250430052741.21145-17-mario.fleischmann@lauterbach.com/

Letting you know so you can track each other's series if need be.

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

Re: [PATCH v3 1/8] Expose gdb_write_register function to consumers of gdbstub

[Julian Ganz]

Hi Rowan,

> From: novafacing <rowanbhart@gmail.com>
> 
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>

As I understand it, the commit subject should be prefixed with the
subsystem the patch (mainly) touches. In this case "plugins".

Commit message bodies would also be appreciated. In this case maybe one
motivating the change in one or two sentences. It's obvious enough from
context here, but you want at least some context when stumbling accross
a commit in isolation (e.g. after consulting git blame).

Reviewed-By: Julian Ganz <neither@nut.email>

Regards,
Julian

Re: [PATCH v3 2/8] Add register write API

[Julian Ganz]

Hi Rowan,

> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 3a850aa216..68c8632fd7 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -893,6 +891,41 @@ typedef struct {
>  QEMU_PLUGIN_API
>  GArray *qemu_plugin_get_registers(void);
> 
> +/**
> + * qemu_plugin_read_register() - read register for current vCPU
> + *
> + * @handle: a @qemu_plugin_reg_handle handle
> + * @buf: A GByteArray for the data owned by the plugin
> + *
> + * This function is only available in a context that register read access is
> + * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
> + *
> + * Returns the size of the read register. The content of @buf is in target byte
> + * order. On failure returns -1.
> + */
> +QEMU_PLUGIN_API
> +int qemu_plugin_read_register(struct qemu_plugin_register *handle,
> +                              GByteArray *buf);
> +
> +/**
> + * qemu_plugin_write_register() - write register for current vCPU
> + *
> + * @handle: a @qemu_plugin_reg_handle handle
> + * @buf: A GByteArray for the data owned by the plugin
> + *
> + * This function is only available in a context that register write access is
> + * explicitly requested via the QEMU_PLUGIN_CB_W_REGS flag.
> + *
> + * The size of @buf must be at least the size of the requested register.
> + * Attempting to write a register with @buf smaller than the register size
> + * will result in a crash or other undesired behavior.
> + *
> + * Returns the number of bytes written. On failure returns 0.
> + */
> +QEMU_PLUGIN_API
> +int qemu_plugin_write_register(struct qemu_plugin_register *handle,
> +                              GByteArray *buf);
> +
>  /**
>   * qemu_plugin_read_memory_vaddr() - read from memory using a virtual address
>   *
> @@ -915,22 +948,6 @@ QEMU_PLUGIN_API
>  bool qemu_plugin_read_memory_vaddr(uint64_t addr,
>                                     GByteArray *data, size_t len);
> 
> -/**
> - * qemu_plugin_read_register() - read register for current vCPU
> - *
> - * @handle: a @qemu_plugin_reg_handle handle
> - * @buf: A GByteArray for the data owned by the plugin
> - *
> - * This function is only available in a context that register read access is
> - * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
> - *
> - * Returns the size of the read register. The content of @buf is in target byte
> - * order. On failure returns -1.
> - */
> -QEMU_PLUGIN_API
> -int qemu_plugin_read_register(struct qemu_plugin_register *handle,
> -                              GByteArray *buf);
> -

Why the code move?

> diff --git a/plugins/api.c b/plugins/api.c
> index 3c9d4832e9..79b2dc20b8 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -433,6 +433,25 @@ GArray *qemu_plugin_get_registers(void)
>      return create_register_handles(regs);
>  }
> 
> +int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> +{
> +    g_assert(current_cpu);
> +
> +    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
> +}
> +
> +int qemu_plugin_write_register(struct qemu_plugin_register *reg, 
> +                               GByteArray *buf)
> +{
> +    g_assert(current_cpu);
> +
> +    if (buf->len == 0) {
> +        return 0;
> +    }
> +
> +    return gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1);
> +}
> +
>  bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data, size_t len)
>  {
>      g_assert(current_cpu);
> @@ -453,13 +472,6 @@ bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data, size_t len)
>      return true;
>  }
>  
> -int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> -{
> -    g_assert(current_cpu);
> -
> -    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
> -}
> -

Again, what was the reason for moving `qemu_plugin_read_register`?

Reviewed-By: Julian Ganz <neither@nut.email>

Regards,
Julian

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Julian Ganz]

Hi Rowan,

> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index eff8430b4a..d4f229abd9 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -1014,6 +1014,102 @@ QEMU_PLUGIN_API
>  bool qemu_plugin_write_memory_vaddr(uint64_t addr,
>                                     GByteArray *data);
>  
> <snip>
> +/**
> + * qemu_plugin_translate_vaddr() - translate a virtual address to a physical one
> + *
> + * @vaddr: virtual address to translate
> + * @hwaddr: pointer to store the physical address
> + *
> + * This function is only valid in vCPU context (i.e. in callbacks) and is only
> + * valid for softmmu targets.
> + *
> + * Returns true on success and false on failure.
> + */
> +QEMU_PLUGIN_API
> +bool qemu_plugin_translate_vaddr(uint64_t vaddr, uint64_t *hwaddr);

As a (testing) plugin author, I'm quite interested in this function in
particular. For the next iteration, would you be willing to split this
one out into its own patch? This would enhance the chance of it getting
picked up even if other parts of the series should not make it.

> diff --git a/plugins/api.c b/plugins/api.c
> index 19c10bb39e..5983768783 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -569,6 +569,106 @@ bool qemu_plugin_write_memory_vaddr(uint64_t addr, GByteArray *data)
> <snip> 
> +bool qemu_plugin_translate_vaddr(uint64_t vaddr, uint64_t *hwaddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    g_assert(current_cpu);
> +
> +    CPUState *cpu = current_cpu;
> +
> +    uint64_t res = cpu_get_phys_page_debug(cpu, vaddr);
> +
> +    if (res == (uint64_t)-1) {
> +        return false;
> +    }
> +
> +    *hwaddr = res | (vaddr & ~TARGET_PAGE_MASK);
> +
> +    return true;
> +#else
> +    return false;
> +#endif
> +}

This definition strikes me as odd. What was your reason to assert
`current_cpu` here, but not in the other two functions? Also a bit
surprising is the declaration of `cpu` if you use it in just one place
(rather than just use `current_cpu` directly as for the assertion).

And there is no reason in particular why the vCPU could not be a
function parameter of `qemu_plugin_translate_vaddr`, right? You don't
have the same restrictions as in `qemu_plugin_read_memory_hwaddr` or
`qemu_plugin_hwaddr_operation_result` where you actually touch memory?

Regards,
Julian

Re: [PATCH v3 2/8] Add register write API

[Rowan Hart]

[-- Attachment #1: Type: text/plain, Size: 4652 bytes --]

Hi Julian,

> Again, what was the reason for moving `qemu_plugin_read_register`?

I moved it so it's grouped with get_registers above instead of being
separated below the memory functions. I can move it back, just seemed nicer
that way.

-Rowan

On Thu, May 22, 2025, 4:59 AM Julian Ganz <neither@nut.email> wrote:

> Hi Rowan,
>
> > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> > index 3a850aa216..68c8632fd7 100644
> > --- a/include/qemu/qemu-plugin.h
> > +++ b/include/qemu/qemu-plugin.h
> > @@ -893,6 +891,41 @@ typedef struct {
> >  QEMU_PLUGIN_API
> >  GArray *qemu_plugin_get_registers(void);
> >
> > +/**
> > + * qemu_plugin_read_register() - read register for current vCPU
> > + *
> > + * @handle: a @qemu_plugin_reg_handle handle
> > + * @buf: A GByteArray for the data owned by the plugin
> > + *
> > + * This function is only available in a context that register read
> access is
> > + * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
> > + *
> > + * Returns the size of the read register. The content of @buf is in
> target byte
> > + * order. On failure returns -1.
> > + */
> > +QEMU_PLUGIN_API
> > +int qemu_plugin_read_register(struct qemu_plugin_register *handle,
> > +                              GByteArray *buf);
> > +
> > +/**
> > + * qemu_plugin_write_register() - write register for current vCPU
> > + *
> > + * @handle: a @qemu_plugin_reg_handle handle
> > + * @buf: A GByteArray for the data owned by the plugin
> > + *
> > + * This function is only available in a context that register write
> access is
> > + * explicitly requested via the QEMU_PLUGIN_CB_W_REGS flag.
> > + *
> > + * The size of @buf must be at least the size of the requested register.
> > + * Attempting to write a register with @buf smaller than the register
> size
> > + * will result in a crash or other undesired behavior.
> > + *
> > + * Returns the number of bytes written. On failure returns 0.
> > + */
> > +QEMU_PLUGIN_API
> > +int qemu_plugin_write_register(struct qemu_plugin_register *handle,
> > +                              GByteArray *buf);
> > +
> >  /**
> >   * qemu_plugin_read_memory_vaddr() - read from memory using a virtual
> address
> >   *
> > @@ -915,22 +948,6 @@ QEMU_PLUGIN_API
> >  bool qemu_plugin_read_memory_vaddr(uint64_t addr,
> >                                     GByteArray *data, size_t len);
> >
> > -/**
> > - * qemu_plugin_read_register() - read register for current vCPU
> > - *
> > - * @handle: a @qemu_plugin_reg_handle handle
> > - * @buf: A GByteArray for the data owned by the plugin
> > - *
> > - * This function is only available in a context that register read
> access is
> > - * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
> > - *
> > - * Returns the size of the read register. The content of @buf is in
> target byte
> > - * order. On failure returns -1.
> > - */
> > -QEMU_PLUGIN_API
> > -int qemu_plugin_read_register(struct qemu_plugin_register *handle,
> > -                              GByteArray *buf);
> > -
>
> Why the code move?
>
> > diff --git a/plugins/api.c b/plugins/api.c
> > index 3c9d4832e9..79b2dc20b8 100644
> > --- a/plugins/api.c
> > +++ b/plugins/api.c
> > @@ -433,6 +433,25 @@ GArray *qemu_plugin_get_registers(void)
> >      return create_register_handles(regs);
> >  }
> >
> > +int qemu_plugin_read_register(struct qemu_plugin_register *reg,
> GByteArray *buf)
> > +{
> > +    g_assert(current_cpu);
> > +
> > +    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) -
> 1);
> > +}
> > +
> > +int qemu_plugin_write_register(struct qemu_plugin_register *reg,
> > +                               GByteArray *buf)
> > +{
> > +    g_assert(current_cpu);
> > +
> > +    if (buf->len == 0) {
> > +        return 0;
> > +    }
> > +
> > +    return gdb_write_register(current_cpu, buf->data,
> GPOINTER_TO_INT(reg) - 1);
> > +}
> > +
> >  bool qemu_plugin_read_memory_vaddr(uint64_t addr, GByteArray *data,
> size_t len)
> >  {
> >      g_assert(current_cpu);
> > @@ -453,13 +472,6 @@ bool qemu_plugin_read_memory_vaddr(uint64_t addr,
> GByteArray *data, size_t len)
> >      return true;
> >  }
> >
> > -int qemu_plugin_read_register(struct qemu_plugin_register *reg,
> GByteArray *buf)
> > -{
> > -    g_assert(current_cpu);
> > -
> > -    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) -
> 1);
> > -}
> > -
>
> Again, what was the reason for moving `qemu_plugin_read_register`?
>
> Reviewed-By: Julian Ganz <neither@nut.email>
>
> Regards,
> Julian
>

[-- Attachment #2: Type: text/html, Size: 5605 bytes --]

Re: [PATCH v3 2/8] Add register write API

[Julian Ganz]

Hi Rowan,

May 22, 2025 at 5:02 PM, Rowan Hart wrote:
> > Again, what was the reason for moving `qemu_plugin_read_register`?
> 
> I moved it so it's grouped with get_registers above instead of being separated below the memory functions. I can move it back, just seemed nicer that way.

The move itself is totally acceptable. However, the commit message
should, in my opinion, state why you did so.

Regards,
Julian

Re: [PATCH v3 2/8] Add register write API

[Alex Bennée]

Rowan Hart <rowanbhart@gmail.com> writes:

> From: novafacing <rowanbhart@gmail.com>
>
> Signed-off-by: novafacing <rowanbhart@gmail.com>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
>  include/qemu/qemu-plugin.h | 57 +++++++++++++++++++++++++-------------
>  plugins/api.c              | 26 ++++++++++++-----
>  2 files changed, 56 insertions(+), 27 deletions(-)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 3a850aa216..68c8632fd7 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -254,9 +254,6 @@ typedef struct {
>   * @QEMU_PLUGIN_CB_NO_REGS: callback does not access the CPU's regs
>   * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
>   * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
> - *
> - * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change
> - * system register state.

I would expect us to:

 a) handle the QEMU_PLUGIN_CB_RW_REGS
 b) try and enforce we are only being called from such callbacks

Otherwise TCG won't know to restore register state from what has been
written to.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Pierrick Bouvier]

On 5/22/25 4:59 AM, Julian Ganz wrote:
> This definition strikes me as odd. What was your reason to assert
> `current_cpu` here, but not in the other two functions? Also a bit
> surprising is the declaration of `cpu` if you use it in just one place
> (rather than just use `current_cpu` directly as for the assertion).
> 
> And there is no reason in particular why the vCPU could not be a
> function parameter of `qemu_plugin_translate_vaddr`, right? You don't
> have the same restrictions as in `qemu_plugin_read_memory_hwaddr` or
> `qemu_plugin_hwaddr_operation_result` where you actually touch memory?
>

That's a good point, adding a "unsigned int vcpu_index" to the signature 
should be enough to query current or any other vcpu easily.

> Regards,
> Julian

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Pierrick Bouvier]

On 5/21/25 8:34 PM, Rowan Hart wrote:
> Well, first I just noticed that I left a debug print in this function!
> So I'll fix that.
> 
>> Reading this patch, and patch 3 (Add address space API), I am not sure
>> AddressSpace is something we want to leak in plugins interface.
>> It is a concept *very* internal to QEMU, and not reflecting directly
>> something concerning the emulated architecture (it is related, but not
>> officially described for it).
>>
>> The same way qemu_plugin_write_memory_vaddr is only valid in the
>> current page table setup, we could assume the same for current address
>> space, and return an error if memory is not mapped with current AS.
>> Eventually, we could read/write a given hwaddr in all existing address
>> spaces (starting with current mapped one), if it makes sense to do
>> this, which I'm not sure about.
>>
>> What are your thoughts on this?
> 
> I definitely see the arguments for not exposing it even as an opaque
> struct, internality not withstanding it also adds some complexity for
> plugin authors.
> 
> My thought with exposing it is kind of twofold. First, there are
> specific address spaces like the secure address space on ARM or I/O
> memory on x86 that plugins might like to access and I think it's easiest
> to facilitate that if we just let them choose which one they want to r/w
> to. Second, I wanted to lean towards being less restrictive now to avoid
> having to go back and remove restrictions later since even though it's
> very internal, it doesn't seem very likely to change.
> 
> That said, if you think it's more trouble than it's worth I'm totally
> fine with just defaulting to writing to the current AS (or to
> cpu-memory, whichever's more reasonable). Your call, just let me know
> which way you think is best for v4 :)

I understand your point, but to the opposite of registers, I think we 
should refrain from exposing all this.
For now, we can just use the current AS.

Later, we could consider to add a new architecture specific parameter 
for that need (being a union, with fields per base architecture). So 
people writing architecture specific plugins can have a solution.

AddressSpace as = {.arm = ADDRESS_SPACE_ARM_SECURE};
qemu_plugin_read_memory_hwaddr_as(addr, data, as);

>> qemu_plugin_translate_vaddr is fine for me.
> I did have a question about this -- one of the test plugins prints out
> vaddr, haddr from qemu_plugin_insn_haddr, and the translated haddr from
> qemu_plugin_translate_vaddr. When running with direct memory mappings in
> a system test, the vaddr = translated haddr, which is correct, but the
> haddr from qemu_plugin_insn_haddr was incorrect (it was 0x7f....f<actual
> address>). Is this expected behavior?
>

qemu_plugin_insn_haddr returns directly a pointer to instruction in 
(host) memory, which is different from hwaddr, thus the void* signature.
Name is pretty confusing though, qemu_plugin_insn_ptr could have been a 
better name.

> Thanks for the feedback!
> 
> 

Re: [PATCH v3 2/8] Add register write API

[Rowan Hart]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

>  a) handle the QEMU_PLUGIN_CB_RW_REGS

I missed that this was not already handled. I'll fix that.

> b) try and enforce we are only being called from such callbacks

Sure, beyond documentation I suppose we can add and check a flag to ensure
this. I think it's a good idea to reduce foot guns from just calling it
from any old place.

-Rowan

[-- Attachment #2: Type: text/html, Size: 467 bytes --]

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Rowan Hart]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

>
>
> > This definition strikes me as odd. What was your reason to assert
> > `current_cpu` here, but not in the other two functions? Also a bit
> > surprising is the declaration of `cpu` if you use it in just one place
> > (rather than just use `current_cpu` directly as for the assertion).
> >
> > And there is no reason in particular why the vCPU could not be a
> > function parameter of `qemu_plugin_translate_vaddr`, right? You don't
> > have the same restrictions as in `qemu_plugin_read_memory_hwaddr` or
> > `qemu_plugin_hwaddr_operation_result` where you actually touch memory?
> >
>
> That's a good point, adding a "unsigned int vcpu_index" to the signature
> should be enough to query current or any other vcpu easily.
>
This is a really nice idea, it might be nice to make a vcpu version of
read/write register too. For memory, I'd think going with the current
memory is probably fine, I don't see any configs with different memory per
vcpu?

>

[-- Attachment #2: Type: text/html, Size: 1466 bytes --]

Re: [PATCH v3 5/8] Add memory hardware address read/write API

[Pierrick Bouvier]

On 5/22/25 2:01 PM, Rowan Hart wrote:
> 
>      > This definition strikes me as odd. What was your reason to assert
>      > `current_cpu` here, but not in the other two functions? Also a bit
>      > surprising is the declaration of `cpu` if you use it in just one
>     place
>      > (rather than just use `current_cpu` directly as for the assertion).
>      >
>      > And there is no reason in particular why the vCPU could not be a
>      > function parameter of `qemu_plugin_translate_vaddr`, right? You don't
>      > have the same restrictions as in `qemu_plugin_read_memory_hwaddr` or
>      > `qemu_plugin_hwaddr_operation_result` where you actually touch
>     memory?
>      >
> 
>     That's a good point, adding a "unsigned int vcpu_index" to the
>     signature
>     should be enough to query current or any other vcpu easily.
> 
> This is a really nice idea, it might be nice to make a vcpu version of 
> read/write register too. For memory, I'd think going with the current 
> memory is probably fine, I don't see any configs with different memory 
> per vcpu?
> 

Thinking about it twice, and after reading Alex comments for writing 
registers, it's probably not a good idea to allow such side effects on 
other vcpus (on registers and memory).
In case of registers, there is nothing ensuring they will be written 
correctly, so it only makes sense for current_cpu, as we are in a 
callback running on it.
Safer to have the same semantic for memory read/write also, so please 
forget my idea.