On Mon, 25 Aug 2025 12:35:51 +0200
Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
> Hi Igor,
>
> On 21/8/25 17:56, 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.
>
> I don't understand the beginning of this sentence (even prepending
> the patch subject).
yep, it looks botched, how about following?
"
The helpers form load-acquire/store-release pair and ensure
that appropriate barriers are in place in case checks happen
outside of BQL.
Use them to replace open-coded checkers/setters across the code,
to make sure that barriers are not missed.
Helpers also make code a bit more readable.
"
>
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > Reviewed-by: Peter Xu <pet...@redhat.com>
> > Reviewed-by: Jason J. Herne <jjhe...@linux.ibm.com>
> > ---
> > v5: fix copy/paste error in doc comment of cpu_set_interrupt()
> > "Jason J. Herne" <jjhe...@linux.ibm.com>
> > v4:
> > add cpu_set_interrupt() and merge helpers patch with
> > patches that use them (v3 6-7,9/10).
> > Peter Xu <pet...@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.
>
> Can we use plural form to express we might test for more
> than one interrupt in the mask?
>
> -> cpu_test_interrupts()
it follow the same pattern as existing cpu_interrupt()/
generic_handle_interrupt() which take the same 'mask'
and cpu_handle_interrupt() that also handles a bunch of interrupts
and CPUState::interrupt_request also in singular form.
I don't have preference here but plural here would be
inconsistent with existing pattern all over QEMU and
it would be better to be consistent here.
>
> otherwise cpu_test_interrupt_mask().
>
> > + *
> > + * 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;
> How missing "qemu/atomic.h" header.
it's included implicitly via "qemu/rcu_queue.h"
>
> > +}
> > +
> > +/**
> > + * cpu_set_interrupt:
> > + * @cpu: The CPU to set pending interrupt(s) on.
> > + * @mask: The interrupts to set.
>
> Rename plural: cpu_set_interrupts() or _mask().
>
> > + *
> > + * 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);
>
> (indent is off)
checkpatch was fine with it and it is (4+4) which is
within codestyle rules and the pattern widely used in QEMU.
>
> > +}
> > +
> The field is described in CPUState as:
>
> @interrupt_request: Indicates a pending interrupt request.
>
> Can we update the description, mentioning its access should be
> via the proper cpu_test_interrupts/cpu_set_interrupts/... helpers?
I didn't do this because not all places are using helpers
and there are places which just wouldn't use them
(example: plain copy of the field).
I'd would leave this change out.
> > 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))) {
>
> Maybe nitpicking, but I'd introduce the API and use it first only where
> we already use atomic accesses. Then convert the rest of the code base
> to this API in a distinct patch.
I thought so and it was this way before v4,
but I was asked to merge API and users in the same patch.
It's all depends on how many changes we are going to squash together.
>
> > 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;
> > 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;
>
> Can we have a cpu_clear_interrupts() helper for completion? Likely we
> need the same BQL-safe access.
see below
>
> > 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 */
> Looking at v4 applied on top of v10.1.0-rc4 reconstructing patch
> #6 with v5:
>
> $ git grep -F -- '->interrupt_request'
>
> * Missing cpu_test_interrupts()
>
> target/arm/machine.c:968: env->irq_line_state =
> cs->interrupt_request &
it's not a test but a bitmask copy in cpu_post_load()
where a barrier won't make a difference.
though for consistency sake we can introduce something like
int cpu_get_interrupt(CPUState*)
that will use acquire, but that won't guarantee much as
copy might get stale after it's read.
> target/i386/helper.c:600: int sipi = cs->interrupt_request &
> CPU_INTERRUPT_SIPI;
ditto
> target/i386/kvm/kvm.c:5067: events.smi.pending =
> cs->interrupt_request & CPU_INTERRUPT_SMI;
> target/i386/kvm/kvm.c:5068: events.smi.latched_init =
> cs->interrupt_request & CPU_INTERRUPT_INIT;
these indeed could use the helper
>
> * Possible uses of qatomic_load_acquire()?
>
> accel/tcg/cpu-exec.c:801: int interrupt_request =
> cpu->interrupt_request;
for TCG interrupt processing seems to be under BQL, so it
shouldn't make a difference.
It might be a separate patch, but tit would be hard to come up
with justification for it.
But see 8/8, this line is moved closer to user and
cpu_handle_interrupt() is cleaned up.
> accel/tcg/tcg-accel-ops-icount.c:152: int old_mask =
> cpu->interrupt_request;
another copy example, executed within cpu thread,
we don't really need acquire here and following
tcg_handle_interrupt->cpu_set_interrupt adds implicit one.
>
> * Candidates for cpu_clear_interrupts()
it wasn't goal of this series
(Paolo suggested to include cpu_interrupt()+cpu_test_interrupt()
test pair assuming it would be relatively small change, and here we are).
Also taking in account that setting interrupts are usually* done under BQL,
cpu_clear_interrupts() would be just a wrapper without a barrier.
and then we already have cpu_reset_interrupt() which should clear interrupts,
taking BQL if needed.
Anyways it's quite involving refactoring, derailing series even further
from its scope.
If it's deemed necessary probably another series taking care of
interrupt clearing tree wide is in order. (but I'd prefer take care of
it separately of this series).
> accel/tcg/cpu-exec.c:784: cpu->interrupt_request &=
> ~CPU_INTERRUPT_DEBUG;
> accel/tcg/cpu-exec.c:794: cpu->interrupt_request &=
> ~CPU_INTERRUPT_HALT;
> accel/tcg/cpu-exec.c:842: cpu->interrupt_request &=
> ~CPU_INTERRUPT_EXITTB;
> hw/core/cpu-common.c:79: cpu->interrupt_request &= ~mask;
> hw/core/cpu-system.c:207: cpu->interrupt_request &= ~0x01;
> target/avr/helper.c:50: cs->interrupt_request &=
> ~CPU_INTERRUPT_RESET;
> target/avr/helper.c:62: cs->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
> target/i386/helper.c:605: cs->interrupt_request = sipi;
> target/i386/hvf/x86hvf.c:400: cs->interrupt_request &=
> ~CPU_INTERRUPT_NMI;
> target/i386/hvf/x86hvf.c:412: cs->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
> target/i386/hvf/x86hvf.c:440: cs->interrupt_request &=
> ~CPU_INTERRUPT_POLL;
> target/i386/hvf/x86hvf.c:453: cs->interrupt_request &=
> ~CPU_INTERRUPT_TPR;
> target/i386/kvm/kvm.c:5069: cs->interrupt_request &=
> ~(CPU_INTERRUPT_INIT | CPU_INTERRUPT_SMI);
* this one might be cleared outside of BQL
(kvm_cpu_exec->kvm_arch_put_registers->kvm_put_vcpu_events path)
Paolo, Peter,
should we take BQL here, if not then why?
(kvm_arch_pre_run() that does run after it, does take BQL when resetting
interrupts)
> target/i386/kvm/kvm.c:5459: cpu->interrupt_request &=
> ~CPU_INTERRUPT_NMI;
> target/i386/kvm/kvm.c:5470: cpu->interrupt_request &=
> ~CPU_INTERRUPT_SMI;
> target/i386/kvm/kvm.c:5505: cpu->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
> target/i386/kvm/kvm.c:5600: cs->interrupt_request &=
> ~CPU_INTERRUPT_MCE;
> target/i386/kvm/kvm.c:5630: cs->interrupt_request &=
> ~CPU_INTERRUPT_POLL;
> target/i386/kvm/kvm.c:5643: cs->interrupt_request &=
> ~CPU_INTERRUPT_TPR;
> target/i386/nvmm/nvmm-all.c:422: cpu->interrupt_request &=
> ~CPU_INTERRUPT_NMI;
> target/i386/nvmm/nvmm-all.c:431: cpu->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
> target/i386/nvmm/nvmm-all.c:440: cpu->interrupt_request &=
> ~CPU_INTERRUPT_SMI;
> target/i386/nvmm/nvmm-all.c:700: cpu->interrupt_request &=
> ~CPU_INTERRUPT_POLL;
> target/i386/nvmm/nvmm-all.c:713: cpu->interrupt_request &=
> ~CPU_INTERRUPT_TPR;
> target/i386/tcg/system/seg_helper.c:181: cs->interrupt_request &=
> ~CPU_INTERRUPT_POLL;
> target/i386/tcg/system/seg_helper.c:189: cs->interrupt_request &=
> ~CPU_INTERRUPT_SMI;
> target/i386/tcg/system/seg_helper.c:194: cs->interrupt_request &=
> ~CPU_INTERRUPT_NMI;
> target/i386/tcg/system/seg_helper.c:199: cs->interrupt_request &=
> ~CPU_INTERRUPT_MCE;
> target/i386/tcg/system/seg_helper.c:204: cs->interrupt_request &=
> ~(CPU_INTERRUPT_HARD |
> target/i386/tcg/system/seg_helper.c:218: cs->interrupt_request &=
> ~CPU_INTERRUPT_VIRQ;
> target/i386/tcg/system/svm_helper.c:827: cs->interrupt_request &=
> ~CPU_INTERRUPT_VIRQ;
I had a question wrt this one, in one of previous versions,
it's buried deep in opcode handling, and hard to tell if this one is
BQL protected or not. Perhaps someone more familiar with TCG can clarify
if it's safe as it is now.
for comparison target/openrisc/sys_helper.c: HELPER(mtspr) does take BQL
explicitly
> target/i386/whpx/whpx-all.c:1474: cpu->interrupt_request &=
> ~CPU_INTERRUPT_NMI;
> target/i386/whpx/whpx-all.c:1481: cpu->interrupt_request &=
> ~CPU_INTERRUPT_SMI;
> target/i386/whpx/whpx-all.c:1505: cpu->interrupt_request
> &= ~CPU_INTERRUPT_HARD;
> target/i386/whpx/whpx-all.c:1523: cpu->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
> target/i386/whpx/whpx-all.c:1610: cpu->interrupt_request &=
> ~CPU_INTERRUPT_POLL;
> target/i386/whpx/whpx-all.c:1626: cpu->interrupt_request &=
> ~CPU_INTERRUPT_TPR;
> target/openrisc/sys_helper.c:199: cs->interrupt_request
> &= ~CPU_INTERRUPT_TIMER;
> target/rx/helper.c:66: cs->interrupt_request &=
> ~CPU_INTERRUPT_FIR;
> target/rx/helper.c:76: cs->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
> target/s390x/tcg/excp_helper.c:562: cs->interrupt_request &=
> ~CPU_INTERRUPT_HARD;
>
