On Mon, 11 Aug 2025 12:31:27 -0400
Peter Xu <pet...@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 <imamm...@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,
>
