| Message ID | 20231103195956.1998255-25-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub and plugin read register and windows support | expand |
On 2023/11/04 4:59, Alex Bennée wrote: > We can only request a list of registers once the vCPU has been > initialised so the user needs to use either call the find function on > vCPU initialisation or during the translation phase. We don't expose > the reg number to the plugin instead hiding it behind an opaque > handle. This allows for a bit of future proofing should the internals > need to be changed while also being hashed against the CPUClass so we > can handle different register sets per-vCPU in hetrogenous situations. > > Having an internal state within the plugins also allows us to expand > the interface in future (for example providing callbacks on register > change if the translator can track changes). > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 > Cc: Akihiko Odaki <akihiko.odaki@daynix.com> > Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > vAJB: > > The main difference to Akikio's version is hiding the gdb register > detail from the plugin for the reasons described above. > --- > include/qemu/qemu-plugin.h | 52 +++++++++++++++++- > plugins/api.c | 102 +++++++++++++++++++++++++++++++++++ > plugins/qemu-plugins.symbols | 2 + > 3 files changed, 154 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index 50a9957279..e5c16df5ca 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -11,6 +11,7 @@ > #ifndef QEMU_QEMU_PLUGIN_H > #define QEMU_QEMU_PLUGIN_H > > +#include <glib.h> > #include <inttypes.h> > #include <stdbool.h> > #include <stddef.h> > @@ -218,8 +219,8 @@ struct qemu_plugin_insn; > * @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 unused, plugins cannot read or change system > - * register state. > + * 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, > @@ -664,4 +665,51 @@ uint64_t qemu_plugin_end_code(void); > */ > uint64_t qemu_plugin_entry_code(void); > > +/** struct qemu_plugin_register - Opaque handle for a translated instruction */ > +struct qemu_plugin_register; > + > +/** > + * typedef qemu_plugin_reg_descriptor - register descriptions > + * > + * @name: register name > + * @handle: opaque handle for retrieving value with qemu_plugin_read_register > + * @feature: optional feature descriptor, can be NULL > + */ > +typedef struct { > + char name[32]; > + struct qemu_plugin_register *handle; > + const char *feature; > +} qemu_plugin_reg_descriptor; > + > +/** > + * qemu_plugin_find_registers() - return register list > + * @vcpu_index: vcpu to query > + * @reg_pattern: register name pattern > + * > + * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller > + * frees. As the register set of a given vCPU is only available once > + * the vCPU is initialised if you want to monitor registers from the > + * start you should call this from a qemu_plugin_register_vcpu_init_cb() > + * callback. > + */ > +GArray * qemu_plugin_find_registers(unsigned int vcpu_index, const char *reg_pattern); A pattern may be convenient for humans but not for machine. My motivation to introduce the feature is to generate traces consumable by trace-based simulators. Such a plugin needs an exact match of registers.
Akihiko Odaki <akihiko.odaki@daynix.com> writes: (re-adding qemu-devel which my mail client dropped a few messages ago, sorry) > On 2023/11/06 19:46, Alex Bennée wrote: >> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >> >>> On 2023/11/06 18:30, Alex Bennée wrote: >>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >>>> >>>>> On 2023/11/04 4:59, Alex Bennée wrote: >>>>>> We can only request a list of registers once the vCPU has been >>>>>> initialised so the user needs to use either call the find function on >>>>>> vCPU initialisation or during the translation phase. We don't expose >>>>>> the reg number to the plugin instead hiding it behind an opaque >>>>>> handle. This allows for a bit of future proofing should the internals >>>>>> need to be changed while also being hashed against the CPUClass so we >>>>>> can handle different register sets per-vCPU in hetrogenous situations. >>>>>> Having an internal state within the plugins also allows us to expand >>>>>> the interface in future (for example providing callbacks on register >>>>>> change if the translator can track changes). >>>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 >>>>>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>>> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> >>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>>>> +struct qemu_plugin_register; >>>>>> + >>>>>> +/** >>>>>> + * typedef qemu_plugin_reg_descriptor - register descriptions >>>>>> + * >>>>>> + * @name: register name >>>>>> + * @handle: opaque handle for retrieving value with qemu_plugin_read_register >>>>>> + * @feature: optional feature descriptor, can be NULL >>>>>> + */ >>>>>> +typedef struct { >>>>>> + char name[32]; >>>>>> + struct qemu_plugin_register *handle; >>>>>> + const char *feature; >>>>>> +} qemu_plugin_reg_descriptor; >>>>>> + >>>>>> +/** >>>>>> + * qemu_plugin_find_registers() - return register list >>>>>> + * @vcpu_index: vcpu to query >>>>>> + * @reg_pattern: register name pattern >>>>>> + * >>>>>> + * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller >>>>>> + * frees. As the register set of a given vCPU is only available once >>>>>> + * the vCPU is initialised if you want to monitor registers from the >>>>>> + * start you should call this from a qemu_plugin_register_vcpu_init_cb() >>>>>> + * callback. >>>>>> + */ >>>>>> +GArray * qemu_plugin_find_registers(unsigned int vcpu_index, const char *reg_pattern); >>>>> >>>>> A pattern may be convenient for humans but not for machine. My >>>>> motivation to introduce the feature is to generate traces consumable >>>>> by trace-based simulators. Such a plugin needs an exact match of >>>>> registers. >>>> That's true - but we don't have any such users in the code base. >>>> However >>>> for exact matches the details are in qemu_plugin_reg_descriptor so you >>>> can always filter there if you want. >>> >>> I designed the feature to read registers for users outside of the code >>> base so the code base has no practical user. >>> I added the feature to log register values to execlog but it's only >>> for demonstration and it is useless for practical use; >> I wouldn't say its useless - and it is important to have in-tree >> code to >> exercise the various parts of the API we expose. > > I mean it is useless except for demonstration. Having some code for > demonstration is good but we shouldn't overfit the API to it. > >> To be clear is your objection just to the way >> qemu_plugin_find_registers() works or the whole concept of using a >> handle instead of the register number? I'm certainly open to better ways >> of doing the former but as explained in the commit I think the handle >> based approach is a more hygienic interface that gives us scope to >> improve it going forward. > > Yes, my major concern is that the pattern matching. OK. Another potential consumer I thought about during implementing the internal API was HMP which would also benefit from a more human wildcard type search. So I think the resolution of this is having two APIs, one returning a list of qemu_plugin_reg_descriptor and one returning a single descriptor only with an exact match. I thought exposing features and registers in two calls was a bit clunky though so how about: struct qemu_plugin_reg_descriptor * qemu_plugin_find_register(unsigned int vcpu_index, const char *name, const char *gdb_feature); which will only reply on an exact match (although I still think register names are unique enough you can get away without gdb_feature). > I'm fine with the use of a pointer instead of the register number. A > pointer is indeed more random for each run so it will prevent the user > from hardcoding it. > >> As we are so close to soft-freeze I suggest I re-spin the series to >> include 1-12/29 and the windows bits and we can work on a better API for >> the 9.0 release. > > I'm not in haste either and fine to delay it for the 9.0 release. OK I'll get as much as I can merged now and leave the final API bits for when the tree opens up. I don't suppose you have any idea why: target/riscv: Use GDBFeature for dynamic XML caused a regression? The XML generated looks identical but the communication diverges with the riscv-csr response: gdbstub_io_command Received: qXfer:features:read:riscv-csr.xm gdbstub_io_command Received: qXfer:features:read:riscv-csr.xm gdbstub_io_binaryreply 0x0000: 6c 3c 3f 78 6d 6c 20 76 65 7 gdbstub_io_binaryreply 0x0000: 6c 3c 3f 78 6d 6c 20 76 65 7 gdbstub_io_binaryreply 0x0010: 31 2e 30 22 3f 3e 3c 21 44 4 gdbstub_io_binaryreply 0x0010: 31 2e 30 22 3f 3e 3c 21 44 4 gdbstub_io_binaryreply 0x0020: 66 65 61 74 75 72 65 20 53 5 gdbstub_io_binaryreply 0x0020: 66 65 61 74 75 72 65 20 53 5 gdbstub_io_binaryreply 0x0030: 67 64 62 2d 74 61 72 67 65 7 gdbstub_io_binaryreply 0x0030: 67 64 62 2d 74 61 72 67 65 7 gdbstub_io_binaryreply 0x0040: 3c 66 65 61 74 75 72 65 20 6 gdbstub_io_binaryreply 0x0040: 3c 66 65 61 74 75 72 65 20 6 gdbstub_io_binaryreply 0x0050: 72 67 2e 67 6e 75 2e 67 64 6 gdbstub_io_binaryreply 0x0050: 72 67 2e 67 6e 75 2e 67 64 6 gdbstub_io_binaryreply 0x0060: 2e 63 73 72 22 3e 3c 72 65 6 gdbstub_io_binaryreply 0x0060: 2e 63 73 72 22 3e 3c 72 65 6 gdbstub_io_binaryreply 0x0070: 22 66 66 6c 61 67 73 22 20 6 gdbstub_io_binaryreply 0x0070: 22 66 66 6c 61 67 73 22 20 6 gdbstub_io_binaryreply 0x0080: 3d 22 36 34 22 20 72 65 67 6 gdbstub_io_binaryreply 0x0080: 3d 22 36 34 22 20 72 65 67 6 gdbstub_io_binaryreply 0x0090: 22 2f 3e 3c 72 65 67 20 6e 6 | gdbstub_io_binaryreply 0x0090: 22 20 74 79 70 65 3d 22 69 6 gdbstub_io_binaryreply 0x00a0: 6d 22 20 62 69 74 73 69 7a 6 | gdbstub_io_binaryreply 0x00a0: 65 67 20 6e 61 6d 65 3d 22 6 gdbstub_io_binaryreply 0x00b0: 72 65 67 6e 75 6d 3d 22 36 3 | gdbstub_io_binaryreply 0x00b0: 74 73 69 7a 65 3d 22 36 34 2 gdbstub_io_binaryreply 0x00c0: 67 20 6e 61 6d 65 3d 22 66 6 | gdbstub_io_binaryreply 0x00c0: 6d 3d 22 36 38 22 20 74 79 7 gdbstub_io_binaryreply 0x00d0: 74 73 69 7a 65 3d 22 36 34 2 | gdbstub_io_binaryreply 0x00d0: 22 2f 3e 3c 72 65 67 20 6e 6 gdbstub_io_binaryreply 0x00e0: 6d 3d 22 36 39 22 2f 3e 3c 7 | gdbstub_io_binaryreply 0x00e0: 73 72 22 20 62 69 74 73 69 7 gdbstub_io_binaryreply 0x00f0: 65 3d 22 63 79 63 6c 65 22 2 | gdbstub_io_binaryreply 0x00f0: 20 72 65 67 6e 75 6d 3d 22 3 gdbstub_io_binaryreply 0x0100: 65 3d 22 36 34 22 20 72 65 6 | gdbstub_io_binaryreply 0x0100: 65 3d 22 69 6e 74 22 2f 3e 3 gdbstub_io_binaryreply 0x0110: 31 33 38 22 2f 3e 3c 72 65 6 | gdbstub_io_binaryreply 0x0110: 6d 65 3d 22 63 79 63 6c 65 2 gdbstub_io_binaryreply 0x0120: 22 74 69 6d 65 22 20 62 69 7 | gdbstub_io_binaryreply 0x0120: 7a 65 3d 22 36 34 22 20 72 6 gdbstub_io_binaryreply 0x0130: 36 34 22 20 72 65 67 6e 75 6 | gdbstub_io_binaryreply 0x0130: 33 31 33 38 22 20 74 79 70 6 gdbstub_io_binaryreply 0x0140: 22 2f 3e 3c 72 65 67 20 6e 6 | gdbstub_io_binaryreply 0x0140: 2f 3e 3c 72 65 67 20 6e 61 6 gdbstub_io_binaryreply 0x0150: 73 74 72 65 74 22 20 62 69 7 | gdbstub_io_binaryreply 0x0150: 65 22 20 62 69 74 73 69 7a 6 gdbstub_io_binaryreply 0x0160: 36 34 22 20 72 65 67 6e 75 6 | gdbstub_io_binaryreply 0x0160: 72 65 67 6e 75 6d 3d 22 33 3 gdbstub_io_binaryreply 0x0170: 22 2f 3e 3c 2f 66 65 61 74 7 | gdbstub_io_binaryreply 0x0170: 70 65 3d 22 69 6e 74 22 2f 3 > gdbstub_io_binaryreply 0x0180: 61 6d 65 3d 22 69 6e 73 74 7 > gdbstub_io_binaryreply 0x0190: 74 73 69 7a 65 3d 22 36 34 2 > gdbstub_io_binaryreply 0x01a0: 6d 3d 22 33 31 34 30 22 20 7 > gdbstub_io_binaryreply 0x01b0: 6e 74 22 2f 3e 3c 2f 66 65 6 gdbstub_io_command Received: qTStatus gdbstub_io_command Received: qTStatus gdbstub_io_reply Sent: gdbstub_io_reply Sent: gdbstub_io_command Received: ? gdbstub_io_command Received: ? gdbstub_io_reply Sent: T05thread:p2003b4.2003b4; | gdbstub_io_reply Sent: T05thread:p2011b6.2011b6; Was this the reason for the misa_max cleanups?
On 2023/11/06 20:40, Alex Bennée wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > > (re-adding qemu-devel which my mail client dropped a few messages ago, sorry) > >> On 2023/11/06 19:46, Alex Bennée wrote: >>> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >>> >>>> On 2023/11/06 18:30, Alex Bennée wrote: >>>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >>>>> >>>>>> On 2023/11/04 4:59, Alex Bennée wrote: >>>>>>> We can only request a list of registers once the vCPU has been >>>>>>> initialised so the user needs to use either call the find function on >>>>>>> vCPU initialisation or during the translation phase. We don't expose >>>>>>> the reg number to the plugin instead hiding it behind an opaque >>>>>>> handle. This allows for a bit of future proofing should the internals >>>>>>> need to be changed while also being hashed against the CPUClass so we >>>>>>> can handle different register sets per-vCPU in hetrogenous situations. >>>>>>> Having an internal state within the plugins also allows us to expand >>>>>>> the interface in future (for example providing callbacks on register >>>>>>> change if the translator can track changes). >>>>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 >>>>>>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>>>> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> >>>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>>>>> +struct qemu_plugin_register; >>>>>>> + >>>>>>> +/** >>>>>>> + * typedef qemu_plugin_reg_descriptor - register descriptions >>>>>>> + * >>>>>>> + * @name: register name >>>>>>> + * @handle: opaque handle for retrieving value with qemu_plugin_read_register >>>>>>> + * @feature: optional feature descriptor, can be NULL >>>>>>> + */ >>>>>>> +typedef struct { >>>>>>> + char name[32]; >>>>>>> + struct qemu_plugin_register *handle; >>>>>>> + const char *feature; >>>>>>> +} qemu_plugin_reg_descriptor; >>>>>>> + >>>>>>> +/** >>>>>>> + * qemu_plugin_find_registers() - return register list >>>>>>> + * @vcpu_index: vcpu to query >>>>>>> + * @reg_pattern: register name pattern >>>>>>> + * >>>>>>> + * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller >>>>>>> + * frees. As the register set of a given vCPU is only available once >>>>>>> + * the vCPU is initialised if you want to monitor registers from the >>>>>>> + * start you should call this from a qemu_plugin_register_vcpu_init_cb() >>>>>>> + * callback. >>>>>>> + */ >>>>>>> +GArray * qemu_plugin_find_registers(unsigned int vcpu_index, const char *reg_pattern); >>>>>> >>>>>> A pattern may be convenient for humans but not for machine. My >>>>>> motivation to introduce the feature is to generate traces consumable >>>>>> by trace-based simulators. Such a plugin needs an exact match of >>>>>> registers. >>>>> That's true - but we don't have any such users in the code base. >>>>> However >>>>> for exact matches the details are in qemu_plugin_reg_descriptor so you >>>>> can always filter there if you want. >>>> >>>> I designed the feature to read registers for users outside of the code >>>> base so the code base has no practical user. >>>> I added the feature to log register values to execlog but it's only >>>> for demonstration and it is useless for practical use; >>> I wouldn't say its useless - and it is important to have in-tree >>> code to >>> exercise the various parts of the API we expose. >> >> I mean it is useless except for demonstration. Having some code for >> demonstration is good but we shouldn't overfit the API to it. >> >>> To be clear is your objection just to the way >>> qemu_plugin_find_registers() works or the whole concept of using a >>> handle instead of the register number? I'm certainly open to better ways >>> of doing the former but as explained in the commit I think the handle >>> based approach is a more hygienic interface that gives us scope to >>> improve it going forward. >> >> Yes, my major concern is that the pattern matching. > > OK. Another potential consumer I thought about during implementing the > internal API was HMP which would also benefit from a more human wildcard > type search. So I think the resolution of this is having two APIs, one > returning a list of qemu_plugin_reg_descriptor and one returning a > single descriptor only with an exact match. I don't think qemu_plugin_find_registers() is so versetile that it should be a public API. What is appropriate as a user interface depends more on the context. For HMP, it may be better to implement command completion instead of having a wildcard. Some may want regular expressions instead of GLib patterns. Some may want POSIX-compliant glob instead of GLib-specific pattern match (the quirks of GLib pattern is documented at https://gitlab.gnome.org/GNOME/glib/-/blob/2.78.1/glib/gpattern.c#L33). I think it's better to expose an array of register names and let the plugin do the pattern match in a way appropriate for the specific use case. > > I thought exposing features and registers in two calls was a bit clunky > though so how about: > > struct qemu_plugin_reg_descriptor * > qemu_plugin_find_register(unsigned int vcpu_index, > const char *name, > const char *gdb_feature); > > which will only reply on an exact match (although I still think > register names are unique enough you can get away without gdb_feature). > >> I'm fine with the use of a pointer instead of the register number. A >> pointer is indeed more random for each run so it will prevent the user >> from hardcoding it. >> >>> As we are so close to soft-freeze I suggest I re-spin the series to >>> include 1-12/29 and the windows bits and we can work on a better API for >>> the 9.0 release. >> >> I'm not in haste either and fine to delay it for the 9.0 release. > > OK I'll get as much as I can merged now and leave the final API bits for > when the tree opens up. I don't suppose you have any idea why: > > target/riscv: Use GDBFeature for dynamic XML > > caused a regression? The XML generated looks identical but the > communication diverges with the riscv-csr response: > > gdbstub_io_command Received: qXfer:features:read:riscv-csr.xm gdbstub_io_command Received: qXfer:features:read:riscv-csr.xm > gdbstub_io_binaryreply 0x0000: 6c 3c 3f 78 6d 6c 20 76 65 7 gdbstub_io_binaryreply 0x0000: 6c 3c 3f 78 6d 6c 20 76 65 7 > gdbstub_io_binaryreply 0x0010: 31 2e 30 22 3f 3e 3c 21 44 4 gdbstub_io_binaryreply 0x0010: 31 2e 30 22 3f 3e 3c 21 44 4 > gdbstub_io_binaryreply 0x0020: 66 65 61 74 75 72 65 20 53 5 gdbstub_io_binaryreply 0x0020: 66 65 61 74 75 72 65 20 53 5 > gdbstub_io_binaryreply 0x0030: 67 64 62 2d 74 61 72 67 65 7 gdbstub_io_binaryreply 0x0030: 67 64 62 2d 74 61 72 67 65 7 > gdbstub_io_binaryreply 0x0040: 3c 66 65 61 74 75 72 65 20 6 gdbstub_io_binaryreply 0x0040: 3c 66 65 61 74 75 72 65 20 6 > gdbstub_io_binaryreply 0x0050: 72 67 2e 67 6e 75 2e 67 64 6 gdbstub_io_binaryreply 0x0050: 72 67 2e 67 6e 75 2e 67 64 6 > gdbstub_io_binaryreply 0x0060: 2e 63 73 72 22 3e 3c 72 65 6 gdbstub_io_binaryreply 0x0060: 2e 63 73 72 22 3e 3c 72 65 6 > gdbstub_io_binaryreply 0x0070: 22 66 66 6c 61 67 73 22 20 6 gdbstub_io_binaryreply 0x0070: 22 66 66 6c 61 67 73 22 20 6 > gdbstub_io_binaryreply 0x0080: 3d 22 36 34 22 20 72 65 67 6 gdbstub_io_binaryreply 0x0080: 3d 22 36 34 22 20 72 65 67 6 > gdbstub_io_binaryreply 0x0090: 22 2f 3e 3c 72 65 67 20 6e 6 | gdbstub_io_binaryreply 0x0090: 22 20 74 79 70 65 3d 22 69 6 > gdbstub_io_binaryreply 0x00a0: 6d 22 20 62 69 74 73 69 7a 6 | gdbstub_io_binaryreply 0x00a0: 65 67 20 6e 61 6d 65 3d 22 6 > gdbstub_io_binaryreply 0x00b0: 72 65 67 6e 75 6d 3d 22 36 3 | gdbstub_io_binaryreply 0x00b0: 74 73 69 7a 65 3d 22 36 34 2 > gdbstub_io_binaryreply 0x00c0: 67 20 6e 61 6d 65 3d 22 66 6 | gdbstub_io_binaryreply 0x00c0: 6d 3d 22 36 38 22 20 74 79 7 > gdbstub_io_binaryreply 0x00d0: 74 73 69 7a 65 3d 22 36 34 2 | gdbstub_io_binaryreply 0x00d0: 22 2f 3e 3c 72 65 67 20 6e 6 > gdbstub_io_binaryreply 0x00e0: 6d 3d 22 36 39 22 2f 3e 3c 7 | gdbstub_io_binaryreply 0x00e0: 73 72 22 20 62 69 74 73 69 7 > gdbstub_io_binaryreply 0x00f0: 65 3d 22 63 79 63 6c 65 22 2 | gdbstub_io_binaryreply 0x00f0: 20 72 65 67 6e 75 6d 3d 22 3 > gdbstub_io_binaryreply 0x0100: 65 3d 22 36 34 22 20 72 65 6 | gdbstub_io_binaryreply 0x0100: 65 3d 22 69 6e 74 22 2f 3e 3 > gdbstub_io_binaryreply 0x0110: 31 33 38 22 2f 3e 3c 72 65 6 | gdbstub_io_binaryreply 0x0110: 6d 65 3d 22 63 79 63 6c 65 2 > gdbstub_io_binaryreply 0x0120: 22 74 69 6d 65 22 20 62 69 7 | gdbstub_io_binaryreply 0x0120: 7a 65 3d 22 36 34 22 20 72 6 > gdbstub_io_binaryreply 0x0130: 36 34 22 20 72 65 67 6e 75 6 | gdbstub_io_binaryreply 0x0130: 33 31 33 38 22 20 74 79 70 6 > gdbstub_io_binaryreply 0x0140: 22 2f 3e 3c 72 65 67 20 6e 6 | gdbstub_io_binaryreply 0x0140: 2f 3e 3c 72 65 67 20 6e 61 6 > gdbstub_io_binaryreply 0x0150: 73 74 72 65 74 22 20 62 69 7 | gdbstub_io_binaryreply 0x0150: 65 22 20 62 69 74 73 69 7a 6 > gdbstub_io_binaryreply 0x0160: 36 34 22 20 72 65 67 6e 75 6 | gdbstub_io_binaryreply 0x0160: 72 65 67 6e 75 6d 3d 22 33 3 > gdbstub_io_binaryreply 0x0170: 22 2f 3e 3c 2f 66 65 61 74 7 | gdbstub_io_binaryreply 0x0170: 70 65 3d 22 69 6e 74 22 2f 3 > > gdbstub_io_binaryreply 0x0180: 61 6d 65 3d 22 69 6e 73 74 7 > > gdbstub_io_binaryreply 0x0190: 74 73 69 7a 65 3d 22 36 34 2 > > gdbstub_io_binaryreply 0x01a0: 6d 3d 22 33 31 34 30 22 20 7 > > gdbstub_io_binaryreply 0x01b0: 6e 74 22 2f 3e 3c 2f 66 65 6 > gdbstub_io_command Received: qTStatus gdbstub_io_command Received: qTStatus > gdbstub_io_reply Sent: gdbstub_io_reply Sent: > gdbstub_io_command Received: ? gdbstub_io_command Received: ? > gdbstub_io_reply Sent: T05thread:p2003b4.2003b4; | gdbstub_io_reply Sent: T05thread:p2011b6.2011b6; > > Was this the reason for the misa_max cleanups? The misa_max cleanups are needed for "gdbstub: Simplify XML lookup", not for "target/riscv: Use GDBFeature for dynamic XML".
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index 50a9957279..e5c16df5ca 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -11,6 +11,7 @@ #ifndef QEMU_QEMU_PLUGIN_H #define QEMU_QEMU_PLUGIN_H +#include <glib.h> #include <inttypes.h> #include <stdbool.h> #include <stddef.h> @@ -218,8 +219,8 @@ struct qemu_plugin_insn; * @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 unused, plugins cannot read or change system - * register state. + * 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, @@ -664,4 +665,51 @@ uint64_t qemu_plugin_end_code(void); */ uint64_t qemu_plugin_entry_code(void); +/** struct qemu_plugin_register - Opaque handle for a translated instruction */ +struct qemu_plugin_register; + +/** + * typedef qemu_plugin_reg_descriptor - register descriptions + * + * @name: register name + * @handle: opaque handle for retrieving value with qemu_plugin_read_register + * @feature: optional feature descriptor, can be NULL + */ +typedef struct { + char name[32]; + struct qemu_plugin_register *handle; + const char *feature; +} qemu_plugin_reg_descriptor; + +/** + * qemu_plugin_find_registers() - return register list + * @vcpu_index: vcpu to query + * @reg_pattern: register name pattern + * + * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller + * frees. As the register set of a given vCPU is only available once + * the vCPU is initialised if you want to monitor registers from the + * start you should call this from a qemu_plugin_register_vcpu_init_cb() + * callback. + */ +GArray * qemu_plugin_find_registers(unsigned int vcpu_index, const char *reg_pattern); + +/** + * qemu_plugin_read_register() - read register + * + * @vcpu: vcpu index + * @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. + * + * Returns the size of the read register. The content of @buf is in target byte + * order. On failure returns -1 + */ +int qemu_plugin_read_register(unsigned int vcpu, + struct qemu_plugin_register *handle, + GByteArray *buf); + + #endif /* QEMU_QEMU_PLUGIN_H */ diff --git a/plugins/api.c b/plugins/api.c index ac39cdea0b..2644af5bb3 100644 --- a/plugins/api.c +++ b/plugins/api.c @@ -8,6 +8,7 @@ * * qemu_plugin_tb * qemu_plugin_insn + * qemu_plugin_register * * Which can then be passed back into the API to do additional things. * As such all the public functions in here are exported in @@ -35,10 +36,12 @@ */ #include "qemu/osdep.h" +#include "qemu/main-loop.h" #include "qemu/plugin.h" #include "qemu/log.h" #include "tcg/tcg.h" #include "exec/exec-all.h" +#include "exec/gdbstub.h" #include "exec/ram_addr.h" #include "disas/disas.h" #include "plugin.h" @@ -435,3 +438,102 @@ uint64_t qemu_plugin_entry_code(void) #endif return entry; } + +/* + * Register handles + * + * The plugin infrastructure keeps hold of these internal data + * structures which are presented to plugins as opaque handles. They + * are global to the system and therefor additions to the hash table + * must be protected by the @reg_handle_lock. + * + * In order to future proof for up-coming heterogeneous work we want + * different entries for each CPU type while sharing them in the + * common case of multiple cores of the same type. + */ + +static QemuMutex reg_handle_lock; + +struct qemu_plugin_register { + const char *name; + int gdb_reg_num; +}; + +static GHashTable *reg_handles; /* hash table of PluginReg */ + +/* Generate a stable key - would xxhash be overkill? */ +static gpointer cpu_plus_reg_to_key(CPUState *cs, int gdb_regnum) +{ + uintptr_t key = (uintptr_t) cs->cc; + key ^= gdb_regnum; + return GUINT_TO_POINTER(key); +} + +/* + * Create register handles. + * + * We need to create a handle for each register so the plugin + * infrastructure can call gdbstub to read a register. We also + * construct a result array with those handles and some ancillary data + * the plugin might find useful. + */ + +static GArray * create_register_handles(CPUState *cs, GArray *gdbstub_regs) { + GArray *find_data = g_array_new(true, true, sizeof(qemu_plugin_reg_descriptor)); + + WITH_QEMU_LOCK_GUARD(®_handle_lock) { + + if (!reg_handles) { + reg_handles = g_hash_table_new(g_direct_hash, g_direct_equal); + } + + for (int i=0; i < gdbstub_regs->len; i++) { + GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i); + gpointer key = cpu_plus_reg_to_key(cs, grd->gdb_reg); + struct qemu_plugin_register *val = g_hash_table_lookup(reg_handles, key); + + /* Doesn't exist, create one */ + if (!val) { + val = g_new0(struct qemu_plugin_register, 1); + val->gdb_reg_num = grd->gdb_reg; + val->name = grd->name; + + g_hash_table_insert(reg_handles, key, val); + } + + /* Create a record for the plugin */ + qemu_plugin_reg_descriptor desc = { + .handle = val, + .feature = g_intern_string(grd->feature_name) + }; + g_strlcpy(desc.name, val->name, sizeof(desc.name)); + g_array_append_val(find_data, desc); + } + } + + return find_data; +} + +GArray * qemu_plugin_find_registers(unsigned int vcpu, const char *reg_pattern) +{ + CPUState *cs = qemu_get_cpu(vcpu); + if (cs) { + g_autoptr(GArray) regs = gdb_find_registers(cs, reg_pattern); + return regs->len ? create_register_handles(cs, regs) : NULL; + } else { + return NULL; + } +} + +int qemu_plugin_read_register(unsigned int vcpu, struct qemu_plugin_register *reg, GByteArray *buf) +{ + CPUState *cs = qemu_get_cpu(vcpu); + /* assert with debugging on? */ + return gdb_read_register(cs, buf, reg->gdb_reg_num); +} + +static void __attribute__((__constructor__)) qemu_api_init(void) +{ + qemu_mutex_init(®_handle_lock); + +} diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols index 71f6c90549..86928f5f50 100644 --- a/plugins/qemu-plugins.symbols +++ b/plugins/qemu-plugins.symbols @@ -42,4 +42,6 @@ qemu_plugin_tb_vaddr; qemu_plugin_uninstall; qemu_plugin_vcpu_for_each; + qemu_plugin_find_registers; + qemu_plugin_read_register; };
We can only request a list of registers once the vCPU has been initialised so the user needs to use either call the find function on vCPU initialisation or during the translation phase. We don't expose the reg number to the plugin instead hiding it behind an opaque handle. This allows for a bit of future proofing should the internals need to be changed while also being hashed against the CPUClass so we can handle different register sets per-vCPU in hetrogenous situations. Having an internal state within the plugins also allows us to expand the interface in future (for example providing callbacks on register change if the translator can track changes). Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 Cc: Akihiko Odaki <akihiko.odaki@daynix.com> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- vAJB: The main difference to Akikio's version is hiding the gdb register detail from the plugin for the reasons described above. --- include/qemu/qemu-plugin.h | 52 +++++++++++++++++- plugins/api.c | 102 +++++++++++++++++++++++++++++++++++ plugins/qemu-plugins.symbols | 2 + 3 files changed, 154 insertions(+), 2 deletions(-)