On 02/03/2026 14:03, Alex Bennée wrote: > 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.
Ack, thanks for the quick reply! > >> >> 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 >
