[v3] Add additional plugin API functions to read and write memory and registers

[PATCH v3 2/8] Add register write API

Posted by Rowan Hart 7 months ago

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

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

Posted by Alex Bennée 7 months ago

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 2/8] Add register write API

Posted by Rowan Hart 7 months ago

>  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

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

Posted by Julian Ganz 7 months ago

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 2/8] Add register write API

Posted by Rowan Hart 7 months ago

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
>

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

Posted by Julian Ganz 7 months ago

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

Posted by Pierrick Bouvier 7 months ago

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>