[PATCH 00/10] accel/tcg: refactor the cpu-exec loop

[PATCH 00/10] accel/tcg: refactor the cpu-exec loop

[Alex Bennée]

This is an RFC for something I was playing with over the weekend to
try and reduce target specific details polluting cpu-exec. In the end
it does get rid of TARGET_I386 #ifdef'ery but I couldn't go further to
just building one version of cpu-exec-user and cpu-exec-softmmu
because there are still per target differences.

Anyway what do people think?

Alex Bennée (10):
  metadata: add .git-blame-ignore-revs
  accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu
  accel/tcg: move i386 halt handling to sysemu_ops
  accel/tcg: don't bother with ifdef for CPU_DUMP_CCOP
  accel/tcg: remove the fake_user_interrupt guards
  includes: move irq definitions out of cpu-all.h
  accel/tcg: use QEMU_IOTHREAD_LOCK_GUARD to cover the exit
  accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback
  accel/tcg: re-inline the filtering of virtual IRQs but data driven
  accel/tcg: remove unused includes

 include/exec/cpu-all.h           |  52 +--------------
 include/exec/cpu-irq.h           |  83 +++++++++++++++++++++++
 include/exec/poison.h            |  13 ----
 include/hw/core/sysemu-cpu-ops.h |  16 +++++
 include/hw/core/tcg-cpu-ops.h    |   7 +-
 target/i386/cpu-internal.h       |   2 +
 accel/tcg/cpu-exec-common.c      |  30 ---------
 accel/tcg/cpu-exec-softmmu.c     |  66 +++++++++++++++++++
 accel/tcg/cpu-exec.c             | 109 ++++++++++---------------------
 target/i386/cpu-sysemu.c         |  29 ++++++++
 target/i386/cpu.c                |   2 +
 target/i386/tcg/tcg-cpu.c        |   1 +
 .git-blame-ignore-revs           |  18 +++++
 accel/tcg/meson.build            |  10 +++
 14 files changed, 267 insertions(+), 171 deletions(-)
 create mode 100644 include/exec/cpu-irq.h
 create mode 100644 accel/tcg/cpu-exec-softmmu.c
 create mode 100644 .git-blame-ignore-revs

-- 
2.39.2

[PATCH 01/10] metadata: add .git-blame-ignore-revs

[Alex Bennée]

Someone mentioned this on IRC so I thought I would try it out with a
few commits that are pure code style fixes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 .git-blame-ignore-revs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 .git-blame-ignore-revs

diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs
new file mode 100644
index 0000000000..24208ece8c
--- /dev/null
+++ b/.git-blame-ignore-revs
@@ -0,0 +1,18 @@
+#
+# List of code-formatting clean ups the git blame can ignore
+#
+#   git blame --ignore-revs-file .git-blame-ignore-revs
+#
+# or
+#
+#   git config blame.ignoreRevsFile .git-blame-ignore-revs
+#
+
+# gdbstub: clean-up indents
+ad9e4585b3c7425759d3eea697afbca71d2c2082
+
+# e1000e: fix code style
+0eadd56bf53ab196a16d492d7dd31c62e1c24c32
+
+# target/riscv: coding style fixes
+8c7feddddd9218b407792120bcfda0347ed16205
-- 
2.39.2

Re: [PATCH 01/10] metadata: add .git-blame-ignore-revs

[Philippe Mathieu-Daudé]

On 20/3/23 11:10, Alex Bennée wrote:
> Someone mentioned this on IRC so I thought I would try it out with a
> few commits that are pure code style fixes.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   .git-blame-ignore-revs | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>   create mode 100644 .git-blame-ignore-revs
> 
> diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs
> new file mode 100644
> index 0000000000..24208ece8c
> --- /dev/null
> +++ b/.git-blame-ignore-revs
> @@ -0,0 +1,18 @@
> +#
> +# List of code-formatting clean ups the git blame can ignore
> +#
> +#   git blame --ignore-revs-file .git-blame-ignore-revs
> +#
> +# or
> +#
> +#   git config blame.ignoreRevsFile .git-blame-ignore-revs
> +#
> +
> +# gdbstub: clean-up indents
> +ad9e4585b3c7425759d3eea697afbca71d2c2082
> +
> +# e1000e: fix code style
> +0eadd56bf53ab196a16d492d7dd31c62e1c24c32
> +
> +# target/riscv: coding style fixes
> +8c7feddddd9218b407792120bcfda0347ed16205

Please amend:

+# replace TABs with spaces
+48805df9c22a0700fba4b3b548fafaa21726ca68

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Alex Bennée]

This doesn't save much as cpu-exec-common still needs to be built
per-target for its knowledge of CPUState but this helps with keeping
things organised.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cpu-exec-common.c  | 30 ----------------------
 accel/tcg/cpu-exec-softmmu.c | 50 ++++++++++++++++++++++++++++++++++++
 accel/tcg/meson.build        | 10 ++++++++
 3 files changed, 60 insertions(+), 30 deletions(-)
 create mode 100644 accel/tcg/cpu-exec-softmmu.c

diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
index e7962c9348..c6b0ad303e 100644
--- a/accel/tcg/cpu-exec-common.c
+++ b/accel/tcg/cpu-exec-common.c
@@ -32,36 +32,6 @@ void cpu_loop_exit_noexc(CPUState *cpu)
     cpu_loop_exit(cpu);
 }
 
-#if defined(CONFIG_SOFTMMU)
-void cpu_reloading_memory_map(void)
-{
-    if (qemu_in_vcpu_thread() && current_cpu->running) {
-        /* The guest can in theory prolong the RCU critical section as long
-         * as it feels like. The major problem with this is that because it
-         * can do multiple reconfigurations of the memory map within the
-         * critical section, we could potentially accumulate an unbounded
-         * collection of memory data structures awaiting reclamation.
-         *
-         * Because the only thing we're currently protecting with RCU is the
-         * memory data structures, it's sufficient to break the critical section
-         * in this callback, which we know will get called every time the
-         * memory map is rearranged.
-         *
-         * (If we add anything else in the system that uses RCU to protect
-         * its data structures, we will need to implement some other mechanism
-         * to force TCG CPUs to exit the critical section, at which point this
-         * part of this callback might become unnecessary.)
-         *
-         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
-         * only protects cpu->as->dispatch. Since we know our caller is about
-         * to reload it, it's safe to split the critical section.
-         */
-        rcu_read_unlock();
-        rcu_read_lock();
-    }
-}
-#endif
-
 void cpu_loop_exit(CPUState *cpu)
 {
     /* Undo the setting in cpu_tb_exec.  */
diff --git a/accel/tcg/cpu-exec-softmmu.c b/accel/tcg/cpu-exec-softmmu.c
new file mode 100644
index 0000000000..2318dd8c7d
--- /dev/null
+++ b/accel/tcg/cpu-exec-softmmu.c
@@ -0,0 +1,50 @@
+/*
+ *  Emulator main CPU execution loop, softmmu bits
+ *
+ *  Copyright (c) 2003-2005 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu.h"
+#include "sysemu/cpus.h"
+
+void cpu_reloading_memory_map(void)
+{
+    if (qemu_in_vcpu_thread() && current_cpu->running) {
+        /* The guest can in theory prolong the RCU critical section as long
+         * as it feels like. The major problem with this is that because it
+         * can do multiple reconfigurations of the memory map within the
+         * critical section, we could potentially accumulate an unbounded
+         * collection of memory data structures awaiting reclamation.
+         *
+         * Because the only thing we're currently protecting with RCU is the
+         * memory data structures, it's sufficient to break the critical section
+         * in this callback, which we know will get called every time the
+         * memory map is rearranged.
+         *
+         * (If we add anything else in the system that uses RCU to protect
+         * its data structures, we will need to implement some other mechanism
+         * to force TCG CPUs to exit the critical section, at which point this
+         * part of this callback might become unnecessary.)
+         *
+         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
+         * only protects cpu->as->dispatch. Since we know our caller is about
+         * to reload it, it's safe to split the critical section.
+         */
+        rcu_read_unlock();
+        rcu_read_lock();
+    }
+}
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index aeb20a6ef0..bdc086b90d 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -1,3 +1,9 @@
+#
+# Currently most things here end up in specific_ss eventually because
+# they need knowledge of CPUState. Stuff that that doesn't can live in
+# common user, softmmu or overall code
+#
+
 tcg_ss = ss.source_set()
 tcg_ss.add(files(
   'tcg-all.c',
@@ -9,6 +15,7 @@ tcg_ss.add(files(
   'translate-all.c',
   'translator.c',
 ))
+
 tcg_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
 tcg_ss.add(when: 'CONFIG_SOFTMMU', if_false: files('user-exec-stub.c'))
 tcg_ss.add(when: 'CONFIG_PLUGIN', if_true: [files('plugin-gen.c')])
@@ -27,3 +34,6 @@ tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
   'tcg-accel-ops-icount.c',
   'tcg-accel-ops-rr.c',
 ))
+
+# Common softmmu code
+softmmu_ss.add(files('cpu-exec-softmmu.c'))
-- 
2.39.2

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Claudio Fontana]

How is this conditional on CONFIG_TCG? To me it looks like this breaks !CONFIG_TCG.
Careful, the meson.build in accel/tcg/meson.build is always recursed.

This code was in tcg_ss before, why not simply add it to tcg_ss and then to specific_ss along with the other tcg pieces?

Ciao,

C


On 3/20/23 11:10, Alex Bennée wrote:
> This doesn't save much as cpu-exec-common still needs to be built
> per-target for its knowledge of CPUState but this helps with keeping
> things organised.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  accel/tcg/cpu-exec-common.c  | 30 ----------------------
>  accel/tcg/cpu-exec-softmmu.c | 50 ++++++++++++++++++++++++++++++++++++
>  accel/tcg/meson.build        | 10 ++++++++
>  3 files changed, 60 insertions(+), 30 deletions(-)
>  create mode 100644 accel/tcg/cpu-exec-softmmu.c
> 
> diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
> index e7962c9348..c6b0ad303e 100644
> --- a/accel/tcg/cpu-exec-common.c
> +++ b/accel/tcg/cpu-exec-common.c
> @@ -32,36 +32,6 @@ void cpu_loop_exit_noexc(CPUState *cpu)
>      cpu_loop_exit(cpu);
>  }
>  
> -#if defined(CONFIG_SOFTMMU)
> -void cpu_reloading_memory_map(void)
> -{
> -    if (qemu_in_vcpu_thread() && current_cpu->running) {
> -        /* The guest can in theory prolong the RCU critical section as long
> -         * as it feels like. The major problem with this is that because it
> -         * can do multiple reconfigurations of the memory map within the
> -         * critical section, we could potentially accumulate an unbounded
> -         * collection of memory data structures awaiting reclamation.
> -         *
> -         * Because the only thing we're currently protecting with RCU is the
> -         * memory data structures, it's sufficient to break the critical section
> -         * in this callback, which we know will get called every time the
> -         * memory map is rearranged.
> -         *
> -         * (If we add anything else in the system that uses RCU to protect
> -         * its data structures, we will need to implement some other mechanism
> -         * to force TCG CPUs to exit the critical section, at which point this
> -         * part of this callback might become unnecessary.)
> -         *
> -         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
> -         * only protects cpu->as->dispatch. Since we know our caller is about
> -         * to reload it, it's safe to split the critical section.
> -         */
> -        rcu_read_unlock();
> -        rcu_read_lock();
> -    }
> -}
> -#endif
> -
>  void cpu_loop_exit(CPUState *cpu)
>  {
>      /* Undo the setting in cpu_tb_exec.  */
> diff --git a/accel/tcg/cpu-exec-softmmu.c b/accel/tcg/cpu-exec-softmmu.c
> new file mode 100644
> index 0000000000..2318dd8c7d
> --- /dev/null
> +++ b/accel/tcg/cpu-exec-softmmu.c
> @@ -0,0 +1,50 @@
> +/*
> + *  Emulator main CPU execution loop, softmmu bits
> + *
> + *  Copyright (c) 2003-2005 Fabrice Bellard
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/cpu.h"
> +#include "sysemu/cpus.h"
> +
> +void cpu_reloading_memory_map(void)
> +{
> +    if (qemu_in_vcpu_thread() && current_cpu->running) {
> +        /* The guest can in theory prolong the RCU critical section as long
> +         * as it feels like. The major problem with this is that because it
> +         * can do multiple reconfigurations of the memory map within the
> +         * critical section, we could potentially accumulate an unbounded
> +         * collection of memory data structures awaiting reclamation.
> +         *
> +         * Because the only thing we're currently protecting with RCU is the
> +         * memory data structures, it's sufficient to break the critical section
> +         * in this callback, which we know will get called every time the
> +         * memory map is rearranged.
> +         *
> +         * (If we add anything else in the system that uses RCU to protect
> +         * its data structures, we will need to implement some other mechanism
> +         * to force TCG CPUs to exit the critical section, at which point this
> +         * part of this callback might become unnecessary.)
> +         *
> +         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
> +         * only protects cpu->as->dispatch. Since we know our caller is about
> +         * to reload it, it's safe to split the critical section.
> +         */
> +        rcu_read_unlock();
> +        rcu_read_lock();
> +    }
> +}
> diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
> index aeb20a6ef0..bdc086b90d 100644
> --- a/accel/tcg/meson.build
> +++ b/accel/tcg/meson.build
> @@ -1,3 +1,9 @@
> +#
> +# Currently most things here end up in specific_ss eventually because
> +# they need knowledge of CPUState. Stuff that that doesn't can live in
> +# common user, softmmu or overall code
> +#
> +
>  tcg_ss = ss.source_set()
>  tcg_ss.add(files(
>    'tcg-all.c',
> @@ -9,6 +15,7 @@ tcg_ss.add(files(
>    'translate-all.c',
>    'translator.c',
>  ))
> +
>  tcg_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
>  tcg_ss.add(when: 'CONFIG_SOFTMMU', if_false: files('user-exec-stub.c'))
>  tcg_ss.add(when: 'CONFIG_PLUGIN', if_true: [files('plugin-gen.c')])
> @@ -27,3 +34,6 @@ tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
>    'tcg-accel-ops-icount.c',
>    'tcg-accel-ops-rr.c',
>  ))
> +
> +# Common softmmu code
> +softmmu_ss.add(files('cpu-exec-softmmu.c'))

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> How is this conditional on CONFIG_TCG? To me it looks like this breaks !CONFIG_TCG.
> Careful, the meson.build in accel/tcg/meson.build is always recursed.

Surely it shouldn't be in accel/tcg then?

> This code was in tcg_ss before, why not simply add it to tcg_ss and
> then to specific_ss along with the other tcg pieces?

tcg_ss is rebuilt for every target. So far the code in cpu-exec-softmmu
should only need to for softmmu targets and hopefully share the same
binary for all variants.

I guess I'll have to do something more in line with the recent
re-factoring of gdbstub:

  # We build two versions of gdbstub, one for each mode
  gdb_user_ss.add(files('gdbstub.c', 'user.c'))
  gdb_softmmu_ss.add(files('gdbstub.c', 'softmmu.c'))

  gdb_user_ss = gdb_user_ss.apply(config_host, strict: false)
  gdb_softmmu_ss = gdb_softmmu_ss.apply(config_host, strict: false)

  libgdb_user = static_library('gdb_user',
                               gdb_user_ss.sources() + genh,
                               name_suffix: 'fa',
                               c_args: '-DCONFIG_USER_ONLY')

  libgdb_softmmu = static_library('gdb_softmmu',
                                  gdb_softmmu_ss.sources() + genh,
                                  name_suffix: 'fa')

  gdb_user = declare_dependency(link_whole: libgdb_user)
  user_ss.add(gdb_user)
  gdb_softmmu = declare_dependency(link_whole: libgdb_softmmu)
  softmmu_ss.add(gdb_softmmu)


>
> Ciao,
>
> C
>
>
> On 3/20/23 11:10, Alex Bennée wrote:
>> This doesn't save much as cpu-exec-common still needs to be built
>> per-target for its knowledge of CPUState but this helps with keeping
>> things organised.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  accel/tcg/cpu-exec-common.c  | 30 ----------------------
>>  accel/tcg/cpu-exec-softmmu.c | 50 ++++++++++++++++++++++++++++++++++++
>>  accel/tcg/meson.build        | 10 ++++++++
>>  3 files changed, 60 insertions(+), 30 deletions(-)
>>  create mode 100644 accel/tcg/cpu-exec-softmmu.c
>> 
>> diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
>> index e7962c9348..c6b0ad303e 100644
>> --- a/accel/tcg/cpu-exec-common.c
>> +++ b/accel/tcg/cpu-exec-common.c
>> @@ -32,36 +32,6 @@ void cpu_loop_exit_noexc(CPUState *cpu)
>>      cpu_loop_exit(cpu);
>>  }
>>  
>> -#if defined(CONFIG_SOFTMMU)
>> -void cpu_reloading_memory_map(void)
>> -{
>> -    if (qemu_in_vcpu_thread() && current_cpu->running) {
>> -        /* The guest can in theory prolong the RCU critical section as long
>> -         * as it feels like. The major problem with this is that because it
>> -         * can do multiple reconfigurations of the memory map within the
>> -         * critical section, we could potentially accumulate an unbounded
>> -         * collection of memory data structures awaiting reclamation.
>> -         *
>> -         * Because the only thing we're currently protecting with RCU is the
>> -         * memory data structures, it's sufficient to break the critical section
>> -         * in this callback, which we know will get called every time the
>> -         * memory map is rearranged.
>> -         *
>> -         * (If we add anything else in the system that uses RCU to protect
>> -         * its data structures, we will need to implement some other mechanism
>> -         * to force TCG CPUs to exit the critical section, at which point this
>> -         * part of this callback might become unnecessary.)
>> -         *
>> -         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
>> -         * only protects cpu->as->dispatch. Since we know our caller is about
>> -         * to reload it, it's safe to split the critical section.
>> -         */
>> -        rcu_read_unlock();
>> -        rcu_read_lock();
>> -    }
>> -}
>> -#endif
>> -
>>  void cpu_loop_exit(CPUState *cpu)
>>  {
>>      /* Undo the setting in cpu_tb_exec.  */
>> diff --git a/accel/tcg/cpu-exec-softmmu.c b/accel/tcg/cpu-exec-softmmu.c
>> new file mode 100644
>> index 0000000000..2318dd8c7d
>> --- /dev/null
>> +++ b/accel/tcg/cpu-exec-softmmu.c
>> @@ -0,0 +1,50 @@
>> +/*
>> + *  Emulator main CPU execution loop, softmmu bits
>> + *
>> + *  Copyright (c) 2003-2005 Fabrice Bellard
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/core/cpu.h"
>> +#include "sysemu/cpus.h"
>> +
>> +void cpu_reloading_memory_map(void)
>> +{
>> +    if (qemu_in_vcpu_thread() && current_cpu->running) {
>> +        /* The guest can in theory prolong the RCU critical section as long
>> +         * as it feels like. The major problem with this is that because it
>> +         * can do multiple reconfigurations of the memory map within the
>> +         * critical section, we could potentially accumulate an unbounded
>> +         * collection of memory data structures awaiting reclamation.
>> +         *
>> +         * Because the only thing we're currently protecting with RCU is the
>> +         * memory data structures, it's sufficient to break the critical section
>> +         * in this callback, which we know will get called every time the
>> +         * memory map is rearranged.
>> +         *
>> +         * (If we add anything else in the system that uses RCU to protect
>> +         * its data structures, we will need to implement some other mechanism
>> +         * to force TCG CPUs to exit the critical section, at which point this
>> +         * part of this callback might become unnecessary.)
>> +         *
>> +         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
>> +         * only protects cpu->as->dispatch. Since we know our caller is about
>> +         * to reload it, it's safe to split the critical section.
>> +         */
>> +        rcu_read_unlock();
>> +        rcu_read_lock();
>> +    }
>> +}
>> diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
>> index aeb20a6ef0..bdc086b90d 100644
>> --- a/accel/tcg/meson.build
>> +++ b/accel/tcg/meson.build
>> @@ -1,3 +1,9 @@
>> +#
>> +# Currently most things here end up in specific_ss eventually because
>> +# they need knowledge of CPUState. Stuff that that doesn't can live in
>> +# common user, softmmu or overall code
>> +#
>> +
>>  tcg_ss = ss.source_set()
>>  tcg_ss.add(files(
>>    'tcg-all.c',
>> @@ -9,6 +15,7 @@ tcg_ss.add(files(
>>    'translate-all.c',
>>    'translator.c',
>>  ))
>> +
>>  tcg_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
>>  tcg_ss.add(when: 'CONFIG_SOFTMMU', if_false: files('user-exec-stub.c'))
>>  tcg_ss.add(when: 'CONFIG_PLUGIN', if_true: [files('plugin-gen.c')])
>> @@ -27,3 +34,6 @@ tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
>>    'tcg-accel-ops-icount.c',
>>    'tcg-accel-ops-rr.c',
>>  ))
>> +
>> +# Common softmmu code
>> +softmmu_ss.add(files('cpu-exec-softmmu.c'))


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Claudio Fontana]

On 3/20/23 14:32, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> How is this conditional on CONFIG_TCG? To me it looks like this breaks !CONFIG_TCG.
>> Careful, the meson.build in accel/tcg/meson.build is always recursed.
> 
> Surely it shouldn't be in accel/tcg then?


Hi Alex,

maybe we did not understand each other.

What I mean is that accel/tcg/meson.build is not conditionally read, it is _always_ read.

Therefore TCG-specific code needs to be conditionally included using the CONFIG_TCG.

Ciao,

Claudio

> 
>> This code was in tcg_ss before, why not simply add it to tcg_ss and
>> then to specific_ss along with the other tcg pieces?
> 
> tcg_ss is rebuilt for every target. So far the code in cpu-exec-softmmu
> should only need to for softmmu targets and hopefully share the same
> binary for all variants.
> 
> I guess I'll have to do something more in line with the recent
> re-factoring of gdbstub:
> 
>   # We build two versions of gdbstub, one for each mode
>   gdb_user_ss.add(files('gdbstub.c', 'user.c'))
>   gdb_softmmu_ss.add(files('gdbstub.c', 'softmmu.c'))
> 
>   gdb_user_ss = gdb_user_ss.apply(config_host, strict: false)
>   gdb_softmmu_ss = gdb_softmmu_ss.apply(config_host, strict: false)
> 
>   libgdb_user = static_library('gdb_user',
>                                gdb_user_ss.sources() + genh,
>                                name_suffix: 'fa',
>                                c_args: '-DCONFIG_USER_ONLY')
> 
>   libgdb_softmmu = static_library('gdb_softmmu',
>                                   gdb_softmmu_ss.sources() + genh,
>                                   name_suffix: 'fa')
> 
>   gdb_user = declare_dependency(link_whole: libgdb_user)
>   user_ss.add(gdb_user)
>   gdb_softmmu = declare_dependency(link_whole: libgdb_softmmu)
>   softmmu_ss.add(gdb_softmmu)
> 
> 
>>
>> Ciao,
>>
>> C
>>
>>
>> On 3/20/23 11:10, Alex Bennée wrote:
>>> This doesn't save much as cpu-exec-common still needs to be built
>>> per-target for its knowledge of CPUState but this helps with keeping
>>> things organised.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  accel/tcg/cpu-exec-common.c  | 30 ----------------------
>>>  accel/tcg/cpu-exec-softmmu.c | 50 ++++++++++++++++++++++++++++++++++++
>>>  accel/tcg/meson.build        | 10 ++++++++
>>>  3 files changed, 60 insertions(+), 30 deletions(-)
>>>  create mode 100644 accel/tcg/cpu-exec-softmmu.c
>>>
>>> diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
>>> index e7962c9348..c6b0ad303e 100644
>>> --- a/accel/tcg/cpu-exec-common.c
>>> +++ b/accel/tcg/cpu-exec-common.c
>>> @@ -32,36 +32,6 @@ void cpu_loop_exit_noexc(CPUState *cpu)
>>>      cpu_loop_exit(cpu);
>>>  }
>>>  
>>> -#if defined(CONFIG_SOFTMMU)
>>> -void cpu_reloading_memory_map(void)
>>> -{
>>> -    if (qemu_in_vcpu_thread() && current_cpu->running) {
>>> -        /* The guest can in theory prolong the RCU critical section as long
>>> -         * as it feels like. The major problem with this is that because it
>>> -         * can do multiple reconfigurations of the memory map within the
>>> -         * critical section, we could potentially accumulate an unbounded
>>> -         * collection of memory data structures awaiting reclamation.
>>> -         *
>>> -         * Because the only thing we're currently protecting with RCU is the
>>> -         * memory data structures, it's sufficient to break the critical section
>>> -         * in this callback, which we know will get called every time the
>>> -         * memory map is rearranged.
>>> -         *
>>> -         * (If we add anything else in the system that uses RCU to protect
>>> -         * its data structures, we will need to implement some other mechanism
>>> -         * to force TCG CPUs to exit the critical section, at which point this
>>> -         * part of this callback might become unnecessary.)
>>> -         *
>>> -         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
>>> -         * only protects cpu->as->dispatch. Since we know our caller is about
>>> -         * to reload it, it's safe to split the critical section.
>>> -         */
>>> -        rcu_read_unlock();
>>> -        rcu_read_lock();
>>> -    }
>>> -}
>>> -#endif
>>> -
>>>  void cpu_loop_exit(CPUState *cpu)
>>>  {
>>>      /* Undo the setting in cpu_tb_exec.  */
>>> diff --git a/accel/tcg/cpu-exec-softmmu.c b/accel/tcg/cpu-exec-softmmu.c
>>> new file mode 100644
>>> index 0000000000..2318dd8c7d
>>> --- /dev/null
>>> +++ b/accel/tcg/cpu-exec-softmmu.c
>>> @@ -0,0 +1,50 @@
>>> +/*
>>> + *  Emulator main CPU execution loop, softmmu bits
>>> + *
>>> + *  Copyright (c) 2003-2005 Fabrice Bellard
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/core/cpu.h"
>>> +#include "sysemu/cpus.h"
>>> +
>>> +void cpu_reloading_memory_map(void)
>>> +{
>>> +    if (qemu_in_vcpu_thread() && current_cpu->running) {
>>> +        /* The guest can in theory prolong the RCU critical section as long
>>> +         * as it feels like. The major problem with this is that because it
>>> +         * can do multiple reconfigurations of the memory map within the
>>> +         * critical section, we could potentially accumulate an unbounded
>>> +         * collection of memory data structures awaiting reclamation.
>>> +         *
>>> +         * Because the only thing we're currently protecting with RCU is the
>>> +         * memory data structures, it's sufficient to break the critical section
>>> +         * in this callback, which we know will get called every time the
>>> +         * memory map is rearranged.
>>> +         *
>>> +         * (If we add anything else in the system that uses RCU to protect
>>> +         * its data structures, we will need to implement some other mechanism
>>> +         * to force TCG CPUs to exit the critical section, at which point this
>>> +         * part of this callback might become unnecessary.)
>>> +         *
>>> +         * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
>>> +         * only protects cpu->as->dispatch. Since we know our caller is about
>>> +         * to reload it, it's safe to split the critical section.
>>> +         */
>>> +        rcu_read_unlock();
>>> +        rcu_read_lock();
>>> +    }
>>> +}
>>> diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
>>> index aeb20a6ef0..bdc086b90d 100644
>>> --- a/accel/tcg/meson.build
>>> +++ b/accel/tcg/meson.build
>>> @@ -1,3 +1,9 @@
>>> +#
>>> +# Currently most things here end up in specific_ss eventually because
>>> +# they need knowledge of CPUState. Stuff that that doesn't can live in
>>> +# common user, softmmu or overall code
>>> +#
>>> +
>>>  tcg_ss = ss.source_set()
>>>  tcg_ss.add(files(
>>>    'tcg-all.c',
>>> @@ -9,6 +15,7 @@ tcg_ss.add(files(
>>>    'translate-all.c',
>>>    'translator.c',
>>>  ))
>>> +
>>>  tcg_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
>>>  tcg_ss.add(when: 'CONFIG_SOFTMMU', if_false: files('user-exec-stub.c'))
>>>  tcg_ss.add(when: 'CONFIG_PLUGIN', if_true: [files('plugin-gen.c')])
>>> @@ -27,3 +34,6 @@ tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
>>>    'tcg-accel-ops-icount.c',
>>>    'tcg-accel-ops-rr.c',
>>>  ))
>>> +
>>> +# Common softmmu code
>>> +softmmu_ss.add(files('cpu-exec-softmmu.c'))
> 
> 

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> On 3/20/23 14:32, Alex Bennée wrote:
>> 
>> Claudio Fontana <cfontana@suse.de> writes:
>> 
>>> How is this conditional on CONFIG_TCG? To me it looks like this breaks !CONFIG_TCG.
>>> Careful, the meson.build in accel/tcg/meson.build is always recursed.
>> 
>> Surely it shouldn't be in accel/tcg then?
>
>
> Hi Alex,
>
> maybe we did not understand each other.
>
> What I mean is that accel/tcg/meson.build is not conditionally read, it is _always_ read.
>
> Therefore TCG-specific code needs to be conditionally included using
> the CONFIG_TCG.

Ahh I see now, right I can fix that up next revision if there is
interest in this approach.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Richard Henderson]

On 3/20/23 07:33, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 3/20/23 14:32, Alex Bennée wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> How is this conditional on CONFIG_TCG? To me it looks like this breaks !CONFIG_TCG.
>>>> Careful, the meson.build in accel/tcg/meson.build is always recursed.
>>>
>>> Surely it shouldn't be in accel/tcg then?
>>
>>
>> Hi Alex,
>>
>> maybe we did not understand each other.
>>
>> What I mean is that accel/tcg/meson.build is not conditionally read, it is _always_ read.
>>
>> Therefore TCG-specific code needs to be conditionally included using
>> the CONFIG_TCG.
> 
> Ahh I see now, right I can fix that up next revision if there is
> interest in this approach.

Yes, that seems fine.  With either when: or subdir_done(),

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 02/10] accel/tcg: move cpu_reloading_memory_map into cpu-exec-softmmu

[Alessandro Di Federico via]

On Mon, 20 Mar 2023 10:10:27 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> This doesn't save much as cpu-exec-common still needs to be built
> per-target for its knowledge of CPUState but this helps with keeping
> things organised.

> --- /dev/null
> +++ b/accel/tcg/cpu-exec-softmmu.c

Could `cpu_reloading_memory_map` be pushed closer to its only user
(softmmu/physmem.c) instead of creating a new file in accel/tcg?

Maybe I'm missing something, but I see other usages of current_cpu in
softmmu:

    $ git grep 'current_cpu->' softmmu/|cat
    softmmu/cpus.c:        current_cpu->stop = true;
    softmmu/memory.c:        return current_cpu->cpu_index;
    softmmu/runstate.c:        current_cpu->crash_occurred = true;

Maybe you envision more stuff in cpu-exec-softmmu.c.

Reviewed-by: Alessandro Di Federico <ale@rev.ng>

-- 
Alessandro Di Federico
rev.ng Labs

[PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Alex Bennée]

We don't want to be polluting the core run loop code with target
specific handling, punt it to sysemu_ops where it belongs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/sysemu-cpu-ops.h |  5 +++++
 target/i386/cpu-internal.h       |  1 +
 accel/tcg/cpu-exec.c             | 14 +++-----------
 target/i386/cpu-sysemu.c         | 12 ++++++++++++
 target/i386/cpu.c                |  1 +
 5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index ee169b872c..c9d30172c4 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
      * GUEST_PANICKED events.
      */
     GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
+    /**
+     * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
+     * @cs: The CPUState
+     */
+    void (*handle_cpu_halt)(CPUState *cpu);
     /**
      * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
      * 32-bit VM coredump.
diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
index 9baac5c0b4..75b302fb33 100644
--- a/target/i386/cpu-internal.h
+++ b/target/i386/cpu-internal.h
@@ -65,6 +65,7 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
 void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
 void x86_cpu_machine_reset_cb(void *opaque);
+void x86_cpu_handle_halt(CPUState *cs);
 #endif /* !CONFIG_USER_ONLY */
 
 #endif /* I386_CPU_INTERNAL_H */
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index c815f2dbfd..5e5906e199 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -22,6 +22,7 @@
 #include "qapi/error.h"
 #include "qapi/type-helpers.h"
 #include "hw/core/tcg-cpu-ops.h"
+#include "hw/core/sysemu-cpu-ops.h"
 #include "trace.h"
 #include "disas/disas.h"
 #include "exec/exec-all.h"
@@ -30,9 +31,6 @@
 #include "qemu/rcu.h"
 #include "exec/log.h"
 #include "qemu/main-loop.h"
-#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
-#include "hw/i386/apic.h"
-#endif
 #include "sysemu/cpus.h"
 #include "exec/cpu-all.h"
 #include "sysemu/cpu-timers.h"
@@ -650,15 +648,9 @@ static inline bool cpu_handle_halt(CPUState *cpu)
 {
 #ifndef CONFIG_USER_ONLY
     if (cpu->halted) {
-#if defined(TARGET_I386)
-        if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
-            X86CPU *x86_cpu = X86_CPU(cpu);
-            qemu_mutex_lock_iothread();
-            apic_poll_irq(x86_cpu->apic_state);
-            cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
-            qemu_mutex_unlock_iothread();
+        if (cpu->cc->sysemu_ops->handle_cpu_halt) {
+            cpu->cc->sysemu_ops->handle_cpu_halt(cpu);
         }
-#endif /* TARGET_I386 */
         if (!cpu_has_work(cpu)) {
             return true;
         }
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 28115edf44..e545bf7590 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "sysemu/xen.h"
 #include "sysemu/whpx.h"
@@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
      }
 }
 
+void x86_cpu_handle_halt(CPUState *cpu)
+{
+    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
+        X86CPU *x86_cpu = X86_CPU(cpu);
+        qemu_mutex_lock_iothread();
+        apic_poll_irq(x86_cpu->apic_state);
+        cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
+        qemu_mutex_unlock_iothread();
+    }
+}
+
 GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6576287e5b..67027d28b0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7241,6 +7241,7 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
     .get_phys_page_attrs_debug = x86_cpu_get_phys_page_attrs_debug,
     .asidx_from_attrs = x86_asidx_from_attrs,
     .get_crash_info = x86_cpu_get_crash_info,
+    .handle_cpu_halt = x86_cpu_handle_halt,
     .write_elf32_note = x86_cpu_write_elf32_note,
     .write_elf64_note = x86_cpu_write_elf64_note,
     .write_elf32_qemunote = x86_cpu_write_elf32_qemunote,
-- 
2.39.2

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Philippe Mathieu-Daudé]

On 20/3/23 11:10, Alex Bennée wrote:
> We don't want to be polluting the core run loop code with target
> specific handling, punt it to sysemu_ops where it belongs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/sysemu-cpu-ops.h |  5 +++++
>   target/i386/cpu-internal.h       |  1 +
>   accel/tcg/cpu-exec.c             | 14 +++-----------
>   target/i386/cpu-sysemu.c         | 12 ++++++++++++
>   target/i386/cpu.c                |  1 +
>   5 files changed, 22 insertions(+), 11 deletions(-)


> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 28115edf44..e545bf7590 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -18,6 +18,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>   #include "cpu.h"
>   #include "sysemu/xen.h"
>   #include "sysemu/whpx.h"

Missing "hw/i386/apic.h" which declares apic_poll_irq() ...


> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>        }
>   }
>   
> +void x86_cpu_handle_halt(CPUState *cpu)
> +{
> +    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
> +        X86CPU *x86_cpu = X86_CPU(cpu);
> +        qemu_mutex_lock_iothread();
> +        apic_poll_irq(x86_cpu->apic_state);

... used here.

> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
> +        qemu_mutex_unlock_iothread();
> +    }
> +}

Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 20/3/23 11:10, Alex Bennée wrote:
>> We don't want to be polluting the core run loop code with target
>> specific handling, punt it to sysemu_ops where it belongs.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/hw/core/sysemu-cpu-ops.h |  5 +++++
>>   target/i386/cpu-internal.h       |  1 +
>>   accel/tcg/cpu-exec.c             | 14 +++-----------
>>   target/i386/cpu-sysemu.c         | 12 ++++++++++++
>>   target/i386/cpu.c                |  1 +
>>   5 files changed, 22 insertions(+), 11 deletions(-)
>
>
>> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
>> index 28115edf44..e545bf7590 100644
>> --- a/target/i386/cpu-sysemu.c
>> +++ b/target/i386/cpu-sysemu.c
>> @@ -18,6 +18,7 @@
>>    */
>>     #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>   #include "cpu.h"
>>   #include "sysemu/xen.h"
>>   #include "sysemu/whpx.h"
>
> Missing "hw/i386/apic.h" which declares apic_poll_irq() ...

I really need to get to the bottom of why some build hosts get away
without the include. Some additional path through the include maze
driven by config-host.h?
>
>
>> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>>        }
>>   }
>>   +void x86_cpu_handle_halt(CPUState *cpu)
>> +{
>> +    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
>> +        X86CPU *x86_cpu = X86_CPU(cpu);
>> +        qemu_mutex_lock_iothread();
>> +        apic_poll_irq(x86_cpu->apic_state);
>
> ... used here.
>
>> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>> +        qemu_mutex_unlock_iothread();
>> +    }
>> +}
>
> Otherwise,
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Claudio Fontana]

Hi Alex, all,

again, this moves TCG-only code to common code, no?

Even if this happens to work, the idea is to avoid adding unneeded accel TCG code to a KVM-only binary.

We need to keep in mind all dimensions when we do refactorings:

user-mode vs sysemu,
the architecture,
the accel, in particular tcg, non-tcg (which could be not compiled in, built-in, or loaded as separate module).

In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
but it is possible also to move in unneeded code in a way that does not generate compile or link-time errors, so we need to be a bit alert to that.

Ciao,

C 


On 3/20/23 11:10, Alex Bennée wrote:
> We don't want to be polluting the core run loop code with target
> specific handling, punt it to sysemu_ops where it belongs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/core/sysemu-cpu-ops.h |  5 +++++
>  target/i386/cpu-internal.h       |  1 +
>  accel/tcg/cpu-exec.c             | 14 +++-----------
>  target/i386/cpu-sysemu.c         | 12 ++++++++++++
>  target/i386/cpu.c                |  1 +
>  5 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index ee169b872c..c9d30172c4 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>       * GUEST_PANICKED events.
>       */
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
> +    /**
> +     * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
> +     * @cs: The CPUState
> +     */
> +    void (*handle_cpu_halt)(CPUState *cpu);
>      /**
>       * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>       * 32-bit VM coredump.
> diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
> index 9baac5c0b4..75b302fb33 100644
> --- a/target/i386/cpu-internal.h
> +++ b/target/i386/cpu-internal.h
> @@ -65,6 +65,7 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
>  void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
>  void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
>  void x86_cpu_machine_reset_cb(void *opaque);
> +void x86_cpu_handle_halt(CPUState *cs);
>  #endif /* !CONFIG_USER_ONLY */
>  
>  #endif /* I386_CPU_INTERNAL_H */
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index c815f2dbfd..5e5906e199 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -22,6 +22,7 @@
>  #include "qapi/error.h"
>  #include "qapi/type-helpers.h"
>  #include "hw/core/tcg-cpu-ops.h"
> +#include "hw/core/sysemu-cpu-ops.h"
>  #include "trace.h"
>  #include "disas/disas.h"
>  #include "exec/exec-all.h"
> @@ -30,9 +31,6 @@
>  #include "qemu/rcu.h"
>  #include "exec/log.h"
>  #include "qemu/main-loop.h"
> -#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
> -#include "hw/i386/apic.h"
> -#endif
>  #include "sysemu/cpus.h"
>  #include "exec/cpu-all.h"
>  #include "sysemu/cpu-timers.h"
> @@ -650,15 +648,9 @@ static inline bool cpu_handle_halt(CPUState *cpu)
>  {
>  #ifndef CONFIG_USER_ONLY
>      if (cpu->halted) {
> -#if defined(TARGET_I386)
> -        if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
> -            X86CPU *x86_cpu = X86_CPU(cpu);
> -            qemu_mutex_lock_iothread();
> -            apic_poll_irq(x86_cpu->apic_state);
> -            cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
> -            qemu_mutex_unlock_iothread();
> +        if (cpu->cc->sysemu_ops->handle_cpu_halt) {
> +            cpu->cc->sysemu_ops->handle_cpu_halt(cpu);
>          }
> -#endif /* TARGET_I386 */
>          if (!cpu_has_work(cpu)) {
>              return true;
>          }
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 28115edf44..e545bf7590 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "sysemu/xen.h"
>  #include "sysemu/whpx.h"
> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>       }
>  }
>  
> +void x86_cpu_handle_halt(CPUState *cpu)
> +{
> +    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
> +        X86CPU *x86_cpu = X86_CPU(cpu);
> +        qemu_mutex_lock_iothread();
> +        apic_poll_irq(x86_cpu->apic_state);
> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
> +        qemu_mutex_unlock_iothread();
> +    }
> +}
> +
>  GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 6576287e5b..67027d28b0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7241,6 +7241,7 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
>      .get_phys_page_attrs_debug = x86_cpu_get_phys_page_attrs_debug,
>      .asidx_from_attrs = x86_asidx_from_attrs,
>      .get_crash_info = x86_cpu_get_crash_info,
> +    .handle_cpu_halt = x86_cpu_handle_halt,
>      .write_elf32_note = x86_cpu_write_elf32_note,
>      .write_elf64_note = x86_cpu_write_elf64_note,
>      .write_elf32_qemunote = x86_cpu_write_elf32_qemunote,

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Philippe Mathieu-Daudé]

On 20/3/23 16:23, Claudio Fontana wrote:
> Hi Alex, all,
> 
> again, this moves TCG-only code to common code, no?

Oh, good point.

> Even if this happens to work, the idea is to avoid adding unneeded accel TCG code to a KVM-only binary.

Could yet another AccelSysemuCPUOps *accel struct in SysemuCPUOps
help being stricter? ...

> We need to keep in mind all dimensions when we do refactorings:
> 
> user-mode vs sysemu,
> the architecture,
> the accel, in particular tcg, non-tcg (which could be not compiled in, built-in, or loaded as separate module).
> 
> In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
> but it is possible also to move in unneeded code in a way that does not generate compile or link-time errors, so we need to be a bit alert to that.
> 
> Ciao,
> 
> C
> 
> 
> On 3/20/23 11:10, Alex Bennée wrote:
>> We don't want to be polluting the core run loop code with target
>> specific handling, punt it to sysemu_ops where it belongs.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/hw/core/sysemu-cpu-ops.h |  5 +++++
>>   target/i386/cpu-internal.h       |  1 +
>>   accel/tcg/cpu-exec.c             | 14 +++-----------
>>   target/i386/cpu-sysemu.c         | 12 ++++++++++++
>>   target/i386/cpu.c                |  1 +
>>   5 files changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
>> index ee169b872c..c9d30172c4 100644
>> --- a/include/hw/core/sysemu-cpu-ops.h
>> +++ b/include/hw/core/sysemu-cpu-ops.h
>> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>>        * GUEST_PANICKED events.
>>        */
>>       GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>> +    /**
>> +     * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
>> +     * @cs: The CPUState
>> +     */

Perhaps insert within a 'tcg' structure for now.

     #ifdef CONFIG_TCG
     struct {

>> +    void (*handle_cpu_halt)(CPUState *cpu);

     } tcg;
     #endif

Then we could extract as accel.

>>       /**
>>        * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>>        * 32-bit VM coredump.

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Claudio Fontana]

On 3/20/23 16:34, Philippe Mathieu-Daudé wrote:
> On 20/3/23 16:23, Claudio Fontana wrote:
>> Hi Alex, all,
>>
>> again, this moves TCG-only code to common code, no?
> 
> Oh, good point.
> 
>> Even if this happens to work, the idea is to avoid adding unneeded accel TCG code to a KVM-only binary.
> 
> Could yet another AccelSysemuCPUOps *accel struct in SysemuCPUOps
> help being stricter? ...

Just a thought, in general I wonder if we could devise a less error prone way to keep things in the right place.
Just thinking out loud here, something like a QEMU_ATTRIBUTE_TCG, _KVM, ... to add to symbols to avoid ending up in the wrong binary.

Keeping in mind all these dimensions is probably very taxing, maybe getting some support from the build system would be beneficial,
checking that a build requested with specific features contains only compatible objects.

Any ideas?

Ciao,

C

> 
>> We need to keep in mind all dimensions when we do refactorings:
>>
>> user-mode vs sysemu,
>> the architecture,
>> the accel, in particular tcg, non-tcg (which could be not compiled in, built-in, or loaded as separate module).
>>
>> In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
>> but it is possible also to move in unneeded code in a way that does not generate compile or link-time errors, so we need to be a bit alert to that.
>>
>> Ciao,
>>
>> C
>>
>>
>> On 3/20/23 11:10, Alex Bennée wrote:
>>> We don't want to be polluting the core run loop code with target
>>> specific handling, punt it to sysemu_ops where it belongs.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>   include/hw/core/sysemu-cpu-ops.h |  5 +++++
>>>   target/i386/cpu-internal.h       |  1 +
>>>   accel/tcg/cpu-exec.c             | 14 +++-----------
>>>   target/i386/cpu-sysemu.c         | 12 ++++++++++++
>>>   target/i386/cpu.c                |  1 +
>>>   5 files changed, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
>>> index ee169b872c..c9d30172c4 100644
>>> --- a/include/hw/core/sysemu-cpu-ops.h
>>> +++ b/include/hw/core/sysemu-cpu-ops.h
>>> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>>>        * GUEST_PANICKED events.
>>>        */
>>>       GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>>> +    /**
>>> +     * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
>>> +     * @cs: The CPUState
>>> +     */
> 
> Perhaps insert within a 'tcg' structure for now.
> 
>      #ifdef CONFIG_TCG
>      struct {
> 
>>> +    void (*handle_cpu_halt)(CPUState *cpu);
> 
>      } tcg;
>      #endif
> 
> Then we could extract as accel.
> 
>>>       /**
>>>        * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>>>        * 32-bit VM coredump.
> 
> 

Re: [PATCH 03/10] accel/tcg: move i386 halt handling to sysemu_ops

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> Hi Alex, all,
>
> again, this moves TCG-only code to common code, no?
>
> Even if this happens to work, the idea is to avoid adding unneeded accel TCG code to a KVM-only binary.
>
> We need to keep in mind all dimensions when we do refactorings:
>
> user-mode vs sysemu,
> the architecture,
> the accel, in particular tcg, non-tcg (which could be not compiled in,
> built-in, or loaded as separate module).
>
> In many cases, testing with --disable-tcg --enable-kvm helps to avoid breakages,
> but it is possible also to move in unneeded code in a way that does
> not generate compile or link-time errors, so we need to be a bit alert
> to that.

You are right of course, it should be kept tcg only. I guess using
tcgops instead of sysemu_ops.

>
> Ciao,
>
> C 
>
>
> On 3/20/23 11:10, Alex Bennée wrote:
>> We don't want to be polluting the core run loop code with target
>> specific handling, punt it to sysemu_ops where it belongs.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  include/hw/core/sysemu-cpu-ops.h |  5 +++++
>>  target/i386/cpu-internal.h       |  1 +
>>  accel/tcg/cpu-exec.c             | 14 +++-----------
>>  target/i386/cpu-sysemu.c         | 12 ++++++++++++
>>  target/i386/cpu.c                |  1 +
>>  5 files changed, 22 insertions(+), 11 deletions(-)
>> 
>> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
>> index ee169b872c..c9d30172c4 100644
>> --- a/include/hw/core/sysemu-cpu-ops.h
>> +++ b/include/hw/core/sysemu-cpu-ops.h
>> @@ -48,6 +48,11 @@ typedef struct SysemuCPUOps {
>>       * GUEST_PANICKED events.
>>       */
>>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>> +    /**
>> +     * @handle_cpu_halt: Callback for special handling during cpu_handle_halt()
>> +     * @cs: The CPUState
>> +     */
>> +    void (*handle_cpu_halt)(CPUState *cpu);
>>      /**
>>       * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
>>       * 32-bit VM coredump.
>> diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
>> index 9baac5c0b4..75b302fb33 100644
>> --- a/target/i386/cpu-internal.h
>> +++ b/target/i386/cpu-internal.h
>> @@ -65,6 +65,7 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
>>  void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
>>  void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
>>  void x86_cpu_machine_reset_cb(void *opaque);
>> +void x86_cpu_handle_halt(CPUState *cs);
>>  #endif /* !CONFIG_USER_ONLY */
>>  
>>  #endif /* I386_CPU_INTERNAL_H */
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index c815f2dbfd..5e5906e199 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -22,6 +22,7 @@
>>  #include "qapi/error.h"
>>  #include "qapi/type-helpers.h"
>>  #include "hw/core/tcg-cpu-ops.h"
>> +#include "hw/core/sysemu-cpu-ops.h"
>>  #include "trace.h"
>>  #include "disas/disas.h"
>>  #include "exec/exec-all.h"
>> @@ -30,9 +31,6 @@
>>  #include "qemu/rcu.h"
>>  #include "exec/log.h"
>>  #include "qemu/main-loop.h"
>> -#if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
>> -#include "hw/i386/apic.h"
>> -#endif
>>  #include "sysemu/cpus.h"
>>  #include "exec/cpu-all.h"
>>  #include "sysemu/cpu-timers.h"
>> @@ -650,15 +648,9 @@ static inline bool cpu_handle_halt(CPUState *cpu)
>>  {
>>  #ifndef CONFIG_USER_ONLY
>>      if (cpu->halted) {
>> -#if defined(TARGET_I386)
>> -        if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
>> -            X86CPU *x86_cpu = X86_CPU(cpu);
>> -            qemu_mutex_lock_iothread();
>> -            apic_poll_irq(x86_cpu->apic_state);
>> -            cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>> -            qemu_mutex_unlock_iothread();
>> +        if (cpu->cc->sysemu_ops->handle_cpu_halt) {
>> +            cpu->cc->sysemu_ops->handle_cpu_halt(cpu);
>>          }
>> -#endif /* TARGET_I386 */
>>          if (!cpu_has_work(cpu)) {
>>              return true;
>>          }
>> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
>> index 28115edf44..e545bf7590 100644
>> --- a/target/i386/cpu-sysemu.c
>> +++ b/target/i386/cpu-sysemu.c
>> @@ -18,6 +18,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "cpu.h"
>>  #include "sysemu/xen.h"
>>  #include "sysemu/whpx.h"
>> @@ -310,6 +311,17 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>>       }
>>  }
>>  
>> +void x86_cpu_handle_halt(CPUState *cpu)
>> +{
>> +    if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
>> +        X86CPU *x86_cpu = X86_CPU(cpu);
>> +        qemu_mutex_lock_iothread();
>> +        apic_poll_irq(x86_cpu->apic_state);
>> +        cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>> +        qemu_mutex_unlock_iothread();
>> +    }
>> +}
>> +
>>  GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
>>  {
>>      X86CPU *cpu = X86_CPU(cs);
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 6576287e5b..67027d28b0 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -7241,6 +7241,7 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
>>      .get_phys_page_attrs_debug = x86_cpu_get_phys_page_attrs_debug,
>>      .asidx_from_attrs = x86_asidx_from_attrs,
>>      .get_crash_info = x86_cpu_get_crash_info,
>> +    .handle_cpu_halt = x86_cpu_handle_halt,
>>      .write_elf32_note = x86_cpu_write_elf32_note,
>>      .write_elf64_note = x86_cpu_write_elf64_note,
>>      .write_elf32_qemunote = x86_cpu_write_elf32_qemunote,


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 04/10] accel/tcg: don't bother with ifdef for CPU_DUMP_CCOP

[Alex Bennée]

While only i386 dumps anything useful for the flag it could
potentially be used by others. Front ends that don't understand the
flag will ignore it anyway.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cpu-exec.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 5e5906e199..f883be197f 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -309,14 +309,11 @@ static void log_cpu_exec(target_ulong pc, CPUState *cpu,
         if (qemu_loglevel_mask(CPU_LOG_TB_CPU)) {
             FILE *logfile = qemu_log_trylock();
             if (logfile) {
-                int flags = 0;
+                int flags = CPU_DUMP_CCOP;;
 
                 if (qemu_loglevel_mask(CPU_LOG_TB_FPU)) {
                     flags |= CPU_DUMP_FPU;
                 }
-#if defined(TARGET_I386)
-                flags |= CPU_DUMP_CCOP;
-#endif
                 cpu_dump_state(cpu, logfile, flags);
                 qemu_log_unlock(logfile);
             }
-- 
2.39.2

Re: [PATCH 04/10] accel/tcg: don't bother with ifdef for CPU_DUMP_CCOP

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> +                int flags = CPU_DUMP_CCOP;;

two ;

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 04/10] accel/tcg: don't bother with ifdef for CPU_DUMP_CCOP

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> +                int flags = CPU_DUMP_CCOP;;

Actually, since you can't turn it off, we should just remove it and the test in i386.


r~

[PATCH 05/10] accel/tcg: remove the fake_user_interrupt guards

[Alex Bennée]

At the cost of an empty tcg_ops field for most targets we can avoid
target specific hacks in cpu-exec.c

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/tcg-cpu-ops.h |  2 +-
 accel/tcg/cpu-exec.c          | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index 20e3c0ffbb..66c0cecdde 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -50,7 +50,7 @@ struct TCGCPUOps {
     void (*debug_excp_handler)(CPUState *cpu);
 
 #ifdef NEED_CPU_H
-#if defined(CONFIG_USER_ONLY) && defined(TARGET_I386)
+#if defined(CONFIG_USER_ONLY)
     /**
      * @fake_user_interrupt: Callback for 'fake exception' handling.
      *
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index f883be197f..ea2e7004fe 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -698,13 +698,13 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
         return true;
     } else {
 #if defined(CONFIG_USER_ONLY)
-        /* if user mode only, we simulate a fake exception
-           which will be handled outside the cpu execution
-           loop */
-#if defined(TARGET_I386)
-        CPUClass *cc = CPU_GET_CLASS(cpu);
-        cc->tcg_ops->fake_user_interrupt(cpu);
-#endif /* TARGET_I386 */
+        /*
+         * For some user mode handling we simulate a fake exception
+         * which will be handled outside the cpu execution loop
+         */
+        if (cpu->cc->tcg_ops->fake_user_interrupt) {
+            cpu->cc->tcg_ops->fake_user_interrupt(cpu);
+        }
         *ret = cpu->exception_index;
         cpu->exception_index = -1;
         return true;
-- 
2.39.2

Re: [PATCH 05/10] accel/tcg: remove the fake_user_interrupt guards

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> At the cost of an empty tcg_ops field for most targets we can avoid
> target specific hacks in cpu-exec.c
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/hw/core/tcg-cpu-ops.h |  2 +-
>   accel/tcg/cpu-exec.c          | 14 +++++++-------
>   2 files changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 06/10] includes: move irq definitions out of cpu-all.h

[Alex Bennée]

These are common across all versions of the system so it would help if
we could use them for common code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/cpu-all.h | 52 +-------------------------
 include/exec/cpu-irq.h | 83 ++++++++++++++++++++++++++++++++++++++++++
 include/exec/poison.h  | 13 -------
 3 files changed, 84 insertions(+), 64 deletions(-)
 create mode 100644 include/exec/cpu-irq.h

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 2eb1176538..6b8085cf19 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -297,57 +297,7 @@ void *page_get_target_data(target_ulong address)
 
 CPUArchState *cpu_copy(CPUArchState *env);
 
-/* Flags for use in ENV->INTERRUPT_PENDING.
-
-   The numbers assigned here are non-sequential in order to preserve
-   binary compatibility with the vmstate dump.  Bit 0 (0x0001) was
-   previously used for CPU_INTERRUPT_EXIT, and is cleared when loading
-   the vmstate dump.  */
-
-/* External hardware interrupt pending.  This is typically used for
-   interrupts from devices.  */
-#define CPU_INTERRUPT_HARD        0x0002
-
-/* Exit the current TB.  This is typically used when some system-level device
-   makes some change to the memory mapping.  E.g. the a20 line change.  */
-#define CPU_INTERRUPT_EXITTB      0x0004
-
-/* Halt the CPU.  */
-#define CPU_INTERRUPT_HALT        0x0020
-
-/* Debug event pending.  */
-#define CPU_INTERRUPT_DEBUG       0x0080
-
-/* Reset signal.  */
-#define CPU_INTERRUPT_RESET       0x0400
-
-/* Several target-specific external hardware interrupts.  Each target/cpu.h
-   should define proper names based on these defines.  */
-#define CPU_INTERRUPT_TGT_EXT_0   0x0008
-#define CPU_INTERRUPT_TGT_EXT_1   0x0010
-#define CPU_INTERRUPT_TGT_EXT_2   0x0040
-#define CPU_INTERRUPT_TGT_EXT_3   0x0200
-#define CPU_INTERRUPT_TGT_EXT_4   0x1000
-
-/* Several target-specific internal interrupts.  These differ from the
-   preceding target-specific interrupts in that they are intended to
-   originate from within the cpu itself, typically in response to some
-   instruction being executed.  These, therefore, are not masked while
-   single-stepping within the debugger.  */
-#define CPU_INTERRUPT_TGT_INT_0   0x0100
-#define CPU_INTERRUPT_TGT_INT_1   0x0800
-#define CPU_INTERRUPT_TGT_INT_2   0x2000
-
-/* First unused bit: 0x4000.  */
-
-/* The set of all bits that should be masked when single-stepping.  */
-#define CPU_INTERRUPT_SSTEP_MASK \
-    (CPU_INTERRUPT_HARD          \
-     | CPU_INTERRUPT_TGT_EXT_0   \
-     | CPU_INTERRUPT_TGT_EXT_1   \
-     | CPU_INTERRUPT_TGT_EXT_2   \
-     | CPU_INTERRUPT_TGT_EXT_3   \
-     | CPU_INTERRUPT_TGT_EXT_4)
+#include "exec/cpu-irq.h"
 
 #ifdef CONFIG_USER_ONLY
 
diff --git a/include/exec/cpu-irq.h b/include/exec/cpu-irq.h
new file mode 100644
index 0000000000..58bd98d812
--- /dev/null
+++ b/include/exec/cpu-irq.h
@@ -0,0 +1,83 @@
+/*
+ * Internal execution defines for qemu irqs
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef EXEC_CPU_IRQ_H
+#define EXEC_CPU_IRQ_H
+
+/*
+ * Flags for use in ENV->INTERRUPT_PENDING.
+ *
+ * The numbers assigned here are non-sequential in order to preserve
+ * binary compatibility with the vmstate dump.  Bit 0 (0x0001) was
+ * previously used for CPU_INTERRUPT_EXIT, and is cleared when loading
+ * the vmstate dump.
+ */
+
+/*
+ * External hardware interrupt pending.  This is typically used for
+ * interrupts from devices.
+ */
+#define CPU_INTERRUPT_HARD        0x0002
+
+/*
+ * Exit the current TB.  This is typically used when some system-level device
+ * makes some change to the memory mapping.  E.g. the a20 line change.
+ */
+#define CPU_INTERRUPT_EXITTB      0x0004
+
+/* Halt the CPU.  */
+#define CPU_INTERRUPT_HALT        0x0020
+
+/* Debug event pending.  */
+#define CPU_INTERRUPT_DEBUG       0x0080
+
+/* Reset signal.  */
+#define CPU_INTERRUPT_RESET       0x0400
+
+/* Several target-specific external hardware interrupts.  Each target/cpu.h
+   should define proper names based on these defines.  */
+#define CPU_INTERRUPT_TGT_EXT_0   0x0008
+#define CPU_INTERRUPT_TGT_EXT_1   0x0010
+#define CPU_INTERRUPT_TGT_EXT_2   0x0040
+#define CPU_INTERRUPT_TGT_EXT_3   0x0200
+#define CPU_INTERRUPT_TGT_EXT_4   0x1000
+
+/*
+ * Several target-specific internal interrupts. These differ from the
+ * preceding target-specific interrupts in that they are intended to
+ * originate from within the cpu itself, typically in response to some
+ * instruction being executed. These, therefore, are not masked while
+ * single-stepping within the debugger.
+ */
+#define CPU_INTERRUPT_TGT_INT_0   0x0100
+#define CPU_INTERRUPT_TGT_INT_1   0x0800
+#define CPU_INTERRUPT_TGT_INT_2   0x2000
+
+/* First unused bit: 0x4000.  */
+
+/* The set of all bits that should be masked when single-stepping.  */
+#define CPU_INTERRUPT_SSTEP_MASK \
+    (CPU_INTERRUPT_HARD          \
+     | CPU_INTERRUPT_TGT_EXT_0   \
+     | CPU_INTERRUPT_TGT_EXT_1   \
+     | CPU_INTERRUPT_TGT_EXT_2   \
+     | CPU_INTERRUPT_TGT_EXT_3   \
+     | CPU_INTERRUPT_TGT_EXT_4)
+
+#endif /* EXEC_CPU_IRQ_H */
diff --git a/include/exec/poison.h b/include/exec/poison.h
index 140daa4a85..a0ab1d7d46 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -52,19 +52,6 @@
 #pragma GCC poison TARGET_PAGE_BITS
 #pragma GCC poison TARGET_PAGE_ALIGN
 
-#pragma GCC poison CPU_INTERRUPT_HARD
-#pragma GCC poison CPU_INTERRUPT_EXITTB
-#pragma GCC poison CPU_INTERRUPT_HALT
-#pragma GCC poison CPU_INTERRUPT_DEBUG
-#pragma GCC poison CPU_INTERRUPT_TGT_EXT_0
-#pragma GCC poison CPU_INTERRUPT_TGT_EXT_1
-#pragma GCC poison CPU_INTERRUPT_TGT_EXT_2
-#pragma GCC poison CPU_INTERRUPT_TGT_EXT_3
-#pragma GCC poison CPU_INTERRUPT_TGT_EXT_4
-#pragma GCC poison CPU_INTERRUPT_TGT_INT_0
-#pragma GCC poison CPU_INTERRUPT_TGT_INT_1
-#pragma GCC poison CPU_INTERRUPT_TGT_INT_2
-
 #pragma GCC poison CONFIG_ALPHA_DIS
 #pragma GCC poison CONFIG_CRIS_DIS
 #pragma GCC poison CONFIG_HPPA_DIS
-- 
2.39.2

Re: [PATCH 06/10] includes: move irq definitions out of cpu-all.h

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> These are common across all versions of the system so it would help if
> we could use them for common code.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/exec/cpu-all.h | 52 +-------------------------
>   include/exec/cpu-irq.h | 83 ++++++++++++++++++++++++++++++++++++++++++
>   include/exec/poison.h  | 13 -------
>   3 files changed, 84 insertions(+), 64 deletions(-)
>   create mode 100644 include/exec/cpu-irq.h

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/10] includes: move irq definitions out of cpu-all.h

[Alessandro Di Federico via]

On Mon, 20 Mar 2023 10:10:31 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> +#define CPU_INTERRUPT_HARD        0x0002

Out of curiosity, do we have a policy when to use `const` globals as
opposed to `#define`?
In theory, if a constant is never used in any preprocessor directive,
we could turn it into a global `const`.

It'd be nice to reduce the pre-processor usage, sometimes I end up
debugging build issues by taking a compiler's command line and adding
`-E`, and the less you use the pre-processor, the better.

Reviewed-by: Alessandro Di Federico <ale@rev.ng>

-- 
Alessandro Di Federico
rev.ng Labs

Re: [PATCH 06/10] includes: move irq definitions out of cpu-all.h

[Richard Henderson]

On 3/21/23 09:06, Alessandro Di Federico wrote:
> On Mon, 20 Mar 2023 10:10:31 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
>> +#define CPU_INTERRUPT_HARD        0x0002
> 
> Out of curiosity, do we have a policy when to use `const` globals as
> opposed to `#define`?
> In theory, if a constant is never used in any preprocessor directive,
> we could turn it into a global `const`.

No, this is not c++.


r~

Re: [PATCH 06/10] includes: move irq definitions out of cpu-all.h

[Alessandro Di Federico]

On Tue, 21 Mar 2023 22:25:02 -0700
Richard Henderson <richard.henderson@linaro.org> wrote:

> No, this is not c++.

It's not C++, but this seems to be the direction the C language is
headed: C23 will introduce constexpr. No biggie though, I understand
it' be a huge change for little return.

-- 
Alessandro Di Federico
rev.ng Labs

[PATCH 07/10] accel/tcg: use QEMU_IOTHREAD_LOCK_GUARD to cover the exit

[Alex Bennée]

This avoids us having to make sure each exit path does an unlock.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cpu-exec.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index ea2e7004fe..daa6e24daf 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -774,7 +774,9 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
 
     if (unlikely(qatomic_read(&cpu->interrupt_request))) {
         int interrupt_request;
-        qemu_mutex_lock_iothread();
+        /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
+        QEMU_IOTHREAD_LOCK_GUARD();
+
         interrupt_request = cpu->interrupt_request;
         if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
             /* Mask out external interrupts for this step. */
@@ -783,7 +785,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
         if (interrupt_request & CPU_INTERRUPT_DEBUG) {
             cpu->interrupt_request &= ~CPU_INTERRUPT_DEBUG;
             cpu->exception_index = EXCP_DEBUG;
-            qemu_mutex_unlock_iothread();
             return true;
         }
 #if !defined(CONFIG_USER_ONLY)
@@ -794,7 +795,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
             cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
             cpu->halted = 1;
             cpu->exception_index = EXCP_HLT;
-            qemu_mutex_unlock_iothread();
             return true;
         }
 #if defined(TARGET_I386)
@@ -805,14 +805,12 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
             cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
             do_cpu_init(x86_cpu);
             cpu->exception_index = EXCP_HALTED;
-            qemu_mutex_unlock_iothread();
             return true;
         }
 #else
         else if (interrupt_request & CPU_INTERRUPT_RESET) {
             replay_interrupt();
             cpu_reset(cpu);
-            qemu_mutex_unlock_iothread();
             return true;
         }
 #endif /* !TARGET_I386 */
@@ -835,7 +833,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
                  */
                 if (unlikely(cpu->singlestep_enabled)) {
                     cpu->exception_index = EXCP_DEBUG;
-                    qemu_mutex_unlock_iothread();
                     return true;
                 }
                 cpu->exception_index = -1;
@@ -852,9 +849,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
                the program flow was changed */
             *last_tb = NULL;
         }
-
-        /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
-        qemu_mutex_unlock_iothread();
     }
 
     /* Finally, check if we need to exit to the main loop.  */
-- 
2.39.2

Re: [PATCH 07/10] accel/tcg: use QEMU_IOTHREAD_LOCK_GUARD to cover the exit

[Philippe Mathieu-Daudé]

On 20/3/23 11:10, Alex Bennée wrote:
> This avoids us having to make sure each exit path does an unlock.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/10] accel/tcg: use QEMU_IOTHREAD_LOCK_GUARD to cover the exit

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> This avoids us having to make sure each exit path does an unlock.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/sysemu-cpu-ops.h | 11 +++++++++++
 target/i386/cpu-internal.h       |  1 +
 accel/tcg/cpu-exec-softmmu.c     | 16 ++++++++++++++++
 accel/tcg/cpu-exec.c             | 31 ++++++++++---------------------
 target/i386/cpu-sysemu.c         | 17 +++++++++++++++++
 target/i386/cpu.c                |  1 +
 6 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index c9d30172c4..d53907b517 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -53,6 +53,15 @@ typedef struct SysemuCPUOps {
      * @cs: The CPUState
      */
     void (*handle_cpu_halt)(CPUState *cpu);
+    /**
+     * @handle_cpu_interrupt: handle init/reset interrupts
+     * @cs: The CPUState
+     * @irq_request: the interrupt request
+     *
+     * Most architectures share a common handler. Returns true if the
+     * handler did indeed handle and interrupt.
+     */
+    bool (*handle_cpu_interrupt)(CPUState *cpu,  int irq_request);
     /**
      * @write_elf32_note: Callback for writing a CPU-specific ELF note to a
      * 32-bit VM coredump.
@@ -94,4 +103,6 @@ typedef struct SysemuCPUOps {
 
 } SysemuCPUOps;
 
+bool common_cpu_handle_interrupt(CPUState *cpu,  int irq_request);
+
 #endif /* SYSEMU_CPU_OPS_H */
diff --git a/target/i386/cpu-internal.h b/target/i386/cpu-internal.h
index 75b302fb33..4fee4e125e 100644
--- a/target/i386/cpu-internal.h
+++ b/target/i386/cpu-internal.h
@@ -66,6 +66,7 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp);
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp);
 void x86_cpu_machine_reset_cb(void *opaque);
 void x86_cpu_handle_halt(CPUState *cs);
+bool x86_cpu_handle_interrupt(CPUState *cpu,  int irq_request);
 #endif /* !CONFIG_USER_ONLY */
 
 #endif /* I386_CPU_INTERNAL_H */
diff --git a/accel/tcg/cpu-exec-softmmu.c b/accel/tcg/cpu-exec-softmmu.c
index 2318dd8c7d..89e6cb2e3a 100644
--- a/accel/tcg/cpu-exec-softmmu.c
+++ b/accel/tcg/cpu-exec-softmmu.c
@@ -18,7 +18,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "exec/replay-core.h"
+#include "exec/cpu-irq.h"
 #include "hw/core/cpu.h"
+#include "hw/core/sysemu-cpu-ops.h"
 #include "sysemu/cpus.h"
 
 void cpu_reloading_memory_map(void)
@@ -48,3 +52,15 @@ void cpu_reloading_memory_map(void)
         rcu_read_lock();
     }
 }
+
+/* Called with BQL held */
+bool common_cpu_handle_interrupt(CPUState *cpu,  int interrupt_request)
+{
+    if (interrupt_request & CPU_INTERRUPT_RESET) {
+        replay_interrupt();
+        cpu_reset(cpu);
+        return true;
+    } else {
+        return false;
+    }
+}
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index daa6e24daf..8fa19b7222 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -797,28 +797,17 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
             cpu->exception_index = EXCP_HLT;
             return true;
         }
-#if defined(TARGET_I386)
-        else if (interrupt_request & CPU_INTERRUPT_INIT) {
-            X86CPU *x86_cpu = X86_CPU(cpu);
-            CPUArchState *env = &x86_cpu->env;
-            replay_interrupt();
-            cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
-            do_cpu_init(x86_cpu);
-            cpu->exception_index = EXCP_HALTED;
-            return true;
-        }
-#else
-        else if (interrupt_request & CPU_INTERRUPT_RESET) {
-            replay_interrupt();
-            cpu_reset(cpu);
+        else if (cpu->cc->sysemu_ops->handle_cpu_interrupt &&
+                 cpu->cc->sysemu_ops->handle_cpu_interrupt(cpu, interrupt_request)) {
+                return true;
+        } else if (common_cpu_handle_interrupt(cpu, interrupt_request)) {
             return true;
-        }
-#endif /* !TARGET_I386 */
-        /* The target hook has 3 exit conditions:
-           False when the interrupt isn't processed,
-           True when it is, and we should restart on a new TB,
-           and via longjmp via cpu_loop_exit.  */
-        else {
+        } else {
+            /*
+             * The target hook has 3 exit conditions: False when the
+             * interrupt isn't processed, True when it is, and we should
+             * restart on a new TB, and via longjmp via cpu_loop_exit.
+             */
             CPUClass *cc = CPU_GET_CLASS(cpu);
 
             if (cc->tcg_ops->cpu_exec_interrupt &&
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index e545bf7590..5638ed4aa4 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -31,6 +31,7 @@
 #include "hw/qdev-properties.h"
 
 #include "exec/address-spaces.h"
+#include "exec/replay-core.h"
 #include "hw/i386/apic_internal.h"
 
 #include "cpu-internal.h"
@@ -322,6 +323,22 @@ void x86_cpu_handle_halt(CPUState *cpu)
     }
 }
 
+/* Called with BQL held */
+bool x86_cpu_handle_interrupt(CPUState *cpu, int interrupt_request)
+{
+    if (interrupt_request & CPU_INTERRUPT_INIT) {
+        X86CPU *x86_cpu = X86_CPU(cpu);
+        CPUArchState *env = &x86_cpu->env;
+        replay_interrupt();
+        cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
+        do_cpu_init(x86_cpu);
+        cpu->exception_index = EXCP_HALTED;
+        return true;
+    } else {
+        return false;
+    }
+}
+
 GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 67027d28b0..1b66583987 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7242,6 +7242,7 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
     .asidx_from_attrs = x86_asidx_from_attrs,
     .get_crash_info = x86_cpu_get_crash_info,
     .handle_cpu_halt = x86_cpu_handle_halt,
+    .handle_cpu_interrupt = x86_cpu_handle_interrupt,
     .write_elf32_note = x86_cpu_write_elf32_note,
     .write_elf64_note = x86_cpu_write_elf64_note,
     .write_elf32_qemunote = x86_cpu_write_elf32_qemunote,
-- 
2.39.2

Re: [PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/sysemu-cpu-ops.h | 11 +++++++++++
>   target/i386/cpu-internal.h       |  1 +
>   accel/tcg/cpu-exec-softmmu.c     | 16 ++++++++++++++++
>   accel/tcg/cpu-exec.c             | 31 ++++++++++---------------------
>   target/i386/cpu-sysemu.c         | 17 +++++++++++++++++
>   target/i386/cpu.c                |  1 +
>   6 files changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index c9d30172c4..d53907b517 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -53,6 +53,15 @@ typedef struct SysemuCPUOps {
>        * @cs: The CPUState
>        */
>       void (*handle_cpu_halt)(CPUState *cpu);
> +    /**
> +     * @handle_cpu_interrupt: handle init/reset interrupts
> +     * @cs: The CPUState
> +     * @irq_request: the interrupt request
> +     *
> +     * Most architectures share a common handler. Returns true if the
> +     * handler did indeed handle and interrupt.
> +     */

and -> the? or any?

This should be a tcg hook, not a sysemu hook, per the previous one.
I would very much like it to never be NULL, but instead your new 
common_cpu_handle_interrupt function.

> -#if defined(TARGET_I386)
> -        else if (interrupt_request & CPU_INTERRUPT_INIT) {
> -            X86CPU *x86_cpu = X86_CPU(cpu);
> -            CPUArchState *env = &x86_cpu->env;
> -            replay_interrupt();
> -            cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
> -            do_cpu_init(x86_cpu);
> -            cpu->exception_index = EXCP_HALTED;
> -            return true;
> -        }
> -#else
> -        else if (interrupt_request & CPU_INTERRUPT_RESET) {
> -            replay_interrupt();
> -            cpu_reset(cpu);
> +        else if (cpu->cc->sysemu_ops->handle_cpu_interrupt &&
> +                 cpu->cc->sysemu_ops->handle_cpu_interrupt(cpu, interrupt_request)) {
> +                return true;
> +        } else if (common_cpu_handle_interrupt(cpu, interrupt_request)) {
>               return true;

... because this is pretty ugly, and incorrectly indented.


r~

Re: [PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/20/23 03:10, Alex Bennée wrote:
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/hw/core/sysemu-cpu-ops.h | 11 +++++++++++
>>   target/i386/cpu-internal.h       |  1 +
>>   accel/tcg/cpu-exec-softmmu.c     | 16 ++++++++++++++++
>>   accel/tcg/cpu-exec.c             | 31 ++++++++++---------------------
>>   target/i386/cpu-sysemu.c         | 17 +++++++++++++++++
>>   target/i386/cpu.c                |  1 +
>>   6 files changed, 56 insertions(+), 21 deletions(-)
>> diff --git a/include/hw/core/sysemu-cpu-ops.h
>> b/include/hw/core/sysemu-cpu-ops.h
>> index c9d30172c4..d53907b517 100644
>> --- a/include/hw/core/sysemu-cpu-ops.h
>> +++ b/include/hw/core/sysemu-cpu-ops.h
>> @@ -53,6 +53,15 @@ typedef struct SysemuCPUOps {
>>        * @cs: The CPUState
>>        */
>>       void (*handle_cpu_halt)(CPUState *cpu);
>> +    /**
>> +     * @handle_cpu_interrupt: handle init/reset interrupts
>> +     * @cs: The CPUState
>> +     * @irq_request: the interrupt request
>> +     *
>> +     * Most architectures share a common handler. Returns true if the
>> +     * handler did indeed handle and interrupt.
>> +     */
>
> and -> the? or any?
>
> This should be a tcg hook, not a sysemu hook, per the previous one.
> I would very much like it to never be NULL, but instead your new
> common_cpu_handle_interrupt function.

I was trying to figure out how to instantiate a default but ran into
const problems eventually forcing me to give up.

Why a TCG hook? Do we not process any interrupts for KVM or HVF?

>
>> -#if defined(TARGET_I386)
>> -        else if (interrupt_request & CPU_INTERRUPT_INIT) {
>> -            X86CPU *x86_cpu = X86_CPU(cpu);
>> -            CPUArchState *env = &x86_cpu->env;
>> -            replay_interrupt();
>> -            cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
>> -            do_cpu_init(x86_cpu);
>> -            cpu->exception_index = EXCP_HALTED;
>> -            return true;
>> -        }
>> -#else
>> -        else if (interrupt_request & CPU_INTERRUPT_RESET) {
>> -            replay_interrupt();
>> -            cpu_reset(cpu);
>> +        else if (cpu->cc->sysemu_ops->handle_cpu_interrupt &&
>> +                 cpu->cc->sysemu_ops->handle_cpu_interrupt(cpu, interrupt_request)) {
>> +                return true;
>> +        } else if (common_cpu_handle_interrupt(cpu, interrupt_request)) {
>>               return true;
>
> ... because this is pretty ugly, and incorrectly indented.
>
>
> r~


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 08/10] accel/tcg: push i386 specific hacks into handle_cpu_interrupt callback

[Richard Henderson]

On 3/20/23 10:14, Alex Bennée wrote:
>> This should be a tcg hook, not a sysemu hook, per the previous one.
>> I would very much like it to never be NULL, but instead your new
>> common_cpu_handle_interrupt function.
> 
> I was trying to figure out how to instantiate a default but ran into
> const problems eventually forcing me to give up.

You initialize it for each instance individually, not in one central place.

> Why a TCG hook? Do we not process any interrupts for KVM or HVF?

No.


r~

[PATCH 09/10] accel/tcg: re-inline the filtering of virtual IRQs but data driven

[Alex Bennée]

Although only I386 currently uses it it is not inconceivable that
other arches might find this facility useful.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/tcg-cpu-ops.h |  5 +++++
 accel/tcg/cpu-exec.c          | 29 +++++++++--------------------
 target/i386/tcg/tcg-cpu.c     |  1 +
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index 66c0cecdde..8e8df8c330 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -121,6 +121,11 @@ struct TCGCPUOps {
      */
     bool (*io_recompile_replay_branch)(CPUState *cpu,
                                        const TranslationBlock *tb);
+    /**
+     * @virtual_interrupts: IRQs that can be ignored for replay purposes
+     */
+    int virtual_interrupts;
+
 #else
     /**
      * record_sigsegv:
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 8fa19b7222..56be7956e7 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -737,22 +737,6 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
     return false;
 }
 
-#ifndef CONFIG_USER_ONLY
-/*
- * CPU_INTERRUPT_POLL is a virtual event which gets converted into a
- * "real" interrupt event later. It does not need to be recorded for
- * replay purposes.
- */
-static inline bool need_replay_interrupt(int interrupt_request)
-{
-#if defined(TARGET_I386)
-    return !(interrupt_request & CPU_INTERRUPT_POLL);
-#else
-    return true;
-#endif
-}
-#endif /* !CONFIG_USER_ONLY */
-
 static inline bool cpu_handle_interrupt(CPUState *cpu,
                                         TranslationBlock **last_tb)
 {
@@ -808,11 +792,16 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
              * interrupt isn't processed, True when it is, and we should
              * restart on a new TB, and via longjmp via cpu_loop_exit.
              */
-            CPUClass *cc = CPU_GET_CLASS(cpu);
+            struct TCGCPUOps const *tcg_ops = cpu->cc->tcg_ops;
 
-            if (cc->tcg_ops->cpu_exec_interrupt &&
-                cc->tcg_ops->cpu_exec_interrupt(cpu, interrupt_request)) {
-                if (need_replay_interrupt(interrupt_request)) {
+            if (tcg_ops->cpu_exec_interrupt &&
+                tcg_ops->cpu_exec_interrupt(cpu, interrupt_request)) {
+                /*
+                 * Virtual events gets converted into a "real"
+                 * interrupt event later. They do not need to be
+                 * recorded for replay purposes.
+                 */
+                if (!(interrupt_request & tcg_ops->virtual_interrupts)) {
                     replay_interrupt();
                 }
                 /*
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index b942c306d6..750ae0f945 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -104,6 +104,7 @@ static const struct TCGCPUOps x86_tcg_ops = {
     .do_unaligned_access = x86_cpu_do_unaligned_access,
     .debug_excp_handler = breakpoint_handler,
     .debug_check_breakpoint = x86_debug_check_breakpoint,
+    .virtual_interrupts = CPU_INTERRUPT_POLL,
 #endif /* !CONFIG_USER_ONLY */
 };
 
-- 
2.39.2

Re: [PATCH 09/10] accel/tcg: re-inline the filtering of virtual IRQs but data driven

[Philippe Mathieu-Daudé]

On 20/3/23 11:10, Alex Bennée wrote:
> Although only I386 currently uses it it is not inconceivable that
> other arches might find this facility useful.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/tcg-cpu-ops.h |  5 +++++
>   accel/tcg/cpu-exec.c          | 29 +++++++++--------------------
>   target/i386/tcg/tcg-cpu.c     |  1 +
>   3 files changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index 66c0cecdde..8e8df8c330 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -121,6 +121,11 @@ struct TCGCPUOps {
>        */
>       bool (*io_recompile_replay_branch)(CPUState *cpu,
>                                          const TranslationBlock *tb);
> +    /**
> +     * @virtual_interrupts: IRQs that can be ignored for replay purposes
> +     */
> +    int virtual_interrupts;

Maybe rename as "virtual_interrupts_mask" and use 'unsigned' type?

Re: [PATCH 09/10] accel/tcg: re-inline the filtering of virtual IRQs but data driven

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> Although only I386 currently uses it it is not inconceivable that
> other arches might find this facility useful.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/hw/core/tcg-cpu-ops.h |  5 +++++
>   accel/tcg/cpu-exec.c          | 29 +++++++++--------------------
>   target/i386/tcg/tcg-cpu.c     |  1 +
>   3 files changed, 15 insertions(+), 20 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 10/10] accel/tcg: remove unused includes

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cpu-exec.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 56be7956e7..90e327c3bb 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -19,20 +19,16 @@
 
 #include "qemu/osdep.h"
 #include "qemu/qemu-print.h"
-#include "qapi/error.h"
-#include "qapi/type-helpers.h"
 #include "hw/core/tcg-cpu-ops.h"
 #include "hw/core/sysemu-cpu-ops.h"
 #include "trace.h"
 #include "disas/disas.h"
-#include "exec/exec-all.h"
 #include "tcg/tcg.h"
 #include "qemu/atomic.h"
 #include "qemu/rcu.h"
 #include "exec/log.h"
 #include "qemu/main-loop.h"
 #include "sysemu/cpus.h"
-#include "exec/cpu-all.h"
 #include "sysemu/cpu-timers.h"
 #include "exec/replay-core.h"
 #include "sysemu/tcg.h"
-- 
2.39.2

Re: [PATCH 10/10] accel/tcg: remove unused includes

[Richard Henderson]

On 3/20/23 03:10, Alex Bennée wrote:
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 4 ----
>   1 file changed, 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 10/10] accel/tcg: remove unused includes

[Alessandro Di Federico via]

Did we ever try include-what-you-use on QEMU?

    https://github.com/include-what-you-use/include-what-you-use

I guess it doesn't play well with the tons of different configurations
we have, but in my experience even without auto-applying its
suggestions it can be beneficial to catch low hanging fruits.

Reviewed-by: Alessandro Di Federico <ale@rev.ng>

-- 
Alessandro Di Federico
rev.ng Labs