the helper forms load-acquire/store-release pair with
tcg_handle_interrupt/generic_handle_interrupt and can be used
for checking interrupts outside of BQL

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/core/cpu.h     | 12 ++++++++++++
 accel/tcg/tcg-accel-ops.c |  3 ++-
 system/cpus.c             |  3 ++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5eaf41a566..d0460c01cf 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -942,6 +942,18 @@ 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_pc:
  * @cpu: The CPU to set the program counter for.
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 3b0d7d298e..02c7600bb7 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -97,7 +97,8 @@ 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;
+    qatomic_store_release(&cpu->interrupt_request,
+        cpu->interrupt_request | mask);
 
     /*
      * If called from iothread context, wake the target cpu in
diff --git a/system/cpus.c b/system/cpus.c
index 256723558d..8e543488c3 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -256,7 +256,8 @@ int64_t cpus_get_elapsed_ticks(void)
 
 void generic_handle_interrupt(CPUState *cpu, int mask)
 {
-    cpu->interrupt_request |= mask;
+    qatomic_store_release(&cpu->interrupt_request,
+        cpu->interrupt_request | mask);
 
     if (!qemu_cpu_is_self(cpu)) {
         qemu_cpu_kick(cpu);
-- 
2.47.1

On Fri, Aug 08, 2025 at 02:01:33PM +0200, Igor Mammedov wrote:
> the helper forms load-acquire/store-release pair with
> tcg_handle_interrupt/generic_handle_interrupt and can be used
> for checking interrupts outside of BQL
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/core/cpu.h     | 12 ++++++++++++
>  accel/tcg/tcg-accel-ops.c |  3 ++-
>  system/cpus.c             |  3 ++-
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 5eaf41a566..d0460c01cf 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -942,6 +942,18 @@ 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_pc:
>   * @cpu: The CPU to set the program counter for.
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index 3b0d7d298e..02c7600bb7 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -97,7 +97,8 @@ 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;
> +    qatomic_store_release(&cpu->interrupt_request,
> +        cpu->interrupt_request | mask);
>  
>      /*
>       * If called from iothread context, wake the target cpu in
> diff --git a/system/cpus.c b/system/cpus.c
> index 256723558d..8e543488c3 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -256,7 +256,8 @@ int64_t cpus_get_elapsed_ticks(void)
>  
>  void generic_handle_interrupt(CPUState *cpu, int mask)
>  {
> -    cpu->interrupt_request |= mask;
> +    qatomic_store_release(&cpu->interrupt_request,
> +        cpu->interrupt_request | mask);
>  
>      if (!qemu_cpu_is_self(cpu)) {
>          qemu_cpu_kick(cpu);
> -- 
> 2.47.1
> 

Besides the two writters mentioned above, I still see some more:

*** accel/tcg/user-exec.c:
cpu_interrupt[52]              cpu->interrupt_request |= mask;
*** hw/intc/s390_flic.c:
qemu_s390_flic_notify[193]     cs->interrupt_request |= CPU_INTERRUPT_HARD;
*** hw/openrisc/cputimer.c:
openrisc_timer_cb[108]         cs->interrupt_request |= CPU_INTERRUPT_TIMER;
*** target/arm/helper.c:
arm_cpu_do_interrupt[9150]     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
*** target/i386/tcg/system/svm_helper.c:
helper_vmrun[406]              cs->interrupt_request |= CPU_INTERRUPT_VIRQ;

Do they better as well be converted to use store_release too?

The other nitpick is this patch introduces the reader helper but without
using it.

It can be squashed into the other patch where the reader helper will be
applied tree-wide.  IMHO that would be better explaining the helper on its
own.

Thanks,

-- 
Peter Xu

On Mon, 11 Aug 2025 12:31:27 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Fri, Aug 08, 2025 at 02:01:33PM +0200, Igor Mammedov wrote:
> > the helper forms load-acquire/store-release pair with
> > tcg_handle_interrupt/generic_handle_interrupt and can be used
> > for checking interrupts outside of BQL
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/core/cpu.h     | 12 ++++++++++++
> >  accel/tcg/tcg-accel-ops.c |  3 ++-
> >  system/cpus.c             |  3 ++-
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 5eaf41a566..d0460c01cf 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -942,6 +942,18 @@ 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_pc:
> >   * @cpu: The CPU to set the program counter for.
> > diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> > index 3b0d7d298e..02c7600bb7 100644
> > --- a/accel/tcg/tcg-accel-ops.c
> > +++ b/accel/tcg/tcg-accel-ops.c
> > @@ -97,7 +97,8 @@ 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;
> > +    qatomic_store_release(&cpu->interrupt_request,
> > +        cpu->interrupt_request | mask);
> >  
> >      /*
> >       * If called from iothread context, wake the target cpu in
> > diff --git a/system/cpus.c b/system/cpus.c
> > index 256723558d..8e543488c3 100644
> > --- a/system/cpus.c
> > +++ b/system/cpus.c
> > @@ -256,7 +256,8 @@ int64_t cpus_get_elapsed_ticks(void)
> >  
> >  void generic_handle_interrupt(CPUState *cpu, int mask)
> >  {
> > -    cpu->interrupt_request |= mask;
> > +    qatomic_store_release(&cpu->interrupt_request,
> > +        cpu->interrupt_request | mask);
> >  
> >      if (!qemu_cpu_is_self(cpu)) {
> >          qemu_cpu_kick(cpu);
> > -- 
> > 2.47.1
> >   
> 
> Besides the two writters mentioned above, I still see some more:
> 
> *** accel/tcg/user-exec.c:
> cpu_interrupt[52]              cpu->interrupt_request |= mask;

I don't know if external interrupts can happen in user mode,
for same thread(self) exceptions we don't really need it.

> *** hw/intc/s390_flic.c:
> qemu_s390_flic_notify[193]     cs->interrupt_request |= CPU_INTERRUPT_HARD;

later on the function, for cpus it deemed not to ignore,
explicitly calls cpu_interrupt() which will do store_release.
I'd tentatively would say we don't need explicit store_release here

Anyways CCing David, perhaps he would recall why it's setting interrupt for all
but ignores to actually rise it for some.

> *** hw/openrisc/cputimer.c:
> openrisc_timer_cb[108]         cs->interrupt_request |= CPU_INTERRUPT_TIMER;

this one seems to be similar to generic_handle_interrupt(),
interrupt request from external thread, so I'd add store_release here.

Arguably, it should be calling cpu_interrupt() instead of opencoding it.
(the questionable part is that it set interrupt conditionally but
kicks vccpu always - is this really necessary?)


> *** target/arm/helper.c:
> arm_cpu_do_interrupt[9150]     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
on kvm it can be reached via main thread or vcpu(self) thread.
so tentatively we need it here.

(ps: kvm_arch_on_sigbus_vcpu()->arm_cpu_do_interrupt() is called under BQL
while kvm_on_sigbus() is called without one from signal handler,
which theoretically might trample on the thread running the 1st vcpu)

> *** target/i386/tcg/system/svm_helper.c:
> helper_vmrun[406]              cs->interrupt_request |= CPU_INTERRUPT_VIRQ;
this one seems to be self targeted (i.e. same thread),
perhaps TCG experts can comment on it.

CCing TCG folks as series touches a few TCG bits anyway


> Do they better as well be converted to use store_release too?

alternatively, for consistency sake we can add a helper to set interrupt
with store_release included and do a blanket tree wide change like with
cpu_test_interrupt().
 
> The other nitpick is this patch introduces the reader helper but without
> using it.
> 
> It can be squashed into the other patch where the reader helper will be
> applied tree-wide.  IMHO that would be better explaining the helper on its
> own.

I can merge it with 7/10 that adds 1st user for the helper in kvm/i386 code.
That has less chances for the store/acquire change to be drowned in
tree wide patch (9/10)

> 
> Thanks,
>

On Tue, Aug 12, 2025 at 05:00:35PM +0200, Igor Mammedov wrote:
> > Do they better as well be converted to use store_release too?
> 
> alternatively, for consistency sake we can add a helper to set interrupt
> with store_release included and do a blanket tree wide change like with
> cpu_test_interrupt().

Yep, that sounds like the simplest. Unless there's any concern on possible
performance regressions due to the rest conversions on store_release, those
can be special treated with open-code, tagged with reasoning as comment.

>  
> > The other nitpick is this patch introduces the reader helper but without
> > using it.
> > 
> > It can be squashed into the other patch where the reader helper will be
> > applied tree-wide.  IMHO that would be better explaining the helper on its
> > own.
> 
> I can merge it with 7/10 that adds 1st user for the helper in kvm/i386 code.
> That has less chances for the store/acquire change to be drowned in
> tree wide patch (9/10)

For mem barrier changes, IMHO the two sides are better in one patch, hence
the two helpers need better to appear in one patch to show the pairing of
them.  That's what this patche does.

Then, from any helper POV it's better one helper introduced together in the
patch where it will be used to justify the helper with the applicable context.

From that POV, the clean way, IMHO, should be that we have one prior patch
introducing the reader/writter helpers, together using it globally with
existing users.  Then the kvm patch can use the new helpers.

No strong opinion here, though..

Thanks,

-- 
Peter Xu