|
| From: | Akihiko Odaki |
| Subject: | Re: [PATCH 24/29] plugins: add an API to read registers |
| Date: | Tue, 7 Nov 2023 15:56:26 +0900 |
| User-agent: | Mozilla Thunderbird |
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".
| [Prev in Thread] | Current Thread | [Next in Thread] |