[PATCH for 6.2 v1 0/7] more tcg, plugin, test and build fixes

[PATCH for 6.2 v1 0/7] more tcg, plugin, test and build fixes

[Alex Bennée]

Hi,

As the release process rolls on here if the current state of my
for-6.2 tree. There are fixes for TCG, plugins, build and test.

The following still need review:

 - plugins/meson.build: fix linker issue with weird paths
 - tests/avocado: fix tcg_plugin mem access count test
 - accel/tcg: suppress IRQ check for special TBs

Alex Bennée (4):
  accel/tcg: suppress IRQ check for special TBs
  tests/avocado: fix tcg_plugin mem access count test
  plugins/meson.build: fix linker issue with weird paths
  gdbstub: handle a potentially racing TaskState

Pavel Dovgalyuk (1):
  softmmu: fix watchpoint-interrupt races

Philippe Mathieu-Daudé (1):
  MAINTAINERS: Add section for Aarch64 GitLab custom runner

Willian Rampazzo (1):
  MAINTAINERS: Remove me as a reviewer for the build and test/avocado

 include/exec/exec-all.h      |  1 +
 include/exec/gen-icount.h    | 21 +++++++++++++++++----
 accel/tcg/cpu-exec.c         | 19 +++++++++++++++++++
 gdbstub.c                    |  2 +-
 MAINTAINERS                  | 10 ++++++++--
 plugins/meson.build          |  4 ++--
 tests/avocado/tcg_plugins.py |  2 +-
 7 files changed, 49 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH v1 1/7] softmmu: fix watchpoint-interrupt races

[Alex Bennée]

From: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>

Watchpoint may be processed in two phases. First one is detecting
the instruction with target memory access. And the second one is
executing only one instruction and setting the debug interrupt flag.
Hardware interrupts can break this sequence when they happen after
the first watchpoint phase.
This patch postpones the interrupt request until watchpoint is
processed.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Message-Id: <163662451431.125458.14945698834107669531.stgit@pasha-ThinkPad-X280>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cpu-exec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 2d14d02f6c..9cb892e326 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
             qemu_mutex_unlock_iothread();
             return true;
         }
+        /* Process watchpoints first, or interrupts will ruin everything */
+        if (cpu->watchpoint_hit) {
+            qemu_mutex_unlock_iothread();
+            return false;
+        }
 #if !defined(CONFIG_USER_ONLY)
         if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
             /* Do nothing */
-- 
2.30.2

[PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs

[Alex Bennée]

Generally when we set cpu->cflags_next_tb it is because we want to
carefully control the execution of the next TB. Currently there is a
race that causes cflags_next_tb to get ignored if an IRQ is processed
before we execute any actual instructions.

To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
this check in the generated code so we know we will definitely execute
the next block.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
---
 include/exec/exec-all.h   |  1 +
 include/exec/gen-icount.h | 21 +++++++++++++++++----
 accel/tcg/cpu-exec.c      | 14 ++++++++++++++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 6bb2a0f7ec..35d8e93976 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -503,6 +503,7 @@ struct TranslationBlock {
 #define CF_USE_ICOUNT    0x00020000
 #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
 #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
+#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
 #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
 #define CF_CLUSTER_SHIFT 24
 
diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 610cba58fe..c57204ddad 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
 {
     TCGv_i32 count;
 
-    tcg_ctx->exitreq_label = gen_new_label();
     if (tb_cflags(tb) & CF_USE_ICOUNT) {
         count = tcg_temp_local_new_i32();
     } else {
@@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
         icount_start_insn = tcg_last_op();
     }
 
-    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+    /*
+     * Emit the check against icount_decr.u32 to see if we should exit
+     * unless we suppress the check with CF_NOIRQ. If we are using
+     * icount and have suppressed interruption the higher level code
+     * should have ensured we don't run more instructions than the
+     * budget.
+     */
+    if (tb_cflags(tb) & CF_NOIRQ) {
+        tcg_ctx->exitreq_label = NULL;
+    } else {
+        tcg_ctx->exitreq_label = gen_new_label();
+        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+    }
 
     if (tb_cflags(tb) & CF_USE_ICOUNT) {
         tcg_gen_st16_i32(count, cpu_env,
@@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
                            tcgv_i32_arg(tcg_constant_i32(num_insns)));
     }
 
-    gen_set_label(tcg_ctx->exitreq_label);
-    tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
+    if (tcg_ctx->exitreq_label) {
+        gen_set_label(tcg_ctx->exitreq_label);
+        tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
+    }
 }
 
 #endif
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 9cb892e326..9e3ed42ceb 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int interrupt_request)
 static inline bool cpu_handle_interrupt(CPUState *cpu,
                                         TranslationBlock **last_tb)
 {
+    /*
+     * If we have special cflags lets not get distracted with IRQs. We
+     * shall exit the loop as soon as the next TB completes what it
+     * needs to do.
+     */
+    if (cpu->cflags_next_tb != -1) {
+        return false;
+    }
+
     /* Clear the interrupt flag now since we're processing
      * cpu->interrupt_request and cpu->exit_request.
      * Ensure zeroing happens before reading cpu->exit_request or
@@ -954,11 +963,16 @@ int cpu_exec(CPUState *cpu)
              * after-access watchpoints.  Since this request should never
              * have CF_INVALID set, -1 is a convenient invalid value that
              * does not require tcg headers for cpu_common_reset.
+             *
+             * As we don't want this special TB being interrupted by
+             * some sort of asynchronous event we apply CF_NOIRQ to
+             * disable the usual event checking.
              */
             cflags = cpu->cflags_next_tb;
             if (cflags == -1) {
                 cflags = curr_cflags(cpu);
             } else {
+                cflags |= CF_NOIRQ;
                 cpu->cflags_next_tb = -1;
             }
 
-- 
2.30.2

[PATCH v1 3/7] tests/avocado: fix tcg_plugin mem access count test

[Alex Bennée]

When we cleaned up argument handling the test was missed.

Fixes: 5ae589faad ("tests/plugins/mem: introduce "track" arg and make args not positional")
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/avocado/tcg_plugins.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/tcg_plugins.py b/tests/avocado/tcg_plugins.py
index 9ca1515c3b..642d2e49e3 100644
--- a/tests/avocado/tcg_plugins.py
+++ b/tests/avocado/tcg_plugins.py
@@ -131,7 +131,7 @@ def test_aarch64_virt_mem_icount(self):
                                                  suffix=".log")
 
         self.run_vm(kernel_path, kernel_command_line,
-                    "tests/plugin/libmem.so,arg=both", plugin_log.name,
+                    "tests/plugin/libmem.so,inline=true,callback=true", plugin_log.name,
                     console_pattern,
                     args=('-icount', 'shift=1'))
 
-- 
2.30.2

[PATCH v1 4/7] plugins/meson.build: fix linker issue with weird paths

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Stefan Weil <sw@weilnetz.de>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/712
---
 plugins/meson.build | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/meson.build b/plugins/meson.build
index aeb386ebae..b3de57853b 100644
--- a/plugins/meson.build
+++ b/plugins/meson.build
@@ -2,9 +2,9 @@ plugin_ldflags = []
 # Modules need more symbols than just those in plugins/qemu-plugins.symbols
 if not enable_modules
   if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host
-    plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.project_build_root() / 'qemu-plugins-ld.symbols')]
+    plugin_ldflags = ['-Wl,--dynamic-list=qemu-plugins-ld.symbols']
   elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host
-    plugin_ldflags = ['-Wl,-exported_symbols_list,' + (meson.project_build_root() / 'qemu-plugins-ld64.symbols')]
+    plugin_ldflags = ['-Wl,-exported_symbols_list,qemu-plugins-ld64.symbols']
   endif
 endif
 
-- 
2.30.2

[PATCH v1 5/7] gdbstub: handle a potentially racing TaskState

[Alex Bennée]

When dealing with multi-threaded userspace programs there is a race
condition with the addition of cpu->opaque (aka TaskState). This is
due to cpu_copy calling cpu_create which updates the global vCPU list.
However the task state isn't set until later. This shouldn't be a
problem because the new thread can't have executed anything yet but
the gdbstub code does liberally iterate through the CPU list in
various places.

This sticking plaster ensure the not yet fully realized vCPU is given
an pid of -1 which should be enough to ensure it doesn't show up
anywhere else.

In the longer term I think the code that manages the association
between vCPUs and attached GDB processes could do with a clean-up and
re-factor.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/730
---
 gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index 23baaef40e..141d7bc4ec 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -94,7 +94,7 @@ static inline int cpu_gdb_index(CPUState *cpu)
 {
 #if defined(CONFIG_USER_ONLY)
     TaskState *ts = (TaskState *) cpu->opaque;
-    return ts->ts_tid;
+    return ts ? ts->ts_tid : -1;
 #else
     return cpu->cpu_index + 1;
 #endif
-- 
2.30.2

[PATCH v1 6/7] MAINTAINERS: Remove me as a reviewer for the build and test/avocado

[Alex Bennée]

From: Willian Rampazzo <willianr@redhat.com>

Remove me as a reviewer for the Build and test automation and the
Integration Testing with the Avocado Framework and add Beraldo
Leal.

Signed-off-by: Willian Rampazzo <willianr@redhat.com>
Reviewed-by: Beraldo Leal <bleal@redhat.com>
Message-Id: <20211122191124.31620-1-willianr@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d3879aa3c1..8f5156bfa7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3469,7 +3469,7 @@ M: Alex Bennée <alex.bennee@linaro.org>
 M: Philippe Mathieu-Daudé <f4bug@amsat.org>
 M: Thomas Huth <thuth@redhat.com>
 R: Wainer dos Santos Moschetta <wainersm@redhat.com>
-R: Willian Rampazzo <willianr@redhat.com>
+R: Beraldo Leal <bleal@redhat.com>
 S: Maintained
 F: .github/lockdown.yml
 F: .gitlab-ci.yml
@@ -3507,7 +3507,7 @@ W: https://trello.com/b/6Qi1pxVn/avocado-qemu
 R: Cleber Rosa <crosa@redhat.com>
 R: Philippe Mathieu-Daudé <philmd@redhat.com>
 R: Wainer dos Santos Moschetta <wainersm@redhat.com>
-R: Willian Rampazzo <willianr@redhat.com>
+R: Beraldo Leal <bleal@redhat.com>
 S: Odd Fixes
 F: tests/avocado/
 
-- 
2.30.2

[PATCH v1 7/7] MAINTAINERS: Add section for Aarch64 GitLab custom runner

[Alex Bennée]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Add a MAINTAINERS section to cover the GitLab YAML config file
containing the jobs run on the custom runner sponsored by the
Works On Arm project [*].

[*] https://developer.arm.com/solutions/infrastructure/works-on-arm

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20211116163226.2719320-1-f4bug@amsat.org>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8f5156bfa7..006a2293ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3511,6 +3511,12 @@ R: Beraldo Leal <bleal@redhat.com>
 S: Odd Fixes
 F: tests/avocado/
 
+GitLab custom runner (Works On Arm Sponsored)
+M: Alex Bennée <alex.bennee@linaro.org>
+M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+S: Maintained
+F: .gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml
+
 Documentation
 -------------
 Build system architecture
-- 
2.30.2

Re: [PATCH v1 4/7] plugins/meson.build: fix linker issue with weird paths

[Philippe Mathieu-Daudé]

On 11/23/21 21:57, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Stefan Weil <sw@weilnetz.de>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/712
> ---
>  plugins/meson.build | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v1 3/7] tests/avocado: fix tcg_plugin mem access count test

[Philippe Mathieu-Daudé]

On 11/23/21 21:57, Alex Bennée wrote:
> When we cleaned up argument handling the test was missed.
> 
> Fixes: 5ae589faad ("tests/plugins/mem: introduce "track" arg and make args not positional")
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/avocado/tcg_plugins.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v1 1/7] softmmu: fix watchpoint-interrupt races

[Richard Henderson]

On 11/23/21 9:57 PM, Alex Bennée wrote:
> From: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> 
> Watchpoint may be processed in two phases. First one is detecting
> the instruction with target memory access. And the second one is
> executing only one instruction and setting the debug interrupt flag.
> Hardware interrupts can break this sequence when they happen after
> the first watchpoint phase.
> This patch postpones the interrupt request until watchpoint is
> processed.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Message-Id: <163662451431.125458.14945698834107669531.stgit@pasha-ThinkPad-X280>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   accel/tcg/cpu-exec.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 2d14d02f6c..9cb892e326 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>               qemu_mutex_unlock_iothread();
>               return true;
>           }
> +        /* Process watchpoints first, or interrupts will ruin everything */
> +        if (cpu->watchpoint_hit) {
> +            qemu_mutex_unlock_iothread();
> +            return false;
> +        }

I think this is redundant with the next patch.


r~

Re: [PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs

[Richard Henderson]

On 11/23/21 9:57 PM, Alex Bennée wrote:
> Generally when we set cpu->cflags_next_tb it is because we want to
> carefully control the execution of the next TB. Currently there is a
> race that causes cflags_next_tb to get ignored if an IRQ is processed
> before we execute any actual instructions.
> 
> To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
> this check in the generated code so we know we will definitely execute
> the next block.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
> ---
>   include/exec/exec-all.h   |  1 +
>   include/exec/gen-icount.h | 21 +++++++++++++++++----
>   accel/tcg/cpu-exec.c      | 14 ++++++++++++++
>   3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 6bb2a0f7ec..35d8e93976 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -503,6 +503,7 @@ struct TranslationBlock {
>   #define CF_USE_ICOUNT    0x00020000
>   #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
>   #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
> +#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
>   #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
>   #define CF_CLUSTER_SHIFT 24
>   
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index 610cba58fe..c57204ddad 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>   {
>       TCGv_i32 count;
>   
> -    tcg_ctx->exitreq_label = gen_new_label();
>       if (tb_cflags(tb) & CF_USE_ICOUNT) {
>           count = tcg_temp_local_new_i32();
>       } else {
> @@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>           icount_start_insn = tcg_last_op();
>       }
>   
> -    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
> +    /*
> +     * Emit the check against icount_decr.u32 to see if we should exit
> +     * unless we suppress the check with CF_NOIRQ. If we are using
> +     * icount and have suppressed interruption the higher level code
> +     * should have ensured we don't run more instructions than the
> +     * budget.
> +     */
> +    if (tb_cflags(tb) & CF_NOIRQ) {
> +        tcg_ctx->exitreq_label = NULL;
> +    } else {
> +        tcg_ctx->exitreq_label = gen_new_label();
> +        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
> +    }
>   
>       if (tb_cflags(tb) & CF_USE_ICOUNT) {
>           tcg_gen_st16_i32(count, cpu_env,
> @@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
>                              tcgv_i32_arg(tcg_constant_i32(num_insns)));
>       }
>   
> -    gen_set_label(tcg_ctx->exitreq_label);
> -    tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
> +    if (tcg_ctx->exitreq_label) {
> +        gen_set_label(tcg_ctx->exitreq_label);
> +        tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
> +    }
>   }
>   
>   #endif

Split patch here, I think.


> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 9cb892e326..9e3ed42ceb 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int interrupt_request)
>   static inline bool cpu_handle_interrupt(CPUState *cpu,
>                                           TranslationBlock **last_tb)
>   {
> +    /*
> +     * If we have special cflags lets not get distracted with IRQs. We
> +     * shall exit the loop as soon as the next TB completes what it
> +     * needs to do.
> +     */

We will *probably* exit the loop, I think.

With watchpoints, we expect to perform the same memory operation, which is expected to hit 
the watchpoint_hit check in cpu_check_watchpoint, which will raise CPU_INTERRUPT_DEBUG.

With SMC, we flush all TBs from the current page, re-execute one insn, and then (probably) 
have to exit to generate the next tb.

With icount, in cpu_loop_exec_tb, we have no idea what's coming; we only know that we want 
no more than N insns to execute.

None of which directly exit the loop -- we need the IRQ check at the beginning of the 
*next* TB to exit the loop.

If we want to force an exit from the loop, we need CF_NO_GOTO_TB | CF_NO_GOTO_PTR.  Which 
is probably a good idea, at least for watchpoints; exit_tb is the fastest way out of the 
TB to recognize the debug interrupt.

The icount usage I find a bit odd.  I don't think that we should suppress an IRQ in that 
case -- we can have up to 510 insns outstanding on icount_budget, which is clearly far too 
many to have IRQs disabled.  I think we should not use cflags_next_tb for this at all, but 
should apply the icount limit later somehow, because an IRQ *could* be recognized 
immediately, with the first TB of the interrupt handler running with limited budget, and 
the icount tick being recognized at that point.

> +             * As we don't want this special TB being interrupted by
> +             * some sort of asynchronous event we apply CF_NOIRQ to
> +             * disable the usual event checking.
>                */
>               cflags = cpu->cflags_next_tb;
>               if (cflags == -1) {
>                   cflags = curr_cflags(cpu);
>               } else {
> +                cflags |= CF_NOIRQ;
>                   cpu->cflags_next_tb = -1;
>               }

Is it clearer to add NOIRQ here, or back where we set cflags_next_tb in the first place?


r~

Re: [PATCH v1 3/7] tests/avocado: fix tcg_plugin mem access count test

[Richard Henderson]

On 11/23/21 9:57 PM, Alex Bennée wrote:
> When we cleaned up argument handling the test was missed.
> 
> Fixes: 5ae589faad ("tests/plugins/mem: introduce "track" arg and make args not positional")
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   tests/avocado/tcg_plugins.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

Re: [PATCH v1 1/7] softmmu: fix watchpoint-interrupt races

[Alex Bennée]

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

> On 11/23/21 9:57 PM, Alex Bennée wrote:
>> From: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>> Watchpoint may be processed in two phases. First one is detecting
>> the instruction with target memory access. And the second one is
>> executing only one instruction and setting the debug interrupt flag.
>> Hardware interrupts can break this sequence when they happen after
>> the first watchpoint phase.
>> This patch postpones the interrupt request until watchpoint is
>> processed.
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Message-Id: <163662451431.125458.14945698834107669531.stgit@pasha-ThinkPad-X280>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   accel/tcg/cpu-exec.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 2d14d02f6c..9cb892e326 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>>               qemu_mutex_unlock_iothread();
>>               return true;
>>           }
>> +        /* Process watchpoints first, or interrupts will ruin everything */
>> +        if (cpu->watchpoint_hit) {
>> +            qemu_mutex_unlock_iothread();
>> +            return false;
>> +        }
>
> I think this is redundant with the next patch.

OK I'll drop it. The function is getting messy anyway.

-- 
Alex Bennée

Re: [PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs

[Alex Bennée]

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

> On 11/23/21 9:57 PM, Alex Bennée wrote:
>> Generally when we set cpu->cflags_next_tb it is because we want to
>> carefully control the execution of the next TB. Currently there is a
>> race that causes cflags_next_tb to get ignored if an IRQ is processed
>> before we execute any actual instructions.
>> To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
>> this check in the generated code so we know we will definitely execute
>> the next block.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
>> ---
>>   include/exec/exec-all.h   |  1 +
>>   include/exec/gen-icount.h | 21 +++++++++++++++++----
>>   accel/tcg/cpu-exec.c      | 14 ++++++++++++++
>>   3 files changed, 32 insertions(+), 4 deletions(-)
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 6bb2a0f7ec..35d8e93976 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -503,6 +503,7 @@ struct TranslationBlock {
>>   #define CF_USE_ICOUNT    0x00020000
>>   #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
>>   #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
>> +#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
>>   #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
>>   #define CF_CLUSTER_SHIFT 24
>>   diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
>> index 610cba58fe..c57204ddad 100644
>> --- a/include/exec/gen-icount.h
>> +++ b/include/exec/gen-icount.h
>> @@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>>   {
>>       TCGv_i32 count;
>>   -    tcg_ctx->exitreq_label = gen_new_label();
>>       if (tb_cflags(tb) & CF_USE_ICOUNT) {
>>           count = tcg_temp_local_new_i32();
>>       } else {
>> @@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>>           icount_start_insn = tcg_last_op();
>>       }
>>   -    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0,
>> tcg_ctx->exitreq_label);
>> +    /*
>> +     * Emit the check against icount_decr.u32 to see if we should exit
>> +     * unless we suppress the check with CF_NOIRQ. If we are using
>> +     * icount and have suppressed interruption the higher level code
>> +     * should have ensured we don't run more instructions than the
>> +     * budget.
>> +     */
>> +    if (tb_cflags(tb) & CF_NOIRQ) {
>> +        tcg_ctx->exitreq_label = NULL;
>> +    } else {
>> +        tcg_ctx->exitreq_label = gen_new_label();
>> +        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
>> +    }
>>         if (tb_cflags(tb) & CF_USE_ICOUNT) {
>>           tcg_gen_st16_i32(count, cpu_env,
>> @@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
>>                              tcgv_i32_arg(tcg_constant_i32(num_insns)));
>>       }
>>   -    gen_set_label(tcg_ctx->exitreq_label);
>> -    tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>> +    if (tcg_ctx->exitreq_label) {
>> +        gen_set_label(tcg_ctx->exitreq_label);
>> +        tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>> +    }
>>   }
>>     #endif
>
> Split patch here, I think.

Not including the header to cpu_handle_interrupt? 

>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 9cb892e326..9e3ed42ceb 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int interrupt_request)
>>   static inline bool cpu_handle_interrupt(CPUState *cpu,
>>                                           TranslationBlock **last_tb)
>>   {
>> +    /*
>> +     * If we have special cflags lets not get distracted with IRQs. We
>> +     * shall exit the loop as soon as the next TB completes what it
>> +     * needs to do.
>> +     */
>
> We will *probably* exit the loop, I think.
>
> With watchpoints, we expect to perform the same memory operation,
> which is expected to hit the watchpoint_hit check in
> cpu_check_watchpoint, which will raise CPU_INTERRUPT_DEBUG.
>
> With SMC, we flush all TBs from the current page, re-execute one insn,
> and then (probably) have to exit to generate the next tb.
>
> With icount, in cpu_loop_exec_tb, we have no idea what's coming; we
> only know that we want no more than N insns to execute.

I think technically we do because all asynchronous interrupt are tied to
the icount (which is part of the budget calculation - icount_get_limit).
In theory we could drop the interrupt check altogether in icount mode
because we should always end and exit to the outer loop when a timer is
going to expire.

> None of which directly exit the loop -- we need the IRQ check at the
> beginning of the *next* TB to exit the loop.
>
> If we want to force an exit from the loop, we need CF_NO_GOTO_TB |
> CF_NO_GOTO_PTR.  Which is probably a good idea, at least for
> watchpoints; exit_tb is the fastest way out of the TB to recognize the
> debug interrupt.
>
> The icount usage I find a bit odd.  I don't think that we should
> suppress an IRQ in that case -- we can have up to 510 insns
> outstanding on icount_budget, which is clearly far too many to have
> IRQs disabled.  I think we should not use cflags_next_tb for this at
> all, but should apply the icount limit later somehow, because an IRQ
> *could* be recognized immediately, with the first TB of the interrupt
> handler running with limited budget, and the icount tick being
> recognized at that point.

I wonder what would happen if we checked u16.high in icount mode? No
timer should ever set it - although I guess it could get set during an
IO operation.

Perhaps really all icount exit checks should be done at the end of
blocks? I suspect that breaks too many assumptions in the rest of the
code.

>
>> +             * As we don't want this special TB being interrupted by
>> +             * some sort of asynchronous event we apply CF_NOIRQ to
>> +             * disable the usual event checking.
>>                */
>>               cflags = cpu->cflags_next_tb;
>>               if (cflags == -1) {
>>                   cflags = curr_cflags(cpu);
>>               } else {
>> +                cflags |= CF_NOIRQ;
>>                   cpu->cflags_next_tb = -1;
>>               }
>
> Is it clearer to add NOIRQ here, or back where we set cflags_next_tb
> in the first place?

Are there cases of setting cpu->cflags_next_tb which we are happy to get
preempted by asynchronous events? I guess in the SMC case it wouldn't
matter because when we get back from the IRQ things get reset?

>
>
> r~


-- 
Alex Bennée

Re: [PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs

[Richard Henderson]

On 11/24/21 11:24 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 11/23/21 9:57 PM, Alex Bennée wrote:
>>> Generally when we set cpu->cflags_next_tb it is because we want to
>>> carefully control the execution of the next TB. Currently there is a
>>> race that causes cflags_next_tb to get ignored if an IRQ is processed
>>> before we execute any actual instructions.
>>> To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
>>> this check in the generated code so we know we will definitely execute
>>> the next block.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
>>> ---
>>>    include/exec/exec-all.h   |  1 +
>>>    include/exec/gen-icount.h | 21 +++++++++++++++++----
>>>    accel/tcg/cpu-exec.c      | 14 ++++++++++++++
>>>    3 files changed, 32 insertions(+), 4 deletions(-)
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index 6bb2a0f7ec..35d8e93976 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -503,6 +503,7 @@ struct TranslationBlock {
>>>    #define CF_USE_ICOUNT    0x00020000
>>>    #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
>>>    #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
>>> +#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
>>>    #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
>>>    #define CF_CLUSTER_SHIFT 24
>>>    diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
>>> index 610cba58fe..c57204ddad 100644
>>> --- a/include/exec/gen-icount.h
>>> +++ b/include/exec/gen-icount.h
>>> @@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>>>    {
>>>        TCGv_i32 count;
>>>    -    tcg_ctx->exitreq_label = gen_new_label();
>>>        if (tb_cflags(tb) & CF_USE_ICOUNT) {
>>>            count = tcg_temp_local_new_i32();
>>>        } else {
>>> @@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>>>            icount_start_insn = tcg_last_op();
>>>        }
>>>    -    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0,
>>> tcg_ctx->exitreq_label);
>>> +    /*
>>> +     * Emit the check against icount_decr.u32 to see if we should exit
>>> +     * unless we suppress the check with CF_NOIRQ. If we are using
>>> +     * icount and have suppressed interruption the higher level code
>>> +     * should have ensured we don't run more instructions than the
>>> +     * budget.
>>> +     */
>>> +    if (tb_cflags(tb) & CF_NOIRQ) {
>>> +        tcg_ctx->exitreq_label = NULL;
>>> +    } else {
>>> +        tcg_ctx->exitreq_label = gen_new_label();
>>> +        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
>>> +    }
>>>          if (tb_cflags(tb) & CF_USE_ICOUNT) {
>>>            tcg_gen_st16_i32(count, cpu_env,
>>> @@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
>>>                               tcgv_i32_arg(tcg_constant_i32(num_insns)));
>>>        }
>>>    -    gen_set_label(tcg_ctx->exitreq_label);
>>> -    tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>>> +    if (tcg_ctx->exitreq_label) {
>>> +        gen_set_label(tcg_ctx->exitreq_label);
>>> +        tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>>> +    }
>>>    }
>>>      #endif
>>
>> Split patch here, I think.
> 
> Not including the header to cpu_handle_interrupt?

Correct.  Introduce CF_NOIRQ without using it yet.

>> With icount, in cpu_loop_exec_tb, we have no idea what's coming; we
>> only know that we want no more than N insns to execute.
> 
> I think technically we do because all asynchronous interrupt are tied to
> the icount (which is part of the budget calculation - icount_get_limit).

Are you sure that's plain icount and not replay?
In icount_get_limit we talk about timers, not any other asynchronous interrupt, like a 
keyboard press.

> In theory we could drop the interrupt check altogether in icount mode
> because we should always end and exit to the outer loop when a timer is
> going to expire.

But we know nothing about synchronous exceptions or anything else.

> I wonder what would happen if we checked u16.high in icount mode? No
> timer should ever set it - although I guess it could get set during an
> IO operation.

Uh, we do, via u32?  I'm not sure what you're saying here.

> Perhaps really all icount exit checks should be done at the end of
> blocks? I suspect that breaks too many assumptions in the rest of the
> code.

There are multiple exits at the end of the block, which is why we do the check at the 
beginning of the next block.  Besides, we have to check that the block we're about to 
execute is within budget.

> Are there cases of setting cpu->cflags_next_tb which we are happy to get
> preempted by asynchronous events?

Well, icount.

> I guess in the SMC case it wouldn't
> matter because when we get back from the IRQ things get reset?

SMC probably would work with an interrupt, but we'd wind up having to repeat the process 
of flushing all TBs on the page, so we might as well perform the one store and get it over 
with.


r~

Re: [PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs

[Alex Bennée]

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

> On 11/24/21 11:24 AM, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> On 11/23/21 9:57 PM, Alex Bennée wrote:
>>>> Generally when we set cpu->cflags_next_tb it is because we want to
>>>> carefully control the execution of the next TB. Currently there is a
>>>> race that causes cflags_next_tb to get ignored if an IRQ is processed
>>>> before we execute any actual instructions.
>>>> To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
>>>> this check in the generated code so we know we will definitely execute
>>>> the next block.
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>>>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
>>>> ---
>>>>    include/exec/exec-all.h   |  1 +
>>>>    include/exec/gen-icount.h | 21 +++++++++++++++++----
>>>>    accel/tcg/cpu-exec.c      | 14 ++++++++++++++
>>>>    3 files changed, 32 insertions(+), 4 deletions(-)
>>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>>> index 6bb2a0f7ec..35d8e93976 100644
>>>> --- a/include/exec/exec-all.h
>>>> +++ b/include/exec/exec-all.h
>>>> @@ -503,6 +503,7 @@ struct TranslationBlock {
>>>>    #define CF_USE_ICOUNT    0x00020000
>>>>    #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
>>>>    #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
>>>> +#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
>>>>    #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
>>>>    #define CF_CLUSTER_SHIFT 24
>>>>    diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
>>>> index 610cba58fe..c57204ddad 100644
>>>> --- a/include/exec/gen-icount.h
>>>> +++ b/include/exec/gen-icount.h
>>>> @@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>>>>    {
>>>>        TCGv_i32 count;
>>>>    -    tcg_ctx->exitreq_label = gen_new_label();
>>>>        if (tb_cflags(tb) & CF_USE_ICOUNT) {
>>>>            count = tcg_temp_local_new_i32();
>>>>        } else {
>>>> @@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
>>>>            icount_start_insn = tcg_last_op();
>>>>        }
>>>>    -    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0,
>>>> tcg_ctx->exitreq_label);
>>>> +    /*
>>>> +     * Emit the check against icount_decr.u32 to see if we should exit
>>>> +     * unless we suppress the check with CF_NOIRQ. If we are using
>>>> +     * icount and have suppressed interruption the higher level code
>>>> +     * should have ensured we don't run more instructions than the
>>>> +     * budget.
>>>> +     */
>>>> +    if (tb_cflags(tb) & CF_NOIRQ) {
>>>> +        tcg_ctx->exitreq_label = NULL;
>>>> +    } else {
>>>> +        tcg_ctx->exitreq_label = gen_new_label();
>>>> +        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
>>>> +    }
>>>>          if (tb_cflags(tb) & CF_USE_ICOUNT) {
>>>>            tcg_gen_st16_i32(count, cpu_env,
>>>> @@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, int num_insns)
>>>>                               tcgv_i32_arg(tcg_constant_i32(num_insns)));
>>>>        }
>>>>    -    gen_set_label(tcg_ctx->exitreq_label);
>>>> -    tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>>>> +    if (tcg_ctx->exitreq_label) {
>>>> +        gen_set_label(tcg_ctx->exitreq_label);
>>>> +        tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>>>> +    }
>>>>    }
>>>>      #endif
>>>
>>> Split patch here, I think.
>> Not including the header to cpu_handle_interrupt?
>
> Correct.  Introduce CF_NOIRQ without using it yet.
>
>>> With icount, in cpu_loop_exec_tb, we have no idea what's coming; we
>>> only know that we want no more than N insns to execute.
>> I think technically we do because all asynchronous interrupt are
>> tied to
>> the icount (which is part of the budget calculation - icount_get_limit).
>
> Are you sure that's plain icount and not replay?
> In icount_get_limit we talk about timers, not any other asynchronous
> interrupt, like a keyboard press.

Hmm right - and I guess other I/O during record of icount. I guess it's
only fully deterministic (outside of replay) if it's a totally
"isolated" from external system events.

>> In theory we could drop the interrupt check altogether in icount mode
>> because we should always end and exit to the outer loop when a timer is
>> going to expire.
>
> But we know nothing about synchronous exceptions or anything else.

Hmm I didn't think we needed to care about synchronous events but now
you have me wandering what happens in icount mode when an exception
happens mid-block?

>> I wonder what would happen if we checked u16.high in icount mode? No
>> timer should ever set it - although I guess it could get set during an
>> IO operation.
>
> Uh, we do, via u32?  I'm not sure what you're saying here.

I mean should we detect if something has called cpu_exit() mid block
rather than just icount expiring.

<snip>
>> Are there cases of setting cpu->cflags_next_tb which we are happy to get
>> preempted by asynchronous events?
>
> Well, icount.
>
>> I guess in the SMC case it wouldn't
>> matter because when we get back from the IRQ things get reset?
>
> SMC probably would work with an interrupt, but we'd wind up having to
> repeat the process of flushing all TBs on the page, so we might as
> well perform the one store and get it over with.

I guess. Makes the patch a bit noisier though...

-- 
Alex Bennée