| Message ID | 20241127121658.88966-5-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: Allow tcg_exec_realizefn() initialize multiple frontends | expand |
On 27/11/24 13:16, Philippe Mathieu-Daudé wrote: > While QEMU architecture bitmask values are only used by > system emulation code, they can be used in generic code > like TCG accelerator. > > Move the declarations to "qemu/arch_id.h" and add the > QemuArch type definition. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/qemu/arch_id.h | 28 ++++++++++++++++++++++++++++ > include/sysemu/arch_init.h | 28 +++------------------------- Alternatively we can restrict TCGCPUOps::arch_id to system emulation, using in the next patch: -- >8 -- diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h index ec3d2b50a9e..6fe0e1a7e97 100644 --- a/include/hw/core/tcg-cpu-ops.h +++ b/include/hw/core/tcg-cpu-ops.h @@ -19,8 +19,6 @@ #include "exec/vaddr.h" struct TCGCPUOps { - QemuArch arch_id; - /** * @initialize_once: Initialize TCG state * @@ -58,6 +56,7 @@ struct TCGCPUOps { void (*debug_excp_handler)(CPUState *cpu); #ifdef CONFIG_USER_ONLY + QemuArch arch_id; /** * @fake_user_interrupt: Callback for 'fake exception' handling. * diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index b37995f7d0c..31a2ab18e7c 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -1072,15 +1072,20 @@ bool tcg_exec_realizefn(CPUState *cpu, Error **errp) { static unsigned initialized_targets; const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops; +#ifndef CONFIG_USER_ONLY + unsigned arch_id = tcg_ops->arch_id; +#else + unsigned arch_id = 1; +#endif - if (!(initialized_targets & tcg_ops->arch_id)) { + if (!(initialized_targets & arch_id)) { /* Check mandatory TCGCPUOps handlers */ #ifndef CONFIG_USER_ONLY assert(tcg_ops->cpu_exec_halt); assert(tcg_ops->cpu_exec_interrupt); #endif /* !CONFIG_USER_ONLY */ tcg_ops->initialize_once(); - initialized_targets |= tcg_ops->arch_id; + initialized_targets |= arch_id; } --- But it add more #ifdef'ry and doesn't seem worthwhile IMHO.
On 11/27/24 06:16, Philippe Mathieu-Daudé wrote: > While QEMU architecture bitmask values are only used by > system emulation code, they can be used in generic code > like TCG accelerator. > > Move the declarations to "qemu/arch_id.h" and add the > QemuArch type definition. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/qemu/arch_id.h | 28 ++++++++++++++++++++++++++++ > include/sysemu/arch_init.h | 28 +++------------------------- > 2 files changed, 31 insertions(+), 25 deletions(-) > create mode 100644 include/qemu/arch_id.h > > diff --git a/include/qemu/arch_id.h b/include/qemu/arch_id.h > new file mode 100644 > index 00000000000..e3e8cf5e724 > --- /dev/null > +++ b/include/qemu/arch_id.h > @@ -0,0 +1,28 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +#ifndef QEMU_ARCH_ID_H > +#define QEMU_ARCH_ID_H > + > +typedef enum QemuArch { /* FIXME this is not an enum */ The comment is not useful. C enums are backed by an implementation type that can hold all values. > + QEMU_ARCH_ALL = -1, ... > + QEMU_ARCH_LOONGARCH = (1 << 23), ... which in this case means int32_t or int64_t, at the compiler's discretion. If you change QEMU_ARCH_ALL to (1 << 24) - 1, then uint32_t and uint64_t become possible implementation types. Are you perhaps being confused by C++ enums, which are more semantically restrictive? Anyway, the code movement is fine. r~
On 11/27/24 06:25, Philippe Mathieu-Daudé wrote: > Alternatively we can restrict TCGCPUOps::arch_id to > system emulation, using in the next patch: > > -- >8 -- > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h > index ec3d2b50a9e..6fe0e1a7e97 100644 > --- a/include/hw/core/tcg-cpu-ops.h > +++ b/include/hw/core/tcg-cpu-ops.h > @@ -19,8 +19,6 @@ > #include "exec/vaddr.h" > > struct TCGCPUOps { > - QemuArch arch_id; > - > /** > * @initialize_once: Initialize TCG state > * > @@ -58,6 +56,7 @@ struct TCGCPUOps { > void (*debug_excp_handler)(CPUState *cpu); > > #ifdef CONFIG_USER_ONLY > + QemuArch arch_id; > /** > * @fake_user_interrupt: Callback for 'fake exception' handling. > * > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index b37995f7d0c..31a2ab18e7c 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -1072,15 +1072,20 @@ bool tcg_exec_realizefn(CPUState *cpu, Error **errp) > { > static unsigned initialized_targets; > const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops; > +#ifndef CONFIG_USER_ONLY > + unsigned arch_id = tcg_ops->arch_id; > +#else > + unsigned arch_id = 1; > +#endif > > - if (!(initialized_targets & tcg_ops->arch_id)) { > + if (!(initialized_targets & arch_id)) { > /* Check mandatory TCGCPUOps handlers */ > #ifndef CONFIG_USER_ONLY > assert(tcg_ops->cpu_exec_halt); > assert(tcg_ops->cpu_exec_interrupt); > #endif /* !CONFIG_USER_ONLY */ > tcg_ops->initialize_once(); > - initialized_targets |= tcg_ops->arch_id; > + initialized_targets |= arch_id; > } > > --- > > But it add more #ifdef'ry and doesn't seem worthwhile IMHO. I agree, not worthwhile. r~
diff --git a/include/qemu/arch_id.h b/include/qemu/arch_id.h new file mode 100644 index 00000000000..e3e8cf5e724 --- /dev/null +++ b/include/qemu/arch_id.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef QEMU_ARCH_ID_H +#define QEMU_ARCH_ID_H + +typedef enum QemuArch { /* FIXME this is not an enum */ + QEMU_ARCH_ALL = -1, + QEMU_ARCH_ALPHA = (1 << 0), + QEMU_ARCH_ARM = (1 << 1), + QEMU_ARCH_I386 = (1 << 3), + QEMU_ARCH_M68K = (1 << 4), + QEMU_ARCH_MICROBLAZE = (1 << 6), + QEMU_ARCH_MIPS = (1 << 7), + QEMU_ARCH_PPC = (1 << 8), + QEMU_ARCH_S390X = (1 << 9), + QEMU_ARCH_SH4 = (1 << 10), + QEMU_ARCH_SPARC = (1 << 11), + QEMU_ARCH_XTENSA = (1 << 12), + QEMU_ARCH_OPENRISC = (1 << 13), + QEMU_ARCH_TRICORE = (1 << 16), + QEMU_ARCH_HPPA = (1 << 18), + QEMU_ARCH_RISCV = (1 << 19), + QEMU_ARCH_RX = (1 << 20), + QEMU_ARCH_AVR = (1 << 21), + QEMU_ARCH_HEXAGON = (1 << 22), + QEMU_ARCH_LOONGARCH = (1 << 23), +} QemuArch; + +#endif diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h index 5b1c1026f3a..01106de5bcb 100644 --- a/include/sysemu/arch_init.h +++ b/include/sysemu/arch_init.h @@ -1,29 +1,7 @@ -#ifndef QEMU_ARCH_INIT_H -#define QEMU_ARCH_INIT_H +#ifndef SYSEMU_ARCH_INIT_H +#define SYSEMU_ARCH_INIT_H - -enum { - QEMU_ARCH_ALL = -1, - QEMU_ARCH_ALPHA = (1 << 0), - QEMU_ARCH_ARM = (1 << 1), - QEMU_ARCH_I386 = (1 << 3), - QEMU_ARCH_M68K = (1 << 4), - QEMU_ARCH_MICROBLAZE = (1 << 6), - QEMU_ARCH_MIPS = (1 << 7), - QEMU_ARCH_PPC = (1 << 8), - QEMU_ARCH_S390X = (1 << 9), - QEMU_ARCH_SH4 = (1 << 10), - QEMU_ARCH_SPARC = (1 << 11), - QEMU_ARCH_XTENSA = (1 << 12), - QEMU_ARCH_OPENRISC = (1 << 13), - QEMU_ARCH_TRICORE = (1 << 16), - QEMU_ARCH_HPPA = (1 << 18), - QEMU_ARCH_RISCV = (1 << 19), - QEMU_ARCH_RX = (1 << 20), - QEMU_ARCH_AVR = (1 << 21), - QEMU_ARCH_HEXAGON = (1 << 22), - QEMU_ARCH_LOONGARCH = (1 << 23), -}; +#include "qemu/arch_id.h" extern const uint32_t arch_type;
While QEMU architecture bitmask values are only used by system emulation code, they can be used in generic code like TCG accelerator. Move the declarations to "qemu/arch_id.h" and add the QemuArch type definition. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/qemu/arch_id.h | 28 ++++++++++++++++++++++++++++ include/sysemu/arch_init.h | 28 +++------------------------- 2 files changed, 31 insertions(+), 25 deletions(-) create mode 100644 include/qemu/arch_id.h