Alex Bennée <[email protected]> writes: > Florian Hofhammer <[email protected]> writes: > >> Some registers should be marked as read-only from a plugin API >> perspective, as writing to them via qemu_plugin_write_register has no >> effect. This includes the program counter, and we expose this fact to >> the plugins with this patch. >> >> Signed-off-by: Florian Hofhammer <[email protected]> >> --- >> include/qemu/qemu-plugin.h | 2 ++ >> plugins/api.c | 17 +++++++++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> index f976b62030..1f25fb2b40 100644 >> --- a/include/qemu/qemu-plugin.h >> +++ b/include/qemu/qemu-plugin.h >> @@ -942,11 +942,13 @@ struct qemu_plugin_register; >> * writing value with qemu_plugin_write_register >> * @name: register name >> * @feature: optional feature descriptor, can be NULL >> + * @is_readonly: true if the register cannot be written via >> qemu_plugin_write_register >> */ >> typedef struct { >> struct qemu_plugin_register *handle; >> const char *name; >> const char *feature; >> + bool is_readonly; >> } qemu_plugin_reg_descriptor; >> >> /** >> diff --git a/plugins/api.c b/plugins/api.c >> index fc19bdb40b..de8c32db50 100644 >> --- a/plugins/api.c >> +++ b/plugins/api.c >> @@ -403,6 +403,12 @@ bool qemu_plugin_bool_parse(const char *name, const >> char *value, bool *ret) >> * ancillary data the plugin might find useful. >> */ >> >> +static const char pc_str[] = "pc"; // generic name for program counter >> +static const char eip_str[] = "eip"; // x86 specific name for program >> counter >> +static const char rip_str[] = "rip"; // x86_64 specific name for program >> counter >> +static const char pswa_str[] = "pswa"; // s390x specific name for program >> counter >> +static const char iaoq_str[] = "iaoq"; // HP/PA specific name for program >> counter >> +static const char rpc_str[] = "rpc"; // microblaze specific name for >> program counter > > It's ugly but I can't think of anything better as you say in the commit > message. > >> static GArray *create_register_handles(GArray *gdbstub_regs) >> { >> GArray *find_data = g_array_new(true, true, >> @@ -417,9 +423,20 @@ static GArray *create_register_handles(GArray >> *gdbstub_regs) >> continue; >> } >> >> + gint plugin_ro_bit = 0; >> /* Create a record for the plugin */ >> desc.handle = GINT_TO_POINTER(grd->gdb_reg + 1); >> desc.name = g_intern_string(grd->name); > > Lets just set desc.is_readonly to false here. > >> + if (!strcmp(desc.name, pc_str) >> + || !strcmp(desc.name, eip_str) >> + || !strcmp(desc.name, rip_str) >> + || !strcmp(desc.name, pswa_str) >> + || !strcmp(desc.name, iaoq_str) >> + || !strcmp(desc.name, rpc_str) >> + ) { >> + plugin_ro_bit = 1; >> + } >> + desc.is_readonly = plugin_ro_bit == 1 ? true : false; > > And fold setting it to true into the if statement. I have a marginal > preference for g_strcmp0(desc.name, eip_str) == 0 style tests.
The option of course would be to skip the register all together. Do we have code which will have multiple vaddr's for the same TB where using the TB data wouldn't be sufficient? > >> desc.feature = g_intern_string(grd->feature_name); >> g_array_append_val(find_data, desc); >> } > > Otherwise: > > Reviewed-by: Alex Bennée <[email protected]> -- Alex Bennée Virtualisation Tech Lead @ Linaro
