Re: [PATCH v5 6/8] add cpu_test_interrupt()/cpu_set_interrupt() helpers and use them tree wide

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 8/21/25 21:26, Igor Mammedov wrote:
> the helpers form load-acquire/store-release pair and use them to replace
> open-coded checkers/setters consistently across the code, which
> ensures that appropriate barriers are in place in case checks happen
> outside of BQL.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
> v5: fix copy/paste error in doc comment of cpu_set_interrupt()
>     "Jason J. Herne" <jjherne@linux.ibm.com>
> v4:
>     add cpu_set_interrupt() and merge helpers patch with
>     patches that use them (v3 6-7,9/10).
>         Peter Xu <peterx@redhat.com>
> ---
>   include/hw/core/cpu.h               | 25 +++++++++++++++++++++
>   accel/tcg/cpu-exec.c                | 10 ++++-----
>   accel/tcg/tcg-accel-ops.c           |  2 +-
>   accel/tcg/user-exec.c               |  2 +-
>   hw/intc/s390_flic.c                 |  2 +-
>   hw/openrisc/cputimer.c              |  2 +-
>   system/cpus.c                       |  2 +-
>   target/alpha/cpu.c                  |  8 +++----
>   target/arm/cpu.c                    | 20 ++++++++---------
>   target/arm/helper.c                 | 18 +++++++--------
>   target/arm/hvf/hvf.c                |  6 ++---
>   target/avr/cpu.c                    |  2 +-
>   target/hppa/cpu.c                   |  2 +-
>   target/i386/hvf/hvf.c               |  4 ++--
>   target/i386/hvf/x86hvf.c            | 21 +++++++++---------
>   target/i386/kvm/kvm.c               | 34 ++++++++++++++---------------
>   target/i386/nvmm/nvmm-all.c         | 24 ++++++++++----------
>   target/i386/tcg/system/seg_helper.c |  2 +-
>   target/i386/tcg/system/svm_helper.c |  2 +-
>   target/i386/whpx/whpx-all.c         | 34 ++++++++++++++---------------
>   target/loongarch/cpu.c              |  2 +-
>   target/m68k/cpu.c                   |  2 +-
>   target/microblaze/cpu.c             |  2 +-
>   target/mips/cpu.c                   |  6 ++---
>   target/mips/kvm.c                   |  2 +-
>   target/openrisc/cpu.c               |  3 +--
>   target/ppc/cpu_init.c               |  2 +-
>   target/ppc/kvm.c                    |  2 +-
>   target/rx/cpu.c                     |  3 +--
>   target/rx/helper.c                  |  2 +-
>   target/s390x/cpu-system.c           |  2 +-
>   target/sh4/cpu.c                    |  2 +-
>   target/sh4/helper.c                 |  2 +-
>   target/sparc/cpu.c                  |  2 +-
>   target/sparc/int64_helper.c         |  4 ++--
>   35 files changed, 141 insertions(+), 119 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 5eaf41a566..1dee9d4c76 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -942,6 +942,31 @@ CPUState *cpu_by_arch_id(int64_t id);
>   
>   void cpu_interrupt(CPUState *cpu, int mask);
>   
> +/**
> + * cpu_test_interrupt:
> + * @cpu: The CPU to check interrupt(s) on.
> + * @mask: The interrupts to check.
> + *
> + * Checks if any of interrupts in @mask are pending on @cpu.
> + */
> +static inline bool cpu_test_interrupt(CPUState *cpu, int mask)
> +{
> +    return qatomic_load_acquire(&cpu->interrupt_request) & mask;
> +}
> +
> +/**
> + * cpu_set_interrupt:
> + * @cpu: The CPU to set pending interrupt(s) on.
> + * @mask: The interrupts to set.
> + *
> + * Sets interrupts in @mask as pending on @cpu.
> + */
> +static inline void cpu_set_interrupt(CPUState *cpu, int mask)
> +{
> +    qatomic_store_release(&cpu->interrupt_request,
> +        cpu->interrupt_request | mask);
> +}
> +
>   /**
>    * cpu_set_pc:
>    * @cpu: The CPU to set the program counter for.
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 713bdb2056..1269c2c6ba 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -778,7 +778,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>        */
>       qatomic_set_mb(&cpu->neg.icount_decr.u16.high, 0);
>   
> -    if (unlikely(qatomic_read(&cpu->interrupt_request))) {
> +    if (unlikely(cpu_test_interrupt(cpu, ~0))) {
>           int interrupt_request;
>           bql_lock();
>           interrupt_request = cpu->interrupt_request;
> @@ -786,7 +786,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>               /* Mask out external interrupts for this step. */
>               interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK;
>           }
> -        if (interrupt_request & CPU_INTERRUPT_DEBUG) {
> +        if (cpu_test_interrupt(cpu, CPU_INTERRUPT_DEBUG)) {
>               cpu->interrupt_request &= ~CPU_INTERRUPT_DEBUG;

Do we need a helper for instances like these (logical &) as well ?
I see a couple more such instances below.


>               cpu->exception_index = EXCP_DEBUG;
>               bql_unlock();
> @@ -795,7 +795,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>   #if !defined(CONFIG_USER_ONLY)
>           if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
>               /* Do nothing */
> -        } else if (interrupt_request & CPU_INTERRUPT_HALT) {
> +        } else if (cpu_test_interrupt(cpu, CPU_INTERRUPT_HALT)) {
>               replay_interrupt();
>               cpu->interrupt_request &= ~CPU_INTERRUPT_HALT;
>               cpu->halted = 1;
> @@ -805,7 +805,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>           } else {
>               const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
>   
> -            if (interrupt_request & CPU_INTERRUPT_RESET) {
> +            if (cpu_test_interrupt(cpu, CPU_INTERRUPT_RESET)) {
>                   replay_interrupt();
>                   tcg_ops->cpu_exec_reset(cpu);
>                   bql_unlock();
> @@ -841,7 +841,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>               interrupt_request = cpu->interrupt_request;
>           }
>   #endif /* !CONFIG_USER_ONLY */
> -        if (interrupt_request & CPU_INTERRUPT_EXITTB) {
> +        if (cpu_test_interrupt(cpu, CPU_INTERRUPT_EXITTB)) {
>               cpu->interrupt_request &= ~CPU_INTERRUPT_EXITTB;
>               /* ensure that no TB jump will be modified as
>                  the program flow was changed */
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index 3b0d7d298e..9c37266c1e 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -97,7 +97,7 @@ static void tcg_cpu_reset_hold(CPUState *cpu)
>   /* mask must never be zero, except for A20 change call */
>   void tcg_handle_interrupt(CPUState *cpu, int mask)
>   {
> -    cpu->interrupt_request |= mask;
> +    cpu_set_interrupt(cpu, mask);
>   
>       /*
>        * If called from iothread context, wake the target cpu in
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index f25d80e2dc..fc2eaf420d 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -49,7 +49,7 @@ __thread uintptr_t helper_retaddr;
>   void cpu_interrupt(CPUState *cpu, int mask)
>   {
>       g_assert(bql_locked());
> -    cpu->interrupt_request |= mask;
> +    cpu_set_interrupt(cpu, mask);
>       qatomic_set(&cpu->neg.icount_decr.u16.high, -1);
>   }
>   

<snip>

> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index a0e77f2673..db841f1260 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7225,7 +7225,7 @@ static int ppc_cpu_mmu_index(CPUState *cs, bool ifetch)
>   #ifndef CONFIG_USER_ONLY
>   static bool ppc_cpu_has_work(CPUState *cs)
>   {
> -    return cs->interrupt_request & CPU_INTERRUPT_HARD;
> +    return cpu_test_interrupt(cs, CPU_INTERRUPT_HARD);
>   }
>   #endif /* !CONFIG_USER_ONLY */
>   
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 015658049e..d145774b09 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1354,7 +1354,7 @@ static int kvmppc_handle_halt(PowerPCCPU *cpu)
>       CPUState *cs = CPU(cpu);
>       CPUPPCState *env = &cpu->env;
>   
> -    if (!(cs->interrupt_request & CPU_INTERRUPT_HARD) &&
> +    if (!cpu_test_interrupt(cs, CPU_INTERRUPT_HARD) &&
>           FIELD_EX64(env->msr, MSR, EE)) {
>           cs->halted = 1;
>           cs->exception_index = EXCP_HLT;

For target/ppc:

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index c6dd5d6f83..da02ae7bf8 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -75,8 +75,7 @@ static void rx_restore_state_to_opc(CPUState *cs,
>   
>   static bool rx_cpu_has_work(CPUState *cs)
>   {
> -    return cs->interrupt_request &
> -        (CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIR);
> +    return cpu_test_interrupt(cs, CPU_INTERRUPT_HARD | CPU_INTERRUPT_FIR);
>   }
>   
>   static int rx_cpu_mmu_index(CPUState *cs, bool ifunc)
> diff --git a/target/rx/helper.c b/target/rx/helper.c
> index 0640ab322b..ce003af421 100644
> --- a/target/rx/helper.c
> +++ b/target/rx/helper.c
> @@ -44,7 +44,7 @@ void rx_cpu_unpack_psw(CPURXState *env, uint32_t psw, int rte)
>   void rx_cpu_do_interrupt(CPUState *cs)
>   {
>       CPURXState *env = cpu_env(cs);
> -    int do_irq = cs->interrupt_request & INT_FLAGS;
> +    int do_irq = cpu_test_interrupt(cs, INT_FLAGS);
>       uint32_t save_psw;
>   
>       env->in_sleep = 0;
> diff --git a/target/s390x/cpu-system.c b/target/s390x/cpu-system.c
> index 709ccd5299..f3a9ffb2a2 100644
> --- a/target/s390x/cpu-system.c
> +++ b/target/s390x/cpu-system.c
> @@ -49,7 +49,7 @@ bool s390_cpu_has_work(CPUState *cs)
>           return false;
>       }
>   
> -    if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
> +    if (!cpu_test_interrupt(cs, CPU_INTERRUPT_HARD)) {
>           return false;
>       }
>   
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 4f561e8c91..21ccb86df4 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -108,7 +108,7 @@ static bool superh_io_recompile_replay_branch(CPUState *cs,
>   
>   static bool superh_cpu_has_work(CPUState *cs)
>   {
> -    return cs->interrupt_request & CPU_INTERRUPT_HARD;
> +    return cpu_test_interrupt(cs, CPU_INTERRUPT_HARD);
>   }
>   #endif /* !CONFIG_USER_ONLY */
>   
> diff --git a/target/sh4/helper.c b/target/sh4/helper.c
> index fb7642bda1..1744ef0e6d 100644
> --- a/target/sh4/helper.c
> +++ b/target/sh4/helper.c
> @@ -58,7 +58,7 @@ int cpu_sh4_is_cached(CPUSH4State *env, target_ulong addr)
>   void superh_cpu_do_interrupt(CPUState *cs)
>   {
>       CPUSH4State *env = cpu_env(cs);
> -    int do_irq = cs->interrupt_request & CPU_INTERRUPT_HARD;
> +    int do_irq = cpu_test_interrupt(cs, CPU_INTERRUPT_HARD);
>       int do_exp, irq_vector = cs->exception_index;
>   
>       /* prioritize exceptions over interrupts */
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index 245caf2de0..c9773f1540 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -783,7 +783,7 @@ static void sparc_restore_state_to_opc(CPUState *cs,
>   #ifndef CONFIG_USER_ONLY
>   static bool sparc_cpu_has_work(CPUState *cs)
>   {
> -    return (cs->interrupt_request & CPU_INTERRUPT_HARD) &&
> +    return cpu_test_interrupt(cs, CPU_INTERRUPT_HARD) &&
>              cpu_interrupts_enabled(cpu_env(cs));
>   }
>   #endif /* !CONFIG_USER_ONLY */
> diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
> index bd14c7a0db..49e4e51c6d 100644
> --- a/target/sparc/int64_helper.c
> +++ b/target/sparc/int64_helper.c
> @@ -89,7 +89,7 @@ void cpu_check_irqs(CPUSPARCState *env)
>        * the next bit is (2 << psrpil).
>        */
>       if (pil < (2 << env->psrpil)) {
> -        if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
> +        if (cpu_test_interrupt(cs, CPU_INTERRUPT_HARD)) {
>               trace_sparc64_cpu_check_irqs_reset_irq(env->interrupt_index);
>               env->interrupt_index = 0;
>               cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> @@ -120,7 +120,7 @@ void cpu_check_irqs(CPUSPARCState *env)
>                   break;
>               }
>           }
> -    } else if (cs->interrupt_request & CPU_INTERRUPT_HARD) {
> +    } else if (cpu_test_interrupt(cs, CPU_INTERRUPT_HARD)) {
>           trace_sparc64_cpu_check_irqs_disabled(pil, env->pil_in, env->softint,
>                                                 env->interrupt_index);
>           env->interrupt_index = 0;