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).
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()
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.
+}
+
+/**
+ * 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)
+}
+
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?
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.
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.
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 &
target/i386/helper.c:600: int sipi = cs->interrupt_request &
CPU_INTERRUPT_SIPI;
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;
* Possible uses of qatomic_load_acquire()?
accel/tcg/cpu-exec.c:801: int interrupt_request =
cpu->interrupt_request;
accel/tcg/tcg-accel-ops-icount.c:152: int old_mask =
cpu->interrupt_request;
* Candidates for cpu_clear_interrupts()
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);
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;
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;
