While it would be technically correct to allow an IRQ to happen (as
the offending instruction never really completed) it messes up
instrumentation. We already take care to only use memory
instrumentation on the block, we should also suppress IRQs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Julian Ganz <neither@nut.email>
---
 accel/tcg/translate-all.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 453eb20ec9..d56ca13cdd 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -633,9 +633,10 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
      * Exit the loop and potentially generate a new TB executing the
      * just the I/O insns. We also limit instrumentation to memory
      * operations only (which execute after completion) so we don't
-     * double instrument the instruction.
+     * double instrument the instruction. Also don't let an IRQ sneak
+     * in before we execute it.
      */
-    cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | n;
+    cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | CF_NOIRQ | n;
 
     if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
         vaddr pc = cpu->cc->get_pc(cpu);
-- 
2.39.5

Hi Alex,

January 9, 2025 at 6:06 PM, "Alex Bennée" wrote:
> While it would be technically correct to allow an IRQ to happen (as
> the offending instruction never really completed) it messes up
> instrumentation. We already take care to only use memory
> instrumentation on the block, we should also suppress IRQs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Julian Ganz <neither@nut.email>
> ---
>  accel/tcg/translate-all.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 453eb20ec9..d56ca13cdd 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -633,9 +633,10 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>  * Exit the loop and potentially generate a new TB executing the
>  * just the I/O insns. We also limit instrumentation to memory
>  * operations only (which execute after completion) so we don't
> - * double instrument the instruction.
> + * double instrument the instruction. Also don't let an IRQ sneak
> + * in before we execute it.
>  */
> - cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | n;
> + cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | CF_NOIRQ | n;
>  
>  if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
>  vaddr pc = cpu->cc->get_pc(cpu);
> -- 
> 2.39.5

Reviewed-by: Julian Ganz <neither@nut.email>

On 1/9/25 09:06, Alex Bennée wrote:
> While it would be technically correct to allow an IRQ to happen (as
> the offending instruction never really completed) it messes up
> instrumentation. We already take care to only use memory
> instrumentation on the block, we should also suppress IRQs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Julian Ganz <neither@nut.email>
> ---
>   accel/tcg/translate-all.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

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

r~

> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 453eb20ec9..d56ca13cdd 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -633,9 +633,10 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>        * Exit the loop and potentially generate a new TB executing the
>        * just the I/O insns. We also limit instrumentation to memory
>        * operations only (which execute after completion) so we don't
> -     * double instrument the instruction.
> +     * double instrument the instruction. Also don't let an IRQ sneak
> +     * in before we execute it.
>        */
> -    cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | n;
> +    cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | CF_NOIRQ | n;
>   
>       if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
>           vaddr pc = cpu->cc->get_pc(cpu);

On 1/9/25 09:06, Alex Bennée wrote:
> While it would be technically correct to allow an IRQ to happen (as
> the offending instruction never really completed) it messes up
> instrumentation. We already take care to only use memory
> instrumentation on the block, we should also suppress IRQs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Julian Ganz <neither@nut.email>
> ---
>   accel/tcg/translate-all.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 453eb20ec9..d56ca13cdd 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -633,9 +633,10 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>        * Exit the loop and potentially generate a new TB executing the
>        * just the I/O insns. We also limit instrumentation to memory
>        * operations only (which execute after completion) so we don't
> -     * double instrument the instruction.
> +     * double instrument the instruction. Also don't let an IRQ sneak
> +     * in before we execute it.
>        */
> -    cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | n;
> +    cpu->cflags_next_tb = curr_cflags(cpu) | CF_MEMI_ONLY | CF_NOIRQ | n;
>   
>       if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
>           vaddr pc = cpu->cc->get_pc(cpu);

Thanks for the fix Alex, I confirm it solved the issue observed with 
discon plugin with aarch64 guest.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>