On 2023/11/06 20:40, Alex Bennée wrote:
Akihiko Odaki <akihiko.od...@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.od...@daynix.com> writes:
On 2023/11/06 18:30, Alex Bennée wrote:
Akihiko Odaki <akihiko.od...@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.od...@daynix.com>
Based-on: <20231025093128.33116-18-akihiko.od...@daynix.com>
Signed-off-by: Alex Bennée <alex.ben...@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".
