[PATCH v2 00/11] powerpc: KCSAN fix warnings and mark accesses

[PATCH v2 00/11] powerpc: KCSAN fix warnings and mark accesses

[Rohan McLure]

v1 of this patch series available here:
Link: https://lore.kernel.org/linuxppc-dev/20230508020120.218494-1-rmclure@linux.ibm.com/

The KCSAN sanitiser notifies programmers of instances where unmarked
accesses to shared state has lead to a data race, or when the compiler
has liberty to reorder an unmarked access and so generate a data race.
This patch series deals with benign data races, which nonetheless need
annotation in order to ensure the correctness of the emitted code.

In keeping with the principles given in
tools/memory-model/Documentation/access-marking.txt, racing reads of
shared state for purely diagnostic/debug purposes are annotated with
data_race, while reads/writes that are examples of intention polling of
shared variables are performed with READ_ONCE, WRITE_ONCE.

These changes remove the majority of warnings observable on pseries and
powernv, where for development, I was able to narrow down to only power
relevant bugs by temporarily disabling sanitisation for all other files.
Future patch series will deal with the subtler bugs which persist under
this configuration.

KCSAN races addressed:
 - qspinlock: assignign of qnode->locked and polling
 - check_return_regs_valid [h]srr_valid
 - arch_cpu_idle idle callback
 - powernv idle_state paca entry (polling the bit-lock is viewed by
   KCSAN as asynchronous access to the fields it protects)
 - Asynchronous access to irq_data->hwirq
 - Opal asynchronous event handling
 - IPIs

Miscellaneous other changes:

 - Annotate the asm-generic/mmiowb code, which riscv and powerpc each
   consume
 - Update usages of qnode->locked in powerpc's qspinlock interpretation
   to reflect the comment beside this field

v2:
 - Match READ_ONCE with WRITE_ONCE and vice versa where required
 - In arch/powerpc/lib/qspinlock.c, use kcsan_release() to notify KCSAN
   of locked being assigned prior to publish, and remove extraneous
   compiler barrier (publish_tail_cpu features memory clobber).
 - Keep polarity for locked variable in qspinlock
 - Remove extraneous READ_ONCE in mmiowb()
 - Use data_race() for power_save callback to remove instrumentation, as
   there is no real data race

Rohan McLure (11):
  powerpc: qspinlock: Mark accesses to qnode lock checks
  powerpc: qspinlock: Enforce qnode writes prior to publishing to queue
  asm-generic/mmiowb: Mark accesses to fix KCSAN warnings
  powerpc: Mark [h]ssr_valid accesses in check_return_regs_valid
  powerpc: Mark accesses to power_save callback in arch_cpu_idle
  powerpc: powernv: Fix KCSAN datarace warnings on idle_state contention
  powerpc: Annotate accesses to ipi message flags
  powerpc: Mark writes registering ipi to host cpu through kvm and
    polling
  powerpc: powernv: Annotate data races in opal events
  powerpc: powernv: Annotate asynchronous access to opal tokens
  powerpc: Mark asynchronous accesses to irq_data

 arch/powerpc/include/asm/kvm_ppc.h            |  4 ++--
 arch/powerpc/include/asm/paca.h               |  1 +
 arch/powerpc/include/asm/ptrace.h             |  4 ++--
 arch/powerpc/kernel/idle.c                    |  9 ++++++---
 arch/powerpc/kernel/interrupt.c               | 14 ++++++--------
 arch/powerpc/kernel/irq.c                     |  2 +-
 arch/powerpc/kernel/smp.c                     |  4 ++--
 arch/powerpc/kvm/book3s_hv_builtin.c          |  4 ++--
 arch/powerpc/lib/qspinlock.c                  | 11 +++++++++--
 arch/powerpc/platforms/powernv/idle.c         | 16 +++++++++-------
 arch/powerpc/platforms/powernv/opal-async.c   |  6 +++---
 arch/powerpc/platforms/powernv/opal-irqchip.c |  6 +++---
 arch/powerpc/platforms/powernv/pci-ioda.c     | 12 ++++++------
 include/asm-generic/mmiowb.h                  | 14 +++++++++-----
 include/linux/irq.h                           |  2 +-
 kernel/irq/irqdomain.c                        |  4 ++--
 16 files changed, 64 insertions(+), 49 deletions(-)

-- 
2.37.2

[PATCH v2 01/11] powerpc: qspinlock: Mark accesses to qnode lock checks

[Rohan McLure]

The powerpc implemenation of qspinlocks will both poll and spin on the
bitlock guarding a qnode. Mark these accesses with READ_ONCE to convey
to KCSAN that polling is intentional here.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/lib/qspinlock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index e4bd145255d0..b76c1f6acce5 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -435,7 +435,7 @@ static __always_inline bool yield_to_prev(struct qspinlock *lock, struct qnode *
 
 	smp_rmb(); /* See __yield_to_locked_owner comment */
 
-	if (!node->locked) {
+	if (!READ_ONCE(node->locked)) {
 		yield_to_preempted(prev_cpu, yield_count);
 		spin_begin();
 		return preempted;
@@ -584,7 +584,7 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
 
 		/* Wait for mcs node lock to be released */
 		spin_begin();
-		while (!node->locked) {
+		while (!READ_ONCE(node->locked)) {
 			spec_barrier();
 
 			if (yield_to_prev(lock, node, old, paravirt))
-- 
2.37.2

[PATCH v2 02/11] powerpc: qspinlock: Enforce qnode writes prior to publishing to queue

[Rohan McLure]

Annotate the release barrier and memory clobber (in effect, producing a
compiler barrier) in the publish_tail_cpu call. These barriers have the
effect of ensuring that qnode attributes are all written to prior to
publish the node to the waitqueue.

Even while the initial write to the 'locked' attribute is guaranteed to
terminate prior to the node being visible, KCSAN still complains that
the write is reorderable by the compiler. Issue a kcsan_release() to
inform KCSAN of the release barrier contained in publish_tail_cpu().

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
v2: Remove extraneous compiler barrier, but annotate release-barrier
contained in call publish_tail_cpu(), and include kcsan_release().
---
 arch/powerpc/lib/qspinlock.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index b76c1f6acce5..253620979d0c 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -161,6 +161,8 @@ static __always_inline u32 publish_tail_cpu(struct qspinlock *lock, u32 tail)
 {
 	u32 prev, tmp;
 
+	kcsan_release();
+
 	asm volatile(
 "\t"	PPC_RELEASE_BARRIER "						\n"
 "1:	lwarx	%0,0,%2		# publish_tail_cpu			\n"
@@ -570,6 +572,11 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
 
 	tail = encode_tail_cpu(node->cpu);
 
+	/*
+	 * Assign all attributes of a node before it can be published.
+	 * Issues an lwsync, serving as a release barrier, as well as a
+	 * compiler barrier.
+	 */
 	old = publish_tail_cpu(lock, tail);
 
 	/*
-- 
2.37.2

[PATCH v2 03/11] asm-generic/mmiowb: Mark accesses to fix KCSAN warnings

[Rohan McLure]

Prior to this patch, data races are detectable by KCSAN of the following
forms:

[1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
    or otherwise outside of a critical section
[2] Interrupted critical sections, where the interrupt will itself
    acquire a lock

In case [1], calling context does not need an mmiowb() call to be
issued, otherwise it would do so itself. Such calls to
mmiowb_set_pending() are either idempotent or no-ops.

In case [2], irrespective of when the interrupt occurs, the interrupt
will acquire and release its locks prior to its return, nesting_count
will continue balanced. In the worst case, the interrupted critical
section during a mmiowb_spin_unlock() call observes an mmiowb to be
pending and afterward is interrupted, leading to an extraneous call to
mmiowb(). This data race is clearly innocuous.

Mark all potentially asynchronous memory accesses with READ_ONCE or
WRITE_ONCE, including increments and decrements to nesting_count. This
has the effect of removing KCSAN warnings at consumer's callsites.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reported-by: Gautam Menghani <gautam@linux.ibm.com>
Tested-by: Gautam Menghani <gautam@linux.ibm.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
---
 include/asm-generic/mmiowb.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/asm-generic/mmiowb.h b/include/asm-generic/mmiowb.h
index 5698fca3bf56..6dea28c8835b 100644
--- a/include/asm-generic/mmiowb.h
+++ b/include/asm-generic/mmiowb.h
@@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void)
 	struct mmiowb_state *ms = __mmiowb_state();
 
 	if (likely(ms->nesting_count))
-		ms->mmiowb_pending = ms->nesting_count;
+		WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count);
 }
 
 static inline void mmiowb_spin_lock(void)
 {
 	struct mmiowb_state *ms = __mmiowb_state();
-	ms->nesting_count++;
+
+	/* Increment need not be atomic. Nestedness is balanced over interrupts. */
+	WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1);
 }
 
 static inline void mmiowb_spin_unlock(void)
 {
 	struct mmiowb_state *ms = __mmiowb_state();
+	u16 pending = READ_ONCE(ms->mmiowb_pending);
 
-	if (unlikely(ms->mmiowb_pending)) {
-		ms->mmiowb_pending = 0;
+	WRITE_ONCE(ms->mmiowb_pending, 0);
+	if (unlikely(pending)) {
 		mmiowb();
 	}
 
-	ms->nesting_count--;
+	/* Decrement need not be atomic. Nestedness is balanced over interrupts. */
+	WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1);
 }
 #else
 #define mmiowb_set_pending()		do { } while (0)
-- 
2.37.2

[PATCH v2 04/11] powerpc: Mark [h]ssr_valid accesses in check_return_regs_valid

[Rohan McLure]

Checks to see if the [H]SRR registers have been clobbered by (soft)
NMI interrupts imply the possibility for a data race on the
[h]srr_valid entries in the PACA. Annotate accesses to these fields with
READ_ONCE, removing the need for the barrier.

The diagnostic can use plain-access reads and writes, but annotate with
data_race.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/ptrace.h |  4 ++--
 arch/powerpc/kernel/interrupt.c   | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 0eb90a013346..9db8b16567e2 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -180,8 +180,8 @@ void do_syscall_trace_leave(struct pt_regs *regs);
 static inline void set_return_regs_changed(void)
 {
 #ifdef CONFIG_PPC_BOOK3S_64
-	local_paca->hsrr_valid = 0;
-	local_paca->srr_valid = 0;
+	WRITE_ONCE(local_paca->hsrr_valid, 0);
+	WRITE_ONCE(local_paca->srr_valid, 0);
 #endif
 }
 
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index e34c72285b4e..1f033f11b871 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -125,7 +125,7 @@ static notrace void check_return_regs_valid(struct pt_regs *regs)
 	case 0x1600:
 	case 0x1800:
 		validp = &local_paca->hsrr_valid;
-		if (!*validp)
+		if (!READ_ONCE(*validp))
 			return;
 
 		srr0 = mfspr(SPRN_HSRR0);
@@ -135,7 +135,7 @@ static notrace void check_return_regs_valid(struct pt_regs *regs)
 		break;
 	default:
 		validp = &local_paca->srr_valid;
-		if (!*validp)
+		if (!READ_ONCE(*validp))
 			return;
 
 		srr0 = mfspr(SPRN_SRR0);
@@ -161,19 +161,17 @@ static notrace void check_return_regs_valid(struct pt_regs *regs)
 	 * such things will get caught most of the time, statistically
 	 * enough to be able to get a warning out.
 	 */
-	barrier();
-
-	if (!*validp)
+	if (!READ_ONCE(*validp))
 		return;
 
-	if (!warned) {
-		warned = true;
+	if (!data_race(warned)) {
+		data_race(warned = true);
 		printk("%sSRR0 was: %lx should be: %lx\n", h, srr0, regs->nip);
 		printk("%sSRR1 was: %lx should be: %lx\n", h, srr1, regs->msr);
 		show_regs(regs);
 	}
 
-	*validp = 0; /* fixup */
+	WRITE_ONCE(*validp, 0); /* fixup */
 #endif
 }
 
-- 
2.37.2

[PATCH v2 05/11] powerpc: Mark accesses to power_save callback in arch_cpu_idle

[Rohan McLure]

The power_save callback can be overwritten by another core at boot time.
Specifically, null values will be replaced exactly once with the callback
suitable for the particular platform (PowerNV / pseries lpars), making
this value a good candidate for __ro_after_init.

Even with this the case, KCSAN sees unmarked reads to the callback
variable, and notices that unfortunate compiler reorderings could lead
to distinct function pointers being read. In reality this is impossible,
so don't instrument at this read.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
v2: Mark instances at init where the callback is written to, and
data_race() read as there is no capacity for the value to change
underneath.
---
 arch/powerpc/kernel/idle.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
index b1c0418b25c8..43d96c0e3b96 100644
--- a/arch/powerpc/kernel/idle.c
+++ b/arch/powerpc/kernel/idle.c
@@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable);
 
 static int __init powersave_off(char *arg)
 {
-	ppc_md.power_save = NULL;
+	WRITE_ONCE(ppc_md.power_save, NULL);
 	cpuidle_disable = IDLE_POWERSAVE_OFF;
 	return 1;
 }
@@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off);
 
 void arch_cpu_idle(void)
 {
+	/* power_save callback assigned only at init so no data race */
+	void (*power_save)(void) = data_race(ppc_md.power_save);
+
 	ppc64_runlatch_off();
 
-	if (ppc_md.power_save) {
-		ppc_md.power_save();
+	if (power_save) {
+		power_save();
 		/*
 		 * Some power_save functions return with
 		 * interrupts enabled, some don't.
-- 
2.37.2

[PATCH v2 06/11] powerpc: powernv: Fix KCSAN datarace warnings on idle_state contention

[Rohan McLure]

The idle_state entry in the PACA on PowerNV features a bit which is
atomically tested and set through ldarx/stdcx. to be used as a spinlock.
This lock then guards access to other bit fields of idle_state. KCSAN
cannot differentiate between any of these bitfield accesses as they all
are implemented by 8-byte store/load instructions, thus cores contending
on the bit-lock appear to data race with modifications to idle_state.

Separate the bit-lock entry from the data guarded by the lock to avoid
the possibility of data races being detected by KCSAN.

Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
v2: Remove extraneous WRITE_ONCE on paca thread_idle_state, which are
only read diagnostically.
---
 arch/powerpc/include/asm/paca.h       |  1 +
 arch/powerpc/platforms/powernv/idle.c | 16 +++++++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index da0377f46597..cb325938766a 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -191,6 +191,7 @@ struct paca_struct {
 #ifdef CONFIG_PPC_POWERNV
 	/* PowerNV idle fields */
 	/* PNV_CORE_IDLE_* bits, all siblings work on thread 0 paca */
+	unsigned long idle_lock; /* A value of 1 means acquired */
 	unsigned long idle_state;
 	union {
 		/* P7/P8 specific fields */
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 841cb7f31f4f..c1e0ecb014a5 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -246,9 +246,9 @@ static inline void atomic_lock_thread_idle(void)
 {
 	int cpu = raw_smp_processor_id();
 	int first = cpu_first_thread_sibling(cpu);
-	unsigned long *state = &paca_ptrs[first]->idle_state;
+	unsigned long *lock = &paca_ptrs[first]->idle_lock;
 
-	while (unlikely(test_and_set_bit_lock(NR_PNV_CORE_IDLE_LOCK_BIT, state)))
+	while (unlikely(test_and_set_bit_lock(NR_PNV_CORE_IDLE_LOCK_BIT, lock)))
 		barrier();
 }
 
@@ -258,29 +258,31 @@ static inline void atomic_unlock_and_stop_thread_idle(void)
 	int first = cpu_first_thread_sibling(cpu);
 	unsigned long thread = 1UL << cpu_thread_in_core(cpu);
 	unsigned long *state = &paca_ptrs[first]->idle_state;
+	unsigned long *lock = &paca_ptrs[first]->idle_lock;
 	u64 s = READ_ONCE(*state);
 	u64 new, tmp;
 
-	BUG_ON(!(s & PNV_CORE_IDLE_LOCK_BIT));
+	BUG_ON(!(READ_ONCE(*lock) & PNV_CORE_IDLE_LOCK_BIT));
 	BUG_ON(s & thread);
 
 again:
-	new = (s | thread) & ~PNV_CORE_IDLE_LOCK_BIT;
+	new = s | thread;
 	tmp = cmpxchg(state, s, new);
 	if (unlikely(tmp != s)) {
 		s = tmp;
 		goto again;
 	}
+	clear_bit_unlock(NR_PNV_CORE_IDLE_LOCK_BIT, lock);
 }
 
 static inline void atomic_unlock_thread_idle(void)
 {
 	int cpu = raw_smp_processor_id();
 	int first = cpu_first_thread_sibling(cpu);
-	unsigned long *state = &paca_ptrs[first]->idle_state;
+	unsigned long *lock = &paca_ptrs[first]->idle_lock;
 
-	BUG_ON(!test_bit(NR_PNV_CORE_IDLE_LOCK_BIT, state));
-	clear_bit_unlock(NR_PNV_CORE_IDLE_LOCK_BIT, state);
+	BUG_ON(!test_bit(NR_PNV_CORE_IDLE_LOCK_BIT, lock));
+	clear_bit_unlock(NR_PNV_CORE_IDLE_LOCK_BIT, lock);
 }
 
 /* P7 and P8 */
-- 
2.37.2

[PATCH v2 07/11] powerpc: Annotate accesses to ipi message flags

[Rohan McLure]

IPI message flags are observed and consequently consumed in the
smp_ipi_demux_relaxed function, which handles these message sources
until it observes none more arriving. Mark the checked loop guard with
READ_ONCE, to signal to KCSAN that the read is known to be volatile, and
that non-determinism is expected. Mark write for message source in
smp_muxed_ipi_set_message().

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
v2: Add missing WRITE_ONCE() in smp_muxed_ipi_set_message().
---
 arch/powerpc/kernel/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 6b90f10a6c81..fb35a147b4fa 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -289,7 +289,7 @@ void smp_muxed_ipi_set_message(int cpu, int msg)
 	 * Order previous accesses before accesses in the IPI handler.
 	 */
 	smp_mb();
-	message[msg] = 1;
+	WRITE_ONCE(message[msg], 1);
 }
 
 void smp_muxed_ipi_message_pass(int cpu, int msg)
@@ -348,7 +348,7 @@ irqreturn_t smp_ipi_demux_relaxed(void)
 		if (all & IPI_MESSAGE(PPC_MSG_NMI_IPI))
 			nmi_ipi_action(0, NULL);
 #endif
-	} while (info->messages);
+	} while (READ_ONCE(info->messages));
 
 	return IRQ_HANDLED;
 }
-- 
2.37.2

[PATCH v2 08/11] powerpc: Mark writes registering ipi to host cpu through kvm and polling

[Rohan McLure]

Mark writes to hypervisor ipi state so that KCSAN recognises these
asynchronous issue of kvmppc_{set,clear}_host_ipi to be intended, with
atomic writes. Mark asynchronous polls to this variable in
kvm_ppc_read_one_intr().

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
v2: Add read-side annotations to both polling locations in
kvm_ppc_read_one_intr().
---
 arch/powerpc/include/asm/kvm_ppc.h   | 4 ++--
 arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index bc57d058ad5b..d701df006c08 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -548,12 +548,12 @@ static inline void kvmppc_set_host_ipi(int cpu)
 	 * pairs with the barrier in kvmppc_clear_host_ipi()
 	 */
 	smp_mb();
-	paca_ptrs[cpu]->kvm_hstate.host_ipi = 1;
+	WRITE_ONCE(paca_ptrs[cpu]->kvm_hstate.host_ipi, 1);
 }
 
 static inline void kvmppc_clear_host_ipi(int cpu)
 {
-	paca_ptrs[cpu]->kvm_hstate.host_ipi = 0;
+	WRITE_ONCE(paca_ptrs[cpu]->kvm_hstate.host_ipi, 0);
 	/*
 	 * order clearing of host_ipi flag vs. processing of IPI messages
 	 *
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
index da85f046377a..0f5b021fa559 100644
--- a/arch/powerpc/kvm/book3s_hv_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_builtin.c
@@ -406,7 +406,7 @@ static long kvmppc_read_one_intr(bool *again)
 		return 1;
 
 	/* see if a host IPI is pending */
-	host_ipi = local_paca->kvm_hstate.host_ipi;
+	host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
 	if (host_ipi)
 		return 1;
 
@@ -466,7 +466,7 @@ static long kvmppc_read_one_intr(bool *again)
 		 * meantime. If it's clear, we bounce the interrupt to the
 		 * guest
 		 */
-		host_ipi = local_paca->kvm_hstate.host_ipi;
+		host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
 		if (unlikely(host_ipi != 0)) {
 			/* We raced with the host,
 			 * we need to resend that IPI, bummer
-- 
2.37.2

[PATCH v2 09/11] powerpc: powernv: Annotate data races in opal events

[Rohan McLure]

The kopald thread handles opal events as they appear, but by polling a
static bit-vector in last_outstanding_events. Annotate these data races
accordingly. We are not at risk of missing events, but use of READ_ONCE,
WRITE_ONCE will assist readers in seeing that kopald only consumes the
events it is aware of when it is scheduled. Also removes extraneous
KCSAN warnings.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/opal-irqchip.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-irqchip.c b/arch/powerpc/platforms/powernv/opal-irqchip.c
index d55652b5f6fa..f9a7001dacb7 100644
--- a/arch/powerpc/platforms/powernv/opal-irqchip.c
+++ b/arch/powerpc/platforms/powernv/opal-irqchip.c
@@ -59,7 +59,7 @@ void opal_handle_events(void)
 
 		cond_resched();
 	}
-	last_outstanding_events = 0;
+	WRITE_ONCE(last_outstanding_events, 0);
 	if (opal_poll_events(&events) != OPAL_SUCCESS)
 		return;
 	e = be64_to_cpu(events) & opal_event_irqchip.mask;
@@ -69,7 +69,7 @@ void opal_handle_events(void)
 
 bool opal_have_pending_events(void)
 {
-	if (last_outstanding_events & opal_event_irqchip.mask)
+	if (READ_ONCE(last_outstanding_events) & opal_event_irqchip.mask)
 		return true;
 	return false;
 }
@@ -124,7 +124,7 @@ static irqreturn_t opal_interrupt(int irq, void *data)
 	__be64 events;
 
 	opal_handle_interrupt(virq_to_hw(irq), &events);
-	last_outstanding_events = be64_to_cpu(events);
+	WRITE_ONCE(last_outstanding_events, be64_to_cpu(events));
 	if (opal_have_pending_events())
 		opal_wake_poller();
 
-- 
2.37.2

[PATCH v2 10/11] powerpc: powernv: Annotate asynchronous access to opal tokens

[Rohan McLure]

The opal-async.c unit contains code for polling event sources, which
implies intentional data races. Ensure that the compiler will atomically
access such variables by means of {READ,WRITE}_ONCE calls, which in turn
inform KCSAN that polling behaviour is intended.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-async.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
index c094fdf5825c..282d2ac6fbb0 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -146,7 +146,7 @@ int opal_async_wait_response(uint64_t token, struct opal_msg *msg)
 	 * functional.
 	 */
 	opal_wake_poller();
-	wait_event(opal_async_wait, opal_async_tokens[token].state
+	wait_event(opal_async_wait, READ_ONCE(opal_async_tokens[token].state)
 			== ASYNC_TOKEN_COMPLETED);
 	memcpy(msg, &opal_async_tokens[token].response, sizeof(*msg));
 
@@ -185,7 +185,7 @@ int opal_async_wait_response_interruptible(uint64_t token, struct opal_msg *msg)
 	 * interruptible version before doing anything else with the
 	 * token.
 	 */
-	if (opal_async_tokens[token].state == ASYNC_TOKEN_ALLOCATED) {
+	if (READ_ONCE(opal_async_tokens[token].state) == ASYNC_TOKEN_ALLOCATED) {
 		spin_lock_irqsave(&opal_async_comp_lock, flags);
 		if (opal_async_tokens[token].state == ASYNC_TOKEN_ALLOCATED)
 			opal_async_tokens[token].state = ASYNC_TOKEN_DISPATCHED;
@@ -199,7 +199,7 @@ int opal_async_wait_response_interruptible(uint64_t token, struct opal_msg *msg)
 	 */
 	opal_wake_poller();
 	ret = wait_event_interruptible(opal_async_wait,
-			opal_async_tokens[token].state ==
+			READ_ONCE(opal_async_tokens[token].state) ==
 			ASYNC_TOKEN_COMPLETED);
 	if (!ret)
 		memcpy(msg, &opal_async_tokens[token].response, sizeof(*msg));
-- 
2.37.2

[PATCH v2 11/11] powerpc: Mark asynchronous accesses to irq_data

[Rohan McLure]

KCSAN revealed that while irq_data entries are written to either from
behind a mutex, or otherwise atomically, accesses to irq_data->hwirq can
occur asynchronously, without volatile annotation. Mark these accesses
with READ_ONCE to avoid unfortunate compiler reorderings and remove
KCSAN warnings.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
 arch/powerpc/kernel/irq.c                 |  2 +-
 arch/powerpc/platforms/powernv/pci-ioda.c | 12 ++++++------
 include/linux/irq.h                       |  2 +-
 kernel/irq/irqdomain.c                    |  4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 6f7d4edaa0bc..4ac192755510 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -353,7 +353,7 @@ void do_softirq_own_stack(void)
 irq_hw_number_t virq_to_hw(unsigned int virq)
 {
 	struct irq_data *irq_data = irq_get_irq_data(virq);
-	return WARN_ON(!irq_data) ? 0 : irq_data->hwirq;
+	return WARN_ON(!irq_data) ? 0 : READ_ONCE(irq_data->hwirq);
 }
 EXPORT_SYMBOL_GPL(virq_to_hw);
 
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index f851f4983423..141491e86bba 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1986,7 +1986,7 @@ int64_t pnv_opal_pci_msi_eoi(struct irq_data *d)
 	struct pci_controller *hose = irq_data_get_irq_chip_data(d->parent_data);
 	struct pnv_phb *phb = hose->private_data;
 
-	return opal_pci_msi_eoi(phb->opal_id, d->parent_data->hwirq);
+	return opal_pci_msi_eoi(phb->opal_id, READ_ONCE(d->parent_data->hwirq));
 }
 
 /*
@@ -2162,11 +2162,11 @@ static void pnv_msi_compose_msg(struct irq_data *d, struct msi_msg *msg)
 	struct pnv_phb *phb = hose->private_data;
 	int rc;
 
-	rc = __pnv_pci_ioda_msi_setup(phb, pdev, d->hwirq,
+	rc = __pnv_pci_ioda_msi_setup(phb, pdev, READ_ONCE(d->hwirq),
 				      entry->pci.msi_attrib.is_64, msg);
 	if (rc)
 		dev_err(&pdev->dev, "Failed to setup %s-bit MSI #%ld : %d\n",
-			entry->pci.msi_attrib.is_64 ? "64" : "32", d->hwirq, rc);
+			entry->pci.msi_attrib.is_64 ? "64" : "32", data_race(d->hwirq), rc);
 }
 
 /*
@@ -2184,7 +2184,7 @@ static void pnv_msi_eoi(struct irq_data *d)
 		 * since it is translated into a vector number in
 		 * OPAL, use that directly.
 		 */
-		WARN_ON_ONCE(opal_pci_msi_eoi(phb->opal_id, d->hwirq));
+		WARN_ON_ONCE(opal_pci_msi_eoi(phb->opal_id, READ_ONCE(d->hwirq)));
 	}
 
 	irq_chip_eoi_parent(d);
@@ -2263,9 +2263,9 @@ static void pnv_irq_domain_free(struct irq_domain *domain, unsigned int virq,
 	struct pnv_phb *phb = hose->private_data;
 
 	pr_debug("%s bridge %pOF %d/%lx #%d\n", __func__, hose->dn,
-		 virq, d->hwirq, nr_irqs);
+		 virq, data_race(d->hwirq), nr_irqs);
 
-	msi_bitmap_free_hwirqs(&phb->msi_bmp, d->hwirq, nr_irqs);
+	msi_bitmap_free_hwirqs(&phb->msi_bmp, READ_ONCE(d->hwirq), nr_irqs);
 	/* XIVE domain is cleared through ->msi_free() */
 }
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index b1b28affb32a..a6888bcb3c5b 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -452,7 +452,7 @@ static inline bool irqd_affinity_on_activate(struct irq_data *d)
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 {
-	return d->hwirq;
+	return READ_ONCE(d->hwirq);
 }
 
 /**
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index f34760a1e222..dd9054494f84 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -549,7 +549,7 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 		 "virq%i doesn't exist; cannot disassociate\n", irq))
 		return;
 
-	hwirq = irq_data->hwirq;
+	hwirq = READ_ONCE(irq_data->hwirq);
 
 	mutex_lock(&domain->root->mutex);
 
@@ -948,7 +948,7 @@ struct irq_desc *__irq_resolve_mapping(struct irq_domain *domain,
 	if (irq_domain_is_nomap(domain)) {
 		if (hwirq < domain->hwirq_max) {
 			data = irq_domain_get_irq_data(domain, hwirq);
-			if (data && data->hwirq == hwirq)
+			if (data && READ_ONCE(data->hwirq) == hwirq)
 				desc = irq_data_to_desc(data);
 			if (irq && desc)
 				*irq = hwirq;
-- 
2.37.2

Re: [PATCH v2 03/11] asm-generic/mmiowb: Mark accesses to fix KCSAN warnings

[Michael Ellerman]

Rohan McLure <rmclure@linux.ibm.com> writes:
> Prior to this patch, data races are detectable by KCSAN of the following
> forms:
>
> [1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
>     or otherwise outside of a critical section
> [2] Interrupted critical sections, where the interrupt will itself
>     acquire a lock
>
> In case [1], calling context does not need an mmiowb() call to be
> issued, otherwise it would do so itself. Such calls to
> mmiowb_set_pending() are either idempotent or no-ops.
>
> In case [2], irrespective of when the interrupt occurs, the interrupt
> will acquire and release its locks prior to its return, nesting_count
> will continue balanced. In the worst case, the interrupted critical
> section during a mmiowb_spin_unlock() call observes an mmiowb to be
> pending and afterward is interrupted, leading to an extraneous call to
> mmiowb(). This data race is clearly innocuous.
>
> Mark all potentially asynchronous memory accesses with READ_ONCE or
> WRITE_ONCE, including increments and decrements to nesting_count. This
> has the effect of removing KCSAN warnings at consumer's callsites.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Reported-by: Gautam Menghani <gautam@linux.ibm.com>
> Tested-by: Gautam Menghani <gautam@linux.ibm.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
> ---
>  include/asm-generic/mmiowb.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

This will need wider review, it's used by a lot of other architectures.

Probably best to pull this one out of the series and post it separately.
Add at least linux-kernel and linux-arch to Cc, and probably Will as
he's the original author.

cheers

Re: [PATCH v2 02/11] powerpc: qspinlock: Enforce qnode writes prior to publishing to queue

[Nicholas Piggin]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> Annotate the release barrier and memory clobber (in effect, producing a
> compiler barrier) in the publish_tail_cpu call. These barriers have the
> effect of ensuring that qnode attributes are all written to prior to
> publish the node to the waitqueue.
>
> Even while the initial write to the 'locked' attribute is guaranteed to
> terminate prior to the node being visible, KCSAN still complains that
> the write is reorderable by the compiler. Issue a kcsan_release() to
> inform KCSAN of the release barrier contained in publish_tail_cpu().
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> v2: Remove extraneous compiler barrier, but annotate release-barrier
> contained in call publish_tail_cpu(), and include kcsan_release().
> ---
>  arch/powerpc/lib/qspinlock.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
> index b76c1f6acce5..253620979d0c 100644
> --- a/arch/powerpc/lib/qspinlock.c
> +++ b/arch/powerpc/lib/qspinlock.c
> @@ -161,6 +161,8 @@ static __always_inline u32 publish_tail_cpu(struct qspinlock *lock, u32 tail)
>  {
>  	u32 prev, tmp;
>  
> +	kcsan_release();
> +
>  	asm volatile(
>  "\t"	PPC_RELEASE_BARRIER "						\n"
>  "1:	lwarx	%0,0,%2		# publish_tail_cpu			\n"
> @@ -570,6 +572,11 @@ static __always_inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, b
>  
>  	tail = encode_tail_cpu(node->cpu);
>  
> +	/*
> +	 * Assign all attributes of a node before it can be published.
> +	 * Issues an lwsync, serving as a release barrier, as well as a
> +	 * compiler barrier.
> +	 */
>  	old = publish_tail_cpu(lock, tail);

Possibly better to be with the publish function, hopefully the name
gives away it has a release barrier then store that makes it visible.
But that's nitpicking.

Thanks for the qspinlock fixes.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Re: [PATCH v2 03/11] asm-generic/mmiowb: Mark accesses to fix KCSAN warnings

[Nicholas Piggin]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> Prior to this patch, data races are detectable by KCSAN of the following
> forms:
>
> [1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
>     or otherwise outside of a critical section
> [2] Interrupted critical sections, where the interrupt will itself
>     acquire a lock
>
> In case [1], calling context does not need an mmiowb() call to be
> issued, otherwise it would do so itself. Such calls to
> mmiowb_set_pending() are either idempotent or no-ops.
>
> In case [2], irrespective of when the interrupt occurs, the interrupt
> will acquire and release its locks prior to its return, nesting_count
> will continue balanced. In the worst case, the interrupted critical
> section during a mmiowb_spin_unlock() call observes an mmiowb to be
> pending and afterward is interrupted, leading to an extraneous call to
> mmiowb(). This data race is clearly innocuous.
>
> Mark all potentially asynchronous memory accesses with READ_ONCE or
> WRITE_ONCE, including increments and decrements to nesting_count. This
> has the effect of removing KCSAN warnings at consumer's callsites.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Reported-by: Gautam Menghani <gautam@linux.ibm.com>
> Tested-by: Gautam Menghani <gautam@linux.ibm.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
> ---
>  include/asm-generic/mmiowb.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/asm-generic/mmiowb.h b/include/asm-generic/mmiowb.h
> index 5698fca3bf56..6dea28c8835b 100644
> --- a/include/asm-generic/mmiowb.h
> +++ b/include/asm-generic/mmiowb.h
> @@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void)
>  	struct mmiowb_state *ms = __mmiowb_state();
>  
>  	if (likely(ms->nesting_count))
> -		ms->mmiowb_pending = ms->nesting_count;
> +		WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count);
>  }
>  
>  static inline void mmiowb_spin_lock(void)
>  {
>  	struct mmiowb_state *ms = __mmiowb_state();
> -	ms->nesting_count++;
> +
> +	/* Increment need not be atomic. Nestedness is balanced over interrupts. */
> +	WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1);
>  }
>  
>  static inline void mmiowb_spin_unlock(void)
>  {
>  	struct mmiowb_state *ms = __mmiowb_state();
> +	u16 pending = READ_ONCE(ms->mmiowb_pending);
>  
> -	if (unlikely(ms->mmiowb_pending)) {
> -		ms->mmiowb_pending = 0;
> +	WRITE_ONCE(ms->mmiowb_pending, 0);
> +	if (unlikely(pending)) {
>  		mmiowb();
>  	}
>  
> -	ms->nesting_count--;
> +	/* Decrement need not be atomic. Nestedness is balanced over interrupts. */
> +	WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1);

Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
data_race() maybe but I don't know if it's even classed as a data
race. How does KCSAN handle/annotate preempt_count, for example?

Thanks,
Nick

Re: [PATCH v2 05/11] powerpc: Mark accesses to power_save callback in arch_cpu_idle

[Nicholas Piggin]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> The power_save callback can be overwritten by another core at boot time.
> Specifically, null values will be replaced exactly once with the callback
> suitable for the particular platform (PowerNV / pseries lpars), making
> this value a good candidate for __ro_after_init.
>
> Even with this the case, KCSAN sees unmarked reads to the callback
> variable, and notices that unfortunate compiler reorderings could lead
> to distinct function pointers being read. In reality this is impossible,
> so don't instrument at this read.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> v2: Mark instances at init where the callback is written to, and
> data_race() read as there is no capacity for the value to change
> underneath.
> ---
>  arch/powerpc/kernel/idle.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> index b1c0418b25c8..43d96c0e3b96 100644
> --- a/arch/powerpc/kernel/idle.c
> +++ b/arch/powerpc/kernel/idle.c
> @@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable);
>  
>  static int __init powersave_off(char *arg)
>  {
> -	ppc_md.power_save = NULL;
> +	WRITE_ONCE(ppc_md.power_save, NULL);
>  	cpuidle_disable = IDLE_POWERSAVE_OFF;
>  	return 1;
>  }

Shouldn't need the WRITE_ONCE if you don't need a READ_ONCE. Does
data_race work here too? What about the other writers? Does
KCSAN know it's single threaded in early boot so skips marking,
but perhaps this comes later? Would be good to have a little
comment if so.

Thanks,
Nick

> @@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off);
>  
>  void arch_cpu_idle(void)
>  {
> +	/* power_save callback assigned only at init so no data race */
> +	void (*power_save)(void) = data_race(ppc_md.power_save);
> +
>  	ppc64_runlatch_off();
>  
> -	if (ppc_md.power_save) {
> -		ppc_md.power_save();
> +	if (power_save) {
> +		power_save();
>  		/*
>  		 * Some power_save functions return with
>  		 * interrupts enabled, some don't.
> -- 
> 2.37.2

Re: [PATCH v2 06/11] powerpc: powernv: Fix KCSAN datarace warnings on idle_state contention

[Nicholas Piggin]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> The idle_state entry in the PACA on PowerNV features a bit which is
> atomically tested and set through ldarx/stdcx. to be used as a spinlock.
> This lock then guards access to other bit fields of idle_state. KCSAN
> cannot differentiate between any of these bitfield accesses as they all
> are implemented by 8-byte store/load instructions, thus cores contending
> on the bit-lock appear to data race with modifications to idle_state.
>
> Separate the bit-lock entry from the data guarded by the lock to avoid
> the possibility of data races being detected by KCSAN.
>
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Re: [PATCH v2 07/11] powerpc: Annotate accesses to ipi message flags

[Nicholas Piggin]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> IPI message flags are observed and consequently consumed in the
> smp_ipi_demux_relaxed function, which handles these message sources
> until it observes none more arriving. Mark the checked loop guard with
> READ_ONCE, to signal to KCSAN that the read is known to be volatile, and
> that non-determinism is expected. Mark write for message source in
> smp_muxed_ipi_set_message().
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Re: [PATCH v2 08/11] powerpc: Mark writes registering ipi to host cpu through kvm and polling

[Nicholas Piggin]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> Mark writes to hypervisor ipi state so that KCSAN recognises these
> asynchronous issue of kvmppc_{set,clear}_host_ipi to be intended, with
> atomic writes. Mark asynchronous polls to this variable in
> kvm_ppc_read_one_intr().
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>

What's the go with accesses in asm? Does it just assume you know
what you're doing?

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
> v2: Add read-side annotations to both polling locations in
> kvm_ppc_read_one_intr().
> ---
>  arch/powerpc/include/asm/kvm_ppc.h   | 4 ++--
>  arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index bc57d058ad5b..d701df006c08 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -548,12 +548,12 @@ static inline void kvmppc_set_host_ipi(int cpu)
>  	 * pairs with the barrier in kvmppc_clear_host_ipi()
>  	 */
>  	smp_mb();
> -	paca_ptrs[cpu]->kvm_hstate.host_ipi = 1;
> +	WRITE_ONCE(paca_ptrs[cpu]->kvm_hstate.host_ipi, 1);
>  }
>  
>  static inline void kvmppc_clear_host_ipi(int cpu)
>  {
> -	paca_ptrs[cpu]->kvm_hstate.host_ipi = 0;
> +	WRITE_ONCE(paca_ptrs[cpu]->kvm_hstate.host_ipi, 0);
>  	/*
>  	 * order clearing of host_ipi flag vs. processing of IPI messages
>  	 *
> diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
> index da85f046377a..0f5b021fa559 100644
> --- a/arch/powerpc/kvm/book3s_hv_builtin.c
> +++ b/arch/powerpc/kvm/book3s_hv_builtin.c
> @@ -406,7 +406,7 @@ static long kvmppc_read_one_intr(bool *again)
>  		return 1;
>  
>  	/* see if a host IPI is pending */
> -	host_ipi = local_paca->kvm_hstate.host_ipi;
> +	host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
>  	if (host_ipi)
>  		return 1;
>  
> @@ -466,7 +466,7 @@ static long kvmppc_read_one_intr(bool *again)
>  		 * meantime. If it's clear, we bounce the interrupt to the
>  		 * guest
>  		 */
> -		host_ipi = local_paca->kvm_hstate.host_ipi;
> +		host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
>  		if (unlikely(host_ipi != 0)) {
>  			/* We raced with the host,
>  			 * we need to resend that IPI, bummer
> -- 
> 2.37.2

Re: [PATCH v2 08/11] powerpc: Mark writes registering ipi to host cpu through kvm and polling

[Rohan McLure]

> On 15 May 2023, at 3:53 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
>> Mark writes to hypervisor ipi state so that KCSAN recognises these
>> asynchronous issue of kvmppc_{set,clear}_host_ipi to be intended, with
>> atomic writes. Mark asynchronous polls to this variable in
>> kvm_ppc_read_one_intr().
>> 
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> 
> What's the go with accesses in asm? Does it just assume you know
> what you're doing?

Exactly, KCSAN only emits instrumentation calls to around load/store 
instructions that the compiler itself generated. So by default, asm
accesses are not instrumented.

Thanks

> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 
>> ---
>> v2: Add read-side annotations to both polling locations in
>> kvm_ppc_read_one_intr().
>> ---
>> arch/powerpc/include/asm/kvm_ppc.h   | 4 ++--
>> arch/powerpc/kvm/book3s_hv_builtin.c | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index bc57d058ad5b..d701df006c08 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -548,12 +548,12 @@ static inline void kvmppc_set_host_ipi(int cpu)
>> * pairs with the barrier in kvmppc_clear_host_ipi()
>> */
>> smp_mb();
>> - paca_ptrs[cpu]->kvm_hstate.host_ipi = 1;
>> + WRITE_ONCE(paca_ptrs[cpu]->kvm_hstate.host_ipi, 1);
>> }
>> 
>> static inline void kvmppc_clear_host_ipi(int cpu)
>> {
>> - paca_ptrs[cpu]->kvm_hstate.host_ipi = 0;
>> + WRITE_ONCE(paca_ptrs[cpu]->kvm_hstate.host_ipi, 0);
>> /*
>> * order clearing of host_ipi flag vs. processing of IPI messages
>> *
>> diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
>> index da85f046377a..0f5b021fa559 100644
>> --- a/arch/powerpc/kvm/book3s_hv_builtin.c
>> +++ b/arch/powerpc/kvm/book3s_hv_builtin.c
>> @@ -406,7 +406,7 @@ static long kvmppc_read_one_intr(bool *again)
>> return 1;
>> 
>> /* see if a host IPI is pending */
>> - host_ipi = local_paca->kvm_hstate.host_ipi;
>> + host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
>> if (host_ipi)
>> return 1;
>> 
>> @@ -466,7 +466,7 @@ static long kvmppc_read_one_intr(bool *again)
>> * meantime. If it's clear, we bounce the interrupt to the
>> * guest
>> */
>> - host_ipi = local_paca->kvm_hstate.host_ipi;
>> + host_ipi = READ_ONCE(local_paca->kvm_hstate.host_ipi);
>> if (unlikely(host_ipi != 0)) {
>> /* We raced with the host,
>> * we need to resend that IPI, bummer
>> -- 
>> 2.37.2
> 

Re: [PATCH v2 05/11] powerpc: Mark accesses to power_save callback in arch_cpu_idle

[Rohan McLure]

> On 15 May 2023, at 3:50 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
>> The power_save callback can be overwritten by another core at boot time.
>> Specifically, null values will be replaced exactly once with the callback
>> suitable for the particular platform (PowerNV / pseries lpars), making
>> this value a good candidate for __ro_after_init.
>> 
>> Even with this the case, KCSAN sees unmarked reads to the callback
>> variable, and notices that unfortunate compiler reorderings could lead
>> to distinct function pointers being read. In reality this is impossible,
>> so don't instrument at this read.
>> 
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> v2: Mark instances at init where the callback is written to, and
>> data_race() read as there is no capacity for the value to change
>> underneath.
>> ---
>> arch/powerpc/kernel/idle.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
>> index b1c0418b25c8..43d96c0e3b96 100644
>> --- a/arch/powerpc/kernel/idle.c
>> +++ b/arch/powerpc/kernel/idle.c
>> @@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable);
>> 
>> static int __init powersave_off(char *arg)
>> {
>> - ppc_md.power_save = NULL;
>> + WRITE_ONCE(ppc_md.power_save, NULL);
>> cpuidle_disable = IDLE_POWERSAVE_OFF;
>> return 1;
>> }
> 
> Shouldn't need the WRITE_ONCE if you don't need a READ_ONCE. Does
> data_race work here too? What about the other writers? Does
> KCSAN know it's single threaded in early boot so skips marking,
> but perhaps this comes later? Would be good to have a little
> comment if so.

Apologies, yep I was meant to remove this WRITE_ONCE now that
the read-side has data_race. Sorry for the confusion.

> 
> Thanks,
> Nick
> 
>> @@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off);
>> 
>> void arch_cpu_idle(void)
>> {
>> + /* power_save callback assigned only at init so no data race */
>> + void (*power_save)(void) = data_race(ppc_md.power_save);
>> +
>> ppc64_runlatch_off();
>> 
>> - if (ppc_md.power_save) {
>> - ppc_md.power_save();
>> + if (power_save) {
>> + power_save();
>> /*
>> * Some power_save functions return with
>> * interrupts enabled, some don't.
>> -- 
>> 2.37.2
> 

Re: [PATCH v2 03/11] asm-generic/mmiowb: Mark accesses to fix KCSAN warnings

[Rohan McLure]

[-- Attachment #1: Type: text/plain, Size: 3109 bytes --]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> Prior to this patch, data races are detectable by KCSAN of the following
> forms:
> 
> [1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
>    or otherwise outside of a critical section
> [2] Interrupted critical sections, where the interrupt will itself
>    acquire a lock
> 
> In case [1], calling context does not need an mmiowb() call to be
> issued, otherwise it would do so itself. Such calls to
> mmiowb_set_pending() are either idempotent or no-ops.
> 
> In case [2], irrespective of when the interrupt occurs, the interrupt
> will acquire and release its locks prior to its return, nesting_count
> will continue balanced. In the worst case, the interrupted critical
> section during a mmiowb_spin_unlock() call observes an mmiowb to be
> pending and afterward is interrupted, leading to an extraneous call to
> mmiowb(). This data race is clearly innocuous.
> 
> Mark all potentially asynchronous memory accesses with READ_ONCE or
> WRITE_ONCE, including increments and decrements to nesting_count. This
> has the effect of removing KCSAN warnings at consumer's callsites.
> 
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Reported-by: Gautam Menghani <gautam@linux.ibm.com>
> Tested-by: Gautam Menghani <gautam@linux.ibm.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
> ---
> include/asm-generic/mmiowb.h | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/asm-generic/mmiowb.h b/include/asm-generic/mmiowb.h
> index 5698fca3bf56..6dea28c8835b 100644
> --- a/include/asm-generic/mmiowb.h
> +++ b/include/asm-generic/mmiowb.h
> @@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void)
> 	struct mmiowb_state *ms = __mmiowb_state();
> 
> 	if (likely(ms->nesting_count))
> -		ms->mmiowb_pending = ms->nesting_count;
> +		WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count);
> }
> 
> static inline void mmiowb_spin_lock(void)
> {
> 	struct mmiowb_state *ms = __mmiowb_state();
> -	ms->nesting_count++;
> +
> +	/* Increment need not be atomic. Nestedness is balanced over interrupts. */
> +	WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1);
> }
> 
> static inline void mmiowb_spin_unlock(void)
> {
> 	struct mmiowb_state *ms = __mmiowb_state();
> +	u16 pending = READ_ONCE(ms->mmiowb_pending);
> 
> -	if (unlikely(ms->mmiowb_pending)) {
> -		ms->mmiowb_pending = 0;
> +	WRITE_ONCE(ms->mmiowb_pending, 0);
> +	if (unlikely(pending)) {
> 		mmiowb();
> 	}
> 
> -	ms->nesting_count--;
> +	/* Decrement need not be atomic. Nestedness is balanced over interrupts. */
> +	WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1);

Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
data_race() maybe but I don't know if it's even classed as a data
race. How does KCSAN handle/annotate preempt_count, for example?

Thanks,
Nick

[-- Attachment #2: Type: text/html, Size: 10384 bytes --]

Re: [PATCH v2 03/11] asm-generic/mmiowb: Mark accesses to fix KCSAN warnings

[Rohan McLure]

On 23 May 2023, at 10:28 am, Rohan McLure <rmclure@linux.ibm.com> wrote:
> 
> On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
>> Prior to this patch, data races are detectable by KCSAN of the following
>> forms:
>> 
>> [1] Asynchronous calls to mmiowb_set_pending() from an interrupt context
>>    or otherwise outside of a critical section
>> [2] Interrupted critical sections, where the interrupt will itself
>>    acquire a lock
>> 
>> In case [1], calling context does not need an mmiowb() call to be
>> issued, otherwise it would do so itself. Such calls to
>> mmiowb_set_pending() are either idempotent or no-ops.
>> 
>> In case [2], irrespective of when the interrupt occurs, the interrupt
>> will acquire and release its locks prior to its return, nesting_count
>> will continue balanced. In the worst case, the interrupted critical
>> section during a mmiowb_spin_unlock() call observes an mmiowb to be
>> pending and afterward is interrupted, leading to an extraneous call to
>> mmiowb(). This data race is clearly innocuous.
>> 
>> Mark all potentially asynchronous memory accesses with READ_ONCE or
>> WRITE_ONCE, including increments and decrements to nesting_count. This
>> has the effect of removing KCSAN warnings at consumer's callsites.
>> 
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
>> Reported-by: Gautam Menghani <gautam@linux.ibm.com>
>> Tested-by: Gautam Menghani <gautam@linux.ibm.com>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> v2: Remove extraneous READ_ONCE in mmiowb_set_pending for nesting_count
>> ---
>> include/asm-generic/mmiowb.h | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/include/asm-generic/mmiowb.h b/include/asm-generic/mmiowb.h
>> index 5698fca3bf56..6dea28c8835b 100644
>> --- a/include/asm-generic/mmiowb.h
>> +++ b/include/asm-generic/mmiowb.h
>> @@ -37,25 +37,29 @@ static inline void mmiowb_set_pending(void)
>> struct mmiowb_state *ms = __mmiowb_state();
>> 
>> if (likely(ms->nesting_count))
>> - ms->mmiowb_pending = ms->nesting_count;
>> + WRITE_ONCE(ms->mmiowb_pending, ms->nesting_count);
>> }
>> 
>> static inline void mmiowb_spin_lock(void)
>> {
>> struct mmiowb_state *ms = __mmiowb_state();
>> - ms->nesting_count++;
>> +
>> + /* Increment need not be atomic. Nestedness is balanced over interrupts. */
>> + WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) + 1);
>> }
>> 
>> static inline void mmiowb_spin_unlock(void)
>> {
>> struct mmiowb_state *ms = __mmiowb_state();
>> + u16 pending = READ_ONCE(ms->mmiowb_pending);
>> 
>> - if (unlikely(ms->mmiowb_pending)) {
>> - ms->mmiowb_pending = 0;
>> + WRITE_ONCE(ms->mmiowb_pending, 0);
>> + if (unlikely(pending)) {
>> mmiowb();
>> }
>> 
>> - ms->nesting_count--;
>> + /* Decrement need not be atomic. Nestedness is balanced over interrupts. */
>> + WRITE_ONCE(ms->nesting_count, READ_ONCE(ms->nesting_count) - 1);
> 
> Still think the nesting_counts don't need WRITE_ONCE/READ_ONCE.
> data_race() maybe but I don't know if it's even classed as a data
> race. How does KCSAN handle/annotate preempt_count, for example?

Wow sorry my mail client has some unhelpful keybindings - I don’t know why it
thought I’d want to resend your last item!

Yeah I agree, we don’t need the compiler guarantees of WRITE_ONCE/READ_ONCE, and
yet it’s also not a real data-race. I think I’ll apply data_race() and comment as
I’m still seeing KCSAN warnings here.

Just from inspection, it appears as if __preempt_count_{add,sub} are unmarked and
so likely to generate KCSAN warnings also, but also asm-generic/preempt.h I think
hasn’t been updated to address any such warnings.

> 
> Thanks,
> Nick

Re: (subset) [PATCH v2 00/11] powerpc: KCSAN fix warnings and mark accesses

[Michael Ellerman]

On Wed, 10 May 2023 13:31:06 +1000, Rohan McLure wrote:
> v1 of this patch series available here:
> Link: https://lore.kernel.org/linuxppc-dev/20230508020120.218494-1-rmclure@linux.ibm.com/
> 
> The KCSAN sanitiser notifies programmers of instances where unmarked
> accesses to shared state has lead to a data race, or when the compiler
> has liberty to reorder an unmarked access and so generate a data race.
> This patch series deals with benign data races, which nonetheless need
> annotation in order to ensure the correctness of the emitted code.
> 
> [...]

Patches 1, 2, 4, 6-9 applied to powerpc/next.

[01/11] powerpc: qspinlock: Mark accesses to qnode lock checks
        https://git.kernel.org/powerpc/c/03d44ee80eac980a869ed3d5637ed85de6fb957f
[02/11] powerpc: qspinlock: Enforce qnode writes prior to publishing to queue
        https://git.kernel.org/powerpc/c/6f3136326ee47ae2dd5dac9306c9b08ccbc7e81e
[04/11] powerpc: Mark [h]ssr_valid accesses in check_return_regs_valid
        https://git.kernel.org/powerpc/c/be286b8637d417a7d7eb25dc3a509c10d0afef66
[06/11] powerpc: powernv: Fix KCSAN datarace warnings on idle_state contention
        https://git.kernel.org/powerpc/c/b0c5b4f1ee3687c57dab65ac0729a4d61967f032
[07/11] powerpc: Annotate accesses to ipi message flags
        https://git.kernel.org/powerpc/c/8608f14b49a0a3f8644a326d32dc1bf7ed78836a
[08/11] powerpc: Mark writes registering ipi to host cpu through kvm and polling
        https://git.kernel.org/powerpc/c/86dacd967b80114c0c6cf0648ed1dcaea8853937
[09/11] powerpc: powernv: Annotate data races in opal events
        https://git.kernel.org/powerpc/c/331e2cad6d168ac5ccb25ae34bdc305b8b731bc0

cheers