diff mbox series

[PATCH-for-10.0,4/6] include: Expose QemuArch in 'qemu/arch_id.h'

Message ID 20241127121658.88966-5-philmd@linaro.org
State New
Headers show
Series accel/tcg: Allow tcg_exec_realizefn() initialize multiple frontends | expand

Commit Message

Philippe Mathieu-Daudé Nov. 27, 2024, 12:16 p.m. UTC 
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

Comments

Philippe Mathieu-Daudé Nov. 27, 2024, 12:25 p.m. UTC | #1 
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.
Richard Henderson Nov. 27, 2024, 4:45 p.m. UTC | #2 
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~
Richard Henderson Nov. 27, 2024, 4:46 p.m. UTC | #3 
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 mbox series

Patch

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;