Florian Hofhammer <[email protected]> writes: > On 24/02/2026 18:49, Alex Bennée wrote: >> Florian Hofhammer <[email protected]> writes: >> >>> The opaque register handle encodes whether a register is read-only in >>> the lowest bit and prevents writing to the register via the plugin API >>> in this case. >>> >>> Signed-off-by: Florian Hofhammer <[email protected]> >>> --- >>> plugins/api.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/plugins/api.c b/plugins/api.c >>> index b2c52d2a09..3a8479ddce 100644 >>> --- a/plugins/api.c >>> +++ b/plugins/api.c >>> @@ -424,6 +424,7 @@ static GArray *create_register_handles(GArray >>> *gdbstub_regs) >>> for (int i = 0; i < gdbstub_regs->len; i++) { >>> GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i); >>> qemu_plugin_reg_descriptor desc; >>> + gint plugin_ro_bit = 0; >>> >>> /* skip "un-named" regs */ >>> if (!grd->name) { >>> @@ -431,7 +432,6 @@ static GArray *create_register_handles(GArray >>> *gdbstub_regs) >>> } >>> >>> /* Create a record for the plugin */ >>> - desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1); >>> desc.name = g_intern_string(grd->name); >>> desc.is_readonly = false; >>> if (g_strcmp0(desc.name, pc_str) == 0 >>> @@ -442,7 +442,9 @@ static GArray *create_register_handles(GArray >>> *gdbstub_regs) >>> || g_strcmp0(desc.name, rpc_str) == 0 >>> ) { >>> desc.is_readonly = true; >>> + plugin_ro_bit = 1; >>> } >>> + desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit); >>> desc.feature = g_intern_string(grd->feature_name); >>> g_array_append_val(find_data, desc); >>> } >>> @@ -467,7 +469,7 @@ bool qemu_plugin_read_register(struct >>> qemu_plugin_register *reg, >>> return false; >>> } >>> >>> - return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1) >>> > 0); >>> + return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) >>> > 0); >>> } >>> >>> bool qemu_plugin_write_register(struct qemu_plugin_register *reg, >>> @@ -476,11 +478,12 @@ bool 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 && >>> - qemu_plugin_get_cb_flags() != >>> QEMU_PLUGIN_CB_RW_REGS_PC)) { >>> + qemu_plugin_get_cb_flags() != >>> QEMU_PLUGIN_CB_RW_REGS_PC) >>> + || (GPOINTER_TO_INT(reg) & 1)) { >> >> Maybe this is better as: >> >> g_assert(GPOINTER_TO_INT(reg) & 1 == 0); >> >> if (buf->len == 0 || (qemu_plugin_get_cb_flags() != >> QEMU_PLUGIN_CB_RW_REGS &&... >> >> again the plugin is trying to do something it shouldn't. > > As far as I can tell, there's currently no mechanism in the test setup > to check that a certain assertion is triggered. In the previous test, I > just checked whether the API returns false when I try to write to a > read-only register, but now I'd need to check in my test whether the > assert triggers.
Ahh I see - I was going to say we don't really need to test for abuse of the API triggering asserts but you next test does indeed do that. I think we can live without explicitly adding a test case for attempts to write to read only registers. > > Should I check that in a wrapper script, test only the read path, or > follow a completely different approach for this? > >> >>> return false; >>> } >>> >>> - return (gdb_write_register(current_cpu, buf->data, >>> GPOINTER_TO_INT(reg) - 1) > 0); >>> + return (gdb_write_register(current_cpu, buf->data, >>> GPOINTER_TO_INT(reg) >> 1) > 0); >>> } >>> >>> void qemu_plugin_set_pc(uint64_t vaddr) >> > > Best regards, > Florian -- Alex Bennée Virtualisation Tech Lead @ Linaro
