* [PATCH v14 00/18] plugins: Allow to read registers
@ 2023-10-19 10:26 Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 01/18] gdbstub: Add num_regs member to GDBFeature Akihiko Odaki
` (18 more replies)
0 siblings, 19 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki
Based-on: <20231019101030.128431-1-akihiko.odaki@daynix.com>
("[PATCH v5 0/6] gdbstub and TCG plugin improvements")
I and other people in the University of Tokyo, where I research processor
design, found TCG plugins are very useful for processor design exploration.
The feature we find missing is the capability to read registers from
plugins. In this series, I propose to add such a capability by reusing
gdbstub code.
The reuse of gdbstub code ensures the long-term stability of the TCG plugin
interface for register access without incurring a burden to maintain yet
another interface for register access.
This process to add TCG plugin involves four major changes. The first one
is to add GDBFeature structure that represents a GDB feature, which usually
includes registers. GDBFeature can be generated from static XML files or
dynamically generated by architecture-specific code. In fact, this is a
refactoring independent of the feature this series adds, and potentially
it's benefitial even without the plugin feature. The plugin feature will
utilize this new structure to describe registers exposed to plugins.
The second one is to make gdb_read_register/gdb_write_register usable
outside of gdbstub context.
The third one is to actually make registers readable for plugins.
The execlog plugin will have new options to demonstrate the new feature.
I also have a plugin that uses this new feature to generate execution
traces for Sniper processor simulator, which is available at:
https://github.com/shioya-lab/sniper/tree/akihikodaki/bb
The below is a summary of patches:
Patch [01, 12] introduces num_regs member to GDBFeature.
Patch 13 adds members useful to identify registers to GDBFeature.
Patch 14 makes registers readable outside of gdbstub context.
Patch [15, 18] add the feature to read registers from plugins.
V13 -> V14:
Fixed RISC-V CSR register numbers.
V12 -> V13:
Rebased to "[PATCH v4 0/5] gdbstub and TCG plugin improvements".
V11 -> V12:
Rebased to "[PATCH v2 0/3] gdbstub and TCG plugin improvements".
V10 -> V11:
Fixed QEMU_PLUGIN_CB_RW_REGS.
V9 -> V10:
Rebased to "[PATCH 0/4] gdbstub and TCG plugin improvements".
Dropped patch "plugins: Support C++".
Removed the unnecessary QEMU_PLUGIN_VERSION change.
V8 -> V9:
Rebased to "[PATCH 00/25] October maintainer omnibus pre-PR (tests,
gdbstub, plugins)".
Added patch "target/riscv: Move MISA limits to class".
Added patch "target/riscv: Remove misa_mxl validation".
Added patch "target/riscv: Validate misa_mxl_max only once".
Added patch "plugins: Use different helpers when reading".
Moved contrib/plugins/cc.cc to tests/plugin/cc.cc.
V7 -> V8:
Rebased to "[PATCH v3 00/12] gdbstub and TCG plugin improvements".
Clarified that initialization and exit hooks affect TCG state.
Simplified by adding the core feature to gdb_regs.
V6 -> V7:
Rebased to "[PATCH v2 00/11] gdbstub and TCG plugin improvements".
Replaced functions to get register identifiers.
V5 -> V6:
Rebased to "[PATCH 0/8] gdbstub and TCG plugin improvements".
V4 -> V5:
Corrected g_rw_lock_writer_lock() call. (Richard Henderson)
Replaced abort() with g_assert_not_reached(). (Richard Henderson)
Fixed CSR name leak in target/riscv. (Richard Henderson)
Removed gdb_has_xml variable.
V3 -> V4:
Added execlog changes I forgot to include in the last version.
V2 -> V3:
Added patch "hw/core/cpu: Return static value with gdb_arch_name()".
Added patch "gdbstub: Dynamically allocate target.xml buffer".
(Alex Bennée)
Added patch "gdbstub: Introduce GDBFeatureBuilder". (Alex Bennée)
Dropped Reviewed-by tags for "target/*: Use GDBFeature for dynamic XML".
Changed gdb_find_static_feature() to abort on failure. (Alex Bennée)
Changed the execlog plugin to log the register value only when changed.
(Alex Bennée)
Dropped 0x prefixes for register value logs for conciseness.
V1 -> V2:
Added SPDX-License-Identifier: GPL-2.0-or-later. (Philippe Mathieu-Daudé)
Split long lines. (Philippe Mathieu-Daudé)
Renamed gdb_features to gdb_static_features (Philippe Mathieu-Daudé)
Dropped RFC.
Akihiko Odaki (18):
gdbstub: Add num_regs member to GDBFeature
gdbstub: Introduce gdb_find_static_feature()
gdbstub: Introduce GDBFeatureBuilder
target/arm: Use GDBFeature for dynamic XML
target/ppc: Use GDBFeature for dynamic XML
target/riscv: Use GDBFeature for dynamic XML
gdbstub: Use GDBFeature for gdb_register_coprocessor
gdbstub: Use GDBFeature for GDBRegisterState
gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb
gdbstub: Simplify XML lookup
gdbstub: Infer number of core registers from XML
hw/core/cpu: Remove gdb_get_dynamic_xml member
gdbstub: Add members to identify registers to GDBFeature
gdbstub: Expose functions to read registers
cpu: Call plugin hooks only when ready
plugins: Use different helpers when reading registers
plugins: Allow to read registers
contrib/plugins: Allow to log registers
docs/devel/tcg-plugins.rst | 10 +-
accel/tcg/plugin-helpers.h | 3 +-
include/exec/gdbstub.h | 42 +++++-
include/exec/plugin-gen.h | 4 +-
include/hw/core/cpu.h | 11 +-
include/qemu/plugin.h | 3 +
include/qemu/qemu-plugin.h | 50 ++++++-
plugins/plugin.h | 5 +-
target/arm/cpu.h | 27 ++--
target/arm/internals.h | 14 +-
target/hexagon/internal.h | 4 +-
target/microblaze/cpu.h | 4 +-
target/ppc/cpu-qom.h | 4 +-
target/ppc/cpu.h | 2 -
target/riscv/cpu.h | 5 +-
target/s390x/cpu.h | 2 -
accel/tcg/plugin-gen.c | 41 ++++--
accel/tcg/translator.c | 2 +-
contrib/plugins/execlog.c | 120 ++++++++++++-----
cpu-target.c | 11 --
gdbstub/gdbstub.c | 246 ++++++++++++++++++++++++++---------
hw/core/cpu-common.c | 15 ++-
plugins/api.c | 34 ++++-
plugins/core.c | 28 ++--
target/arm/cpu.c | 2 -
target/arm/cpu64.c | 1 -
target/arm/gdbstub.c | 230 ++++++++++++++++----------------
target/arm/gdbstub64.c | 122 ++++++++---------
target/avr/cpu.c | 1 -
target/hexagon/cpu.c | 4 +-
target/hexagon/gdbstub.c | 10 +-
target/i386/cpu.c | 2 -
target/loongarch/cpu.c | 2 -
target/loongarch/gdbstub.c | 13 +-
target/m68k/cpu.c | 1 -
target/m68k/helper.c | 26 +++-
target/microblaze/cpu.c | 6 +-
target/microblaze/gdbstub.c | 9 +-
target/ppc/cpu_init.c | 7 -
target/ppc/gdbstub.c | 114 ++++++++--------
target/riscv/cpu.c | 15 ---
target/riscv/gdbstub.c | 139 +++++++++++---------
target/rx/cpu.c | 1 -
target/s390x/cpu.c | 1 -
target/s390x/gdbstub.c | 105 +++++++++------
plugins/qemu-plugins.symbols | 3 +
scripts/feature_to_c.py | 58 ++++++++-
47 files changed, 983 insertions(+), 576 deletions(-)
--
2.42.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v14 01/18] gdbstub: Add num_regs member to GDBFeature
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 02/18] gdbstub: Introduce gdb_find_static_feature() Akihiko Odaki
` (17 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, John Snow,
Cleber Rosa
Currently the number of registers exposed to GDB is written as magic
numbers in code. Derive the number of registers GDB actually see from
XML files to replace the magic numbers in code later.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
include/exec/gdbstub.h | 1 +
scripts/feature_to_c.py | 46 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 1a01c35f8e..a43aa34dad 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -13,6 +13,7 @@
typedef struct GDBFeature {
const char *xmlname;
const char *xml;
+ int num_regs;
} GDBFeature;
diff --git a/scripts/feature_to_c.py b/scripts/feature_to_c.py
index bcbcb83beb..e04d6b2df7 100755
--- a/scripts/feature_to_c.py
+++ b/scripts/feature_to_c.py
@@ -1,7 +1,7 @@
#!/usr/bin/env python3
# SPDX-License-Identifier: GPL-2.0-or-later
-import os, sys
+import os, sys, xml.etree.ElementTree
def writeliteral(indent, bytes):
sys.stdout.write(' ' * indent)
@@ -39,10 +39,52 @@ def writeliteral(indent, bytes):
with open(input, 'rb') as file:
read = file.read()
+ parser = xml.etree.ElementTree.XMLPullParser(['start', 'end'])
+ parser.feed(read)
+ events = parser.read_events()
+ event, element = next(events)
+ if event != 'start':
+ sys.stderr.write(f'unexpected event: {event}\n')
+ exit(1)
+ if element.tag != 'feature':
+ sys.stderr.write(f'unexpected start tag: {element.tag}\n')
+ exit(1)
+
+ regnum = 0
+ regnums = []
+ tags = ['feature']
+ for event, element in events:
+ if event == 'end':
+ if element.tag != tags[len(tags) - 1]:
+ sys.stderr.write(f'unexpected end tag: {element.tag}\n')
+ exit(1)
+
+ tags.pop()
+ if element.tag == 'feature':
+ break
+ elif event == 'start':
+ if len(tags) < 2 and element.tag == 'reg':
+ if 'regnum' in element.attrib:
+ regnum = int(element.attrib['regnum'])
+
+ regnums.append(regnum)
+ regnum += 1
+
+ tags.append(element.tag)
+ else:
+ raise Exception(f'unexpected event: {event}\n')
+
+ if len(tags):
+ sys.stderr.write('unterminated feature tag\n')
+ exit(1)
+
+ base_reg = min(regnums)
+ num_regs = max(regnums) - base_reg + 1 if len(regnums) else 0
+
sys.stdout.write(' {\n')
writeliteral(8, bytes(os.path.basename(input), 'utf-8'))
sys.stdout.write(',\n')
writeliteral(8, read)
- sys.stdout.write('\n },\n')
+ sys.stdout.write(f',\n {num_regs},\n }},\n')
sys.stdout.write(' { NULL }\n};\n')
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 02/18] gdbstub: Introduce gdb_find_static_feature()
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 01/18] gdbstub: Add num_regs member to GDBFeature Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 03/18] gdbstub: Introduce GDBFeatureBuilder Akihiko Odaki
` (16 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Richard Henderson
This function is useful to determine the number of registers exposed to
GDB from the XML name.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/gdbstub.h | 2 ++
gdbstub/gdbstub.c | 13 +++++++++++++
2 files changed, 15 insertions(+)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index a43aa34dad..071021415a 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -44,6 +44,8 @@ void gdb_register_coprocessor(CPUState *cpu,
*/
int gdbserver_start(const char *port_or_device);
+const GDBFeature *gdb_find_static_feature(const char *xmlname);
+
void gdb_set_stop_cpu(CPUState *cpu);
/* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 29540a0284..ae24c4848f 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -422,6 +422,19 @@ static const char *get_feature_xml(const char *p, const char **newp,
return NULL;
}
+const GDBFeature *gdb_find_static_feature(const char *xmlname)
+{
+ const GDBFeature *feature;
+
+ for (feature = gdb_static_features; feature->xmlname; feature++) {
+ if (!strcmp(feature->xmlname, xmlname)) {
+ return feature;
+ }
+ }
+
+ g_assert_not_reached();
+}
+
static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 03/18] gdbstub: Introduce GDBFeatureBuilder
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 01/18] gdbstub: Add num_regs member to GDBFeature Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 02/18] gdbstub: Introduce gdb_find_static_feature() Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 04/18] target/arm: Use GDBFeature for dynamic XML Akihiko Odaki
` (15 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Richard Henderson
GDBFeatureBuilder unifies the logic to generate dynamic GDBFeature.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/gdbstub.h | 23 +++++++++++++++
gdbstub/gdbstub.c | 65 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 071021415a..97875634f3 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -16,6 +16,12 @@ typedef struct GDBFeature {
int num_regs;
} GDBFeature;
+typedef struct GDBFeatureBuilder {
+ GDBFeature *feature;
+ GPtrArray *xml;
+ int base_reg;
+} GDBFeatureBuilder;
+
/* Get or set a register. Returns the size of the register. */
typedef int (*gdb_get_reg_cb)(CPUArchState *env, GByteArray *buf, int reg);
@@ -44,6 +50,23 @@ void gdb_register_coprocessor(CPUState *cpu,
*/
int gdbserver_start(const char *port_or_device);
+void gdb_feature_builder_init(GDBFeatureBuilder *builder, GDBFeature *feature,
+ const char *name, const char *xmlname,
+ int base_reg);
+
+void G_GNUC_PRINTF(2, 3)
+gdb_feature_builder_append_tag(const GDBFeatureBuilder *builder,
+ const char *format, ...);
+
+void gdb_feature_builder_append_reg(const GDBFeatureBuilder *builder,
+ const char *name,
+ int bitsize,
+ int regnum,
+ const char *type,
+ const char *group);
+
+void gdb_feature_builder_end(const GDBFeatureBuilder *builder);
+
const GDBFeature *gdb_find_static_feature(const char *xmlname);
void gdb_set_stop_cpu(CPUState *cpu);
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index ae24c4848f..ebb912da1b 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -422,6 +422,71 @@ static const char *get_feature_xml(const char *p, const char **newp,
return NULL;
}
+void gdb_feature_builder_init(GDBFeatureBuilder *builder, GDBFeature *feature,
+ const char *name, const char *xmlname,
+ int base_reg)
+{
+ char *header = g_markup_printf_escaped(
+ "<?xml version=\"1.0\"?>"
+ "<!DOCTYPE feature SYSTEM \"gdb-target.dtd\">"
+ "<feature name=\"%s\">",
+ name);
+
+ builder->feature = feature;
+ builder->xml = g_ptr_array_new();
+ g_ptr_array_add(builder->xml, header);
+ builder->base_reg = base_reg;
+ feature->xmlname = xmlname;
+ feature->num_regs = 0;
+}
+
+void gdb_feature_builder_append_tag(const GDBFeatureBuilder *builder,
+ const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ g_ptr_array_add(builder->xml, g_markup_vprintf_escaped(format, ap));
+ va_end(ap);
+}
+
+void gdb_feature_builder_append_reg(const GDBFeatureBuilder *builder,
+ const char *name,
+ int bitsize,
+ int regnum,
+ const char *type,
+ const char *group)
+{
+ if (builder->feature->num_regs < regnum) {
+ builder->feature->num_regs = regnum;
+ }
+
+ if (group) {
+ gdb_feature_builder_append_tag(
+ builder,
+ "<reg name=\"%s\" bitsize=\"%d\" regnum=\"%d\" type=\"%s\" group=\"%s\"/>",
+ name, bitsize, builder->base_reg + regnum, type, group);
+ } else {
+ gdb_feature_builder_append_tag(
+ builder,
+ "<reg name=\"%s\" bitsize=\"%d\" regnum=\"%d\" type=\"%s\"/>",
+ name, bitsize, builder->base_reg + regnum, type);
+ }
+}
+
+void gdb_feature_builder_end(const GDBFeatureBuilder *builder)
+{
+ g_ptr_array_add(builder->xml, (void *)"</feature>");
+ g_ptr_array_add(builder->xml, NULL);
+
+ builder->feature->xml = g_strjoinv(NULL, (void *)builder->xml->pdata);
+
+ for (guint i = 0; i < builder->xml->len - 2; i++) {
+ g_free(g_ptr_array_index(builder->xml, i));
+ }
+
+ g_ptr_array_free(builder->xml, TRUE);
+}
+
const GDBFeature *gdb_find_static_feature(const char *xmlname)
{
const GDBFeature *feature;
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 04/18] target/arm: Use GDBFeature for dynamic XML
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (2 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 03/18] gdbstub: Introduce GDBFeatureBuilder Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 05/18] target/ppc: " Akihiko Odaki
` (14 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Richard Henderson,
Peter Maydell, qemu-arm
In preparation for a change to use GDBFeature as a parameter of
gdb_register_coprocessor(), convert the internal representation of
dynamic feature from plain XML to GDBFeature.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 21 +++---
target/arm/internals.h | 2 +-
target/arm/gdbstub.c | 142 ++++++++++++++++++++---------------------
target/arm/gdbstub64.c | 95 +++++++++++++--------------
4 files changed, 123 insertions(+), 137 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a9edfb8353..3702ddaab8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -25,6 +25,7 @@
#include "hw/registerfields.h"
#include "cpu-qom.h"
#include "exec/cpu-defs.h"
+#include "exec/gdbstub.h"
#include "qapi/qapi-types-common.h"
/* ARM processors have a weak memory model */
@@ -136,23 +137,21 @@ enum {
*/
/**
- * DynamicGDBXMLInfo:
- * @desc: Contains the XML descriptions.
- * @num: Number of the registers in this XML seen by GDB.
+ * DynamicGDBFeatureInfo:
+ * @desc: Contains the feature descriptions.
* @data: A union with data specific to the set of registers
* @cpregs_keys: Array that contains the corresponding Key of
* a given cpreg with the same order of the cpreg
* in the XML description.
*/
-typedef struct DynamicGDBXMLInfo {
- char *desc;
- int num;
+typedef struct DynamicGDBFeatureInfo {
+ GDBFeature desc;
union {
struct {
uint32_t *keys;
} cpregs;
} data;
-} DynamicGDBXMLInfo;
+} DynamicGDBFeatureInfo;
/* CPU state for each instance of a generic timer (in cp15 c14) */
typedef struct ARMGenericTimer {
@@ -880,10 +879,10 @@ struct ArchCPU {
uint64_t *cpreg_vmstate_values;
int32_t cpreg_vmstate_array_len;
- DynamicGDBXMLInfo dyn_sysreg_xml;
- DynamicGDBXMLInfo dyn_svereg_xml;
- DynamicGDBXMLInfo dyn_m_systemreg_xml;
- DynamicGDBXMLInfo dyn_m_secextreg_xml;
+ DynamicGDBFeatureInfo dyn_sysreg_feature;
+ DynamicGDBFeatureInfo dyn_svereg_feature;
+ DynamicGDBFeatureInfo dyn_m_systemreg_feature;
+ DynamicGDBFeatureInfo dyn_m_secextreg_feature;
/* Timers used by the generic (architected) timer */
QEMUTimer *gt_timer[NUM_GTIMERS];
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 1dd9182a54..99136985ad 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1440,7 +1440,7 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
}
#ifdef TARGET_AARCH64
-int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
+GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg);
int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg);
int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg);
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index b7ace24bfc..0b21ee42e0 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -25,11 +25,11 @@
#include "internals.h"
#include "cpregs.h"
-typedef struct RegisterSysregXmlParam {
+typedef struct RegisterSysregFeatureParam {
CPUState *cs;
- GString *s;
+ GDBFeatureBuilder builder;
int n;
-} RegisterSysregXmlParam;
+} RegisterSysregFeatureParam;
/* Old gdb always expect FPA registers. Newer (xml-aware) gdb only expect
whatever the target description contains. Due to a historical mishap
@@ -215,7 +215,7 @@ static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
const ARMCPRegInfo *ri;
uint32_t key;
- key = cpu->dyn_sysreg_xml.data.cpregs.keys[reg];
+ key = cpu->dyn_sysreg_feature.data.cpregs.keys[reg];
ri = get_arm_cp_reginfo(cpu->cp_regs, key);
if (ri) {
if (cpreg_field_is_64bit(ri)) {
@@ -232,34 +232,32 @@ static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-static void arm_gen_one_xml_sysreg_tag(GString *s, DynamicGDBXMLInfo *dyn_xml,
+static void arm_gen_one_feature_sysreg(GDBFeatureBuilder *builder,
+ DynamicGDBFeatureInfo *dyn_feature,
ARMCPRegInfo *ri, uint32_t ri_key,
- int bitsize, int regnum)
+ int bitsize, int n)
{
- g_string_append_printf(s, "<reg name=\"%s\"", ri->name);
- g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
- g_string_append_printf(s, " regnum=\"%d\"", regnum);
- g_string_append_printf(s, " group=\"cp_regs\"/>");
- dyn_xml->data.cpregs.keys[dyn_xml->num] = ri_key;
- dyn_xml->num++;
+ gdb_feature_builder_append_reg(builder, ri->name, bitsize, n,
+ "int", "cp_regs");
+
+ dyn_feature->data.cpregs.keys[n] = ri_key;
}
-static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
- gpointer p)
+static void arm_register_sysreg_for_feature(gpointer key, gpointer value,
+ gpointer p)
{
uint32_t ri_key = (uintptr_t)key;
ARMCPRegInfo *ri = value;
- RegisterSysregXmlParam *param = (RegisterSysregXmlParam *)p;
- GString *s = param->s;
+ RegisterSysregFeatureParam *param = p;
ARMCPU *cpu = ARM_CPU(param->cs);
CPUARMState *env = &cpu->env;
- DynamicGDBXMLInfo *dyn_xml = &cpu->dyn_sysreg_xml;
+ DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature;
if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
if (arm_feature(env, ARM_FEATURE_AARCH64)) {
if (ri->state == ARM_CP_STATE_AA64) {
- arm_gen_one_xml_sysreg_tag(s , dyn_xml, ri, ri_key, 64,
- param->n++);
+ arm_gen_one_feature_sysreg(¶m->builder, dyn_feature,
+ ri, ri_key, 64, param->n++);
}
} else {
if (ri->state == ARM_CP_STATE_AA32) {
@@ -268,32 +266,32 @@ static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
return;
}
if (ri->type & ARM_CP_64BIT) {
- arm_gen_one_xml_sysreg_tag(s , dyn_xml, ri, ri_key, 64,
- param->n++);
+ arm_gen_one_feature_sysreg(¶m->builder, dyn_feature,
+ ri, ri_key, 64, param->n++);
} else {
- arm_gen_one_xml_sysreg_tag(s , dyn_xml, ri, ri_key, 32,
- param->n++);
+ arm_gen_one_feature_sysreg(¶m->builder, dyn_feature,
+ ri, ri_key, 32, param->n++);
}
}
}
}
}
-static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
+static GDBFeature *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg)
{
ARMCPU *cpu = ARM_CPU(cs);
- GString *s = g_string_new(NULL);
- RegisterSysregXmlParam param = {cs, s, base_reg};
-
- cpu->dyn_sysreg_xml.num = 0;
- cpu->dyn_sysreg_xml.data.cpregs.keys = g_new(uint32_t, g_hash_table_size(cpu->cp_regs));
- g_string_printf(s, "<?xml version=\"1.0\"?>");
- g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
- g_string_append_printf(s, "<feature name=\"org.qemu.gdb.arm.sys.regs\">");
- g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, ¶m);
- g_string_append_printf(s, "</feature>");
- cpu->dyn_sysreg_xml.desc = g_string_free(s, false);
- return cpu->dyn_sysreg_xml.num;
+ RegisterSysregFeatureParam param = {cs};
+ gsize num_regs = g_hash_table_size(cpu->cp_regs);
+
+ gdb_feature_builder_init(¶m.builder,
+ &cpu->dyn_sysreg_feature.desc,
+ "org.qemu.gdb.arm.sys.regs",
+ "system-registers.xml",
+ base_reg);
+ cpu->dyn_sysreg_feature.data.cpregs.keys = g_new(uint32_t, num_regs);
+ g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_feature, ¶m);
+ gdb_feature_builder_end(¶m.builder);
+ return &cpu->dyn_sysreg_feature.desc;
}
#ifdef CONFIG_TCG
@@ -385,31 +383,29 @@ static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg)
return 0; /* TODO */
}
-static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int orig_base_reg)
+static GDBFeature *arm_gen_dynamic_m_systemreg_feature(CPUState *cs,
+ int base_reg)
{
ARMCPU *cpu = ARM_CPU(cs);
CPUARMState *env = &cpu->env;
- GString *s = g_string_new(NULL);
- int base_reg = orig_base_reg;
+ GDBFeatureBuilder builder;
+ int reg = 0;
int i;
- g_string_printf(s, "<?xml version=\"1.0\"?>");
- g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
- g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.m-system\">\n");
+ gdb_feature_builder_init(&builder, &cpu->dyn_m_systemreg_feature.desc,
+ "org.gnu.gdb.arm.m-system", "arm-m-system.xml",
+ base_reg);
for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) {
if (arm_feature(env, m_sysreg_def[i].feature)) {
- g_string_append_printf(s,
- "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n",
- m_sysreg_def[i].name, base_reg++);
+ gdb_feature_builder_append_reg(&builder, m_sysreg_def[i].name, 32,
+ reg++, "int", NULL);
}
}
- g_string_append_printf(s, "</feature>");
- cpu->dyn_m_systemreg_xml.desc = g_string_free(s, false);
- cpu->dyn_m_systemreg_xml.num = base_reg - orig_base_reg;
+ gdb_feature_builder_end(&builder);
- return cpu->dyn_m_systemreg_xml.num;
+ return &cpu->dyn_m_systemreg_feature.desc;
}
#ifndef CONFIG_USER_ONLY
@@ -427,31 +423,31 @@ static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg)
return 0; /* TODO */
}
-static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg)
+static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
+ int base_reg)
{
ARMCPU *cpu = ARM_CPU(cs);
- GString *s = g_string_new(NULL);
- int base_reg = orig_base_reg;
+ GDBFeatureBuilder builder;
+ char *name;
+ int reg = 0;
int i;
- g_string_printf(s, "<?xml version=\"1.0\"?>");
- g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
- g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.secext\">\n");
+ gdb_feature_builder_init(&builder, &cpu->dyn_m_secextreg_feature.desc,
+ "org.gnu.gdb.arm.secext", "arm-m-secext.xml",
+ base_reg);
for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) {
- g_string_append_printf(s,
- "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n",
- m_sysreg_def[i].name, base_reg++);
- g_string_append_printf(s,
- "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n",
- m_sysreg_def[i].name, base_reg++);
+ name = g_strconcat(m_sysreg_def[i].name, "_ns", NULL);
+ gdb_feature_builder_append_reg(&builder, name, 32, reg++,
+ "int", NULL);
+ name = g_strconcat(m_sysreg_def[i].name, "_s", NULL);
+ gdb_feature_builder_append_reg(&builder, name, 32, reg++,
+ "int", NULL);
}
- g_string_append_printf(s, "</feature>");
- cpu->dyn_m_secextreg_xml.desc = g_string_free(s, false);
- cpu->dyn_m_secextreg_xml.num = base_reg - orig_base_reg;
+ gdb_feature_builder_end(&builder);
- return cpu->dyn_m_secextreg_xml.num;
+ return &cpu->dyn_m_secextreg_feature.desc;
}
#endif
#endif /* CONFIG_TCG */
@@ -461,14 +457,14 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
ARMCPU *cpu = ARM_CPU(cs);
if (strcmp(xmlname, "system-registers.xml") == 0) {
- return cpu->dyn_sysreg_xml.desc;
+ return cpu->dyn_sysreg_feature.desc.xml;
} else if (strcmp(xmlname, "sve-registers.xml") == 0) {
- return cpu->dyn_svereg_xml.desc;
+ return cpu->dyn_svereg_feature.desc.xml;
} else if (strcmp(xmlname, "arm-m-system.xml") == 0) {
- return cpu->dyn_m_systemreg_xml.desc;
+ return cpu->dyn_m_systemreg_feature.desc.xml;
#ifndef CONFIG_USER_ONLY
} else if (strcmp(xmlname, "arm-m-secext.xml") == 0) {
- return cpu->dyn_m_secextreg_xml.desc;
+ return cpu->dyn_m_secextreg_feature.desc.xml;
#endif
}
return NULL;
@@ -486,7 +482,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
*/
#ifdef TARGET_AARCH64
if (isar_feature_aa64_sve(&cpu->isar)) {
- int nreg = arm_gen_dynamic_svereg_xml(cs, cs->gdb_num_regs);
+ int nreg = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs)->num_regs;
gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
aarch64_gdb_set_sve_reg, nreg,
"sve-registers.xml", 0);
@@ -532,20 +528,20 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
1, "arm-m-profile-mve.xml", 0);
}
gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
- arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs),
+ arm_gen_dynamic_sysreg_feature(cs, cs->gdb_num_regs)->num_regs,
"system-registers.xml", 0);
#ifdef CONFIG_TCG
if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) {
gdb_register_coprocessor(cs,
arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
- arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs),
+ arm_gen_dynamic_m_systemreg_feature(cs, cs->gdb_num_regs)->num_regs,
"arm-m-system.xml", 0);
#ifndef CONFIG_USER_ONLY
if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
gdb_register_coprocessor(cs,
arm_gdb_get_m_secextreg, arm_gdb_set_m_secextreg,
- arm_gen_dynamic_m_secextreg_xml(cs, cs->gdb_num_regs),
+ arm_gen_dynamic_m_secextreg_feature(cs, cs->gdb_num_regs)->num_regs,
"arm-m-secext.xml", 0);
}
#endif
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index d7b79a6589..5286d5c604 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -247,7 +247,7 @@ int aarch64_gdb_set_pauth_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-static void output_vector_union_type(GString *s, int reg_width,
+static void output_vector_union_type(GDBFeatureBuilder *builder, int reg_width,
const char *name)
{
struct TypeSize {
@@ -282,10 +282,10 @@ static void output_vector_union_type(GString *s, int reg_width,
/* First define types and totals in a whole VL */
for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
- g_string_append_printf(s,
- "<vector id=\"%s%c%c\" type=\"%s\" count=\"%d\"/>",
- name, vec_lanes[i].sz, vec_lanes[i].suffix,
- vec_lanes[i].gdb_type, reg_width / vec_lanes[i].size);
+ gdb_feature_builder_append_tag(
+ builder, "<vector id=\"%s%c%c\" type=\"%s\" count=\"%d\"/>",
+ name, vec_lanes[i].sz, vec_lanes[i].suffix,
+ vec_lanes[i].gdb_type, reg_width / vec_lanes[i].size);
}
/*
@@ -296,86 +296,77 @@ static void output_vector_union_type(GString *s, int reg_width,
for (i = 0; i < ARRAY_SIZE(suf); i++) {
int bits = 8 << i;
- g_string_append_printf(s, "<union id=\"%sn%c\">", name, suf[i]);
+ gdb_feature_builder_append_tag(builder, "<union id=\"%sn%c\">",
+ name, suf[i]);
for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
if (vec_lanes[j].size == bits) {
- g_string_append_printf(s, "<field name=\"%c\" type=\"%s%c%c\"/>",
- vec_lanes[j].suffix, name,
- vec_lanes[j].sz, vec_lanes[j].suffix);
+ gdb_feature_builder_append_tag(
+ builder, "<field name=\"%c\" type=\"%s%c%c\"/>",
+ vec_lanes[j].suffix, name,
+ vec_lanes[j].sz, vec_lanes[j].suffix);
}
}
- g_string_append(s, "</union>");
+ gdb_feature_builder_append_tag(builder, "</union>");
}
/* And now the final union of unions */
- g_string_append_printf(s, "<union id=\"%s\">", name);
+ gdb_feature_builder_append_tag(builder, "<union id=\"%s\">", name);
for (i = ARRAY_SIZE(suf) - 1; i >= 0; i--) {
- g_string_append_printf(s, "<field name=\"%c\" type=\"%sn%c\"/>",
- suf[i], name, suf[i]);
+ gdb_feature_builder_append_tag(builder,
+ "<field name=\"%c\" type=\"%sn%c\"/>",
+ suf[i], name, suf[i]);
}
- g_string_append(s, "</union>");
+ gdb_feature_builder_append_tag(builder, "</union>");
}
-int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
+GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
{
ARMCPU *cpu = ARM_CPU(cs);
- GString *s = g_string_new(NULL);
- DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
int reg_width = cpu->sve_max_vq * 128;
int pred_width = cpu->sve_max_vq * 16;
- int base_reg = orig_base_reg;
+ GDBFeatureBuilder builder;
+ char *name;
+ int reg = 0;
int i;
- g_string_printf(s, "<?xml version=\"1.0\"?>");
- g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
- g_string_append_printf(s, "<feature name=\"org.gnu.gdb.aarch64.sve\">");
+ gdb_feature_builder_init(&builder, &cpu->dyn_svereg_feature.desc,
+ "org.gnu.gdb.aarch64.sve", "sve-registers.xml",
+ base_reg);
/* Create the vector union type. */
- output_vector_union_type(s, reg_width, "svev");
+ output_vector_union_type(&builder, reg_width, "svev");
/* Create the predicate vector type. */
- g_string_append_printf(s,
- "<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
- pred_width / 8);
+ gdb_feature_builder_append_tag(
+ &builder, "<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
+ pred_width / 8);
/* Define the vector registers. */
for (i = 0; i < 32; i++) {
- g_string_append_printf(s,
- "<reg name=\"z%d\" bitsize=\"%d\""
- " regnum=\"%d\" type=\"svev\"/>",
- i, reg_width, base_reg++);
+ name = g_strdup_printf("z%d", i);
+ gdb_feature_builder_append_reg(&builder, name, reg_width, reg++,
+ "svev", NULL);
}
/* fpscr & status registers */
- g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\""
- " regnum=\"%d\" group=\"float\""
- " type=\"int\"/>", base_reg++);
- g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\""
- " regnum=\"%d\" group=\"float\""
- " type=\"int\"/>", base_reg++);
+ gdb_feature_builder_append_reg(&builder, "fpsr", 32, reg++,
+ "int", "float");
+ gdb_feature_builder_append_reg(&builder, "fpcr", 32, reg++,
+ "int", "float");
/* Define the predicate registers. */
for (i = 0; i < 16; i++) {
- g_string_append_printf(s,
- "<reg name=\"p%d\" bitsize=\"%d\""
- " regnum=\"%d\" type=\"svep\"/>",
- i, pred_width, base_reg++);
+ name = g_strdup_printf("p%d", i);
+ gdb_feature_builder_append_reg(&builder, name, pred_width, reg++,
+ "svep", NULL);
}
- g_string_append_printf(s,
- "<reg name=\"ffr\" bitsize=\"%d\""
- " regnum=\"%d\" group=\"vector\""
- " type=\"svep\"/>",
- pred_width, base_reg++);
+ gdb_feature_builder_append_reg(&builder, "ffr", pred_width, reg++,
+ "svep", "vector");
/* Define the vector length pseudo-register. */
- g_string_append_printf(s,
- "<reg name=\"vg\" bitsize=\"64\""
- " regnum=\"%d\" type=\"int\"/>",
- base_reg++);
+ gdb_feature_builder_append_reg(&builder, "vg", 64, reg++, "int", NULL);
- g_string_append_printf(s, "</feature>");
+ gdb_feature_builder_end(&builder);
- info->desc = g_string_free(s, false);
- info->num = base_reg - orig_base_reg;
- return info->num;
+ return &cpu->dyn_svereg_feature.desc;
}
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 05/18] target/ppc: Use GDBFeature for dynamic XML
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (3 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 04/18] target/arm: Use GDBFeature for dynamic XML Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 06/18] target/riscv: " Akihiko Odaki
` (13 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Richard Henderson,
Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
qemu-ppc
In preparation for a change to use GDBFeature as a parameter of
gdb_register_coprocessor(), convert the internal representation of
dynamic feature from plain XML to GDBFeature.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
target/ppc/cpu-qom.h | 4 ++--
target/ppc/cpu.h | 1 -
target/ppc/cpu_init.c | 4 ----
target/ppc/gdbstub.c | 51 ++++++++++++++++---------------------------
4 files changed, 21 insertions(+), 39 deletions(-)
diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
index be33786bd8..8d5ebba5d3 100644
--- a/target/ppc/cpu-qom.h
+++ b/target/ppc/cpu-qom.h
@@ -20,6 +20,7 @@
#ifndef QEMU_PPC_CPU_QOM_H
#define QEMU_PPC_CPU_QOM_H
+#include "exec/gdbstub.h"
#include "hw/core/cpu.h"
#include "qom/object.h"
@@ -186,8 +187,7 @@ struct PowerPCCPUClass {
int bfd_mach;
uint32_t l1_dcache_size, l1_icache_size;
#ifndef CONFIG_USER_ONLY
- unsigned int gdb_num_sprs;
- const char *gdb_spr_xml;
+ GDBFeature gdb_spr;
#endif
const PPCHash64Options *hash64_opts;
struct ppc_radix_page_info *radix_page_info;
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 30392ebeee..848062bc9f 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1384,7 +1384,6 @@ int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
#ifndef CONFIG_USER_ONLY
hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
-void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu);
const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
#endif
int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 40fe14a6c2..a0178c3ce8 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6682,10 +6682,6 @@ static void init_ppc_proc(PowerPCCPU *cpu)
/* PowerPC implementation specific initialisations (SPRs, timers, ...) */
(*pcc->init_proc)(env);
-#if !defined(CONFIG_USER_ONLY)
- ppc_gdb_gen_spr_xml(cpu);
-#endif
-
/* MSR bits & flags consistency checks */
if (env->msr_mask & (1 << 25)) {
switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index ec5731e5d6..e3be3dbd10 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -300,15 +300,23 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
}
#ifndef CONFIG_USER_ONLY
-void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu)
+static void gdb_gen_spr_feature(CPUState *cs)
{
- PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+ PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
CPUPPCState *env = &cpu->env;
- GString *xml;
- char *spr_name;
+ GDBFeatureBuilder builder;
unsigned int num_regs = 0;
int i;
+ if (pcc->gdb_spr.xml) {
+ return;
+ }
+
+ gdb_feature_builder_init(&builder, &pcc->gdb_spr,
+ "org.qemu.power.spr", "power-spr.xml",
+ cs->gdb_num_regs);
+
for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
ppc_spr_t *spr = &env->spr_cb[i];
@@ -326,35 +334,13 @@ void ppc_gdb_gen_spr_xml(PowerPCCPU *cpu)
*/
spr->gdb_id = num_regs;
num_regs++;
- }
-
- if (pcc->gdb_spr_xml) {
- return;
- }
- xml = g_string_new("<?xml version=\"1.0\"?>");
- g_string_append(xml, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
- g_string_append(xml, "<feature name=\"org.qemu.power.spr\">");
-
- for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
- ppc_spr_t *spr = &env->spr_cb[i];
-
- if (!spr->name) {
- continue;
- }
-
- spr_name = g_ascii_strdown(spr->name, -1);
- g_string_append_printf(xml, "<reg name=\"%s\"", spr_name);
- g_free(spr_name);
-
- g_string_append_printf(xml, " bitsize=\"%d\"", TARGET_LONG_BITS);
- g_string_append(xml, " group=\"spr\"/>");
+ gdb_feature_builder_append_reg(&builder, g_ascii_strdown(spr->name, -1),
+ TARGET_LONG_BITS, num_regs,
+ "int", "spr");
}
- g_string_append(xml, "</feature>");
-
- pcc->gdb_num_sprs = num_regs;
- pcc->gdb_spr_xml = g_string_free(xml, false);
+ gdb_feature_builder_end(&builder);
}
const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
@@ -362,7 +348,7 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
if (strcmp(xml_name, "power-spr.xml") == 0) {
- return pcc->gdb_spr_xml;
+ return pcc->gdb_spr.xml;
}
return NULL;
}
@@ -599,7 +585,8 @@ void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *pcc)
32, "power-vsx.xml", 0);
}
#ifndef CONFIG_USER_ONLY
+ gdb_gen_spr_feature(cs);
gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
- pcc->gdb_num_sprs, "power-spr.xml", 0);
+ pcc->gdb_spr.num_regs, "power-spr.xml", 0);
#endif
}
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 06/18] target/riscv: Use GDBFeature for dynamic XML
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (4 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 05/18] target/ppc: " Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 07/18] gdbstub: Use GDBFeature for gdb_register_coprocessor Akihiko Odaki
` (12 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Palmer Dabbelt,
Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza,
Liu Zhiwei, qemu-riscv
In preparation for a change to use GDBFeature as a parameter of
gdb_register_coprocessor(), convert the internal representation of
dynamic feature from plain XML to GDBFeature.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
target/riscv/cpu.h | 5 +--
target/riscv/cpu.c | 4 +--
target/riscv/gdbstub.c | 79 +++++++++++++++++++-----------------------
3 files changed, 40 insertions(+), 48 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index ef10efd1e7..4a1683c8ad 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -24,6 +24,7 @@
#include "hw/registerfields.h"
#include "hw/qdev-properties.h"
#include "exec/cpu-defs.h"
+#include "exec/gdbstub.h"
#include "qemu/cpu-float.h"
#include "qom/object.h"
#include "qemu/int128.h"
@@ -394,8 +395,8 @@ struct ArchCPU {
CPURISCVState env;
- char *dyn_csr_xml;
- char *dyn_vreg_xml;
+ GDBFeature dyn_csr_feature;
+ GDBFeature dyn_vreg_feature;
/* Configuration Settings */
RISCVCPUConfig cfg;
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 72124e57fd..5e083b5798 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1432,9 +1432,9 @@ static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
RISCVCPU *cpu = RISCV_CPU(cs);
if (strcmp(xmlname, "riscv-csr.xml") == 0) {
- return cpu->dyn_csr_xml;
+ return cpu->dyn_csr_feature.xml;
} else if (strcmp(xmlname, "riscv-vector.xml") == 0) {
- return cpu->dyn_vreg_xml;
+ return cpu->dyn_vreg_feature.xml;
}
return NULL;
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index b9528cef5b..cc3d09e080 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -214,13 +214,14 @@ static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
return 0;
}
-static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
+static GDBFeature *riscv_gen_dynamic_csr_feature(CPUState *cs, int base_reg)
{
RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cs);
RISCVCPU *cpu = RISCV_CPU(cs);
CPURISCVState *env = &cpu->env;
- GString *s = g_string_new(NULL);
+ GDBFeatureBuilder builder;
riscv_csr_predicate_fn predicate;
+ const char *name;
int bitsize = 16 << mcc->misa_mxl_max;
int i;
@@ -233,9 +234,9 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
bitsize = 64;
}
- g_string_printf(s, "<?xml version=\"1.0\"?>");
- g_string_append_printf(s, "<!DOCTYPE feature SYSTEM \"gdb-target.dtd\">");
- g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.csr\">");
+ gdb_feature_builder_init(&builder, &cpu->dyn_csr_feature,
+ "org.gnu.gdb.riscv.csr", "riscv-csr.xml",
+ base_reg);
for (i = 0; i < CSR_TABLE_SIZE; i++) {
if (env->priv_ver < csr_ops[i].min_priv_ver) {
@@ -243,72 +244,64 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
}
predicate = csr_ops[i].predicate;
if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
- if (csr_ops[i].name) {
- g_string_append_printf(s, "<reg name=\"%s\"", csr_ops[i].name);
- } else {
- g_string_append_printf(s, "<reg name=\"csr%03x\"", i);
+ g_autofree char *dynamic_name = NULL;
+ name = csr_ops[i].name;
+ if (!name) {
+ dynamic_name = g_strdup_printf("csr%03x", i);
+ name = dynamic_name;
}
- g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
- g_string_append_printf(s, " regnum=\"%d\"/>", base_reg + i);
+
+ gdb_feature_builder_append_reg(&builder, name, bitsize, i,
+ "int", NULL);
}
}
- g_string_append_printf(s, "</feature>");
-
- cpu->dyn_csr_xml = g_string_free(s, false);
+ gdb_feature_builder_end(&builder);
#if !defined(CONFIG_USER_ONLY)
env->debugger = false;
#endif
- return CSR_TABLE_SIZE;
+ return &cpu->dyn_csr_feature;
}
-static int ricsv_gen_dynamic_vector_xml(CPUState *cs, int base_reg)
+static GDBFeature *ricsv_gen_dynamic_vector_feature(CPUState *cs, int base_reg)
{
RISCVCPU *cpu = RISCV_CPU(cs);
- GString *s = g_string_new(NULL);
- g_autoptr(GString) ts = g_string_new("");
+ GDBFeatureBuilder builder;
int reg_width = cpu->cfg.vlen;
- int num_regs = 0;
int i;
- g_string_printf(s, "<?xml version=\"1.0\"?>");
- g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
- g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.vector\">");
+ gdb_feature_builder_init(&builder, &cpu->dyn_vreg_feature,
+ "org.gnu.gdb.riscv.vector", "riscv-vector.xml",
+ base_reg);
/* First define types and totals in a whole VL */
for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
int count = reg_width / vec_lanes[i].size;
- g_string_printf(ts, "%s", vec_lanes[i].id);
- g_string_append_printf(s,
- "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
- ts->str, vec_lanes[i].gdb_type, count);
+ gdb_feature_builder_append_tag(
+ &builder, "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
+ vec_lanes[i].id, vec_lanes[i].gdb_type, count);
}
/* Define unions */
- g_string_append_printf(s, "<union id=\"riscv_vector\">");
+ gdb_feature_builder_append_tag(&builder, "<union id=\"riscv_vector\">");
for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
- g_string_append_printf(s, "<field name=\"%c\" type=\"%s\"/>",
- vec_lanes[i].suffix,
- vec_lanes[i].id);
+ gdb_feature_builder_append_tag(&builder,
+ "<field name=\"%c\" type=\"%s\"/>",
+ vec_lanes[i].suffix, vec_lanes[i].id);
}
- g_string_append(s, "</union>");
+ gdb_feature_builder_append_tag(&builder, "</union>");
/* Define vector registers */
for (i = 0; i < 32; i++) {
- g_string_append_printf(s,
- "<reg name=\"v%d\" bitsize=\"%d\""
- " regnum=\"%d\" group=\"vector\""
- " type=\"riscv_vector\"/>",
- i, reg_width, base_reg++);
- num_regs++;
+ gdb_feature_builder_append_reg(&builder, g_strdup_printf("v%d", i),
+ reg_width, i, "riscv_vector", "vector");
}
- g_string_append_printf(s, "</feature>");
+ gdb_feature_builder_end(&builder);
- cpu->dyn_vreg_xml = g_string_free(s, false);
- return num_regs;
+ return &cpu->dyn_vreg_feature;
}
void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
@@ -324,10 +317,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
32, "riscv-32bit-fpu.xml", 0);
}
if (env->misa_ext & RVV) {
- int base_reg = cs->gdb_num_regs;
gdb_register_coprocessor(cs, riscv_gdb_get_vector,
riscv_gdb_set_vector,
- ricsv_gen_dynamic_vector_xml(cs, base_reg),
+ ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs)->num_regs,
"riscv-vector.xml", 0);
}
switch (mcc->misa_mxl_max) {
@@ -347,9 +339,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
}
if (cpu->cfg.ext_icsr) {
- int base_reg = cs->gdb_num_regs;
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
- riscv_gen_dynamic_csr_xml(cs, base_reg),
+ riscv_gen_dynamic_csr_feature(cs, cs->gdb_num_regs)->num_regs,
"riscv-csr.xml", 0);
}
}
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 07/18] gdbstub: Use GDBFeature for gdb_register_coprocessor
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (5 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 06/18] target/riscv: " Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 08/18] gdbstub: Use GDBFeature for GDBRegisterState Akihiko Odaki
` (11 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Peter Maydell,
Brian Cain, Song Gao, Laurent Vivier, Edgar E. Iglesias,
Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei,
Thomas Huth, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, qemu-arm, qemu-ppc, qemu-riscv, qemu-s390x
This is a tree-wide change to introduce GDBFeature parameter to
gdb_register_coprocessor(). The new parameter just replaces num_regs
and xml parameters for now. GDBFeature will be utilized to simplify XML
lookup in a following change.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Acked-by: Alex Bennée <alex.bennee@linaro.org>
---
include/exec/gdbstub.h | 2 +-
gdbstub/gdbstub.c | 13 +++++++------
target/arm/gdbstub.c | 35 +++++++++++++++++++----------------
target/hexagon/cpu.c | 3 +--
target/loongarch/gdbstub.c | 2 +-
target/m68k/helper.c | 6 +++---
target/microblaze/cpu.c | 5 +++--
target/ppc/gdbstub.c | 11 ++++++-----
target/riscv/gdbstub.c | 20 ++++++++++++--------
target/s390x/gdbstub.c | 28 +++++++---------------------
10 files changed, 60 insertions(+), 65 deletions(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 97875634f3..5b9682d52e 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -38,7 +38,7 @@ typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
*/
void gdb_register_coprocessor(CPUState *cpu,
gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
- int num_regs, const char *xml, int g_pos);
+ const GDBFeature *feature, int g_pos);
/**
* gdbserver_start: start the gdb server
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index ebb912da1b..67c6fd2609 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -544,7 +544,7 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
void gdb_register_coprocessor(CPUState *cpu,
gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
- int num_regs, const char *xml, int g_pos)
+ const GDBFeature *feature, int g_pos)
{
GDBRegisterState *s;
guint i;
@@ -553,7 +553,7 @@ void gdb_register_coprocessor(CPUState *cpu,
for (i = 0; i < cpu->gdb_regs->len; i++) {
/* Check for duplicates. */
s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (strcmp(s->xml, xml) == 0) {
+ if (strcmp(s->xml, feature->xmlname) == 0) {
return;
}
}
@@ -565,17 +565,18 @@ void gdb_register_coprocessor(CPUState *cpu,
g_array_set_size(cpu->gdb_regs, i + 1);
s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
s->base_reg = cpu->gdb_num_regs;
- s->num_regs = num_regs;
+ s->num_regs = feature->num_regs;
s->get_reg = get_reg;
s->set_reg = set_reg;
- s->xml = xml;
+ s->xml = feature->xml;
/* Add to end of list. */
- cpu->gdb_num_regs += num_regs;
+ cpu->gdb_num_regs += feature->num_regs;
if (g_pos) {
if (g_pos != s->base_reg) {
error_report("Error: Bad gdb register numbering for '%s', "
- "expected %d got %d", xml, g_pos, s->base_reg);
+ "expected %d got %d", feature->xml,
+ g_pos, s->base_reg);
} else {
cpu->gdb_num_g_regs = cpu->gdb_num_regs;
}
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 0b21ee42e0..df98ce7462 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -482,14 +482,14 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
*/
#ifdef TARGET_AARCH64
if (isar_feature_aa64_sve(&cpu->isar)) {
- int nreg = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs)->num_regs;
+ GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs);
gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
- aarch64_gdb_set_sve_reg, nreg,
- "sve-registers.xml", 0);
+ aarch64_gdb_set_sve_reg, feature, 0);
} else {
gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
aarch64_gdb_set_fpu_reg,
- 34, "aarch64-fpu.xml", 0);
+ gdb_find_static_feature("aarch64-fpu.xml"),
+ 0);
}
/*
* Note that we report pauth information via the feature name
@@ -500,19 +500,22 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
if (isar_feature_aa64_pauth(&cpu->isar)) {
gdb_register_coprocessor(cs, aarch64_gdb_get_pauth_reg,
aarch64_gdb_set_pauth_reg,
- 4, "aarch64-pauth.xml", 0);
+ gdb_find_static_feature("aarch64-pauth.xml"),
+ 0);
}
#endif
} else {
if (arm_feature(env, ARM_FEATURE_NEON)) {
gdb_register_coprocessor(cs, vfp_gdb_get_reg, vfp_gdb_set_reg,
- 49, "arm-neon.xml", 0);
+ gdb_find_static_feature("arm-neon.xml"),
+ 0);
} else if (cpu_isar_feature(aa32_simd_r32, cpu)) {
gdb_register_coprocessor(cs, vfp_gdb_get_reg, vfp_gdb_set_reg,
- 33, "arm-vfp3.xml", 0);
+ gdb_find_static_feature("arm-vfp3.xml"),
+ 0);
} else if (cpu_isar_feature(aa32_vfp_simd, cpu)) {
gdb_register_coprocessor(cs, vfp_gdb_get_reg, vfp_gdb_set_reg,
- 17, "arm-vfp.xml", 0);
+ gdb_find_static_feature("arm-vfp.xml"), 0);
}
if (!arm_feature(env, ARM_FEATURE_M)) {
/*
@@ -520,29 +523,29 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
* expose to gdb.
*/
gdb_register_coprocessor(cs, vfp_gdb_get_sysreg, vfp_gdb_set_sysreg,
- 2, "arm-vfp-sysregs.xml", 0);
+ gdb_find_static_feature("arm-vfp-sysregs.xml"),
+ 0);
}
}
if (cpu_isar_feature(aa32_mve, cpu) && tcg_enabled()) {
gdb_register_coprocessor(cs, mve_gdb_get_reg, mve_gdb_set_reg,
- 1, "arm-m-profile-mve.xml", 0);
+ gdb_find_static_feature("arm-m-profile-mve.xml"),
+ 0);
}
gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
- arm_gen_dynamic_sysreg_feature(cs, cs->gdb_num_regs)->num_regs,
- "system-registers.xml", 0);
+ arm_gen_dynamic_sysreg_feature(cs, cs->gdb_num_regs),
+ 0);
#ifdef CONFIG_TCG
if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) {
gdb_register_coprocessor(cs,
arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
- arm_gen_dynamic_m_systemreg_feature(cs, cs->gdb_num_regs)->num_regs,
- "arm-m-system.xml", 0);
+ arm_gen_dynamic_m_systemreg_feature(cs, cs->gdb_num_regs), 0);
#ifndef CONFIG_USER_ONLY
if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
gdb_register_coprocessor(cs,
arm_gdb_get_m_secextreg, arm_gdb_set_m_secextreg,
- arm_gen_dynamic_m_secextreg_feature(cs, cs->gdb_num_regs)->num_regs,
- "arm-m-secext.xml", 0);
+ arm_gen_dynamic_m_secextreg_feature(cs, cs->gdb_num_regs), 0);
}
#endif
}
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 1adc11b713..60d52e1e9d 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -342,8 +342,7 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
gdb_register_coprocessor(cs, hexagon_hvx_gdb_read_register,
hexagon_hvx_gdb_write_register,
- NUM_VREGS + NUM_QREGS,
- "hexagon-hvx.xml", 0);
+ gdb_find_static_feature("hexagon-hvx.xml"), 0);
qemu_init_vcpu(cs);
cpu_reset(cs);
diff --git a/target/loongarch/gdbstub.c b/target/loongarch/gdbstub.c
index 5fc2f19e96..843a869450 100644
--- a/target/loongarch/gdbstub.c
+++ b/target/loongarch/gdbstub.c
@@ -118,5 +118,5 @@ static int loongarch_gdb_set_fpu(CPULoongArchState *env,
void loongarch_cpu_register_gdb_regs_for_features(CPUState *cs)
{
gdb_register_coprocessor(cs, loongarch_gdb_get_fpu, loongarch_gdb_set_fpu,
- 41, "loongarch-fpu.xml", 0);
+ gdb_find_static_feature("loongarch-fpu.xml"), 0);
}
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 0a1544cd68..675f2dcd5a 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -152,10 +152,10 @@ void m68k_cpu_init_gdb(M68kCPU *cpu)
if (m68k_feature(env, M68K_FEATURE_CF_FPU)) {
gdb_register_coprocessor(cs, cf_fpu_gdb_get_reg, cf_fpu_gdb_set_reg,
- 11, "cf-fp.xml", 18);
+ gdb_find_static_feature("cf-fp.xml"), 18);
} else if (m68k_feature(env, M68K_FEATURE_FPU)) {
- gdb_register_coprocessor(cs, m68k_fpu_gdb_get_reg,
- m68k_fpu_gdb_set_reg, 11, "m68k-fp.xml", 18);
+ gdb_register_coprocessor(cs, m68k_fpu_gdb_get_reg, m68k_fpu_gdb_set_reg,
+ gdb_find_static_feature("m68k-fp.xml"), 18);
}
/* TODO: Add [E]MAC registers. */
}
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index bbb3335cad..1998f69828 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -297,8 +297,9 @@ static void mb_cpu_initfn(Object *obj)
CPUMBState *env = &cpu->env;
gdb_register_coprocessor(CPU(cpu), mb_cpu_gdb_read_stack_protect,
- mb_cpu_gdb_write_stack_protect, 2,
- "microblaze-stack-protect.xml", 0);
+ mb_cpu_gdb_write_stack_protect,
+ gdb_find_static_feature("microblaze-stack-protect.xml"),
+ 0);
set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index e3be3dbd10..09b852464f 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -570,23 +570,24 @@ void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *pcc)
{
if (pcc->insns_flags & PPC_FLOAT) {
gdb_register_coprocessor(cs, gdb_get_float_reg, gdb_set_float_reg,
- 33, "power-fpu.xml", 0);
+ gdb_find_static_feature("power-fpu.xml"), 0);
}
if (pcc->insns_flags & PPC_ALTIVEC) {
gdb_register_coprocessor(cs, gdb_get_avr_reg, gdb_set_avr_reg,
- 34, "power-altivec.xml", 0);
+ gdb_find_static_feature("power-altivec.xml"),
+ 0);
}
if (pcc->insns_flags & PPC_SPE) {
gdb_register_coprocessor(cs, gdb_get_spe_reg, gdb_set_spe_reg,
- 34, "power-spe.xml", 0);
+ gdb_find_static_feature("power-spe.xml"), 0);
}
if (pcc->insns_flags2 & PPC2_VSX) {
gdb_register_coprocessor(cs, gdb_get_vsx_reg, gdb_set_vsx_reg,
- 32, "power-vsx.xml", 0);
+ gdb_find_static_feature("power-vsx.xml"), 0);
}
#ifndef CONFIG_USER_ONLY
gdb_gen_spr_feature(cs);
gdb_register_coprocessor(cs, gdb_get_spr_reg, gdb_set_spr_reg,
- pcc->gdb_spr.num_regs, "power-spr.xml", 0);
+ &pcc->gdb_spr, 0);
#endif
}
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index cc3d09e080..f222d5050e 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -311,28 +311,32 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
CPURISCVState *env = &cpu->env;
if (env->misa_ext & RVD) {
gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
- 32, "riscv-64bit-fpu.xml", 0);
+ gdb_find_static_feature("riscv-64bit-fpu.xml"),
+ 0);
} else if (env->misa_ext & RVF) {
gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
- 32, "riscv-32bit-fpu.xml", 0);
+ gdb_find_static_feature("riscv-32bit-fpu.xml"),
+ 0);
}
if (env->misa_ext & RVV) {
gdb_register_coprocessor(cs, riscv_gdb_get_vector,
riscv_gdb_set_vector,
- ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs)->num_regs,
- "riscv-vector.xml", 0);
+ ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs),
+ 0);
}
switch (mcc->misa_mxl_max) {
case MXL_RV32:
gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
riscv_gdb_set_virtual,
- 1, "riscv-32bit-virtual.xml", 0);
+ gdb_find_static_feature("riscv-32bit-virtual.xml"),
+ 0);
break;
case MXL_RV64:
case MXL_RV128:
gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
riscv_gdb_set_virtual,
- 1, "riscv-64bit-virtual.xml", 0);
+ gdb_find_static_feature("riscv-64bit-virtual.xml"),
+ 0);
break;
default:
g_assert_not_reached();
@@ -340,7 +344,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
if (cpu->cfg.ext_icsr) {
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
- riscv_gen_dynamic_csr_feature(cs, cs->gdb_num_regs)->num_regs,
- "riscv-csr.xml", 0);
+ riscv_gen_dynamic_csr_feature(cs, cs->gdb_num_regs),
+ 0);
}
}
diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
index 6fbfd41bc8..02c388dc32 100644
--- a/target/s390x/gdbstub.c
+++ b/target/s390x/gdbstub.c
@@ -69,8 +69,6 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
/* the values represent the positions in s390-acr.xml */
#define S390_A0_REGNUM 0
#define S390_A15_REGNUM 15
-/* total number of registers in s390-acr.xml */
-#define S390_NUM_AC_REGS 16
static int cpu_read_ac_reg(CPUS390XState *env, GByteArray *buf, int n)
{
@@ -98,8 +96,6 @@ static int cpu_write_ac_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_FPC_REGNUM 0
#define S390_F0_REGNUM 1
#define S390_F15_REGNUM 16
-/* total number of registers in s390-fpr.xml */
-#define S390_NUM_FP_REGS 17
static int cpu_read_fp_reg(CPUS390XState *env, GByteArray *buf, int n)
{
@@ -132,8 +128,6 @@ static int cpu_write_fp_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_V15L_REGNUM 15
#define S390_V16_REGNUM 16
#define S390_V31_REGNUM 31
-/* total number of registers in s390-vx.xml */
-#define S390_NUM_VREGS 32
static int cpu_read_vreg(CPUS390XState *env, GByteArray *buf, int n)
{
@@ -172,8 +166,6 @@ static int cpu_write_vreg(CPUS390XState *env, uint8_t *mem_buf, int n)
/* the values represent the positions in s390-cr.xml */
#define S390_C0_REGNUM 0
#define S390_C15_REGNUM 15
-/* total number of registers in s390-cr.xml */
-#define S390_NUM_C_REGS 16
#ifndef CONFIG_USER_ONLY
static int cpu_read_c_reg(CPUS390XState *env, GByteArray *buf, int n)
@@ -206,8 +198,6 @@ static int cpu_write_c_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_VIRT_CPUTM_REGNUM 1
#define S390_VIRT_BEA_REGNUM 2
#define S390_VIRT_PREFIX_REGNUM 3
-/* total number of registers in s390-virt.xml */
-#define S390_NUM_VIRT_REGS 4
static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
{
@@ -254,8 +244,6 @@ static int cpu_write_virt_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_VIRT_KVM_PFT_REGNUM 1
#define S390_VIRT_KVM_PFS_REGNUM 2
#define S390_VIRT_KVM_PFC_REGNUM 3
-/* total number of registers in s390-virt-kvm.xml */
-#define S390_NUM_VIRT_KVM_REGS 4
static int cpu_read_virt_kvm_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
{
@@ -303,8 +291,6 @@ static int cpu_write_virt_kvm_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_GS_GSD_REGNUM 1
#define S390_GS_GSSM_REGNUM 2
#define S390_GS_GSEPLA_REGNUM 3
-/* total number of registers in s390-gs.xml */
-#define S390_NUM_GS_REGS 4
static int cpu_read_gs_reg(CPUS390XState *env, GByteArray *buf, int n)
{
@@ -322,33 +308,33 @@ void s390_cpu_gdb_init(CPUState *cs)
{
gdb_register_coprocessor(cs, cpu_read_ac_reg,
cpu_write_ac_reg,
- S390_NUM_AC_REGS, "s390-acr.xml", 0);
+ gdb_find_static_feature("s390-acr.xml"), 0);
gdb_register_coprocessor(cs, cpu_read_fp_reg,
cpu_write_fp_reg,
- S390_NUM_FP_REGS, "s390-fpr.xml", 0);
+ gdb_find_static_feature("s390-fpr.xml"), 0);
gdb_register_coprocessor(cs, cpu_read_vreg,
cpu_write_vreg,
- S390_NUM_VREGS, "s390-vx.xml", 0);
+ gdb_find_static_feature("s390-vx.xml"), 0);
gdb_register_coprocessor(cs, cpu_read_gs_reg,
cpu_write_gs_reg,
- S390_NUM_GS_REGS, "s390-gs.xml", 0);
+ gdb_find_static_feature("s390-gs.xml"), 0);
#ifndef CONFIG_USER_ONLY
gdb_register_coprocessor(cs, cpu_read_c_reg,
cpu_write_c_reg,
- S390_NUM_C_REGS, "s390-cr.xml", 0);
+ gdb_find_static_feature("s390-cr.xml"), 0);
gdb_register_coprocessor(cs, cpu_read_virt_reg,
cpu_write_virt_reg,
- S390_NUM_VIRT_REGS, "s390-virt.xml", 0);
+ gdb_find_static_feature("s390-virt.xml"), 0);
if (kvm_enabled()) {
gdb_register_coprocessor(cs, cpu_read_virt_kvm_reg,
cpu_write_virt_kvm_reg,
- S390_NUM_VIRT_KVM_REGS, "s390-virt-kvm.xml",
+ gdb_find_static_feature("s390-virt-kvm.xml"),
0);
}
#endif
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 08/18] gdbstub: Use GDBFeature for GDBRegisterState
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (6 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 07/18] gdbstub: Use GDBFeature for gdb_register_coprocessor Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 09/18] gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb Akihiko Odaki
` (10 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki
Simplify GDBRegisterState by replacing num_regs and xml members with
one member that points to GDBFeature.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
gdbstub/gdbstub.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 67c6fd2609..ebabbc00f6 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -47,10 +47,9 @@
typedef struct GDBRegisterState {
int base_reg;
- int num_regs;
gdb_get_reg_cb get_reg;
gdb_set_reg_cb set_reg;
- const char *xml;
+ const GDBFeature *feature;
} GDBRegisterState;
GDBState gdbserver_state;
@@ -391,7 +390,7 @@ static const char *get_feature_xml(const char *p, const char **newp,
g_ptr_array_add(
xml,
g_markup_printf_escaped("<xi:include href=\"%s\"/>",
- r->xml));
+ r->feature->xmlname));
}
}
g_ptr_array_add(xml, g_strdup("</target>"));
@@ -513,7 +512,7 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
if (cpu->gdb_regs) {
for (guint i = 0; i < cpu->gdb_regs->len; i++) {
r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) {
+ if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
return r->get_reg(env, buf, reg - r->base_reg);
}
}
@@ -534,7 +533,7 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
if (cpu->gdb_regs) {
for (guint i = 0; i < cpu->gdb_regs->len; i++) {
r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) {
+ if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
return r->set_reg(env, mem_buf, reg - r->base_reg);
}
}
@@ -553,7 +552,7 @@ void gdb_register_coprocessor(CPUState *cpu,
for (i = 0; i < cpu->gdb_regs->len; i++) {
/* Check for duplicates. */
s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (strcmp(s->xml, feature->xmlname) == 0) {
+ if (s->feature == feature) {
return;
}
}
@@ -565,10 +564,9 @@ void gdb_register_coprocessor(CPUState *cpu,
g_array_set_size(cpu->gdb_regs, i + 1);
s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
s->base_reg = cpu->gdb_num_regs;
- s->num_regs = feature->num_regs;
s->get_reg = get_reg;
s->set_reg = set_reg;
- s->xml = feature->xml;
+ s->feature = feature;
/* Add to end of list. */
cpu->gdb_num_regs += feature->num_regs;
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 09/18] gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (7 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 08/18] gdbstub: Use GDBFeature for GDBRegisterState Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 14:20 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 10/18] gdbstub: Simplify XML lookup Akihiko Odaki
` (9 subsequent siblings)
18 siblings, 1 reply; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Peter Maydell,
Brian Cain, Song Gao, Laurent Vivier, Edgar E. Iglesias,
Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei,
Richard Henderson, David Hildenbrand, Ilya Leoshkevich,
Thomas Huth, qemu-arm, qemu-ppc, qemu-riscv, qemu-s390x
Align the parameters of gdb_get_reg_cb and gdb_set_reg_cb with the
gdb_read_register and gdb_write_register members of CPUClass to allow
to unify the logic to access registers of the core and coprocessors
in the future.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/exec/gdbstub.h | 4 +-
target/arm/internals.h | 12 +++---
target/hexagon/internal.h | 4 +-
target/microblaze/cpu.h | 4 +-
gdbstub/gdbstub.c | 6 +--
target/arm/gdbstub.c | 51 ++++++++++++++++--------
target/arm/gdbstub64.c | 27 +++++++++----
target/hexagon/gdbstub.c | 10 ++++-
target/loongarch/gdbstub.c | 11 ++++--
target/m68k/helper.c | 20 ++++++++--
target/microblaze/gdbstub.c | 9 ++++-
target/ppc/gdbstub.c | 46 +++++++++++++++++-----
target/riscv/gdbstub.c | 46 ++++++++++++++++------
target/s390x/gdbstub.c | 77 ++++++++++++++++++++++++++++---------
14 files changed, 236 insertions(+), 91 deletions(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 5b9682d52e..df5df553c7 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -24,8 +24,8 @@ typedef struct GDBFeatureBuilder {
/* Get or set a register. Returns the size of the register. */
-typedef int (*gdb_get_reg_cb)(CPUArchState *env, GByteArray *buf, int reg);
-typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
+typedef int (*gdb_get_reg_cb)(CPUState *cpu, GByteArray *buf, int reg);
+typedef int (*gdb_set_reg_cb)(CPUState *cpu, uint8_t *buf, int reg);
/**
* gdb_register_coprocessor() - register a supplemental set of registers
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 99136985ad..e597b77be3 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1441,12 +1441,12 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
#ifdef TARGET_AARCH64
GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
-int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg);
-int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg);
-int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg);
-int aarch64_gdb_set_fpu_reg(CPUARMState *env, uint8_t *buf, int reg);
-int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg);
-int aarch64_gdb_set_pauth_reg(CPUARMState *env, uint8_t *buf, int reg);
+int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
+int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
+int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
+int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
+int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);
+int aarch64_gdb_set_pauth_reg(CPUState *cs, uint8_t *buf, int reg);
void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
diff --git a/target/hexagon/internal.h b/target/hexagon/internal.h
index d732b6bb3c..beb08cb7e3 100644
--- a/target/hexagon/internal.h
+++ b/target/hexagon/internal.h
@@ -33,8 +33,8 @@
int hexagon_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-int hexagon_hvx_gdb_read_register(CPUHexagonState *env, GByteArray *mem_buf, int n);
-int hexagon_hvx_gdb_write_register(CPUHexagonState *env, uint8_t *mem_buf, int n);
+int hexagon_hvx_gdb_read_register(CPUState *env, GByteArray *mem_buf, int n);
+int hexagon_hvx_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n);
void hexagon_debug_vreg(CPUHexagonState *env, int regnum);
void hexagon_debug_qreg(CPUHexagonState *env, int regnum);
diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
index e43c49d4af..c14b3357b7 100644
--- a/target/microblaze/cpu.h
+++ b/target/microblaze/cpu.h
@@ -370,8 +370,8 @@ G_NORETURN void mb_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
void mb_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
int mb_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
int mb_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-int mb_cpu_gdb_read_stack_protect(CPUArchState *cpu, GByteArray *buf, int reg);
-int mb_cpu_gdb_write_stack_protect(CPUArchState *cpu, uint8_t *buf, int reg);
+int mb_cpu_gdb_read_stack_protect(CPUState *cs, GByteArray *buf, int reg);
+int mb_cpu_gdb_write_stack_protect(CPUState *cs, uint8_t *buf, int reg);
static inline uint32_t mb_cpu_read_msr(const CPUMBState *env)
{
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index ebabbc00f6..4809c90c4a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -502,7 +502,6 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname)
static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
- CPUArchState *env = cpu_env(cpu);
GDBRegisterState *r;
if (reg < cc->gdb_num_core_regs) {
@@ -513,7 +512,7 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
for (guint i = 0; i < cpu->gdb_regs->len; i++) {
r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
- return r->get_reg(env, buf, reg - r->base_reg);
+ return r->get_reg(cpu, buf, reg - r->base_reg);
}
}
}
@@ -523,7 +522,6 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
- CPUArchState *env = cpu_env(cpu);
GDBRegisterState *r;
if (reg < cc->gdb_num_core_regs) {
@@ -534,7 +532,7 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
for (guint i = 0; i < cpu->gdb_regs->len; i++) {
r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
- return r->set_reg(env, mem_buf, reg - r->base_reg);
+ return r->set_reg(cpu, mem_buf, reg - r->base_reg);
}
}
}
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index df98ce7462..8d771c5d46 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -105,9 +105,10 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
return 0;
}
-static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
+static int vfp_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
{
- ARMCPU *cpu = env_archcpu(env);
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
int nregs = cpu_isar_feature(aa32_simd_r32, cpu) ? 32 : 16;
/* VFP data registers are always little-endian. */
@@ -129,9 +130,10 @@ static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
return 0;
}
-static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
+static int vfp_gdb_set_reg(CPUState *cs, uint8_t *buf, int reg)
{
- ARMCPU *cpu = env_archcpu(env);
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
int nregs = cpu_isar_feature(aa32_simd_r32, cpu) ? 32 : 16;
if (reg < nregs) {
@@ -155,8 +157,11 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-static int vfp_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
+static int vfp_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0:
return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]);
@@ -166,8 +171,11 @@ static int vfp_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
return 0;
}
-static int vfp_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
+static int vfp_gdb_set_sysreg(CPUState *cs, uint8_t *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0:
env->vfp.xregs[ARM_VFP_FPSID] = ldl_p(buf);
@@ -179,8 +187,11 @@ static int vfp_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-static int mve_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
+static int mve_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0:
return gdb_get_reg32(buf, env->v7m.vpr);
@@ -189,8 +200,11 @@ static int mve_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
}
}
-static int mve_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
+static int mve_gdb_set_reg(CPUState *cs, uint8_t *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0:
env->v7m.vpr = ldl_p(buf);
@@ -209,9 +223,10 @@ static int mve_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
* We return the number of bytes copied
*/
-static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
+static int arm_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
{
- ARMCPU *cpu = env_archcpu(env);
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
const ARMCPRegInfo *ri;
uint32_t key;
@@ -227,7 +242,7 @@ static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
return 0;
}
-static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
+static int arm_gdb_set_sysreg(CPUState *cs, uint8_t *buf, int reg)
{
return 0;
}
@@ -366,8 +381,11 @@ static int m_sysreg_get(CPUARMState *env, GByteArray *buf,
return gdb_get_reg32(buf, *ptr);
}
-static int arm_gdb_get_m_systemreg(CPUARMState *env, GByteArray *buf, int reg)
+static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
/*
* Here, we emulate MRS instruction, where CONTROL has a mix of
* banked and non-banked bits.
@@ -378,7 +396,7 @@ static int arm_gdb_get_m_systemreg(CPUARMState *env, GByteArray *buf, int reg)
return m_sysreg_get(env, buf, reg, env->v7m.secure);
}
-static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg)
+static int arm_gdb_set_m_systemreg(CPUState *cs, uint8_t *buf, int reg)
{
return 0; /* TODO */
}
@@ -413,12 +431,15 @@ static GDBFeature *arm_gen_dynamic_m_systemreg_feature(CPUState *cs,
* For user-only, we see the non-secure registers via m_systemreg above.
* For secext, encode the non-secure view as even and secure view as odd.
*/
-static int arm_gdb_get_m_secextreg(CPUARMState *env, GByteArray *buf, int reg)
+static int arm_gdb_get_m_secextreg(CPUState *cs, GByteArray *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
return m_sysreg_get(env, buf, reg >> 1, reg & 1);
}
-static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg)
+static int arm_gdb_set_m_secextreg(CPUState *cs, uint8_t *buf, int reg)
{
return 0; /* TODO */
}
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 5286d5c604..caa31ff3fa 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -72,8 +72,11 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
return 0;
}
-int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg)
+int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0 ... 31:
{
@@ -92,8 +95,11 @@ int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg)
}
}
-int aarch64_gdb_set_fpu_reg(CPUARMState *env, uint8_t *buf, int reg)
+int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0 ... 31:
/* 128 bit FP register */
@@ -116,9 +122,10 @@ int aarch64_gdb_set_fpu_reg(CPUARMState *env, uint8_t *buf, int reg)
}
}
-int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg)
+int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
{
- ARMCPU *cpu = env_archcpu(env);
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
switch (reg) {
/* The first 32 registers are the zregs */
@@ -164,9 +171,10 @@ int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg)
return 0;
}
-int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
+int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg)
{
- ARMCPU *cpu = env_archcpu(env);
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
/* The first 32 registers are the zregs */
switch (reg) {
@@ -210,8 +218,11 @@ int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg)
+int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+
switch (reg) {
case 0: /* pauth_dmask */
case 1: /* pauth_cmask */
@@ -241,7 +252,7 @@ int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg)
}
}
-int aarch64_gdb_set_pauth_reg(CPUARMState *env, uint8_t *buf, int reg)
+int aarch64_gdb_set_pauth_reg(CPUState *cs, uint8_t *buf, int reg)
{
/* All pseudo registers are read-only. */
return 0;
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index 54d37e006e..6007e6462b 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -81,8 +81,11 @@ static int gdb_get_qreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
return total;
}
-int hexagon_hvx_gdb_read_register(CPUHexagonState *env, GByteArray *mem_buf, int n)
+int hexagon_hvx_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
{
+ HexagonCPU *cpu = HEXAGON_CPU(cs);
+ CPUHexagonState *env = &cpu->env;
+
if (n < NUM_VREGS) {
return gdb_get_vreg(env, mem_buf, n);
}
@@ -115,8 +118,11 @@ static int gdb_put_qreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
return MAX_VEC_SIZE_BYTES / 8;
}
-int hexagon_hvx_gdb_write_register(CPUHexagonState *env, uint8_t *mem_buf, int n)
+int hexagon_hvx_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
{
+ HexagonCPU *cpu = HEXAGON_CPU(cs);
+ CPUHexagonState *env = &cpu->env;
+
if (n < NUM_VREGS) {
return gdb_put_vreg(env, mem_buf, n);
}
diff --git a/target/loongarch/gdbstub.c b/target/loongarch/gdbstub.c
index 843a869450..22c6889011 100644
--- a/target/loongarch/gdbstub.c
+++ b/target/loongarch/gdbstub.c
@@ -84,9 +84,11 @@ int loongarch_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
return length;
}
-static int loongarch_gdb_get_fpu(CPULoongArchState *env,
- GByteArray *mem_buf, int n)
+static int loongarch_gdb_get_fpu(CPUState *cs, GByteArray *mem_buf, int n)
{
+ LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+ CPULoongArchState *env = &cpu->env;
+
if (0 <= n && n < 32) {
return gdb_get_reg64(mem_buf, env->fpr[n].vreg.D(0));
} else if (32 <= n && n < 40) {
@@ -97,9 +99,10 @@ static int loongarch_gdb_get_fpu(CPULoongArchState *env,
return 0;
}
-static int loongarch_gdb_set_fpu(CPULoongArchState *env,
- uint8_t *mem_buf, int n)
+static int loongarch_gdb_set_fpu(CPUState *cs, uint8_t *mem_buf, int n)
{
+ LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+ CPULoongArchState *env = &cpu->env;
int length = 0;
if (0 <= n && n < 32) {
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 675f2dcd5a..a5ee4d87e3 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -69,8 +69,11 @@ void m68k_cpu_list(void)
g_slist_free(list);
}
-static int cf_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
+static int cf_fpu_gdb_get_reg(CPUState *cs, GByteArray *mem_buf, int n)
{
+ M68kCPU *cpu = M68K_CPU(cs);
+ CPUM68KState *env = &cpu->env;
+
if (n < 8) {
float_status s;
return gdb_get_reg64(mem_buf, floatx80_to_float64(env->fregs[n].d, &s));
@@ -86,8 +89,11 @@ static int cf_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
return 0;
}
-static int cf_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
+static int cf_fpu_gdb_set_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ M68kCPU *cpu = M68K_CPU(cs);
+ CPUM68KState *env = &cpu->env;
+
if (n < 8) {
float_status s;
env->fregs[n].d = float64_to_floatx80(ldq_p(mem_buf), &s);
@@ -106,8 +112,11 @@ static int cf_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
+static int m68k_fpu_gdb_get_reg(CPUState *cs, GByteArray *mem_buf, int n)
{
+ M68kCPU *cpu = M68K_CPU(cs);
+ CPUM68KState *env = &cpu->env;
+
if (n < 8) {
int len = gdb_get_reg16(mem_buf, env->fregs[n].l.upper);
len += gdb_get_reg16(mem_buf, 0);
@@ -125,8 +134,11 @@ static int m68k_fpu_gdb_get_reg(CPUM68KState *env, GByteArray *mem_buf, int n)
return 0;
}
-static int m68k_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
+static int m68k_fpu_gdb_set_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ M68kCPU *cpu = M68K_CPU(cs);
+ CPUM68KState *env = &cpu->env;
+
if (n < 8) {
env->fregs[n].l.upper = lduw_be_p(mem_buf);
env->fregs[n].l.lower = ldq_be_p(mem_buf + 4);
diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
index 29ac6e9c0f..6ffc5ad075 100644
--- a/target/microblaze/gdbstub.c
+++ b/target/microblaze/gdbstub.c
@@ -94,8 +94,10 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
return gdb_get_reg32(mem_buf, val);
}
-int mb_cpu_gdb_read_stack_protect(CPUMBState *env, GByteArray *mem_buf, int n)
+int mb_cpu_gdb_read_stack_protect(CPUState *cs, GByteArray *mem_buf, int n)
{
+ MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
+ CPUMBState *env = &cpu->env;
uint32_t val;
switch (n) {
@@ -153,8 +155,11 @@ int mb_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
return 4;
}
-int mb_cpu_gdb_write_stack_protect(CPUMBState *env, uint8_t *mem_buf, int n)
+int mb_cpu_gdb_write_stack_protect(CPUState *cs, uint8_t *mem_buf, int n)
{
+ MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
+ CPUMBState *env = &cpu->env;
+
switch (n) {
case GDB_SP_SHL:
env->slr = ldl_p(mem_buf);
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 09b852464f..8ca37b6bf9 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -369,8 +369,10 @@ static int gdb_find_spr_idx(CPUPPCState *env, int n)
return -1;
}
-static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
+static int gdb_get_spr_reg(CPUState *cs, GByteArray *buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
int reg;
int len;
@@ -385,8 +387,10 @@ static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
return len;
}
-static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+static int gdb_set_spr_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
int reg;
int len;
@@ -403,8 +407,10 @@ static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
}
#endif
-static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
+static int gdb_get_float_reg(CPUState *cs, GByteArray *buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
uint8_t *mem_buf;
if (n < 32) {
gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
@@ -421,8 +427,11 @@ static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
return 0;
}
-static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+static int gdb_set_float_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
if (n < 32) {
ppc_maybe_bswap_register(env, mem_buf, 8);
*cpu_fpr_ptr(env, n) = ldq_p(mem_buf);
@@ -436,8 +445,10 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
+static int gdb_get_avr_reg(CPUState *cs, GByteArray *buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
uint8_t *mem_buf;
if (n < 32) {
@@ -462,8 +473,11 @@ static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
return 0;
}
-static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+static int gdb_set_avr_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
if (n < 32) {
ppc_avr_t *avr = cpu_avr_ptr(env, n);
ppc_maybe_bswap_register(env, mem_buf, 16);
@@ -484,8 +498,11 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
+static int gdb_get_spe_reg(CPUState *cs, GByteArray *buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
if (n < 32) {
#if defined(TARGET_PPC64)
gdb_get_reg32(buf, env->gpr[n] >> 32);
@@ -508,8 +525,11 @@ static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
return 0;
}
-static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+static int gdb_set_spe_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
if (n < 32) {
#if defined(TARGET_PPC64)
target_ulong lo = (uint32_t)env->gpr[n];
@@ -537,8 +557,11 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
+static int gdb_get_vsx_reg(CPUState *cs, GByteArray *buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
if (n < 32) {
gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
@@ -547,8 +570,11 @@ static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
return 0;
}
-static int gdb_set_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
+static int gdb_set_vsx_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
if (n < 32) {
ppc_maybe_bswap_register(env, mem_buf, 8);
*cpu_vsrl_ptr(env, n) = ldq_p(mem_buf);
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index f222d5050e..5cb550761e 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -108,8 +108,11 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
return length;
}
-static int riscv_gdb_get_fpu(CPURISCVState *env, GByteArray *buf, int n)
+static int riscv_gdb_get_fpu(CPUState *cs, GByteArray *buf, int n)
{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
if (n < 32) {
if (env->misa_ext & RVD) {
return gdb_get_reg64(buf, env->fpr[n]);
@@ -121,8 +124,11 @@ static int riscv_gdb_get_fpu(CPURISCVState *env, GByteArray *buf, int n)
return 0;
}
-static int riscv_gdb_set_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
+static int riscv_gdb_set_fpu(CPUState *cs, uint8_t *mem_buf, int n)
{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
if (n < 32) {
env->fpr[n] = ldq_p(mem_buf); /* always 64-bit */
return sizeof(uint64_t);
@@ -130,8 +136,10 @@ static int riscv_gdb_set_fpu(CPURISCVState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
+static int riscv_gdb_get_vector(CPUState *cs, GByteArray *buf, int n)
{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
uint16_t vlenb = riscv_cpu_cfg(env)->vlen >> 3;
if (n < 32) {
int i;
@@ -146,8 +154,10 @@ static int riscv_gdb_get_vector(CPURISCVState *env, GByteArray *buf, int n)
return 0;
}
-static int riscv_gdb_set_vector(CPURISCVState *env, uint8_t *mem_buf, int n)
+static int riscv_gdb_set_vector(CPUState *cs, uint8_t *mem_buf, int n)
{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
uint16_t vlenb = riscv_cpu_cfg(env)->vlen >> 3;
if (n < 32) {
int i;
@@ -160,8 +170,11 @@ static int riscv_gdb_set_vector(CPURISCVState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int riscv_gdb_get_csr(CPURISCVState *env, GByteArray *buf, int n)
+static int riscv_gdb_get_csr(CPUState *cs, GByteArray *buf, int n)
{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
if (n < CSR_TABLE_SIZE) {
target_ulong val = 0;
int result;
@@ -174,8 +187,11 @@ static int riscv_gdb_get_csr(CPURISCVState *env, GByteArray *buf, int n)
return 0;
}
-static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t *mem_buf, int n)
+static int riscv_gdb_set_csr(CPUState *cs, uint8_t *mem_buf, int n)
{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
if (n < CSR_TABLE_SIZE) {
target_ulong val = ldtul_p(mem_buf);
int result;
@@ -188,25 +204,31 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t *mem_buf, int n)
return 0;
}
-static int riscv_gdb_get_virtual(CPURISCVState *cs, GByteArray *buf, int n)
+static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n)
{
if (n == 0) {
#ifdef CONFIG_USER_ONLY
return gdb_get_regl(buf, 0);
#else
- return gdb_get_regl(buf, cs->priv);
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
+ return gdb_get_regl(buf, env->priv);
#endif
}
return 0;
}
-static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+static int riscv_gdb_set_virtual(CPUState *cs, uint8_t *mem_buf, int n)
{
if (n == 0) {
#ifndef CONFIG_USER_ONLY
- cs->priv = ldtul_p(mem_buf) & 0x3;
- if (cs->priv == PRV_RESERVED) {
- cs->priv = PRV_S;
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
+ env->priv = ldtul_p(mem_buf) & 0x3;
+ if (env->priv == PRV_RESERVED) {
+ env->priv = PRV_S;
}
#endif
return sizeof(target_ulong);
diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
index 02c388dc32..c1e7c59b82 100644
--- a/target/s390x/gdbstub.c
+++ b/target/s390x/gdbstub.c
@@ -70,8 +70,11 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
#define S390_A0_REGNUM 0
#define S390_A15_REGNUM 15
-static int cpu_read_ac_reg(CPUS390XState *env, GByteArray *buf, int n)
+static int cpu_read_ac_reg(CPUState *cs, GByteArray *buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_A0_REGNUM ... S390_A15_REGNUM:
return gdb_get_reg32(buf, env->aregs[n]);
@@ -80,8 +83,11 @@ static int cpu_read_ac_reg(CPUS390XState *env, GByteArray *buf, int n)
}
}
-static int cpu_write_ac_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_ac_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_A0_REGNUM ... S390_A15_REGNUM:
env->aregs[n] = ldl_p(mem_buf);
@@ -97,8 +103,11 @@ static int cpu_write_ac_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_F0_REGNUM 1
#define S390_F15_REGNUM 16
-static int cpu_read_fp_reg(CPUS390XState *env, GByteArray *buf, int n)
+static int cpu_read_fp_reg(CPUState *cs, GByteArray *buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_FPC_REGNUM:
return gdb_get_reg32(buf, env->fpc);
@@ -109,8 +118,11 @@ static int cpu_read_fp_reg(CPUS390XState *env, GByteArray *buf, int n)
}
}
-static int cpu_write_fp_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_fp_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_FPC_REGNUM:
env->fpc = ldl_p(mem_buf);
@@ -129,8 +141,10 @@ static int cpu_write_fp_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_V16_REGNUM 16
#define S390_V31_REGNUM 31
-static int cpu_read_vreg(CPUS390XState *env, GByteArray *buf, int n)
+static int cpu_read_vreg(CPUState *cs, GByteArray *buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
int ret;
switch (n) {
@@ -148,8 +162,11 @@ static int cpu_read_vreg(CPUS390XState *env, GByteArray *buf, int n)
return ret;
}
-static int cpu_write_vreg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_vreg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_V0L_REGNUM ... S390_V15L_REGNUM:
env->vregs[n][1] = ldtul_p(mem_buf + 8);
@@ -168,8 +185,11 @@ static int cpu_write_vreg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_C15_REGNUM 15
#ifndef CONFIG_USER_ONLY
-static int cpu_read_c_reg(CPUS390XState *env, GByteArray *buf, int n)
+static int cpu_read_c_reg(CPUState *cs, GByteArray *buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_C0_REGNUM ... S390_C15_REGNUM:
return gdb_get_regl(buf, env->cregs[n]);
@@ -178,8 +198,11 @@ static int cpu_read_c_reg(CPUS390XState *env, GByteArray *buf, int n)
}
}
-static int cpu_write_c_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_c_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_C0_REGNUM ... S390_C15_REGNUM:
env->cregs[n] = ldtul_p(mem_buf);
@@ -199,8 +222,11 @@ static int cpu_write_c_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_VIRT_BEA_REGNUM 2
#define S390_VIRT_PREFIX_REGNUM 3
-static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
+static int cpu_read_virt_reg(CPUState *cs, GByteArray *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_VIRT_CKC_REGNUM:
return gdb_get_regl(mem_buf, env->ckc);
@@ -215,24 +241,27 @@ static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
}
}
-static int cpu_write_virt_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_virt_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_VIRT_CKC_REGNUM:
env->ckc = ldtul_p(mem_buf);
- cpu_synchronize_post_init(env_cpu(env));
+ cpu_synchronize_post_init(cs);
return 8;
case S390_VIRT_CPUTM_REGNUM:
env->cputm = ldtul_p(mem_buf);
- cpu_synchronize_post_init(env_cpu(env));
+ cpu_synchronize_post_init(cs);
return 8;
case S390_VIRT_BEA_REGNUM:
env->gbea = ldtul_p(mem_buf);
- cpu_synchronize_post_init(env_cpu(env));
+ cpu_synchronize_post_init(cs);
return 8;
case S390_VIRT_PREFIX_REGNUM:
env->psa = ldtul_p(mem_buf);
- cpu_synchronize_post_init(env_cpu(env));
+ cpu_synchronize_post_init(cs);
return 8;
default:
return 0;
@@ -245,8 +274,11 @@ static int cpu_write_virt_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_VIRT_KVM_PFS_REGNUM 2
#define S390_VIRT_KVM_PFC_REGNUM 3
-static int cpu_read_virt_kvm_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
+static int cpu_read_virt_kvm_reg(CPUState *cs, GByteArray *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_VIRT_KVM_PP_REGNUM:
return gdb_get_regl(mem_buf, env->pp);
@@ -261,8 +293,11 @@ static int cpu_read_virt_kvm_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
}
}
-static int cpu_write_virt_kvm_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_virt_kvm_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
switch (n) {
case S390_VIRT_KVM_PP_REGNUM:
env->pp = ldtul_p(mem_buf);
@@ -292,13 +327,19 @@ static int cpu_write_virt_kvm_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
#define S390_GS_GSSM_REGNUM 2
#define S390_GS_GSEPLA_REGNUM 3
-static int cpu_read_gs_reg(CPUS390XState *env, GByteArray *buf, int n)
+static int cpu_read_gs_reg(CPUState *cs, GByteArray *buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
return gdb_get_regl(buf, env->gscb[n]);
}
-static int cpu_write_gs_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+static int cpu_write_gs_reg(CPUState *cs, uint8_t *mem_buf, int n)
{
+ S390CPU *cpu = S390_CPU(cs);
+ CPUS390XState *env = &cpu->env;
+
env->gscb[n] = ldtul_p(mem_buf);
cpu_synchronize_post_init(env_cpu(env));
return 8;
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 10/18] gdbstub: Simplify XML lookup
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (8 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 09/18] gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 14:25 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 11/18] gdbstub: Infer number of core registers from XML Akihiko Odaki
` (8 subsequent siblings)
18 siblings, 1 reply; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki
Now we know all instances of GDBFeature that is used in CPU so we can
traverse them to find XML. This removes the need for a CPU-specific
lookup function for dynamic XMLs.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/exec/gdbstub.h | 2 +
gdbstub/gdbstub.c | 119 +++++++++++++++++++++--------------------
hw/core/cpu-common.c | 5 +-
3 files changed, 66 insertions(+), 60 deletions(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index df5df553c7..b780b583de 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -27,6 +27,8 @@ typedef struct GDBFeatureBuilder {
typedef int (*gdb_get_reg_cb)(CPUState *cpu, GByteArray *buf, int reg);
typedef int (*gdb_set_reg_cb)(CPUState *cpu, uint8_t *buf, int reg);
+void gdb_init_cpu(CPUState *cpu);
+
/**
* gdb_register_coprocessor() - register a supplemental set of registers
* @cpu - the CPU associated with registers
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 4809c90c4a..332f9c6ffc 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -352,6 +352,7 @@ static const char *get_feature_xml(const char *p, const char **newp,
{
CPUState *cpu = gdb_get_first_cpu_in_process(process);
CPUClass *cc = CPU_GET_CLASS(cpu);
+ GDBRegisterState *r;
size_t len;
/*
@@ -365,7 +366,6 @@ static const char *get_feature_xml(const char *p, const char **newp,
/* Is it the main target xml? */
if (strncmp(p, "target.xml", len) == 0) {
if (!process->target_xml) {
- GDBRegisterState *r;
g_autoptr(GPtrArray) xml = g_ptr_array_new_with_free_func(g_free);
g_ptr_array_add(
@@ -380,18 +380,12 @@ static const char *get_feature_xml(const char *p, const char **newp,
g_markup_printf_escaped("<architecture>%s</architecture>",
cc->gdb_arch_name(cpu)));
}
- g_ptr_array_add(
- xml,
- g_markup_printf_escaped("<xi:include href=\"%s\"/>",
- cc->gdb_core_xml_file));
- if (cpu->gdb_regs) {
- for (guint i = 0; i < cpu->gdb_regs->len; i++) {
- r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- g_ptr_array_add(
- xml,
- g_markup_printf_escaped("<xi:include href=\"%s\"/>",
- r->feature->xmlname));
- }
+ for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+ r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ g_ptr_array_add(
+ xml,
+ g_markup_printf_escaped("<xi:include href=\"%s\"/>",
+ r->feature->xmlname));
}
g_ptr_array_add(xml, g_strdup("</target>"));
g_ptr_array_add(xml, NULL);
@@ -400,20 +394,11 @@ static const char *get_feature_xml(const char *p, const char **newp,
}
return process->target_xml;
}
- /* Is it dynamically generated by the target? */
- if (cc->gdb_get_dynamic_xml) {
- g_autofree char *xmlname = g_strndup(p, len);
- const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
- if (xml) {
- return xml;
- }
- }
- /* Is it one of the encoded gdb-xml/ files? */
- for (int i = 0; gdb_static_features[i].xmlname; i++) {
- const char *name = gdb_static_features[i].xmlname;
- if ((strncmp(name, p, len) == 0) &&
- strlen(name) == len) {
- return gdb_static_features[i].xml;
+ /* Is it one of the features? */
+ for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+ r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ if (strncmp(p, r->feature->xmlname, len) == 0) {
+ return r->feature->xml;
}
}
@@ -508,12 +493,10 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
return cc->gdb_read_register(cpu, buf, reg);
}
- if (cpu->gdb_regs) {
- for (guint i = 0; i < cpu->gdb_regs->len; i++) {
- r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
- return r->get_reg(cpu, buf, reg - r->base_reg);
- }
+ for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+ r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
+ return r->get_reg(cpu, buf, reg - r->base_reg);
}
}
return 0;
@@ -528,51 +511,71 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
return cc->gdb_write_register(cpu, mem_buf, reg);
}
- if (cpu->gdb_regs) {
- for (guint i = 0; i < cpu->gdb_regs->len; i++) {
- r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
- return r->set_reg(cpu, mem_buf, reg - r->base_reg);
- }
+ for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+ r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
+ return r->set_reg(cpu, mem_buf, reg - r->base_reg);
}
}
return 0;
}
+static void gdb_register_feature(CPUState *cpu, int base_reg,
+ gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
+ const GDBFeature *feature)
+{
+ guint i = cpu->gdb_regs->len;
+ GDBRegisterState *s;
+
+ g_array_set_size(cpu->gdb_regs, i + 1);
+ s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ s->base_reg = base_reg;
+ s->get_reg = get_reg;
+ s->set_reg = set_reg;
+ s->feature = feature;
+}
+
+void gdb_init_cpu(CPUState *cpu)
+{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+ const GDBFeature *feature;
+
+ cpu->gdb_regs = g_array_new(false, false, sizeof(GDBRegisterState));
+
+ if (cc->gdb_core_xml_file) {
+ feature = gdb_find_static_feature(cc->gdb_core_xml_file);
+ gdb_register_feature(cpu, 0,
+ cc->gdb_read_register, cc->gdb_write_register,
+ feature);
+ }
+
+ cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
+}
+
void gdb_register_coprocessor(CPUState *cpu,
gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
const GDBFeature *feature, int g_pos)
{
GDBRegisterState *s;
guint i;
+ int base_reg = cpu->gdb_num_regs;
- if (cpu->gdb_regs) {
- for (i = 0; i < cpu->gdb_regs->len; i++) {
- /* Check for duplicates. */
- s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- if (s->feature == feature) {
- return;
- }
+ for (i = 0; i < cpu->gdb_regs->len; i++) {
+ /* Check for duplicates. */
+ s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ if (s->feature == feature) {
+ return;
}
- } else {
- cpu->gdb_regs = g_array_new(false, false, sizeof(GDBRegisterState));
- i = 0;
}
- g_array_set_size(cpu->gdb_regs, i + 1);
- s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
- s->base_reg = cpu->gdb_num_regs;
- s->get_reg = get_reg;
- s->set_reg = set_reg;
- s->feature = feature;
+ gdb_register_feature(cpu, base_reg, get_reg, set_reg, feature);
/* Add to end of list. */
cpu->gdb_num_regs += feature->num_regs;
if (g_pos) {
- if (g_pos != s->base_reg) {
+ if (g_pos != base_reg) {
error_report("Error: Bad gdb register numbering for '%s', "
- "expected %d got %d", feature->xml,
- g_pos, s->base_reg);
+ "expected %d got %d", feature->xml, g_pos, base_reg);
} else {
cpu->gdb_num_g_regs = cpu->gdb_num_regs;
}
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index bab8942c30..2a2a6eb3eb 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -27,6 +27,7 @@
#include "qemu/main-loop.h"
#include "exec/log.h"
#include "exec/cpu-common.h"
+#include "exec/gdbstub.h"
#include "qemu/error-report.h"
#include "qemu/qemu-print.h"
#include "sysemu/tcg.h"
@@ -223,11 +224,10 @@ static void cpu_common_unrealizefn(DeviceState *dev)
static void cpu_common_initfn(Object *obj)
{
CPUState *cpu = CPU(obj);
- CPUClass *cc = CPU_GET_CLASS(obj);
+ gdb_init_cpu(cpu);
cpu->cpu_index = UNASSIGNED_CPU_INDEX;
cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
- cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
/* user-mode doesn't have configurable SMP topology */
/* the default value is changed by qemu_init_vcpu() for system-mode */
cpu->nr_cores = 1;
@@ -247,6 +247,7 @@ static void cpu_common_finalize(Object *obj)
{
CPUState *cpu = CPU(obj);
+ g_array_free(cpu->gdb_regs, TRUE);
qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
qemu_mutex_destroy(&cpu->work_mutex);
}
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 11/18] gdbstub: Infer number of core registers from XML
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (9 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 10/18] gdbstub: Simplify XML lookup Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 12/18] hw/core/cpu: Remove gdb_get_dynamic_xml member Akihiko Odaki
` (7 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Eduardo Habkost,
Marcel Apfelbaum, Yanan Wang, Peter Maydell, Michael Rolnik,
Brian Cain, Song Gao, Laurent Vivier, Edgar E. Iglesias,
Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, Yoshinori Sato, Thomas Huth,
Richard Henderson, David Hildenbrand, Ilya Leoshkevich, qemu-arm,
qemu-riscv, qemu-s390x
GDBFeature has the num_regs member so use it where applicable to
remove magic numbers.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/hw/core/cpu.h | 3 ++-
target/s390x/cpu.h | 2 --
gdbstub/gdbstub.c | 5 ++++-
target/arm/cpu.c | 1 -
target/arm/cpu64.c | 1 -
target/avr/cpu.c | 1 -
target/hexagon/cpu.c | 1 -
target/i386/cpu.c | 2 --
target/loongarch/cpu.c | 2 --
target/m68k/cpu.c | 1 -
target/microblaze/cpu.c | 1 -
target/riscv/cpu.c | 1 -
target/rx/cpu.c | 1 -
target/s390x/cpu.c | 1 -
14 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 3968369554..11d4b5cd0c 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -127,7 +127,8 @@ struct SysemuCPUOps;
* @gdb_adjust_breakpoint: Callback for adjusting the address of a
* breakpoint. Used by AVR to handle a gdb mis-feature with
* its Harvard architecture split code and data.
- * @gdb_num_core_regs: Number of core registers accessible to GDB.
+ * @gdb_num_core_regs: Number of core registers accessible to GDB or 0 to infer
+ * from @gdb_core_xml_file.
* @gdb_core_xml_file: File name for core registers GDB XML description.
* @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
* before the insn which triggers a watchpoint rather than after it.
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7bea7075e1..83eafbe4b1 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -452,8 +452,6 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState *env, vaddr *pc,
#define S390_R13_REGNUM 15
#define S390_R14_REGNUM 16
#define S390_R15_REGNUM 17
-/* Total Core Registers. */
-#define S390_NUM_CORE_REGS 18
static inline void setcc(S390CPU *cpu, uint64_t cc)
{
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 332f9c6ffc..78d0bf5858 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -547,9 +547,12 @@ void gdb_init_cpu(CPUState *cpu)
gdb_register_feature(cpu, 0,
cc->gdb_read_register, cc->gdb_write_register,
feature);
+ cpu->gdb_num_regs = cpu->gdb_num_g_regs = feature->num_regs;
}
- cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
+ if (cc->gdb_num_core_regs) {
+ cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
+ }
}
void gdb_register_coprocessor(CPUState *cpu,
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 6c6c551573..6d9bf6a14e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2391,7 +2391,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
#ifndef CONFIG_USER_ONLY
cc->sysemu_ops = &arm_sysemu_ops;
#endif
- cc->gdb_num_core_regs = 26;
cc->gdb_arch_name = arm_gdb_arch_name;
cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml;
cc->gdb_stop_before_watchpoint = true;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 1cb9d5b81a..5c7a4a0bf7 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -792,7 +792,6 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
cc->gdb_read_register = aarch64_cpu_gdb_read_register;
cc->gdb_write_register = aarch64_cpu_gdb_write_register;
- cc->gdb_num_core_regs = 34;
cc->gdb_core_xml_file = "aarch64-core.xml";
cc->gdb_arch_name = aarch64_gdb_arch_name;
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 14d8b9d1f0..01adfb5089 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -244,7 +244,6 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
cc->gdb_read_register = avr_cpu_gdb_read_register;
cc->gdb_write_register = avr_cpu_gdb_write_register;
cc->gdb_adjust_breakpoint = avr_cpu_gdb_adjust_breakpoint;
- cc->gdb_num_core_regs = 35;
cc->gdb_core_xml_file = "avr-cpu.xml";
cc->tcg_ops = &avr_tcg_ops;
}
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 60d52e1e9d..7c1426f70c 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -385,7 +385,6 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
cc->get_pc = hexagon_cpu_get_pc;
cc->gdb_read_register = hexagon_gdb_read_register;
cc->gdb_write_register = hexagon_gdb_write_register;
- cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS;
cc->gdb_stop_before_watchpoint = true;
cc->gdb_core_xml_file = "hexagon-core.xml";
cc->disas_set_info = hexagon_cpu_disas_set_info;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3aab05ddad..fa63ce9311 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7968,10 +7968,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
cc->gdb_arch_name = x86_gdb_arch_name;
#ifdef TARGET_X86_64
cc->gdb_core_xml_file = "i386-64bit.xml";
- cc->gdb_num_core_regs = 66;
#else
cc->gdb_core_xml_file = "i386-32bit.xml";
- cc->gdb_num_core_regs = 50;
#endif
cc->disas_set_info = x86_disas_set_info;
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index ef1bf89dac..1b25920895 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -775,7 +775,6 @@ static void loongarch32_cpu_class_init(ObjectClass *c, void *data)
{
CPUClass *cc = CPU_CLASS(c);
- cc->gdb_num_core_regs = 35;
cc->gdb_core_xml_file = "loongarch-base32.xml";
cc->gdb_arch_name = loongarch32_gdb_arch_name;
}
@@ -789,7 +788,6 @@ static void loongarch64_cpu_class_init(ObjectClass *c, void *data)
{
CPUClass *cc = CPU_CLASS(c);
- cc->gdb_num_core_regs = 35;
cc->gdb_core_xml_file = "loongarch-base64.xml";
cc->gdb_arch_name = loongarch64_gdb_arch_name;
}
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 538d9473c2..5fdbe5602b 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -565,7 +565,6 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
#endif
cc->disas_set_info = m68k_cpu_disas_set_info;
- cc->gdb_num_core_regs = 18;
cc->tcg_ops = &m68k_tcg_ops;
}
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 1998f69828..9d3fbfe159 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -428,7 +428,6 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
cc->sysemu_ops = &mb_sysemu_ops;
#endif
device_class_set_props(dc, mb_properties);
- cc->gdb_num_core_regs = 32 + 25;
cc->gdb_core_xml_file = "microblaze-core.xml";
cc->disas_set_info = mb_disas_set_info;
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5e083b5798..3ee1e6ae03 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1586,7 +1586,6 @@ static void riscv_cpu_common_class_init(ObjectClass *c, void *data)
cc->get_pc = riscv_cpu_get_pc;
cc->gdb_read_register = riscv_cpu_gdb_read_register;
cc->gdb_write_register = riscv_cpu_gdb_write_register;
- cc->gdb_num_core_regs = 33;
cc->gdb_stop_before_watchpoint = true;
cc->disas_set_info = riscv_cpu_disas_set_info;
#ifndef CONFIG_USER_ONLY
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 4d0d3a0c8c..7b9e46d1bc 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -235,7 +235,6 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data)
cc->gdb_write_register = rx_cpu_gdb_write_register;
cc->disas_set_info = rx_cpu_disas_set_info;
- cc->gdb_num_core_regs = 26;
cc->gdb_core_xml_file = "rx-core.xml";
cc->tcg_ops = &rx_tcg_ops;
}
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 6093ab0a12..f49ee0bb46 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -346,7 +346,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
s390_cpu_class_init_sysemu(cc);
#endif
cc->disas_set_info = s390_cpu_disas_set_info;
- cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
cc->gdb_core_xml_file = "s390x-core64.xml";
cc->gdb_arch_name = s390_gdb_arch_name;
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 12/18] hw/core/cpu: Remove gdb_get_dynamic_xml member
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (10 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 11/18] gdbstub: Infer number of core registers from XML Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 15:38 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 13/18] gdbstub: Add members to identify registers to GDBFeature Akihiko Odaki
` (6 subsequent siblings)
18 siblings, 1 reply; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Eduardo Habkost,
Marcel Apfelbaum, Yanan Wang, Peter Maydell, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, Palmer Dabbelt,
Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei, qemu-arm,
qemu-ppc, qemu-riscv
This function is no longer used.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/hw/core/cpu.h | 4 ----
target/arm/cpu.h | 6 ------
target/ppc/cpu.h | 1 -
target/arm/cpu.c | 1 -
target/arm/gdbstub.c | 18 ------------------
target/ppc/cpu_init.c | 3 ---
target/ppc/gdbstub.c | 10 ----------
target/riscv/cpu.c | 14 --------------
8 files changed, 57 deletions(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 11d4b5cd0c..d2e70643f2 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -134,9 +134,6 @@ struct SysemuCPUOps;
* before the insn which triggers a watchpoint rather than after it.
* @gdb_arch_name: Optional callback that returns the architecture name known
* to GDB. The caller must free the returned string with g_free.
- * @gdb_get_dynamic_xml: Callback to return dynamically generated XML for the
- * gdb stub. Returns a pointer to the XML contents for the specified XML file
- * or NULL if the CPU doesn't have a dynamically generated content for it.
* @disas_set_info: Setup architecture specific components of disassembly info
* @adjust_watchpoint_address: Perform a target-specific adjustment to an
* address before attempting to match it against watchpoints.
@@ -167,7 +164,6 @@ struct CPUClass {
const char *gdb_core_xml_file;
const gchar * (*gdb_arch_name)(CPUState *cpu);
- const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3702ddaab8..0e4f1d7d0c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1137,12 +1137,6 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
int arm_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-/* Returns the dynamically generated XML for the gdb stub.
- * Returns a pointer to the XML contents for the specified XML file or NULL
- * if the XML name doesn't match the predefined one.
- */
-const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname);
-
int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
int cpuid, DumpState *s);
int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 848062bc9f..650e6ece70 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1384,7 +1384,6 @@ int ppc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
int ppc_cpu_gdb_write_register_apple(CPUState *cpu, uint8_t *buf, int reg);
#ifndef CONFIG_USER_ONLY
hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
-const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name);
#endif
int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
int cpuid, DumpState *s);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 6d9bf6a14e..e7fe11339b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2392,7 +2392,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
cc->sysemu_ops = &arm_sysemu_ops;
#endif
cc->gdb_arch_name = arm_gdb_arch_name;
- cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml;
cc->gdb_stop_before_watchpoint = true;
cc->disas_set_info = arm_disas_set_info;
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 8d771c5d46..3de9d23079 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -473,24 +473,6 @@ static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
#endif
#endif /* CONFIG_TCG */
-const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
-{
- ARMCPU *cpu = ARM_CPU(cs);
-
- if (strcmp(xmlname, "system-registers.xml") == 0) {
- return cpu->dyn_sysreg_feature.desc.xml;
- } else if (strcmp(xmlname, "sve-registers.xml") == 0) {
- return cpu->dyn_svereg_feature.desc.xml;
- } else if (strcmp(xmlname, "arm-m-system.xml") == 0) {
- return cpu->dyn_m_systemreg_feature.desc.xml;
-#ifndef CONFIG_USER_ONLY
- } else if (strcmp(xmlname, "arm-m-secext.xml") == 0) {
- return cpu->dyn_m_secextreg_feature.desc.xml;
-#endif
- }
- return NULL;
-}
-
void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
{
CPUState *cs = CPU(cpu);
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index a0178c3ce8..909d753b02 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7380,9 +7380,6 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
#endif
cc->gdb_num_core_regs = 71;
-#ifndef CONFIG_USER_ONLY
- cc->gdb_get_dynamic_xml = ppc_gdb_get_dynamic_xml;
-#endif
#ifdef USE_APPLE_GDB
cc->gdb_read_register = ppc_cpu_gdb_read_register_apple;
cc->gdb_write_register = ppc_cpu_gdb_write_register_apple;
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 8ca37b6bf9..f47878a67b 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -342,16 +342,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
gdb_feature_builder_end(&builder);
}
-
-const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
-{
- PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
-
- if (strcmp(xml_name, "power-spr.xml") == 0) {
- return pcc->gdb_spr.xml;
- }
- return NULL;
-}
#endif
#if !defined(CONFIG_USER_ONLY)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3ee1e6ae03..ace400db1c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1427,19 +1427,6 @@ static const gchar *riscv_gdb_arch_name(CPUState *cs)
}
}
-static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
-{
- RISCVCPU *cpu = RISCV_CPU(cs);
-
- if (strcmp(xmlname, "riscv-csr.xml") == 0) {
- return cpu->dyn_csr_feature.xml;
- } else if (strcmp(xmlname, "riscv-vector.xml") == 0) {
- return cpu->dyn_vreg_feature.xml;
- }
-
- return NULL;
-}
-
#ifndef CONFIG_USER_ONLY
static int64_t riscv_get_arch_id(CPUState *cs)
{
@@ -1593,7 +1580,6 @@ static void riscv_cpu_common_class_init(ObjectClass *c, void *data)
cc->get_arch_id = riscv_get_arch_id;
#endif
cc->gdb_arch_name = riscv_gdb_arch_name;
- cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
object_class_property_add(c, "mvendorid", "uint32", cpu_get_mvendorid,
cpu_set_mvendorid, NULL, NULL);
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 13/18] gdbstub: Add members to identify registers to GDBFeature
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (11 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 12/18] hw/core/cpu: Remove gdb_get_dynamic_xml member Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 14/18] gdbstub: Expose functions to read registers Akihiko Odaki
` (5 subsequent siblings)
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, John Snow,
Cleber Rosa, Palmer Dabbelt, Alistair Francis, Bin Meng,
Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, qemu-riscv
These members will be used to help plugins to identify registers.
The added members in instances of GDBFeature dynamically generated by
CPUs will be filled in later changes.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/exec/gdbstub.h | 3 +++
gdbstub/gdbstub.c | 12 +++++++++---
target/riscv/gdbstub.c | 4 +---
scripts/feature_to_c.py | 14 +++++++++++++-
4 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index b780b583de..346151d0f2 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -13,12 +13,15 @@
typedef struct GDBFeature {
const char *xmlname;
const char *xml;
+ const char *name;
+ const char * const *regs;
int num_regs;
} GDBFeature;
typedef struct GDBFeatureBuilder {
GDBFeature *feature;
GPtrArray *xml;
+ GPtrArray *regs;
int base_reg;
} GDBFeatureBuilder;
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 78d0bf5858..fade023559 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -419,9 +419,10 @@ void gdb_feature_builder_init(GDBFeatureBuilder *builder, GDBFeature *feature,
builder->feature = feature;
builder->xml = g_ptr_array_new();
g_ptr_array_add(builder->xml, header);
+ builder->regs = g_ptr_array_new();
builder->base_reg = base_reg;
feature->xmlname = xmlname;
- feature->num_regs = 0;
+ feature->name = name;
}
void gdb_feature_builder_append_tag(const GDBFeatureBuilder *builder,
@@ -440,10 +441,12 @@ void gdb_feature_builder_append_reg(const GDBFeatureBuilder *builder,
const char *type,
const char *group)
{
- if (builder->feature->num_regs < regnum) {
- builder->feature->num_regs = regnum;
+ if (builder->regs->len <= regnum) {
+ g_ptr_array_set_size(builder->regs, regnum + 1);
}
+ builder->regs->pdata[regnum] = (gpointer *)name;
+
if (group) {
gdb_feature_builder_append_tag(
builder,
@@ -469,6 +472,9 @@ void gdb_feature_builder_end(const GDBFeatureBuilder *builder)
}
g_ptr_array_free(builder->xml, TRUE);
+
+ builder->feature->num_regs = builder->regs->len;
+ builder->feature->regs = (void *)g_ptr_array_free(builder->regs, FALSE);
}
const GDBFeature *gdb_find_static_feature(const char *xmlname)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 5cb550761e..d96f5cf2b6 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -266,11 +266,9 @@ static GDBFeature *riscv_gen_dynamic_csr_feature(CPUState *cs, int base_reg)
}
predicate = csr_ops[i].predicate;
if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
- g_autofree char *dynamic_name = NULL;
name = csr_ops[i].name;
if (!name) {
- dynamic_name = g_strdup_printf("csr%03x", i);
- name = dynamic_name;
+ name = g_strdup_printf("csr%03x", i);
}
gdb_feature_builder_append_reg(&builder, name, bitsize, i,
diff --git a/scripts/feature_to_c.py b/scripts/feature_to_c.py
index e04d6b2df7..807af0e685 100755
--- a/scripts/feature_to_c.py
+++ b/scripts/feature_to_c.py
@@ -50,7 +50,9 @@ def writeliteral(indent, bytes):
sys.stderr.write(f'unexpected start tag: {element.tag}\n')
exit(1)
+ feature_name = element.attrib['name']
regnum = 0
+ regnames = []
regnums = []
tags = ['feature']
for event, element in events:
@@ -67,6 +69,7 @@ def writeliteral(indent, bytes):
if 'regnum' in element.attrib:
regnum = int(element.attrib['regnum'])
+ regnames.append(element.attrib['name'])
regnums.append(regnum)
regnum += 1
@@ -85,6 +88,15 @@ def writeliteral(indent, bytes):
writeliteral(8, bytes(os.path.basename(input), 'utf-8'))
sys.stdout.write(',\n')
writeliteral(8, read)
- sys.stdout.write(f',\n {num_regs},\n }},\n')
+ sys.stdout.write(',\n')
+ writeliteral(8, bytes(feature_name, 'utf-8'))
+ sys.stdout.write(',\n (const char * const []) {\n')
+
+ for index, regname in enumerate(regnames):
+ sys.stdout.write(f' [{regnums[index] - base_reg}] =\n')
+ writeliteral(16, bytes(regname, 'utf-8'))
+ sys.stdout.write(',\n')
+
+ sys.stdout.write(f' }},\n {num_regs},\n }},\n')
sys.stdout.write(' { NULL }\n};\n')
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 14/18] gdbstub: Expose functions to read registers
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (12 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 13/18] gdbstub: Add members to identify registers to GDBFeature Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 15:41 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 15/18] cpu: Call plugin hooks only when ready Akihiko Odaki
` (4 subsequent siblings)
18 siblings, 1 reply; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki
gdb_find_feature() and gdb_find_feature_register() find registers.
gdb_read_register() actually reads registers.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/exec/gdbstub.h | 5 +++++
gdbstub/gdbstub.c | 31 ++++++++++++++++++++++++++++++-
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 346151d0f2..308b5c266a 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -74,6 +74,11 @@ void gdb_feature_builder_end(const GDBFeatureBuilder *builder);
const GDBFeature *gdb_find_static_feature(const char *xmlname);
+int gdb_find_feature(CPUState *cpu, const char *name);
+int gdb_find_feature_register(CPUState *cpu, int feature, const char *name);
+
+int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
+
void gdb_set_stop_cpu(CPUState *cpu);
/* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index fade023559..0b64b93960 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -490,7 +490,36 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname)
g_assert_not_reached();
}
-static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
+int gdb_find_feature(CPUState *cpu, const char *name)
+{
+ GDBRegisterState *r;
+
+ for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+ r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
+ if (!strcmp(name, r->feature->name)) {
+ return i;
+ }
+ }
+
+ return -1;
+}
+
+int gdb_find_feature_register(CPUState *cpu, int feature, const char *name)
+{
+ GDBRegisterState *r;
+
+ r = &g_array_index(cpu->gdb_regs, GDBRegisterState, feature);
+
+ for (int i = 0; i < r->feature->num_regs; i++) {
+ if (r->feature->regs[i] && !strcmp(name, r->feature->regs[i])) {
+ return r->base_reg + i;
+ }
+ }
+
+ return -1;
+}
+
+int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
GDBRegisterState *r;
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 15/18] cpu: Call plugin hooks only when ready
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (13 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 14/18] gdbstub: Expose functions to read registers Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 15:58 ` Alex Bennée
2023-10-24 16:41 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 16/18] plugins: Use different helpers when reading registers Akihiko Odaki
` (3 subsequent siblings)
18 siblings, 2 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Richard Henderson,
Paolo Bonzini
The initialization and exit hooks will not affect the state of vCPU
outside TCG context, but they may depend on the state of vCPU.
Therefore, it's better to call plugin hooks after the vCPU state is
fully initialized and before it gets uninitialized.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
cpu-target.c | 11 -----------
hw/core/cpu-common.c | 10 ++++++++++
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/cpu-target.c b/cpu-target.c
index 79363ae370..00cd7f4d69 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -42,7 +42,6 @@
#include "hw/core/accel-cpu.h"
#include "trace/trace-root.h"
#include "qemu/accel.h"
-#include "qemu/plugin.h"
uintptr_t qemu_host_page_size;
intptr_t qemu_host_page_mask;
@@ -143,11 +142,6 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
/* Wait until cpu initialization complete before exposing cpu. */
cpu_list_add(cpu);
- /* Plugin initialization must wait until cpu_index assigned. */
- if (tcg_enabled()) {
- qemu_plugin_vcpu_init_hook(cpu);
- }
-
#ifdef CONFIG_USER_ONLY
assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
qdev_get_vmsd(DEVICE(cpu))->unmigratable);
@@ -174,11 +168,6 @@ void cpu_exec_unrealizefn(CPUState *cpu)
}
#endif
- /* Call the plugin hook before clearing cpu->cpu_index in cpu_list_remove */
- if (tcg_enabled()) {
- qemu_plugin_vcpu_exit_hook(cpu);
- }
-
cpu_list_remove(cpu);
/*
* Now that the vCPU has been removed from the RCU list, we can call
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 2a2a6eb3eb..409397e2b5 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -210,6 +210,11 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
cpu_resume(cpu);
}
+ /* Plugin initialization must wait until the cpu is fully realized. */
+ if (tcg_enabled()) {
+ qemu_plugin_vcpu_init_hook(cpu);
+ }
+
/* NOTE: latest generic point where the cpu is fully realized */
}
@@ -217,6 +222,11 @@ static void cpu_common_unrealizefn(DeviceState *dev)
{
CPUState *cpu = CPU(dev);
+ /* Call the plugin hook before clearing the cpu is fully unrealized */
+ if (tcg_enabled()) {
+ qemu_plugin_vcpu_exit_hook(cpu);
+ }
+
/* NOTE: latest generic point before the cpu is fully unrealized */
cpu_exec_unrealizefn(cpu);
}
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 16/18] plugins: Use different helpers when reading registers
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (14 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 15/18] cpu: Call plugin hooks only when ready Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 16:48 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 17/18] plugins: Allow to read registers Akihiko Odaki
` (2 subsequent siblings)
18 siblings, 1 reply; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Richard Henderson,
Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, Yanan Wang,
Alexandre Iooss, Mahmoud Mandour
This avoids optimizations incompatible when reading registers.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
accel/tcg/plugin-helpers.h | 3 ++-
include/exec/plugin-gen.h | 4 ++--
include/hw/core/cpu.h | 4 ++--
include/qemu/plugin.h | 3 +++
plugins/plugin.h | 5 +++--
accel/tcg/plugin-gen.c | 41 ++++++++++++++++++++++++++++----------
accel/tcg/translator.c | 2 +-
plugins/api.c | 14 +++++++++++--
plugins/core.c | 28 ++++++++++++++++----------
9 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h
index 8e685e0654..11796436f3 100644
--- a/accel/tcg/plugin-helpers.h
+++ b/accel/tcg/plugin-helpers.h
@@ -1,4 +1,5 @@
#ifdef CONFIG_PLUGIN
-DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb, TCG_CALL_NO_RWG | TCG_CALL_PLUGIN, void, i32, ptr)
+DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb_no_wg, TCG_CALL_NO_WG | TCG_CALL_PLUGIN, void, i32, ptr)
+DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb_no_rwg, TCG_CALL_NO_RWG | TCG_CALL_PLUGIN, void, i32, ptr)
DEF_HELPER_FLAGS_4(plugin_vcpu_mem_cb, TCG_CALL_NO_RWG | TCG_CALL_PLUGIN, void, i32, i32, i64, ptr)
#endif
diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
index c4552b5061..b964e1eb5c 100644
--- a/include/exec/plugin-gen.h
+++ b/include/exec/plugin-gen.h
@@ -22,7 +22,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const struct DisasContextBase *db,
bool supress);
void plugin_gen_tb_end(CPUState *cpu, size_t num_insns);
void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db);
-void plugin_gen_insn_end(void);
+void plugin_gen_insn_end(CPUState *cpu);
void plugin_gen_disable_mem_helpers(void);
void plugin_gen_empty_mem_callback(TCGv_i64 addr, uint32_t info);
@@ -39,7 +39,7 @@ static inline
void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db)
{ }
-static inline void plugin_gen_insn_end(void)
+static inline void plugin_gen_insn_end(CPUState *cpu)
{ }
static inline void plugin_gen_tb_end(CPUState *cpu, size_t num_insns)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index d2e70643f2..dbdca8b105 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -437,7 +437,7 @@ struct qemu_work_item;
* @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
* to @trace_dstate).
* @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
- * @plugin_mask: Plugin event bitmap. Modified only via async work.
+ * @plugin_flags: Plugin flags. Modified only via async work.
* @ignore_memory_transaction_failures: Cached copy of the MachineState
* flag of the same name: allows the board to suppress calling of the
* CPU do_transaction_failed hook function.
@@ -529,7 +529,7 @@ struct CPUState {
/* Use by accel-block: CPU is executing an ioctl() */
QemuLockCnt in_ioctl_lock;
- DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
+ unsigned long plugin_flags;
#ifdef CONFIG_PLUGIN
GArray *plugin_mem_cbs;
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 7fdc3a4849..a534b9127b 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -16,6 +16,9 @@
#include "exec/memopidx.h"
#include "hw/core/cpu.h"
+#define QEMU_PLUGIN_FLAG_TB_CB_READ QEMU_PLUGIN_EV_MAX
+#define QEMU_PLUGIN_FLAG_INSN_CB_READ (QEMU_PLUGIN_EV_MAX + 1)
+
/*
* Option parsing/processing.
* Note that we can load an arbitrary number of plugins.
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 5eb2fdbc85..ba0417194f 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -16,6 +16,7 @@
#include "qemu/qht.h"
#define QEMU_PLUGIN_MIN_VERSION 0
+#define QEMU_PLUGIN_FLAG_INSIN_CB_READ QEMU_PLUGIN_EV_MAX
/* global state */
struct qemu_plugin_state {
@@ -31,7 +32,7 @@ struct qemu_plugin_state {
* but with the HT we avoid adding a field to CPUState.
*/
GHashTable *cpu_ht;
- DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
+ unsigned long flags;
/*
* @lock protects the struct as well as ctx->uninstalling.
* The lock must be acquired by all API ops.
@@ -86,7 +87,7 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum qemu_plugin_event ev,
void
plugin_register_dyn_cb__udata(GArray **arr,
qemu_plugin_vcpu_udata_cb_t cb,
- enum qemu_plugin_cb_flags flags, void *udata);
+ unsigned int flags, void *udata);
void plugin_register_vcpu_mem_cb(GArray **arr,
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 78b331b251..3bddd4d3c5 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -90,7 +90,10 @@ enum plugin_gen_cb {
* These helpers are stubs that get dynamically switched out for calls
* direct to the plugin if they are subscribed to.
*/
-void HELPER(plugin_vcpu_udata_cb)(uint32_t cpu_index, void *udata)
+void HELPER(plugin_vcpu_udata_cb_no_wg)(uint32_t cpu_index, void *udata)
+{ }
+
+void HELPER(plugin_vcpu_udata_cb_no_rwg)(uint32_t cpu_index, void *udata)
{ }
void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
@@ -98,7 +101,7 @@ void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
void *userdata)
{ }
-static void gen_empty_udata_cb(void)
+static void gen_empty_udata_cb(void (*gen_helper)(TCGv_i32, TCGv_ptr))
{
TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
TCGv_ptr udata = tcg_temp_ebb_new_ptr();
@@ -106,12 +109,22 @@ static void gen_empty_udata_cb(void)
tcg_gen_movi_ptr(udata, 0);
tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
- gen_helper_plugin_vcpu_udata_cb(cpu_index, udata);
+ gen_helper(cpu_index, udata);
tcg_temp_free_ptr(udata);
tcg_temp_free_i32(cpu_index);
}
+static void gen_empty_udata_cb_no_wg(void)
+{
+ gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_wg);
+}
+
+static void gen_empty_udata_cb_no_rwg(void)
+{
+ gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_rwg);
+}
+
/*
* For now we only support addi_i64.
* When we support more ops, we can generate one empty inline cb for each.
@@ -176,7 +189,7 @@ static void gen_wrapped(enum plugin_gen_from from,
tcg_gen_plugin_cb_end();
}
-static void plugin_gen_empty_callback(enum plugin_gen_from from)
+static void plugin_gen_empty_callback(CPUState *cpu, enum plugin_gen_from from)
{
switch (from) {
case PLUGIN_GEN_AFTER_INSN:
@@ -190,9 +203,15 @@ static void plugin_gen_empty_callback(enum plugin_gen_from from)
*/
gen_wrapped(from, PLUGIN_GEN_ENABLE_MEM_HELPER,
gen_empty_mem_helper);
- /* fall through */
+ gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
+ cpu->plugin_flags & BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) ?
+ gen_empty_udata_cb_no_wg : gen_empty_udata_cb_no_rwg);
+ gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
+ break;
case PLUGIN_GEN_FROM_TB:
- gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb);
+ gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
+ cpu->plugin_flags & BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) ?
+ gen_empty_udata_cb_no_wg : gen_empty_udata_cb_no_rwg);
gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
break;
default:
@@ -796,7 +815,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
{
bool ret = false;
- if (test_bit(QEMU_PLUGIN_EV_VCPU_TB_TRANS, cpu->plugin_mask)) {
+ if (cpu->plugin_flags & BIT(QEMU_PLUGIN_EV_VCPU_TB_TRANS)) {
struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
int i;
@@ -817,7 +836,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
ptb->mem_only = mem_only;
ptb->mem_helper = false;
- plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
+ plugin_gen_empty_callback(cpu, PLUGIN_GEN_FROM_TB);
}
tcg_ctx->plugin_insn = NULL;
@@ -832,7 +851,7 @@ void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
pinsn = qemu_plugin_tb_insn_get(ptb, db->pc_next);
tcg_ctx->plugin_insn = pinsn;
- plugin_gen_empty_callback(PLUGIN_GEN_FROM_INSN);
+ plugin_gen_empty_callback(cpu, PLUGIN_GEN_FROM_INSN);
/*
* Detect page crossing to get the new host address.
@@ -852,9 +871,9 @@ void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
}
}
-void plugin_gen_insn_end(void)
+void plugin_gen_insn_end(CPUState *cpu)
{
- plugin_gen_empty_callback(PLUGIN_GEN_AFTER_INSN);
+ plugin_gen_empty_callback(cpu, PLUGIN_GEN_AFTER_INSN);
}
/*
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 575b9812ad..bec58dd93f 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -189,7 +189,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
* to accurately track instrumented helpers that might access memory.
*/
if (plugin_enabled) {
- plugin_gen_insn_end();
+ plugin_gen_insn_end(cpu);
}
/* Stop translation if translate_insn so indicated. */
diff --git a/plugins/api.c b/plugins/api.c
index 5521b0ad36..326e37cb73 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -89,8 +89,13 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
void *udata)
{
if (!tb->mem_only) {
+ bool read = flags == QEMU_PLUGIN_CB_R_REGS ||
+ flags == QEMU_PLUGIN_CB_RW_REGS;
+
plugin_register_dyn_cb__udata(&tb->cbs[PLUGIN_CB_REGULAR],
- cb, flags, udata);
+ cb,
+ read ? BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) : 0,
+ udata);
}
}
@@ -109,8 +114,13 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
void *udata)
{
if (!insn->mem_only) {
+ bool read = flags == QEMU_PLUGIN_CB_R_REGS ||
+ flags == QEMU_PLUGIN_CB_RW_REGS;
+
plugin_register_dyn_cb__udata(&insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_REGULAR],
- cb, flags, udata);
+ cb,
+ read ? BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) : 0,
+ udata);
}
}
diff --git a/plugins/core.c b/plugins/core.c
index fcd33a2bff..f461e84473 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -55,19 +55,19 @@ struct qemu_plugin_ctx *plugin_id_to_ctx_locked(qemu_plugin_id_t id)
static void plugin_cpu_update__async(CPUState *cpu, run_on_cpu_data data)
{
- bitmap_copy(cpu->plugin_mask, &data.host_ulong, QEMU_PLUGIN_EV_MAX);
+ cpu->plugin_flags = data.host_ulong;
tcg_flush_jmp_cache(cpu);
}
static void plugin_cpu_update__locked(gpointer k, gpointer v, gpointer udata)
{
CPUState *cpu = container_of(k, CPUState, cpu_index);
- run_on_cpu_data mask = RUN_ON_CPU_HOST_ULONG(*plugin.mask);
+ run_on_cpu_data flags = RUN_ON_CPU_HOST_ULONG(plugin.flags);
if (DEVICE(cpu)->realized) {
- async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
+ async_run_on_cpu(cpu, plugin_cpu_update__async, flags);
} else {
- plugin_cpu_update__async(cpu, mask);
+ plugin_cpu_update__async(cpu, flags);
}
}
@@ -83,7 +83,7 @@ void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,
g_free(cb);
ctx->callbacks[ev] = NULL;
if (QLIST_EMPTY_RCU(&plugin.cb_lists[ev])) {
- clear_bit(ev, plugin.mask);
+ plugin.flags &= ~BIT(ev);
g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked, NULL);
}
}
@@ -186,8 +186,8 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
cb->udata = udata;
ctx->callbacks[ev] = cb;
QLIST_INSERT_HEAD_RCU(&plugin.cb_lists[ev], cb, entry);
- if (!test_bit(ev, plugin.mask)) {
- set_bit(ev, plugin.mask);
+ if (!(plugin.flags & BIT(ev))) {
+ plugin.flags |= BIT(ev);
g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked,
NULL);
}
@@ -296,15 +296,20 @@ void plugin_register_inline_op(GArray **arr,
void plugin_register_dyn_cb__udata(GArray **arr,
qemu_plugin_vcpu_udata_cb_t cb,
- enum qemu_plugin_cb_flags flags,
+ unsigned int flags,
void *udata)
{
struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
dyn_cb->userp = udata;
- /* Note flags are discarded as unused. */
dyn_cb->f.vcpu_udata = cb;
dyn_cb->type = PLUGIN_CB_REGULAR;
+
+ if (flags) {
+ QEMU_LOCK_GUARD(&plugin.lock);
+ plugin.flags |= flags;
+ g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked, NULL);
+ }
}
void plugin_register_vcpu_mem_cb(GArray **arr,
@@ -357,7 +362,7 @@ qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
struct qemu_plugin_cb *cb, *next;
enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL;
- if (!test_bit(ev, cpu->plugin_mask)) {
+ if (!(cpu->plugin_flags & BIT(ev))) {
return;
}
@@ -379,7 +384,7 @@ void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
struct qemu_plugin_cb *cb, *next;
enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL_RET;
- if (!test_bit(ev, cpu->plugin_mask)) {
+ if (!(cpu->plugin_flags & BIT(ev))) {
return;
}
@@ -428,6 +433,7 @@ void qemu_plugin_flush_cb(void)
{
qht_iter_remove(&plugin.dyn_cb_arr_ht, free_dyn_cb_arr, NULL);
qht_reset(&plugin.dyn_cb_arr_ht);
+ plugin.flags &= ~BIT(QEMU_PLUGIN_FLAG_INSIN_CB_READ);
plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
}
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 17/18] plugins: Allow to read registers
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (15 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 16/18] plugins: Use different helpers when reading registers Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 18/18] contrib/plugins: Allow to log registers Akihiko Odaki
2023-10-24 17:08 ` [PATCH v14 00/18] plugins: Allow to read registers Alex Bennée
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Alexandre Iooss,
Mahmoud Mandour
It is based on GDB protocol to ensure interface stability.
The timing of the vcpu init hook is also changed so that the hook will
get called after GDB features are initialized.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/qemu/qemu-plugin.h | 50 ++++++++++++++++++++++++++++++++++--
plugins/api.c | 20 +++++++++++++++
plugins/qemu-plugins.symbols | 3 +++
3 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 50a9957279..c0ce700258 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,49 @@ uint64_t qemu_plugin_end_code(void);
*/
uint64_t qemu_plugin_entry_code(void);
+/**
+ * qemu_plugin_find_register_file() - find register file
+ *
+ * @vcpu_index: the index of the vcpu context
+ * @name: the name of the register file.
+ *
+ * Returns the identifier of the register file if it was found, and a negative
+ * value otherwise.
+ *
+ * The names of register files are identical with names of GDB's standard
+ * target features with some extensions. For details, see:
+ * https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html
+ */
+int qemu_plugin_find_register_file(unsigned int vcpu_index, const char *name);
+
+/**
+ * qemu_plugin_find_register() - find register
+ *
+ * @vcpu_index: the index of the vcpu context
+ * @file: the register file identifier determined with
+ * qemu_plugin_find_register_file().
+ * @name: the name of the register.
+ *
+ * The names of register are identical with names used in GDB's standard
+ * target features with some extensions. For details, see:
+ * https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html
+ */
+int qemu_plugin_find_register(unsigned int vcpu_index, int file,
+ const char *name);
+
+/**
+ * qemu_plugin_read_register() - read register
+ *
+ * @buf: the byte array to append the read register content to.
+ * @reg: the register identifier determined with
+ * qemu_plugin_find_register().
+ *
+ * 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.
+ */
+int qemu_plugin_read_register(GByteArray *buf, int reg);
+
#endif /* QEMU_QEMU_PLUGIN_H */
diff --git a/plugins/api.c b/plugins/api.c
index 326e37cb73..89144a2dec 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -35,10 +35,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"
@@ -437,3 +439,21 @@ uint64_t qemu_plugin_entry_code(void)
#endif
return entry;
}
+
+int qemu_plugin_find_register_file(unsigned int vcpu_index, const char *name)
+{
+ QEMU_IOTHREAD_LOCK_GUARD();
+ return gdb_find_feature(qemu_get_cpu(vcpu_index), name);
+}
+
+int qemu_plugin_find_register(unsigned int vcpu_index, int file,
+ const char *name)
+{
+ QEMU_IOTHREAD_LOCK_GUARD();
+ return gdb_find_feature_register(qemu_get_cpu(vcpu_index), file, name);
+}
+
+int qemu_plugin_read_register(GByteArray *buf, int reg)
+{
+ return gdb_read_register(current_cpu, buf, reg);
+}
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 71f6c90549..977f1fcfcb 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -42,4 +42,7 @@
qemu_plugin_tb_vaddr;
qemu_plugin_uninstall;
qemu_plugin_vcpu_for_each;
+ qemu_plugin_find_register_file;
+ qemu_plugin_find_register;
+ qemu_plugin_read_register;
};
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v14 18/18] contrib/plugins: Allow to log registers
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (16 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 17/18] plugins: Allow to read registers Akihiko Odaki
@ 2023-10-19 10:26 ` Akihiko Odaki
2023-10-24 17:08 ` [PATCH v14 00/18] plugins: Allow to read registers Alex Bennée
18 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-19 10:26 UTC (permalink / raw)
Cc: Alex Bennée, Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Akihiko Odaki, Alexandre Iooss,
Mahmoud Mandour, Richard Henderson, Paolo Bonzini
This demonstrates how a register can be read from a plugin.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
docs/devel/tcg-plugins.rst | 10 +++-
contrib/plugins/execlog.c | 120 +++++++++++++++++++++++++++----------
2 files changed, 97 insertions(+), 33 deletions(-)
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 81dcd43a61..c9f8b27590 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -497,6 +497,15 @@ arguments if required::
$ qemu-system-arm $(QEMU_ARGS) \
-plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin
+This plugin can also dump a specified register. The specification of register
+follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__.
+
+Specify the name of the feature that contains the register and the name of the
+register with ``rfile`` and ``reg`` options, respectively::
+
+ $ qemu-system-arm $(QEMU_ARGS) \
+ -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin
+
- contrib/plugins/cache.c
Cache modelling plugin that measures the performance of a given L1 cache
@@ -583,4 +592,3 @@ The following API is generated from the inline documentation in
include the full kernel-doc annotations.
.. kernel-doc:: include/qemu/qemu-plugin.h
-
diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index 82dc2f584e..f3e714c888 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -15,27 +15,43 @@
#include <qemu-plugin.h>
+typedef struct CPU {
+ /* Store last executed instruction on each vCPU as a GString */
+ GString *last_exec;
+ GByteArray *reg_history[2];
+
+ int reg;
+} CPU;
+
QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
-/* Store last executed instruction on each vCPU as a GString */
-static GPtrArray *last_exec;
+static CPU *cpus;
+static int num_cpus;
static GRWLock expand_array_lock;
static GPtrArray *imatches;
static GArray *amatches;
+static char *rfile_name;
+static char *reg_name;
+
/*
- * Expand last_exec array.
+ * Expand cpu array.
*
* As we could have multiple threads trying to do this we need to
* serialise the expansion under a lock.
*/
-static void expand_last_exec(int cpu_index)
+static void expand_cpu(int cpu_index)
{
g_rw_lock_writer_lock(&expand_array_lock);
- while (cpu_index >= last_exec->len) {
- GString *s = g_string_new(NULL);
- g_ptr_array_add(last_exec, s);
+ if (cpu_index >= num_cpus) {
+ cpus = g_realloc_n(cpus, cpu_index + 1, sizeof(*cpus));
+ while (cpu_index >= num_cpus) {
+ cpus[num_cpus].last_exec = g_string_new(NULL);
+ cpus[num_cpus].reg_history[0] = g_byte_array_new();
+ cpus[num_cpus].reg_history[1] = g_byte_array_new();
+ num_cpus++;
+ }
}
g_rw_lock_writer_unlock(&expand_array_lock);
}
@@ -50,8 +66,8 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info,
/* Find vCPU in array */
g_rw_lock_reader_lock(&expand_array_lock);
- g_assert(cpu_index < last_exec->len);
- s = g_ptr_array_index(last_exec, cpu_index);
+ g_assert(cpu_index < num_cpus);
+ s = cpus[cpu_index].last_exec;
g_rw_lock_reader_unlock(&expand_array_lock);
/* Indicate type of memory access */
@@ -77,28 +93,42 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info,
*/
static void vcpu_insn_exec(unsigned int cpu_index, void *udata)
{
- GString *s;
+ int n;
+ int i;
- /* Find or create vCPU in array */
g_rw_lock_reader_lock(&expand_array_lock);
- if (cpu_index >= last_exec->len) {
- g_rw_lock_reader_unlock(&expand_array_lock);
- expand_last_exec(cpu_index);
- g_rw_lock_reader_lock(&expand_array_lock);
- }
- s = g_ptr_array_index(last_exec, cpu_index);
- g_rw_lock_reader_unlock(&expand_array_lock);
/* Print previous instruction in cache */
- if (s->len) {
- qemu_plugin_outs(s->str);
+ if (cpus[cpu_index].last_exec->len) {
+ if (cpus[cpu_index].reg >= 0) {
+ GByteArray *current = cpus[cpu_index].reg_history[0];
+ GByteArray *last = cpus[cpu_index].reg_history[1];
+
+ g_byte_array_set_size(current, 0);
+ n = qemu_plugin_read_register(current, cpus[cpu_index].reg);
+
+ if (n != last->len || memcmp(current->data, last->data, n)) {
+ g_string_append(cpus[cpu_index].last_exec, ", reg,");
+ for (i = 0; i < n; i++) {
+ g_string_append_printf(cpus[cpu_index].last_exec, " %02x",
+ current->data[i]);
+ }
+ }
+
+ cpus[cpu_index].reg_history[0] = last;
+ cpus[cpu_index].reg_history[1] = current;
+ }
+
+ qemu_plugin_outs(cpus[cpu_index].last_exec->str);
qemu_plugin_outs("\n");
}
/* Store new instruction in cache */
/* vcpu_mem will add memory access information to last_exec */
- g_string_printf(s, "%u, ", cpu_index);
- g_string_append(s, (char *)udata);
+ g_string_printf(cpus[cpu_index].last_exec, "%u, ", cpu_index);
+ g_string_append(cpus[cpu_index].last_exec, (char *)udata);
+
+ g_rw_lock_reader_unlock(&expand_array_lock);
}
/**
@@ -167,8 +197,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
QEMU_PLUGIN_MEM_RW, NULL);
/* Register callback on instruction */
- qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec,
- QEMU_PLUGIN_CB_NO_REGS, output);
+ qemu_plugin_register_vcpu_insn_exec_cb(
+ insn, vcpu_insn_exec,
+ rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS,
+ output);
/* reset skip */
skip = (imatches || amatches);
@@ -177,17 +209,33 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
}
}
+static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
+{
+ int reg = -1;
+
+ expand_cpu(vcpu_index);
+
+ if (rfile_name) {
+ int rfile = qemu_plugin_find_register_file(vcpu_index, rfile_name);
+ if (rfile >= 0) {
+ reg = qemu_plugin_find_register(vcpu_index, rfile, reg_name);
+ }
+ }
+
+ g_rw_lock_writer_lock(&expand_array_lock);
+ cpus[vcpu_index].reg = reg;
+ g_rw_lock_writer_unlock(&expand_array_lock);
+}
+
/**
* On plugin exit, print last instruction in cache
*/
static void plugin_exit(qemu_plugin_id_t id, void *p)
{
guint i;
- GString *s;
- for (i = 0; i < last_exec->len; i++) {
- s = g_ptr_array_index(last_exec, i);
- if (s->str) {
- qemu_plugin_outs(s->str);
+ for (i = 0; i < num_cpus; i++) {
+ if (cpus[i].last_exec->str) {
+ qemu_plugin_outs(cpus[i].last_exec->str);
qemu_plugin_outs("\n");
}
}
@@ -224,9 +272,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
* we don't know the size before emulation.
*/
if (info->system_emulation) {
- last_exec = g_ptr_array_sized_new(info->system.max_vcpus);
- } else {
- last_exec = g_ptr_array_new();
+ cpus = g_new(CPU, info->system.max_vcpus);
}
for (int i = 0; i < argc; i++) {
@@ -236,13 +282,23 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
parse_insn_match(tokens[1]);
} else if (g_strcmp0(tokens[0], "afilter") == 0) {
parse_vaddr_match(tokens[1]);
+ } else if (g_strcmp0(tokens[0], "rfile") == 0) {
+ rfile_name = g_strdup(tokens[1]);
+ } else if (g_strcmp0(tokens[0], "reg") == 0) {
+ reg_name = g_strdup(tokens[1]);
} else {
fprintf(stderr, "option parsing failed: %s\n", opt);
return -1;
}
}
+ if ((!rfile_name) != (!reg_name)) {
+ fputs("file and reg need to be set at the same time\n", stderr);
+ return -1;
+ }
+
/* Register translation block and exit callbacks */
+ qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v14 09/18] gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb
2023-10-19 10:26 ` [PATCH v14 09/18] gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb Akihiko Odaki
@ 2023-10-24 14:20 ` Alex Bennée
0 siblings, 0 replies; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 14:20 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Peter Maydell, Brian Cain, Song Gao,
Laurent Vivier, Edgar E. Iglesias, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, Palmer Dabbelt,
Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei,
Richard Henderson, David Hildenbrand, Ilya Leoshkevich,
Thomas Huth, qemu-arm, qemu-ppc, qemu-riscv, qemu-s390x
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> Align the parameters of gdb_get_reg_cb and gdb_set_reg_cb with the
> gdb_read_register and gdb_write_register members of CPUClass to allow
> to unify the logic to access registers of the core and coprocessors
> in the future.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 10/18] gdbstub: Simplify XML lookup
2023-10-19 10:26 ` [PATCH v14 10/18] gdbstub: Simplify XML lookup Akihiko Odaki
@ 2023-10-24 14:25 ` Alex Bennée
0 siblings, 0 replies; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 14:25 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> Now we know all instances of GDBFeature that is used in CPU so we can
> traverse them to find XML. This removes the need for a CPU-specific
> lookup function for dynamic XMLs.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
<snip>
> }
>
> +static void gdb_register_feature(CPUState *cpu, int base_reg,
> + gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> + const GDBFeature *feature)
> +{
> + guint i = cpu->gdb_regs->len;
> + GDBRegisterState *s;
> +
> + g_array_set_size(cpu->gdb_regs, i + 1);
> + s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> + s->base_reg = base_reg;
> + s->get_reg = get_reg;
> + s->set_reg = set_reg;
> + s->feature = feature;
Why not just:
GDBRegisterState s = { base_reg, get_reg, set_reg, feature };
g_array_append_val(cpu->gdb_regs, s);
?
Otherwise:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 12/18] hw/core/cpu: Remove gdb_get_dynamic_xml member
2023-10-19 10:26 ` [PATCH v14 12/18] hw/core/cpu: Remove gdb_get_dynamic_xml member Akihiko Odaki
@ 2023-10-24 15:38 ` Alex Bennée
0 siblings, 0 replies; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 15:38 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Eduardo Habkost, Marcel Apfelbaum,
Yanan Wang, Peter Maydell, Nicholas Piggin,
Daniel Henrique Barboza, Cédric Le Goater, Palmer Dabbelt,
Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei, qemu-arm,
qemu-ppc, qemu-riscv
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> This function is no longer used.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 14/18] gdbstub: Expose functions to read registers
2023-10-19 10:26 ` [PATCH v14 14/18] gdbstub: Expose functions to read registers Akihiko Odaki
@ 2023-10-24 15:41 ` Alex Bennée
0 siblings, 0 replies; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 15:41 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> gdb_find_feature() and gdb_find_feature_register() find registers.
> gdb_read_register() actually reads registers.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> include/exec/gdbstub.h | 5 +++++
> gdbstub/gdbstub.c | 31 ++++++++++++++++++++++++++++++-
> 2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 346151d0f2..308b5c266a 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -74,6 +74,11 @@ void gdb_feature_builder_end(const GDBFeatureBuilder *builder);
>
> const GDBFeature *gdb_find_static_feature(const char *xmlname);
>
> +int gdb_find_feature(CPUState *cpu, const char *name);
> +int gdb_find_feature_register(CPUState *cpu, int feature, const char *name);
> +
> +int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
> +
Can we try to add kdoc comments to the new external facing APIs please.
I should have mentioned that for previous cases as well.
> void gdb_set_stop_cpu(CPUState *cpu);
>
> /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index fade023559..0b64b93960 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -490,7 +490,36 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname)
> g_assert_not_reached();
> }
>
> -static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
> +int gdb_find_feature(CPUState *cpu, const char *name)
> +{
> + GDBRegisterState *r;
> +
> + for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> + r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> + if (!strcmp(name, r->feature->name)) {
> + return i;
> + }
> + }
> +
> + return -1;
> +}
> +
> +int gdb_find_feature_register(CPUState *cpu, int feature, const char *name)
> +{
> + GDBRegisterState *r;
> +
> + r = &g_array_index(cpu->gdb_regs, GDBRegisterState, feature);
> +
> + for (int i = 0; i < r->feature->num_regs; i++) {
> + if (r->feature->regs[i] && !strcmp(name, r->feature->regs[i])) {
> + return r->base_reg + i;
> + }
> + }
> +
> + return -1;
> +}
> +
> +int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> GDBRegisterState *r;
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 15/18] cpu: Call plugin hooks only when ready
2023-10-19 10:26 ` [PATCH v14 15/18] cpu: Call plugin hooks only when ready Akihiko Odaki
@ 2023-10-24 15:58 ` Alex Bennée
2023-10-24 16:41 ` Alex Bennée
1 sibling, 0 replies; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 15:58 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Richard Henderson, Paolo Bonzini
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> The initialization and exit hooks will not affect the state of vCPU
> outside TCG context, but they may depend on the state of vCPU.
> Therefore, it's better to call plugin hooks after the vCPU state is
> fully initialized and before it gets uninitialized.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 15/18] cpu: Call plugin hooks only when ready
2023-10-19 10:26 ` [PATCH v14 15/18] cpu: Call plugin hooks only when ready Akihiko Odaki
2023-10-24 15:58 ` Alex Bennée
@ 2023-10-24 16:41 ` Alex Bennée
1 sibling, 0 replies; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 16:41 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Richard Henderson, Paolo Bonzini
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> The initialization and exit hooks will not affect the state of vCPU
> outside TCG context, but they may depend on the state of vCPU.
> Therefore, it's better to call plugin hooks after the vCPU state is
> fully initialized and before it gets uninitialized.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 16/18] plugins: Use different helpers when reading registers
2023-10-19 10:26 ` [PATCH v14 16/18] plugins: Use different helpers when reading registers Akihiko Odaki
@ 2023-10-24 16:48 ` Alex Bennée
2023-10-25 5:39 ` Akihiko Odaki
0 siblings, 1 reply; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 16:48 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Richard Henderson, Paolo Bonzini,
Eduardo Habkost, Marcel Apfelbaum, Yanan Wang, Alexandre Iooss,
Mahmoud Mandour
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> This avoids optimizations incompatible when reading registers.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> accel/tcg/plugin-helpers.h | 3 ++-
> include/exec/plugin-gen.h | 4 ++--
> include/hw/core/cpu.h | 4 ++--
> include/qemu/plugin.h | 3 +++
> plugins/plugin.h | 5 +++--
> accel/tcg/plugin-gen.c | 41 ++++++++++++++++++++++++++++----------
> accel/tcg/translator.c | 2 +-
> plugins/api.c | 14 +++++++++++--
> plugins/core.c | 28 ++++++++++++++++----------
> 9 files changed, 72 insertions(+), 32 deletions(-)
>
> diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h
> index 8e685e0654..11796436f3 100644
> --- a/accel/tcg/plugin-helpers.h
> +++ b/accel/tcg/plugin-helpers.h
> @@ -1,4 +1,5 @@
<snip>
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -437,7 +437,7 @@ struct qemu_work_item;
> * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
> * to @trace_dstate).
> * @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
> - * @plugin_mask: Plugin event bitmap. Modified only via async work.
> + * @plugin_flags: Plugin flags. Modified only via async work.
> * @ignore_memory_transaction_failures: Cached copy of the MachineState
> * flag of the same name: allows the board to suppress calling of the
> * CPU do_transaction_failed hook function.
> @@ -529,7 +529,7 @@ struct CPUState {
> /* Use by accel-block: CPU is executing an ioctl() */
> QemuLockCnt in_ioctl_lock;
>
> - DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
> + unsigned long plugin_flags;
Why are we dropping DECLARE_BITMAP here? It does ensure we will always
have the right size.
>
> #ifdef CONFIG_PLUGIN
> GArray *plugin_mem_cbs;
> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> index 7fdc3a4849..a534b9127b 100644
> --- a/include/qemu/plugin.h
> +++ b/include/qemu/plugin.h
> @@ -16,6 +16,9 @@
> #include "exec/memopidx.h"
> #include "hw/core/cpu.h"
>
> +#define QEMU_PLUGIN_FLAG_TB_CB_READ QEMU_PLUGIN_EV_MAX
> +#define QEMU_PLUGIN_FLAG_INSN_CB_READ (QEMU_PLUGIN_EV_MAX + 1)
> +
OK this seems like bad code smell. Are we overloading what was
plugin_mask to indicate two different things? I can see we only really
use the flags to indicate syscalls should generate callbacks so maybe
this could be rationlised but at least split into two patches so the
change to original behaviour isn't mixed up with new added behaviour.
> /*
> * Option parsing/processing.
> * Note that we can load an arbitrary number of plugins.
> diff --git a/plugins/plugin.h b/plugins/plugin.h
> index 5eb2fdbc85..ba0417194f 100644
> --- a/plugins/plugin.h
> +++ b/plugins/plugin.h
> @@ -16,6 +16,7 @@
> #include "qemu/qht.h"
>
> #define QEMU_PLUGIN_MIN_VERSION 0
> +#define QEMU_PLUGIN_FLAG_INSIN_CB_READ QEMU_PLUGIN_EV_MAX
Double definition.
>
> /* global state */
> struct qemu_plugin_state {
> @@ -31,7 +32,7 @@ struct qemu_plugin_state {
> * but with the HT we avoid adding a field to CPUState.
> */
> GHashTable *cpu_ht;
> - DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
> + unsigned long flags;
> /*
> * @lock protects the struct as well as ctx->uninstalling.
> * The lock must be acquired by all API ops.
> @@ -86,7 +87,7 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum qemu_plugin_event ev,
> void
> plugin_register_dyn_cb__udata(GArray **arr,
> qemu_plugin_vcpu_udata_cb_t cb,
> - enum qemu_plugin_cb_flags flags, void *udata);
> + unsigned int flags, void *udata);
>
>
> void plugin_register_vcpu_mem_cb(GArray **arr,
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 78b331b251..3bddd4d3c5 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -90,7 +90,10 @@ enum plugin_gen_cb {
> * These helpers are stubs that get dynamically switched out for calls
> * direct to the plugin if they are subscribed to.
> */
> -void HELPER(plugin_vcpu_udata_cb)(uint32_t cpu_index, void *udata)
> +void HELPER(plugin_vcpu_udata_cb_no_wg)(uint32_t cpu_index, void *udata)
> +{ }
> +
> +void HELPER(plugin_vcpu_udata_cb_no_rwg)(uint32_t cpu_index, void *udata)
> { }
>
> void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
> @@ -98,7 +101,7 @@ void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
> void *userdata)
> { }
>
> -static void gen_empty_udata_cb(void)
> +static void gen_empty_udata_cb(void (*gen_helper)(TCGv_i32, TCGv_ptr))
> {
> TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
> TCGv_ptr udata = tcg_temp_ebb_new_ptr();
> @@ -106,12 +109,22 @@ static void gen_empty_udata_cb(void)
> tcg_gen_movi_ptr(udata, 0);
> tcg_gen_ld_i32(cpu_index, tcg_env,
> -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
> - gen_helper_plugin_vcpu_udata_cb(cpu_index, udata);
> + gen_helper(cpu_index, udata);
>
> tcg_temp_free_ptr(udata);
> tcg_temp_free_i32(cpu_index);
> }
>
> +static void gen_empty_udata_cb_no_wg(void)
> +{
> + gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_wg);
> +}
> +
> +static void gen_empty_udata_cb_no_rwg(void)
> +{
> + gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_rwg);
> +}
> +
> /*
> * For now we only support addi_i64.
> * When we support more ops, we can generate one empty inline cb for each.
> @@ -176,7 +189,7 @@ static void gen_wrapped(enum plugin_gen_from from,
> tcg_gen_plugin_cb_end();
> }
>
> -static void plugin_gen_empty_callback(enum plugin_gen_from from)
> +static void plugin_gen_empty_callback(CPUState *cpu, enum plugin_gen_from from)
> {
> switch (from) {
> case PLUGIN_GEN_AFTER_INSN:
> @@ -190,9 +203,15 @@ static void plugin_gen_empty_callback(enum plugin_gen_from from)
> */
> gen_wrapped(from, PLUGIN_GEN_ENABLE_MEM_HELPER,
> gen_empty_mem_helper);
> - /* fall through */
> + gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
> + cpu->plugin_flags & BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) ?
> + gen_empty_udata_cb_no_wg : gen_empty_udata_cb_no_rwg);
> + gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
> + break;
> case PLUGIN_GEN_FROM_TB:
> - gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb);
> + gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
> + cpu->plugin_flags & BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) ?
> + gen_empty_udata_cb_no_wg :
> gen_empty_udata_cb_no_rwg);
What stops us from generating two empty callbacks (one no_wg one no_rwg)
and just discarding the unused ones after the instrumentation pass?
> gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
> break;
> default:
> @@ -796,7 +815,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
> {
> bool ret = false;
>
> - if (test_bit(QEMU_PLUGIN_EV_VCPU_TB_TRANS, cpu->plugin_mask)) {
> + if (cpu->plugin_flags & BIT(QEMU_PLUGIN_EV_VCPU_TB_TRANS)) {
> struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
> int i;
>
> @@ -817,7 +836,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
> ptb->mem_only = mem_only;
> ptb->mem_helper = false;
>
> - plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
> + plugin_gen_empty_callback(cpu, PLUGIN_GEN_FROM_TB);
> }
>
> tcg_ctx->plugin_insn = NULL;
> @@ -832,7 +851,7 @@ void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
>
> pinsn = qemu_plugin_tb_insn_get(ptb, db->pc_next);
> tcg_ctx->plugin_insn = pinsn;
> - plugin_gen_empty_callback(PLUGIN_GEN_FROM_INSN);
> + plugin_gen_empty_callback(cpu, PLUGIN_GEN_FROM_INSN);
>
> /*
> * Detect page crossing to get the new host address.
> @@ -852,9 +871,9 @@ void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
> }
> }
>
> -void plugin_gen_insn_end(void)
> +void plugin_gen_insn_end(CPUState *cpu)
> {
> - plugin_gen_empty_callback(PLUGIN_GEN_AFTER_INSN);
> + plugin_gen_empty_callback(cpu, PLUGIN_GEN_AFTER_INSN);
> }
>
> /*
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 575b9812ad..bec58dd93f 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -189,7 +189,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
> * to accurately track instrumented helpers that might access memory.
> */
> if (plugin_enabled) {
> - plugin_gen_insn_end();
> + plugin_gen_insn_end(cpu);
> }
>
> /* Stop translation if translate_insn so indicated. */
> diff --git a/plugins/api.c b/plugins/api.c
> index 5521b0ad36..326e37cb73 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -89,8 +89,13 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
> void *udata)
> {
> if (!tb->mem_only) {
> + bool read = flags == QEMU_PLUGIN_CB_R_REGS ||
> + flags == QEMU_PLUGIN_CB_RW_REGS;
> +
> plugin_register_dyn_cb__udata(&tb->cbs[PLUGIN_CB_REGULAR],
> - cb, flags, udata);
> + cb,
> + read ? BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) : 0,
> + udata);
> }
> }
>
> @@ -109,8 +114,13 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
> void *udata)
> {
> if (!insn->mem_only) {
> + bool read = flags == QEMU_PLUGIN_CB_R_REGS ||
> + flags == QEMU_PLUGIN_CB_RW_REGS;
> +
> plugin_register_dyn_cb__udata(&insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_REGULAR],
> - cb, flags, udata);
> + cb,
> + read ? BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) : 0,
> + udata);
> }
> }
>
> diff --git a/plugins/core.c b/plugins/core.c
> index fcd33a2bff..f461e84473 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -55,19 +55,19 @@ struct qemu_plugin_ctx *plugin_id_to_ctx_locked(qemu_plugin_id_t id)
>
> static void plugin_cpu_update__async(CPUState *cpu, run_on_cpu_data data)
> {
> - bitmap_copy(cpu->plugin_mask, &data.host_ulong, QEMU_PLUGIN_EV_MAX);
> + cpu->plugin_flags = data.host_ulong;
> tcg_flush_jmp_cache(cpu);
> }
>
> static void plugin_cpu_update__locked(gpointer k, gpointer v, gpointer udata)
> {
> CPUState *cpu = container_of(k, CPUState, cpu_index);
> - run_on_cpu_data mask = RUN_ON_CPU_HOST_ULONG(*plugin.mask);
> + run_on_cpu_data flags = RUN_ON_CPU_HOST_ULONG(plugin.flags);
>
> if (DEVICE(cpu)->realized) {
> - async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
> + async_run_on_cpu(cpu, plugin_cpu_update__async, flags);
> } else {
> - plugin_cpu_update__async(cpu, mask);
> + plugin_cpu_update__async(cpu, flags);
> }
> }
>
> @@ -83,7 +83,7 @@ void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,
> g_free(cb);
> ctx->callbacks[ev] = NULL;
> if (QLIST_EMPTY_RCU(&plugin.cb_lists[ev])) {
> - clear_bit(ev, plugin.mask);
> + plugin.flags &= ~BIT(ev);
> g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked, NULL);
> }
> }
> @@ -186,8 +186,8 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
> cb->udata = udata;
> ctx->callbacks[ev] = cb;
> QLIST_INSERT_HEAD_RCU(&plugin.cb_lists[ev], cb, entry);
> - if (!test_bit(ev, plugin.mask)) {
> - set_bit(ev, plugin.mask);
> + if (!(plugin.flags & BIT(ev))) {
> + plugin.flags |= BIT(ev);
> g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked,
> NULL);
> }
> @@ -296,15 +296,20 @@ void plugin_register_inline_op(GArray **arr,
>
> void plugin_register_dyn_cb__udata(GArray **arr,
> qemu_plugin_vcpu_udata_cb_t cb,
> - enum qemu_plugin_cb_flags flags,
> + unsigned int flags,
> void *udata)
> {
> struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
>
> dyn_cb->userp = udata;
> - /* Note flags are discarded as unused. */
> dyn_cb->f.vcpu_udata = cb;
> dyn_cb->type = PLUGIN_CB_REGULAR;
> +
> + if (flags) {
> + QEMU_LOCK_GUARD(&plugin.lock);
> + plugin.flags |= flags;
> + g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked, NULL);
> + }
> }
>
> void plugin_register_vcpu_mem_cb(GArray **arr,
> @@ -357,7 +362,7 @@ qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
> struct qemu_plugin_cb *cb, *next;
> enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL;
>
> - if (!test_bit(ev, cpu->plugin_mask)) {
> + if (!(cpu->plugin_flags & BIT(ev))) {
> return;
> }
>
> @@ -379,7 +384,7 @@ void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
> struct qemu_plugin_cb *cb, *next;
> enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL_RET;
>
> - if (!test_bit(ev, cpu->plugin_mask)) {
> + if (!(cpu->plugin_flags & BIT(ev))) {
> return;
> }
>
> @@ -428,6 +433,7 @@ void qemu_plugin_flush_cb(void)
> {
> qht_iter_remove(&plugin.dyn_cb_arr_ht, free_dyn_cb_arr, NULL);
> qht_reset(&plugin.dyn_cb_arr_ht);
> + plugin.flags &= ~BIT(QEMU_PLUGIN_FLAG_INSIN_CB_READ);
>
> plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
> }
I'm a bit confused about what we are trying to achieve here. I think
split the changes and some better commentary would help.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 00/18] plugins: Allow to read registers
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
` (17 preceding siblings ...)
2023-10-19 10:26 ` [PATCH v14 18/18] contrib/plugins: Allow to log registers Akihiko Odaki
@ 2023-10-24 17:08 ` Alex Bennée
2023-10-25 5:51 ` Akihiko Odaki
18 siblings, 1 reply; 30+ messages in thread
From: Alex Bennée @ 2023-10-24 17:08 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé
Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> Based-on: <20231019101030.128431-1-akihiko.odaki@daynix.com>
> ("[PATCH v5 0/6] gdbstub and TCG plugin improvements")
>
> I and other people in the University of Tokyo, where I research processor
> design, found TCG plugins are very useful for processor design
> exploration.
So I reviewed this on top of my gdbstub/next series so needed to make
some tweaks as I didn't have the riscv misa_mxl patches from above
because there still seems to be some debate about them.
If we can get a series that doesn't depend on them (or the RiscV guys to
agree with the changes) with the review comments addressed I think I'm
ready to pull at least 1-15 into my tree for the next pull request.
I want to spend a little more time looking over the plugin API itself
and experiment with some alternatives. But I don't think that need hold
up getting the gdbfeature stuff squared away now.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 16/18] plugins: Use different helpers when reading registers
2023-10-24 16:48 ` Alex Bennée
@ 2023-10-25 5:39 ` Akihiko Odaki
2023-10-25 9:34 ` Akihiko Odaki
0 siblings, 1 reply; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-25 5:39 UTC (permalink / raw)
To: Alex Bennée
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Richard Henderson, Paolo Bonzini,
Eduardo Habkost, Marcel Apfelbaum, Yanan Wang, Alexandre Iooss,
Mahmoud Mandour
On 2023/10/25 1:48, Alex Bennée wrote:
>
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
>> This avoids optimizations incompatible when reading registers.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> accel/tcg/plugin-helpers.h | 3 ++-
>> include/exec/plugin-gen.h | 4 ++--
>> include/hw/core/cpu.h | 4 ++--
>> include/qemu/plugin.h | 3 +++
>> plugins/plugin.h | 5 +++--
>> accel/tcg/plugin-gen.c | 41 ++++++++++++++++++++++++++++----------
>> accel/tcg/translator.c | 2 +-
>> plugins/api.c | 14 +++++++++++--
>> plugins/core.c | 28 ++++++++++++++++----------
>> 9 files changed, 72 insertions(+), 32 deletions(-)
>>
>> diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h
>> index 8e685e0654..11796436f3 100644
>> --- a/accel/tcg/plugin-helpers.h
>> +++ b/accel/tcg/plugin-helpers.h
>> @@ -1,4 +1,5 @@
> <snip>
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -437,7 +437,7 @@ struct qemu_work_item;
>> * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
>> * to @trace_dstate).
>> * @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
>> - * @plugin_mask: Plugin event bitmap. Modified only via async work.
>> + * @plugin_flags: Plugin flags. Modified only via async work.
>> * @ignore_memory_transaction_failures: Cached copy of the MachineState
>> * flag of the same name: allows the board to suppress calling of the
>> * CPU do_transaction_failed hook function.
>> @@ -529,7 +529,7 @@ struct CPUState {
>> /* Use by accel-block: CPU is executing an ioctl() */
>> QemuLockCnt in_ioctl_lock;
>>
>> - DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
>> + unsigned long plugin_flags;
>
> Why are we dropping DECLARE_BITMAP here? It does ensure we will always
> have the right size.
>
>>
>> #ifdef CONFIG_PLUGIN
>> GArray *plugin_mem_cbs;
>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>> index 7fdc3a4849..a534b9127b 100644
>> --- a/include/qemu/plugin.h
>> +++ b/include/qemu/plugin.h
>> @@ -16,6 +16,9 @@
>> #include "exec/memopidx.h"
>> #include "hw/core/cpu.h"
>>
>> +#define QEMU_PLUGIN_FLAG_TB_CB_READ QEMU_PLUGIN_EV_MAX
>> +#define QEMU_PLUGIN_FLAG_INSN_CB_READ (QEMU_PLUGIN_EV_MAX + 1)
>> +
>
> OK this seems like bad code smell. Are we overloading what was
> plugin_mask to indicate two different things? I can see we only really
> use the flags to indicate syscalls should generate callbacks so maybe
> this could be rationlised but at least split into two patches so the
> change to original behaviour isn't mixed up with new added behaviour.
I introduced new "flag" concept in v15 to avoid the overloading.
>
>> /*
>> * Option parsing/processing.
>> * Note that we can load an arbitrary number of plugins.
>> diff --git a/plugins/plugin.h b/plugins/plugin.h
>> index 5eb2fdbc85..ba0417194f 100644
>> --- a/plugins/plugin.h
>> +++ b/plugins/plugin.h
>> @@ -16,6 +16,7 @@
>> #include "qemu/qht.h"
>>
>> #define QEMU_PLUGIN_MIN_VERSION 0
>> +#define QEMU_PLUGIN_FLAG_INSIN_CB_READ QEMU_PLUGIN_EV_MAX
>
> Double definition.
Removed in v15.
>
>>
>> /* global state */
>> struct qemu_plugin_state {
>> @@ -31,7 +32,7 @@ struct qemu_plugin_state {
>> * but with the HT we avoid adding a field to CPUState.
>> */
>> GHashTable *cpu_ht;
>> - DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
>> + unsigned long flags;
>> /*
>> * @lock protects the struct as well as ctx->uninstalling.
>> * The lock must be acquired by all API ops.
>> @@ -86,7 +87,7 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum qemu_plugin_event ev,
>> void
>> plugin_register_dyn_cb__udata(GArray **arr,
>> qemu_plugin_vcpu_udata_cb_t cb,
>> - enum qemu_plugin_cb_flags flags, void *udata);
>> + unsigned int flags, void *udata);
>>
>>
>> void plugin_register_vcpu_mem_cb(GArray **arr,
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index 78b331b251..3bddd4d3c5 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -90,7 +90,10 @@ enum plugin_gen_cb {
>> * These helpers are stubs that get dynamically switched out for calls
>> * direct to the plugin if they are subscribed to.
>> */
>> -void HELPER(plugin_vcpu_udata_cb)(uint32_t cpu_index, void *udata)
>> +void HELPER(plugin_vcpu_udata_cb_no_wg)(uint32_t cpu_index, void *udata)
>> +{ }
>> +
>> +void HELPER(plugin_vcpu_udata_cb_no_rwg)(uint32_t cpu_index, void *udata)
>> { }
>>
>> void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
>> @@ -98,7 +101,7 @@ void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
>> void *userdata)
>> { }
>>
>> -static void gen_empty_udata_cb(void)
>> +static void gen_empty_udata_cb(void (*gen_helper)(TCGv_i32, TCGv_ptr))
>> {
>> TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
>> TCGv_ptr udata = tcg_temp_ebb_new_ptr();
>> @@ -106,12 +109,22 @@ static void gen_empty_udata_cb(void)
>> tcg_gen_movi_ptr(udata, 0);
>> tcg_gen_ld_i32(cpu_index, tcg_env,
>> -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
>> - gen_helper_plugin_vcpu_udata_cb(cpu_index, udata);
>> + gen_helper(cpu_index, udata);
>>
>> tcg_temp_free_ptr(udata);
>> tcg_temp_free_i32(cpu_index);
>> }
>>
>> +static void gen_empty_udata_cb_no_wg(void)
>> +{
>> + gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_wg);
>> +}
>> +
>> +static void gen_empty_udata_cb_no_rwg(void)
>> +{
>> + gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_rwg);
>> +}
>> +
>> /*
>> * For now we only support addi_i64.
>> * When we support more ops, we can generate one empty inline cb for each.
>> @@ -176,7 +189,7 @@ static void gen_wrapped(enum plugin_gen_from from,
>> tcg_gen_plugin_cb_end();
>> }
>>
>> -static void plugin_gen_empty_callback(enum plugin_gen_from from)
>> +static void plugin_gen_empty_callback(CPUState *cpu, enum plugin_gen_from from)
>> {
>> switch (from) {
>> case PLUGIN_GEN_AFTER_INSN:
>> @@ -190,9 +203,15 @@ static void plugin_gen_empty_callback(enum plugin_gen_from from)
>> */
>> gen_wrapped(from, PLUGIN_GEN_ENABLE_MEM_HELPER,
>> gen_empty_mem_helper);
>> - /* fall through */
>> + gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
>> + cpu->plugin_flags & BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) ?
>> + gen_empty_udata_cb_no_wg : gen_empty_udata_cb_no_rwg);
>> + gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
>> + break;
>> case PLUGIN_GEN_FROM_TB:
>> - gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb);
>> + gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
>> + cpu->plugin_flags & BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) ?
>> + gen_empty_udata_cb_no_wg :
>> gen_empty_udata_cb_no_rwg);
>
> What stops us from generating two empty callbacks (one no_wg one no_rwg)
> and just discarding the unused ones after the instrumentation pass?
There is nothing stopping us from doing that, but I see no reason to do
so when we can pick one just with a ternary operator.
>
>> gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
>> break;
>> default:
>> @@ -796,7 +815,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
>> {
>> bool ret = false;
>>
>> - if (test_bit(QEMU_PLUGIN_EV_VCPU_TB_TRANS, cpu->plugin_mask)) {
>> + if (cpu->plugin_flags & BIT(QEMU_PLUGIN_EV_VCPU_TB_TRANS)) {
>> struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
>> int i;
>>
>> @@ -817,7 +836,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
>> ptb->mem_only = mem_only;
>> ptb->mem_helper = false;
>>
>> - plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
>> + plugin_gen_empty_callback(cpu, PLUGIN_GEN_FROM_TB);
>> }
>>
>> tcg_ctx->plugin_insn = NULL;
>> @@ -832,7 +851,7 @@ void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
>>
>> pinsn = qemu_plugin_tb_insn_get(ptb, db->pc_next);
>> tcg_ctx->plugin_insn = pinsn;
>> - plugin_gen_empty_callback(PLUGIN_GEN_FROM_INSN);
>> + plugin_gen_empty_callback(cpu, PLUGIN_GEN_FROM_INSN);
>>
>> /*
>> * Detect page crossing to get the new host address.
>> @@ -852,9 +871,9 @@ void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
>> }
>> }
>>
>> -void plugin_gen_insn_end(void)
>> +void plugin_gen_insn_end(CPUState *cpu)
>> {
>> - plugin_gen_empty_callback(PLUGIN_GEN_AFTER_INSN);
>> + plugin_gen_empty_callback(cpu, PLUGIN_GEN_AFTER_INSN);
>> }
>>
>> /*
>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> index 575b9812ad..bec58dd93f 100644
>> --- a/accel/tcg/translator.c
>> +++ b/accel/tcg/translator.c
>> @@ -189,7 +189,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
>> * to accurately track instrumented helpers that might access memory.
>> */
>> if (plugin_enabled) {
>> - plugin_gen_insn_end();
>> + plugin_gen_insn_end(cpu);
>> }
>>
>> /* Stop translation if translate_insn so indicated. */
>> diff --git a/plugins/api.c b/plugins/api.c
>> index 5521b0ad36..326e37cb73 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -89,8 +89,13 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct qemu_plugin_tb *tb,
>> void *udata)
>> {
>> if (!tb->mem_only) {
>> + bool read = flags == QEMU_PLUGIN_CB_R_REGS ||
>> + flags == QEMU_PLUGIN_CB_RW_REGS;
>> +
>> plugin_register_dyn_cb__udata(&tb->cbs[PLUGIN_CB_REGULAR],
>> - cb, flags, udata);
>> + cb,
>> + read ? BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) : 0,
>> + udata);
>> }
>> }
>>
>> @@ -109,8 +114,13 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
>> void *udata)
>> {
>> if (!insn->mem_only) {
>> + bool read = flags == QEMU_PLUGIN_CB_R_REGS ||
>> + flags == QEMU_PLUGIN_CB_RW_REGS;
>> +
>> plugin_register_dyn_cb__udata(&insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_REGULAR],
>> - cb, flags, udata);
>> + cb,
>> + read ? BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) : 0,
>> + udata);
>> }
>> }
>>
>> diff --git a/plugins/core.c b/plugins/core.c
>> index fcd33a2bff..f461e84473 100644
>> --- a/plugins/core.c
>> +++ b/plugins/core.c
>> @@ -55,19 +55,19 @@ struct qemu_plugin_ctx *plugin_id_to_ctx_locked(qemu_plugin_id_t id)
>>
>> static void plugin_cpu_update__async(CPUState *cpu, run_on_cpu_data data)
>> {
>> - bitmap_copy(cpu->plugin_mask, &data.host_ulong, QEMU_PLUGIN_EV_MAX);
>> + cpu->plugin_flags = data.host_ulong;
>> tcg_flush_jmp_cache(cpu);
>> }
>>
>> static void plugin_cpu_update__locked(gpointer k, gpointer v, gpointer udata)
>> {
>> CPUState *cpu = container_of(k, CPUState, cpu_index);
>> - run_on_cpu_data mask = RUN_ON_CPU_HOST_ULONG(*plugin.mask);
>> + run_on_cpu_data flags = RUN_ON_CPU_HOST_ULONG(plugin.flags);
>>
>> if (DEVICE(cpu)->realized) {
>> - async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>> + async_run_on_cpu(cpu, plugin_cpu_update__async, flags);
>> } else {
>> - plugin_cpu_update__async(cpu, mask);
>> + plugin_cpu_update__async(cpu, flags);
>> }
>> }
>>
>> @@ -83,7 +83,7 @@ void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,
>> g_free(cb);
>> ctx->callbacks[ev] = NULL;
>> if (QLIST_EMPTY_RCU(&plugin.cb_lists[ev])) {
>> - clear_bit(ev, plugin.mask);
>> + plugin.flags &= ~BIT(ev);
>> g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked, NULL);
>> }
>> }
>> @@ -186,8 +186,8 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
>> cb->udata = udata;
>> ctx->callbacks[ev] = cb;
>> QLIST_INSERT_HEAD_RCU(&plugin.cb_lists[ev], cb, entry);
>> - if (!test_bit(ev, plugin.mask)) {
>> - set_bit(ev, plugin.mask);
>> + if (!(plugin.flags & BIT(ev))) {
>> + plugin.flags |= BIT(ev);
>> g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked,
>> NULL);
>> }
>> @@ -296,15 +296,20 @@ void plugin_register_inline_op(GArray **arr,
>>
>> void plugin_register_dyn_cb__udata(GArray **arr,
>> qemu_plugin_vcpu_udata_cb_t cb,
>> - enum qemu_plugin_cb_flags flags,
>> + unsigned int flags,
>> void *udata)
>> {
>> struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
>>
>> dyn_cb->userp = udata;
>> - /* Note flags are discarded as unused. */
>> dyn_cb->f.vcpu_udata = cb;
>> dyn_cb->type = PLUGIN_CB_REGULAR;
>> +
>> + if (flags) {
>> + QEMU_LOCK_GUARD(&plugin.lock);
>> + plugin.flags |= flags;
>> + g_hash_table_foreach(plugin.cpu_ht, plugin_cpu_update__locked, NULL);
>> + }
>> }
>>
>> void plugin_register_vcpu_mem_cb(GArray **arr,
>> @@ -357,7 +362,7 @@ qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
>> struct qemu_plugin_cb *cb, *next;
>> enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL;
>>
>> - if (!test_bit(ev, cpu->plugin_mask)) {
>> + if (!(cpu->plugin_flags & BIT(ev))) {
>> return;
>> }
>>
>> @@ -379,7 +384,7 @@ void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
>> struct qemu_plugin_cb *cb, *next;
>> enum qemu_plugin_event ev = QEMU_PLUGIN_EV_VCPU_SYSCALL_RET;
>>
>> - if (!test_bit(ev, cpu->plugin_mask)) {
>> + if (!(cpu->plugin_flags & BIT(ev))) {
>> return;
>> }
>>
>> @@ -428,6 +433,7 @@ void qemu_plugin_flush_cb(void)
>> {
>> qht_iter_remove(&plugin.dyn_cb_arr_ht, free_dyn_cb_arr, NULL);
>> qht_reset(&plugin.dyn_cb_arr_ht);
>> + plugin.flags &= ~BIT(QEMU_PLUGIN_FLAG_INSIN_CB_READ);
>>
>> plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
>> }
>
>
> I'm a bit confused about what we are trying to achieve here. I think
> split the changes and some better commentary would help.
I extracted the change to introduce "flags" into a separate patch in v15.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 00/18] plugins: Allow to read registers
2023-10-24 17:08 ` [PATCH v14 00/18] plugins: Allow to read registers Alex Bennée
@ 2023-10-25 5:51 ` Akihiko Odaki
0 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-25 5:51 UTC (permalink / raw)
To: Alex Bennée, Palmer Dabbelt, Alistair Francis, Bin Meng,
Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei, qemu-riscv
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé
On 2023/10/25 2:08, Alex Bennée wrote:
>
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
>> Based-on: <20231019101030.128431-1-akihiko.odaki@daynix.com>
>> ("[PATCH v5 0/6] gdbstub and TCG plugin improvements")
>>
>> I and other people in the University of Tokyo, where I research processor
>> design, found TCG plugins are very useful for processor design
>> exploration.
>
> So I reviewed this on top of my gdbstub/next series so needed to make
> some tweaks as I didn't have the riscv misa_mxl patches from above
> because there still seems to be some debate about them.
I think we have sorted out major design questions but two patches in
"[PATCH v5 0/6] gdbstub and TCG plugin improvements" still have no
Reviewed-by or Acked-by. Namely,
[PATCH v4 3/5] target/riscv: Move misa_mxl_max to class
[PATCH v4 4/5] target/riscv: Validate misa_mxl_max only once
Can anyone interested in RISC-V look at them?
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v14 16/18] plugins: Use different helpers when reading registers
2023-10-25 5:39 ` Akihiko Odaki
@ 2023-10-25 9:34 ` Akihiko Odaki
0 siblings, 0 replies; 30+ messages in thread
From: Akihiko Odaki @ 2023-10-25 9:34 UTC (permalink / raw)
To: Alex Bennée
Cc: Mikhail Tyutin, Aleksandr Anenkov, qemu-devel,
Philippe Mathieu-Daudé, Richard Henderson, Paolo Bonzini,
Eduardo Habkost, Marcel Apfelbaum, Yanan Wang, Alexandre Iooss,
Mahmoud Mandour
On 2023/10/25 14:39, Akihiko Odaki wrote:
> On 2023/10/25 1:48, Alex Bennée wrote:
>>
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>
>>> This avoids optimizations incompatible when reading registers.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>> accel/tcg/plugin-helpers.h | 3 ++-
>>> include/exec/plugin-gen.h | 4 ++--
>>> include/hw/core/cpu.h | 4 ++--
>>> include/qemu/plugin.h | 3 +++
>>> plugins/plugin.h | 5 +++--
>>> accel/tcg/plugin-gen.c | 41 ++++++++++++++++++++++++++++----------
>>> accel/tcg/translator.c | 2 +-
>>> plugins/api.c | 14 +++++++++++--
>>> plugins/core.c | 28 ++++++++++++++++----------
>>> 9 files changed, 72 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h
>>> index 8e685e0654..11796436f3 100644
>>> --- a/accel/tcg/plugin-helpers.h
>>> +++ b/accel/tcg/plugin-helpers.h
>>> @@ -1,4 +1,5 @@
>> <snip>
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -437,7 +437,7 @@ struct qemu_work_item;
>>> * @trace_dstate_delayed: Delayed changes to trace_dstate (includes
>>> all changes
>>> * to @trace_dstate).
>>> * @trace_dstate: Dynamic tracing state of events for this vCPU
>>> (bitmask).
>>> - * @plugin_mask: Plugin event bitmap. Modified only via async work.
>>> + * @plugin_flags: Plugin flags. Modified only via async work.
>>> * @ignore_memory_transaction_failures: Cached copy of the
>>> MachineState
>>> * flag of the same name: allows the board to suppress calling
>>> of the
>>> * CPU do_transaction_failed hook function.
>>> @@ -529,7 +529,7 @@ struct CPUState {
>>> /* Use by accel-block: CPU is executing an ioctl() */
>>> QemuLockCnt in_ioctl_lock;
>>> - DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
>>> + unsigned long plugin_flags;
>>
>> Why are we dropping DECLARE_BITMAP here? It does ensure we will always
>> have the right size.
>>
>>> #ifdef CONFIG_PLUGIN
>>> GArray *plugin_mem_cbs;
>>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>>> index 7fdc3a4849..a534b9127b 100644
>>> --- a/include/qemu/plugin.h
>>> +++ b/include/qemu/plugin.h
>>> @@ -16,6 +16,9 @@
>>> #include "exec/memopidx.h"
>>> #include "hw/core/cpu.h"
>>> +#define QEMU_PLUGIN_FLAG_TB_CB_READ QEMU_PLUGIN_EV_MAX
>>> +#define QEMU_PLUGIN_FLAG_INSN_CB_READ (QEMU_PLUGIN_EV_MAX + 1)
>>> +
>>
>> OK this seems like bad code smell. Are we overloading what was
>> plugin_mask to indicate two different things? I can see we only really
>> use the flags to indicate syscalls should generate callbacks so maybe
>> this could be rationlised but at least split into two patches so the
>> change to original behaviour isn't mixed up with new added behaviour.
>
> I introduced new "flag" concept in v15 to avoid the overloading.
>
>>
>>> /*
>>> * Option parsing/processing.
>>> * Note that we can load an arbitrary number of plugins.
>>> diff --git a/plugins/plugin.h b/plugins/plugin.h
>>> index 5eb2fdbc85..ba0417194f 100644
>>> --- a/plugins/plugin.h
>>> +++ b/plugins/plugin.h
>>> @@ -16,6 +16,7 @@
>>> #include "qemu/qht.h"
>>> #define QEMU_PLUGIN_MIN_VERSION 0
>>> +#define QEMU_PLUGIN_FLAG_INSIN_CB_READ QEMU_PLUGIN_EV_MAX
>>
>> Double definition.
>
> Removed in v15.
>
>>
>>> /* global state */
>>> struct qemu_plugin_state {
>>> @@ -31,7 +32,7 @@ struct qemu_plugin_state {
>>> * but with the HT we avoid adding a field to CPUState.
>>> */
>>> GHashTable *cpu_ht;
>>> - DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
>>> + unsigned long flags;
>>> /*
>>> * @lock protects the struct as well as ctx->uninstalling.
>>> * The lock must be acquired by all API ops.
>>> @@ -86,7 +87,7 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum
>>> qemu_plugin_event ev,
>>> void
>>> plugin_register_dyn_cb__udata(GArray **arr,
>>> qemu_plugin_vcpu_udata_cb_t cb,
>>> - enum qemu_plugin_cb_flags flags, void
>>> *udata);
>>> + unsigned int flags, void *udata);
>>> void plugin_register_vcpu_mem_cb(GArray **arr,
>>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>>> index 78b331b251..3bddd4d3c5 100644
>>> --- a/accel/tcg/plugin-gen.c
>>> +++ b/accel/tcg/plugin-gen.c
>>> @@ -90,7 +90,10 @@ enum plugin_gen_cb {
>>> * These helpers are stubs that get dynamically switched out for calls
>>> * direct to the plugin if they are subscribed to.
>>> */
>>> -void HELPER(plugin_vcpu_udata_cb)(uint32_t cpu_index, void *udata)
>>> +void HELPER(plugin_vcpu_udata_cb_no_wg)(uint32_t cpu_index, void
>>> *udata)
>>> +{ }
>>> +
>>> +void HELPER(plugin_vcpu_udata_cb_no_rwg)(uint32_t cpu_index, void
>>> *udata)
>>> { }
>>> void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
>>> @@ -98,7 +101,7 @@ void HELPER(plugin_vcpu_mem_cb)(unsigned int
>>> vcpu_index,
>>> void *userdata)
>>> { }
>>> -static void gen_empty_udata_cb(void)
>>> +static void gen_empty_udata_cb(void (*gen_helper)(TCGv_i32, TCGv_ptr))
>>> {
>>> TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
>>> TCGv_ptr udata = tcg_temp_ebb_new_ptr();
>>> @@ -106,12 +109,22 @@ static void gen_empty_udata_cb(void)
>>> tcg_gen_movi_ptr(udata, 0);
>>> tcg_gen_ld_i32(cpu_index, tcg_env,
>>> -offsetof(ArchCPU, env) + offsetof(CPUState,
>>> cpu_index));
>>> - gen_helper_plugin_vcpu_udata_cb(cpu_index, udata);
>>> + gen_helper(cpu_index, udata);
>>> tcg_temp_free_ptr(udata);
>>> tcg_temp_free_i32(cpu_index);
>>> }
>>> +static void gen_empty_udata_cb_no_wg(void)
>>> +{
>>> + gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_wg);
>>> +}
>>> +
>>> +static void gen_empty_udata_cb_no_rwg(void)
>>> +{
>>> + gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_rwg);
>>> +}
>>> +
>>> /*
>>> * For now we only support addi_i64.
>>> * When we support more ops, we can generate one empty inline cb
>>> for each.
>>> @@ -176,7 +189,7 @@ static void gen_wrapped(enum plugin_gen_from from,
>>> tcg_gen_plugin_cb_end();
>>> }
>>> -static void plugin_gen_empty_callback(enum plugin_gen_from from)
>>> +static void plugin_gen_empty_callback(CPUState *cpu, enum
>>> plugin_gen_from from)
>>> {
>>> switch (from) {
>>> case PLUGIN_GEN_AFTER_INSN:
>>> @@ -190,9 +203,15 @@ static void plugin_gen_empty_callback(enum
>>> plugin_gen_from from)
>>> */
>>> gen_wrapped(from, PLUGIN_GEN_ENABLE_MEM_HELPER,
>>> gen_empty_mem_helper);
>>> - /* fall through */
>>> + gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
>>> + cpu->plugin_flags &
>>> BIT(QEMU_PLUGIN_FLAG_INSN_CB_READ) ?
>>> + gen_empty_udata_cb_no_wg :
>>> gen_empty_udata_cb_no_rwg);
>>> + gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
>>> + break;
>>> case PLUGIN_GEN_FROM_TB:
>>> - gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb);
>>> + gen_wrapped(from, PLUGIN_GEN_CB_UDATA,
>>> + cpu->plugin_flags &
>>> BIT(QEMU_PLUGIN_FLAG_TB_CB_READ) ?
>>> + gen_empty_udata_cb_no_wg :
>>> gen_empty_udata_cb_no_rwg);
>>
>> What stops us from generating two empty callbacks (one no_wg one no_rwg)
>> and just discarding the unused ones after the instrumentation pass?
>
> There is nothing stopping us from doing that, but I see no reason to do
> so when we can pick one just with a ternary operator.
I think I get the idea. Please check v16 and see if it does what you
expect. It is indeed much simpler than the previous version.
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-10-25 9:35 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-19 10:26 [PATCH v14 00/18] plugins: Allow to read registers Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 01/18] gdbstub: Add num_regs member to GDBFeature Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 02/18] gdbstub: Introduce gdb_find_static_feature() Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 03/18] gdbstub: Introduce GDBFeatureBuilder Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 04/18] target/arm: Use GDBFeature for dynamic XML Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 05/18] target/ppc: " Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 06/18] target/riscv: " Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 07/18] gdbstub: Use GDBFeature for gdb_register_coprocessor Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 08/18] gdbstub: Use GDBFeature for GDBRegisterState Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 09/18] gdbstub: Change gdb_get_reg_cb and gdb_set_reg_cb Akihiko Odaki
2023-10-24 14:20 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 10/18] gdbstub: Simplify XML lookup Akihiko Odaki
2023-10-24 14:25 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 11/18] gdbstub: Infer number of core registers from XML Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 12/18] hw/core/cpu: Remove gdb_get_dynamic_xml member Akihiko Odaki
2023-10-24 15:38 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 13/18] gdbstub: Add members to identify registers to GDBFeature Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 14/18] gdbstub: Expose functions to read registers Akihiko Odaki
2023-10-24 15:41 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 15/18] cpu: Call plugin hooks only when ready Akihiko Odaki
2023-10-24 15:58 ` Alex Bennée
2023-10-24 16:41 ` Alex Bennée
2023-10-19 10:26 ` [PATCH v14 16/18] plugins: Use different helpers when reading registers Akihiko Odaki
2023-10-24 16:48 ` Alex Bennée
2023-10-25 5:39 ` Akihiko Odaki
2023-10-25 9:34 ` Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 17/18] plugins: Allow to read registers Akihiko Odaki
2023-10-19 10:26 ` [PATCH v14 18/18] contrib/plugins: Allow to log registers Akihiko Odaki
2023-10-24 17:08 ` [PATCH v14 00/18] plugins: Allow to read registers Alex Bennée
2023-10-25 5:51 ` Akihiko Odaki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).