From: Pierrick Bouvier <pierrick.bouvier@linaro.org>
To: Rowan Hart <rowanbhart@gmail.com>, qemu-devel@nongnu.org
Cc: "Eduardo Habkost" <eduardo@habkost.net>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Mahmoud Mandour" <ma.mandourr@gmail.com>,
"Zhao Liu" <zhao1.liu@intel.com>,
"Alexandre Iooss" <erdnaxe@crans.org>,
"Richard Henderson" <richard.henderson@linaro.org>,
"Alex Bennée" <alex.bennee@linaro.org>,
"Yanan Wang" <wangyanan55@huawei.com>
Subject: Re: [PATCH v7 3/9] plugins: Add enforcement of QEMU_PLUGIN_CB flags in register R/W callbacks
Date: Mon, 2 Jun 2025 13:49:24 -0700 [thread overview]
Message-ID: <7f925a2a-20d7-40e6-bf2c-ac3823912a04@linaro.org> (raw)
In-Reply-To: <20250602195706.1043662-4-rowanbhart@gmail.com>
On 6/2/25 12:57 PM, Rowan Hart wrote:
> This patch adds functionality to enforce the requested QEMU_PLUGIN_CB_
> flags level passed when registering a callback function using the
> plugins API. Each time a callback is about to be invoked, a thread-local
> variable will be updated with the level that callback requested. Then,
> called API functions (in particular, the register read and write API)
> will call qemu_plugin_get_cb_flags() to check the level is at least the
> level they require.
>
> Signed-off-by: Rowan Hart <rowanbhart@gmail.com>
> ---
> accel/tcg/plugin-gen.c | 30 ++++++++++++++++++++++++++++++
> include/hw/core/cpu.h | 1 +
> include/qemu/plugin.h | 4 ++++
> include/qemu/qemu-plugin.h | 3 ---
> plugins/api.c | 8 ++++++++
> plugins/core.c | 32 ++++++++++++++++++++++++++------
> 6 files changed, 69 insertions(+), 9 deletions(-)
>
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index c1da753894..9920381a84 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -117,10 +117,20 @@ static TCGv_i32 gen_cpu_index(void)
> static void gen_udata_cb(struct qemu_plugin_regular_cb *cb)
> {
> TCGv_i32 cpu_index = gen_cpu_index();
> + enum qemu_plugin_cb_flags cb_flags =
> + tcg_call_to_qemu_plugin_cb_flags(cb->info->flags);
> + TCGv_i32 flags = tcg_constant_i32(cb_flags);
> + TCGv_i32 clear_flags = tcg_constant_i32(QEMU_PLUGIN_CB_NO_REGS);
> + tcg_gen_st_i32(flags, tcg_env,
> + offsetof(CPUState, neg.plugin_cb_flags) - sizeof(CPUState));
> tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
> tcgv_i32_temp(cpu_index),
> tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
> + tcg_gen_st_i32(clear_flags, tcg_env,
> + offsetof(CPUState, neg.plugin_cb_flags) - sizeof(CPUState));
> tcg_temp_free_i32(cpu_index);
> + tcg_temp_free_i32(flags);
> + tcg_temp_free_i32(clear_flags);
> }
>
> static TCGv_ptr gen_plugin_u64_ptr(qemu_plugin_u64 entry)
> @@ -173,10 +183,20 @@ static void gen_udata_cond_cb(struct qemu_plugin_conditional_cb *cb)
> tcg_gen_ld_i64(val, ptr, 0);
> tcg_gen_brcondi_i64(cond, val, cb->imm, after_cb);
> TCGv_i32 cpu_index = gen_cpu_index();
> + enum qemu_plugin_cb_flags cb_flags =
> + tcg_call_to_qemu_plugin_cb_flags(cb->info->flags);
> + TCGv_i32 flags = tcg_constant_i32(cb_flags);
> + TCGv_i32 clear_flags = tcg_constant_i32(QEMU_PLUGIN_CB_NO_REGS);
> + tcg_gen_st_i32(flags, tcg_env,
> + offsetof(CPUState, neg.plugin_cb_flags) - sizeof(CPUState));
> tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
> tcgv_i32_temp(cpu_index),
> tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
> + tcg_gen_st_i32(clear_flags, tcg_env,
> + offsetof(CPUState, neg.plugin_cb_flags) - sizeof(CPUState));
> tcg_temp_free_i32(cpu_index);
> + tcg_temp_free_i32(flags);
> + tcg_temp_free_i32(clear_flags);
> gen_set_label(after_cb);
>
> tcg_temp_free_i64(val);
> @@ -210,12 +230,22 @@ static void gen_mem_cb(struct qemu_plugin_regular_cb *cb,
> qemu_plugin_meminfo_t meminfo, TCGv_i64 addr)
> {
> TCGv_i32 cpu_index = gen_cpu_index();
> + enum qemu_plugin_cb_flags cb_flags =
> + tcg_call_to_qemu_plugin_cb_flags(cb->info->flags);
> + TCGv_i32 flags = tcg_constant_i32(cb_flags);
> + TCGv_i32 clear_flags = tcg_constant_i32(QEMU_PLUGIN_CB_NO_REGS);
> + tcg_gen_st_i32(flags, tcg_env,
> + offsetof(CPUState, neg.plugin_cb_flags) - sizeof(CPUState));
> tcg_gen_call4(cb->f.vcpu_mem, cb->info, NULL,
> tcgv_i32_temp(cpu_index),
> tcgv_i32_temp(tcg_constant_i32(meminfo)),
> tcgv_i64_temp(addr),
> tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
> + tcg_gen_st_i32(clear_flags, tcg_env,
> + offsetof(CPUState, neg.plugin_cb_flags) - sizeof(CPUState));
> tcg_temp_free_i32(cpu_index);
> + tcg_temp_free_i32(flags);
> + tcg_temp_free_i32(clear_flags);
> }
>
> static void inject_cb(struct qemu_plugin_dyn_cb *cb)
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 1e87f7d393..d3cc9a5224 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -368,6 +368,7 @@ typedef struct CPUNegativeOffsetState {
> GArray *plugin_mem_cbs;
> uint64_t plugin_mem_value_low;
> uint64_t plugin_mem_value_high;
> + int32_t plugin_cb_flags;
> #endif
> IcountDecr icount_decr;
> bool can_do_io;
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index 9726a9ebf3..2fef2e7d71 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -209,6 +209,10 @@ void qemu_plugin_user_prefork_lock(void);
> */
> void qemu_plugin_user_postfork(bool is_child);
>
> +enum qemu_plugin_cb_flags tcg_call_to_qemu_plugin_cb_flags(int flags);
> +
> +enum qemu_plugin_cb_flags qemu_plugin_get_cb_flags(void);
> +
> #else /* !CONFIG_PLUGIN */
>
> static inline void qemu_plugin_add_opts(void)
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index cfe1692ecb..120fb626a6 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,
> diff --git a/plugins/api.c b/plugins/api.c
> index 3a7add50d2..16141f5c25 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -437,6 +437,10 @@ int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
> {
> g_assert(current_cpu);
>
> + if (qemu_plugin_get_cb_flags() == QEMU_PLUGIN_CB_NO_REGS) {
> + return -1;
> + }
> +
> return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
> }
>
> @@ -445,6 +449,10 @@ int qemu_plugin_write_register(struct qemu_plugin_register *reg,
> {
> g_assert(current_cpu);
>
> + if (buf->len == 0 || qemu_plugin_get_cb_flags() != QEMU_PLUGIN_CB_RW_REGS) {
> + return 0;
> + }
> +
Would it be better to return -1 for "qemu_plugin_get_cb_flags() !=
QEMU_PLUGIN_CB_RW_REGS", so user can notice there is something wrong
with flags?
> return gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1);
> }
>
> diff --git a/plugins/core.c b/plugins/core.c
> index eb9281fe54..34bddb6c1c 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -364,14 +364,15 @@ void plugin_register_dyn_cb__udata(GArray **arr,
> enum qemu_plugin_cb_flags flags,
> void *udata)
> {
> - static TCGHelperInfo info[3] = {
> + static TCGHelperInfo info[4] = {
> [QEMU_PLUGIN_CB_NO_REGS].flags = TCG_CALL_NO_RWG,
> [QEMU_PLUGIN_CB_R_REGS].flags = TCG_CALL_NO_WG,
> + [QEMU_PLUGIN_CB_RW_REGS].flags = 0,
> /*
> * Match qemu_plugin_vcpu_udata_cb_t:
> * void (*)(uint32_t, void *)
> */
> - [0 ... 2].typemask = (dh_typemask(void, 0) |
> + [0 ... 3].typemask = (dh_typemask(void, 0) |
> dh_typemask(i32, 1) |
> dh_typemask(ptr, 2))
> };
[QEMU_PLUGIN_CB_RW_REGS].flags = 0 was already set implicitly, as all
elements not explicit set are initialized to 0.
As you can see, [0 ... 2].typemask was set, which shows we initialized
the third element.
Adding [QEMU_PLUGIN_CB_RW_REGS].flags = 0 does not hurt, and is more
explicit, but you don't need to increase array size.
This static array is used to set info field in callback struct, as
.info = &info[flags]. Flags being an enum qemu_plugin_cb_flags, its
value is 0,1 or 2, so adding one entry is useless.
> @@ -393,14 +394,15 @@ void plugin_register_dyn_cond_cb__udata(GArray **arr,
> uint64_t imm,
> void *udata)
> {
> - static TCGHelperInfo info[3] = {
> + static TCGHelperInfo info[4] = {
> [QEMU_PLUGIN_CB_NO_REGS].flags = TCG_CALL_NO_RWG,
> [QEMU_PLUGIN_CB_R_REGS].flags = TCG_CALL_NO_WG,
> + [QEMU_PLUGIN_CB_RW_REGS].flags = 0,
> /*
> * Match qemu_plugin_vcpu_udata_cb_t:
> * void (*)(uint32_t, void *)
> */
> - [0 ... 2].typemask = (dh_typemask(void, 0) |
> + [0 ... 3].typemask = (dh_typemask(void, 0) |
> dh_typemask(i32, 1) |
> dh_typemask(ptr, 2))
> };
> @@ -431,14 +433,15 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
> !__builtin_types_compatible_p(qemu_plugin_meminfo_t, uint32_t) &&
> !__builtin_types_compatible_p(qemu_plugin_meminfo_t, int32_t));
>
> - static TCGHelperInfo info[3] = {
> + static TCGHelperInfo info[4] = {
> [QEMU_PLUGIN_CB_NO_REGS].flags = TCG_CALL_NO_RWG,
> [QEMU_PLUGIN_CB_R_REGS].flags = TCG_CALL_NO_WG,
> + [QEMU_PLUGIN_CB_RW_REGS].flags = 0,
> /*
> * Match qemu_plugin_vcpu_mem_cb_t:
> * void (*)(uint32_t, qemu_plugin_meminfo_t, uint64_t, void *)
> */
> - [0 ... 2].typemask =
> + [0 ... 3].typemask =
> (dh_typemask(void, 0) |
> dh_typemask(i32, 1) |
> (__builtin_types_compatible_p(qemu_plugin_meminfo_t, uint32_t)
> @@ -760,3 +763,20 @@ void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
> g_array_free(score->data, TRUE);
> g_free(score);
> }
> +
> +enum qemu_plugin_cb_flags tcg_call_to_qemu_plugin_cb_flags(int flags)
> +{
> + if (flags & TCG_CALL_NO_RWG) {
> + return QEMU_PLUGIN_CB_NO_REGS;
> + } else if (flags & TCG_CALL_NO_WG) {
> + return QEMU_PLUGIN_CB_R_REGS;
> + } else {
> + return QEMU_PLUGIN_CB_RW_REGS;
> + }
> +}
> +
> +enum qemu_plugin_cb_flags qemu_plugin_get_cb_flags(void)
> +{
> + assert(current_cpu);
> + return current_cpu->neg.plugin_cb_flags;
> +}
next prev parent reply other threads:[~2025-06-02 20:50 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-02 19:56 [PATCH v7 0/9] Add additional plugin API functions to read and write memory and registers Rowan Hart
2025-06-02 19:56 ` [PATCH v7 1/9] gdbstub: Expose gdb_write_register function to consumers of gdbstub Rowan Hart
2025-06-02 19:56 ` [PATCH v7 2/9] plugins: Add register write API Rowan Hart
2025-06-02 19:57 ` [PATCH v7 3/9] plugins: Add enforcement of QEMU_PLUGIN_CB flags in register R/W callbacks Rowan Hart
2025-06-02 20:49 ` Pierrick Bouvier [this message]
2025-06-02 21:44 ` Rowan Hart
2025-06-02 19:57 ` [PATCH v7 4/9] plugins: Add memory virtual address write API Rowan Hart
2025-06-02 19:57 ` [PATCH v7 5/9] plugins: Add memory hardware address read/write API Rowan Hart
2025-06-02 19:57 ` [PATCH v7 6/9] plugins: Add patcher plugin and test Rowan Hart
2025-06-02 19:57 ` [PATCH v7 7/9] plugins: Add hypercalls " Rowan Hart
2025-06-02 19:57 ` [PATCH v7 8/9] plugins: Remove use of qemu_plugin_read_register where it is not permitted Rowan Hart
2025-06-02 19:57 ` [PATCH v7 9/9] plugins: Update plugin version and add notes Rowan Hart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7f925a2a-20d7-40e6-bf2c-ac3823912a04@linaro.org \
--to=pierrick.bouvier@linaro.org \
--cc=alex.bennee@linaro.org \
--cc=eduardo@habkost.net \
--cc=erdnaxe@crans.org \
--cc=ma.mandourr@gmail.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=pbonzini@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=rowanbhart@gmail.com \
--cc=wangyanan55@huawei.com \
--cc=zhao1.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).