Re: [PATCH v3 06/10] introduce cpu_test_interrupt() that will replace open coded checks

[Igor Mammedov <imammedo@redhat.com>]

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,
>