* Re: [PATCH v3 1/3] vfio/pci: Set up bar resources and maps in vfio_pci_core_enable()
From: Matt Evans @ 2026-05-05 16:40 UTC (permalink / raw)
To: Alex Williamson
Cc: Kevin Tian, Jason Gunthorpe, Ankit Agrawal, Alistair Popple,
Leon Romanovsky, Kees Cook, Shameer Kolothum, Yishai Hadas,
Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
In-Reply-To: <20260430141341.163ed827@shazbot.org>
Hi Alex,
On 30/04/2026 21:13, Alex Williamson wrote:
>
> On Thu, 30 Apr 2026 03:03:20 -0700
> Matt Evans <mattev@meta.com> wrote:
>
>> Previously BAR resource requests and the corresponding pci_iomap()
>> were performed on-demand and without synchronisation, which was racy.
>> Rather than add synchronisation, it's simplest to address this by
>> doing both activities from vfio_pci_core_enable().
>>
>> The resource allocation and/or pci_iomap() can still fail; their
>> status is tracked and existing calls to vfio_pci_core_setup_barmap()
>> will fail in a similar way to before. This keeps the point of failure
>> as observed by userspace the same, i.e. failures to request/map unused
>> BARs are benign.
>>
>> Fixes: 7f5764e179c6 ("vfio: use vfio_pci_core_setup_barmap to map bar in mmap")
>> Fixes: 0d77ed3589ac0 ("vfio/pci: Pull BAR mapping setup from read-write path")
>
> Neither of these introduced races, they only moved what they were
> already doing into a function or made use of that shared function for
> what they were already doing. I'm inclined to believe the raciness
> existed from the introduction, 89e1f7d4c66d.
>
>> Signed-off-by: Matt Evans <mattev@meta.com>
>> ---
>> drivers/vfio/pci/vfio_pci_core.c | 33 ++++++++++++++++++++++++++++++++
>> drivers/vfio/pci/vfio_pci_rdwr.c | 29 ++++++++++++----------------
>> 2 files changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index 3f8d093aacf8..eab4f2626b39 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -482,6 +482,38 @@ static int vfio_pci_core_runtime_resume(struct device *dev)
>> }
>> #endif /* CONFIG_PM */
>>
>> +static void vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
>> +{
>> + struct pci_dev *pdev = vdev->pdev;
>> + int i;
>> +
>> + /*
>> + * Eager-request BAR resources, and iomap. Soft failures are
>> + * allowed, and consumers must check the barmap before use in
>> + * order to give compatible user-visible behaviour with the
>> + * previous on-demand allocation method.
>> + */
>> + for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>> + int bar = i + PCI_STD_RESOURCES;
>> + void __iomem *io = ERR_PTR(-ENODEV);
>
> It would collapse the nesting depth to just do:
>
> vdev->barmap[bar] = ERR_PTR(-ENODEV);
>
> if (!pci_resource_len(pdev, i))
> continue;
>
> if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
> pci_dbg(vdev->pdev, "Failed to reserve region %d\n", bar);
> vdev->barmap[bar] = ERR_PTR(-EBUSY);
> continue;
> }
>
> vdev->barmap[bar] = pci_iomap(pdev, bar, 0);
> if (!vdev->barmap[bar]) {
> pci_dbg(vdev->pdev, "Failed to iomap region %d\n", bar);
> vdev->barmap[bar] = ERR_PTR(-ENOMEM);
> }
>
> It's debatable what level to use for the errors, but we were previously
> silent on this, so going all the way to pci_warn() seems unnecessary.
Hm, okay, returned it to a nesting-less format and replaced pci_warn()s
with pci_dbg().
>> +
>> + if (pci_resource_len(pdev, i) > 0) {
>> + if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
>> + pci_warn(vdev->pdev, "Failed to reserve region %d\n", bar);
>> + io = ERR_PTR(-EBUSY);
>> + } else {
>> + io = pci_iomap(pdev, bar, 0);
>> + if (!io) {
>> + pci_warn(vdev->pdev, "Failed to iomap region %d\n",
>> + bar);
>> + io = ERR_PTR(-ENOMEM);
>> + }
>> + }
>> + }
>> + vdev->barmap[bar] = io;
>> + }
>> +}
>> +
>> /*
>> * The pci-driver core runtime PM routines always save the device state
>> * before going into suspended state. If the device is going into low power
>> @@ -568,6 +600,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>> if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>> vdev->has_vga = true;
>>
>> + vfio_pci_core_map_bars(vdev);
>>
>> return 0;
>
> You're missing the barmap test in vfio_pci_core_disable() now, it's
> still testing for NULL, which is (almost?) never true. It needs to
> convert to IS_ERR_OR_NULL().
Arrrrgh, yes it does, thank you. (For the second time, the first being
the !IS_ERR() typo you caught in patch #3 :( Thanks there also; it
slipped by my usual testing routine.)
>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>> index 4251ee03e146..f66ad3d96481 100644
>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>> @@ -200,25 +200,20 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
>>
>> int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
>> {
>> - struct pci_dev *pdev = vdev->pdev;
>> - int ret;
>> - void __iomem *io;
>> -
>> - if (vdev->barmap[bar])
>> - return 0;
>> -
>> - ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
>> - if (ret)
>> - return ret;
>> -
>> - io = pci_iomap(pdev, bar, 0);
>> - if (!io) {
>> - pci_release_selected_regions(pdev, 1 << bar);
>> - return -ENOMEM;
>> - }
>> + /*
>> + * The barmap is set up in vfio_pci_core_enable(). Callers
>> + * use this function to check that the BAR resources are
>> + * requested or that the pci_iomap() was done.
>> + */
>
> Looks like a function level comment to be placed above the function
> definition. TBH, the comment in the previous function could also be
> pulled up as a function level comment.
>
>> + if (bar < 0 || bar >= PCI_STD_NUM_BARS)
>
> Maybe `if ((unsigned)bar >= PCI_STD_NUM_BARS)` but really author
> preference here.
>
>> + return -EINVAL;
>>
>> - vdev->barmap[bar] = io;
>> + /* Did vfio_pci_core_map_bars() set it up yet? */
>> + if (!vdev->barmap[bar])
>> + return -ENODEV;
>
> What hits this? Should it be a WARN_ON_ONCE? It would need to be a use
> case that accesses barmap outside of the window between enable and
> disable, where I think we're defining the contract that it's only valid
> between those events. Both this and the range check could move to the
> iomap implemenation to keep the Fixes: patch reasonably small since
> afaik they're not triggered. The BAR range test could be WARN_ON_ONCE
> as well, only driver bugs should hit it. Thanks,
I've reduced the fix patch #1 to just an IS_ERR test (without the null
or range checks as you suggest). And indeed WARN_ON_ONCE() is a good
idea as only tremendous mishaps would lead to these conditions
triggering (worth testing though).
Also ack on your suggestion on patch #2 to make the call to
nvgrace_gpu_wait_device_ready() more minimalist, and to order the 2x
fixes up front. Posting v4 shortly, cheers!
Thanks,
Matt
^ permalink raw reply
* [PATCH v6 0/7] locking: contended_release tracepoint instrumentation
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin
The existing contention_begin/contention_end tracepoints fire on the
waiter side. The lock holder's identity and stack can be captured at
contention_begin time (e.g. perf lock contention --lock-owner), but
this reflects the holder's state when a waiter arrives, not when the
lock is actually released.
This series adds a contended_release tracepoint that fires on the
holder side when a lock with waiters is released. This provides:
- Hold time estimation: when the holder's own acquisition was
contended, its contention_end (acquisition) and contended_release
can be correlated to measure how long the lock was held under
contention.
- The holder's stack at release time, which may differ from what perf lock
contention --lock-owner captures if the holder does significant work between
the waiter's arrival and the unlock.
Note: for reader/writer locks, the tracepoint fires for every reader
releasing while a writer is waiting, not only for the last reader.
v5 -> v6:
- Use trace_call__contended_release() instead of trace_contended_release(),
where appropriate to avoid a redundant static branch check when the caller
already guards with trace_contended_release_enabled().
- Added acked-by from Paul.
- Rebase on top of the fresh locking/core.
v4 -> v5:
- Split the combined spinning locks patch into separate qspinlock and
qrwlock patches (Paul E. McKenney).
- Factor out __queued_read_unlock()/__queued_write_unlock() as a
separate preparatory commit, mirroring the queued_spin_release()
split (Paul E. McKenney).
- Updated binary size numbers for qspinlock-only change.
- Added Acked-by and Reviewed-by tags where appropriate.
v3 -> v4:
- Fix spurious events in __percpu_up_read(): guard with
rcuwait_active(&sem->writer) to avoid tracing during the RCU grace
period after a writer releases (Sashiko).
- Fix possible use-after-free in semaphore up(): move
trace_contended_release() inside the sem->lock critical section
(Sashiko).
- Fix build failure with CONFIG_PARAVIRT_SPINLOCKS=y: introduce
queued_spin_release() as the arch-overridable unlock primitive,
so queued_spin_unlock() can be a generic tracing wrapper. Convert
x86 (paravirt) and MIPS overrides (Sashiko).
- Add EXPORT_TRACEPOINT_SYMBOL_GPL(contended_release) for module
support (Sashiko).
- Split spinning locks patch: factor out queued_spin_release() as a
separate preparatory commit (Sashiko).
- Make read unlock tracepoint behavior consistent across all
reader/writer lock types: fire for every reader releasing while
a writer is waiting (rwsem, rwbase_rt were previously last-reader
only).
v2 -> v3:
- Added new patch: extend contended_release tracepoint to queued spinlocks
and queued rwlocks (marked as RFC, requesting feedback). This is prompted by
Matthew Wilcox's suggestion to try to come up with generic instrumentation,
instead of instrumenting each "special" lock manually. See [1] for the
discussion.
- Reworked tracepoint placement to fire before the lock is released and
before the waiter is woken where possible, for consistency with
spinning locks where there is no explicit wake (inspired by Usama Arif's
suggestion).
- Remove unnecessary linux/sched.h include from trace/events/lock.h.
RFC -> v2:
- Add trace_contended_release_enabled() guard before waiter checks that
exist only for the tracepoint (Steven Rostedt).
- Rename __percpu_up_read_slowpath() to __percpu_up_read() (Peter
Zijlstra).
- Add extern for __percpu_up_read() (Peter Zijlstra).
- Squashed tracepoint introduction and usage commits (Masami Hiramatsu).
v5: https://lore.kernel.org/all/cover.1776350944.git.d@ilvokhin.com/
v4: https://lore.kernel.org/all/cover.1774536681.git.d@ilvokhin.com/
v3: https://lore.kernel.org/all/cover.1773858853.git.d@ilvokhin.com/
v2: https://lore.kernel.org/all/cover.1773164180.git.d@ilvokhin.com/
RFC: https://lore.kernel.org/all/cover.1772642407.git.d@ilvokhin.com/
[1]: https://lore.kernel.org/all/aa7G1nD7Rd9F4eBH@casper.infradead.org/
Dmitry Ilvokhin (7):
tracing/lock: Remove unnecessary linux/sched.h include
locking/percpu-rwsem: Extract __percpu_up_read()
locking: Add contended_release tracepoint to sleepable locks
locking: Factor out queued_spin_release()
locking: Add contended_release tracepoint to qspinlock
locking: Factor out __queued_read_unlock()/__queued_write_unlock()
locking: Add contended_release tracepoint to qrwlock
arch/mips/include/asm/spinlock.h | 6 ++--
arch/x86/include/asm/paravirt-spinlock.h | 6 ++--
include/asm-generic/qrwlock.h | 38 ++++++++++++++++++++++--
include/asm-generic/qspinlock.h | 33 ++++++++++++++++++--
include/linux/percpu-rwsem.h | 15 ++--------
include/trace/events/lock.h | 18 ++++++++++-
kernel/locking/mutex.c | 4 +++
kernel/locking/percpu-rwsem.c | 29 ++++++++++++++++++
kernel/locking/qrwlock.c | 16 ++++++++++
kernel/locking/qspinlock.c | 8 +++++
kernel/locking/rtmutex.c | 1 +
kernel/locking/rwbase_rt.c | 6 ++++
kernel/locking/rwsem.c | 10 +++++--
kernel/locking/semaphore.c | 4 +++
14 files changed, 167 insertions(+), 27 deletions(-)
--
2.52.0
^ permalink raw reply
* [PATCH v6 1/7] tracing/lock: Remove unnecessary linux/sched.h include
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin, Usama Arif
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
None of the trace events in lock.h reference anything from
linux/sched.h. Remove the unnecessary include.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Acked-by: Usama Arif <usama.arif@linux.dev>
---
include/trace/events/lock.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/trace/events/lock.h b/include/trace/events/lock.h
index 8e89baa3775f..da978f2afb45 100644
--- a/include/trace/events/lock.h
+++ b/include/trace/events/lock.h
@@ -5,7 +5,6 @@
#if !defined(_TRACE_LOCK_H) || defined(TRACE_HEADER_MULTI_READ)
#define _TRACE_LOCK_H
-#include <linux/sched.h>
#include <linux/tracepoint.h>
/* flags for lock:contention_begin */
--
2.52.0
^ permalink raw reply related
* [PATCH v6 2/7] locking/percpu-rwsem: Extract __percpu_up_read()
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin, Usama Arif
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
Move the percpu_up_read() slowpath out of the inline function into a new
__percpu_up_read() to avoid binary size increase from adding a
tracepoint to an inlined function.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Acked-by: Usama Arif <usama.arif@linux.dev>
---
include/linux/percpu-rwsem.h | 15 +++------------
kernel/locking/percpu-rwsem.c | 18 ++++++++++++++++++
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index c8cb010d655e..39d5bf8e6562 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -107,6 +107,8 @@ static inline bool percpu_down_read_trylock(struct percpu_rw_semaphore *sem)
return ret;
}
+extern void __percpu_up_read(struct percpu_rw_semaphore *sem);
+
static inline void percpu_up_read(struct percpu_rw_semaphore *sem)
{
rwsem_release(&sem->dep_map, _RET_IP_);
@@ -118,18 +120,7 @@ static inline void percpu_up_read(struct percpu_rw_semaphore *sem)
if (likely(rcu_sync_is_idle(&sem->rss))) {
this_cpu_dec(*sem->read_count);
} else {
- /*
- * slowpath; reader will only ever wake a single blocked
- * writer.
- */
- smp_mb(); /* B matches C */
- /*
- * In other words, if they see our decrement (presumably to
- * aggregate zero, as that is the only time it matters) they
- * will also see our critical section.
- */
- this_cpu_dec(*sem->read_count);
- rcuwait_wake_up(&sem->writer);
+ __percpu_up_read(sem);
}
preempt_enable();
}
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index ef234469baac..f3ee7a0d6047 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -288,3 +288,21 @@ void percpu_up_write(struct percpu_rw_semaphore *sem)
rcu_sync_exit(&sem->rss);
}
EXPORT_SYMBOL_GPL(percpu_up_write);
+
+void __percpu_up_read(struct percpu_rw_semaphore *sem)
+{
+ lockdep_assert_preemption_disabled();
+ /*
+ * slowpath; reader will only ever wake a single blocked
+ * writer.
+ */
+ smp_mb(); /* B matches C */
+ /*
+ * In other words, if they see our decrement (presumably to
+ * aggregate zero, as that is the only time it matters) they
+ * will also see our critical section.
+ */
+ this_cpu_dec(*sem->read_count);
+ rcuwait_wake_up(&sem->writer);
+}
+EXPORT_SYMBOL_GPL(__percpu_up_read);
--
2.52.0
^ permalink raw reply related
* [PATCH v6 3/7] locking: Add contended_release tracepoint to sleepable locks
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin,
Paul E. McKenney
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
Add the contended_release trace event. This tracepoint fires on the
holder side when a contended lock is released, complementing the
existing contention_begin/contention_end tracepoints which fire on the
waiter side.
This enables correlating lock hold time under contention with waiter
events by lock address.
Add trace_contended_release()/trace_call__contended_release() calls to
the slowpath unlock paths of sleepable locks: mutex, rtmutex, semaphore,
rwsem, percpu-rwsem, and RT-specific rwbase locks.
Where possible, trace_contended_release() fires before the lock is
released and before the waiter is woken. For some lock types, the
tracepoint fires after the release but before the wake. Making the
placement consistent across all lock types is not worth the added
complexity.
For reader/writer locks, the tracepoint fires for every reader releasing
while a writer is waiting, not only for the last reader.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
include/trace/events/lock.h | 17 +++++++++++++++++
kernel/locking/mutex.c | 4 ++++
kernel/locking/percpu-rwsem.c | 11 +++++++++++
kernel/locking/rtmutex.c | 1 +
kernel/locking/rwbase_rt.c | 6 ++++++
kernel/locking/rwsem.c | 10 ++++++++--
kernel/locking/semaphore.c | 4 ++++
7 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/include/trace/events/lock.h b/include/trace/events/lock.h
index da978f2afb45..1ded869cd619 100644
--- a/include/trace/events/lock.h
+++ b/include/trace/events/lock.h
@@ -137,6 +137,23 @@ TRACE_EVENT(contention_end,
TP_printk("%p (ret=%d)", __entry->lock_addr, __entry->ret)
);
+TRACE_EVENT(contended_release,
+
+ TP_PROTO(void *lock),
+
+ TP_ARGS(lock),
+
+ TP_STRUCT__entry(
+ __field(void *, lock_addr)
+ ),
+
+ TP_fast_assign(
+ __entry->lock_addr = lock;
+ ),
+
+ TP_printk("%p", __entry->lock_addr)
+);
+
#endif /* _TRACE_LOCK_H */
/* This part must be outside protection */
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 09534628dc01..43b7f7e281a0 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -1023,6 +1023,9 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
wake_q_add(&wake_q, next);
}
+ if (trace_contended_release_enabled() && waiter)
+ trace_call__contended_release(lock);
+
if (owner & MUTEX_FLAG_HANDOFF)
__mutex_handoff(lock, next);
@@ -1220,6 +1223,7 @@ EXPORT_SYMBOL(ww_mutex_lock_interruptible);
EXPORT_TRACEPOINT_SYMBOL_GPL(contention_begin);
EXPORT_TRACEPOINT_SYMBOL_GPL(contention_end);
+EXPORT_TRACEPOINT_SYMBOL_GPL(contended_release);
/**
* atomic_dec_and_mutex_lock - return holding mutex if we dec to 0
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index f3ee7a0d6047..f7e152c40d6d 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -263,6 +263,9 @@ void percpu_up_write(struct percpu_rw_semaphore *sem)
{
rwsem_release(&sem->dep_map, _RET_IP_);
+ if (trace_contended_release_enabled() && wq_has_sleeper(&sem->waiters))
+ trace_call__contended_release(sem);
+
/*
* Signal the writer is done, no fast path yet.
*
@@ -292,6 +295,14 @@ EXPORT_SYMBOL_GPL(percpu_up_write);
void __percpu_up_read(struct percpu_rw_semaphore *sem)
{
lockdep_assert_preemption_disabled();
+ /*
+ * After percpu_up_write() completes, rcu_sync_is_idle() can still
+ * return false during the grace period, forcing readers into this
+ * slowpath. Only trace when a writer is actually waiting for
+ * readers to drain.
+ */
+ if (trace_contended_release_enabled() && rcuwait_active(&sem->writer))
+ trace_call__contended_release(sem);
/*
* slowpath; reader will only ever wake a single blocked
* writer.
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 69759fde7d10..9a4c3bee50ac 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1470,6 +1470,7 @@ static void __sched rt_mutex_slowunlock(struct rt_mutex_base *lock)
raw_spin_lock_irqsave(&lock->wait_lock, flags);
}
+ trace_contended_release(lock);
/*
* The wakeup next waiter path does not suffer from the above
* race. See the comments there.
diff --git a/kernel/locking/rwbase_rt.c b/kernel/locking/rwbase_rt.c
index 82e078c0665a..2835c9ef9b3f 100644
--- a/kernel/locking/rwbase_rt.c
+++ b/kernel/locking/rwbase_rt.c
@@ -174,6 +174,8 @@ static void __sched __rwbase_read_unlock(struct rwbase_rt *rwb,
static __always_inline void rwbase_read_unlock(struct rwbase_rt *rwb,
unsigned int state)
{
+ if (trace_contended_release_enabled() && rt_mutex_owner(&rwb->rtmutex))
+ trace_call__contended_release(rwb);
/*
* rwb->readers can only hit 0 when a writer is waiting for the
* active readers to leave the critical section.
@@ -205,6 +207,8 @@ static inline void rwbase_write_unlock(struct rwbase_rt *rwb)
unsigned long flags;
raw_spin_lock_irqsave(&rtm->wait_lock, flags);
+ if (trace_contended_release_enabled() && rt_mutex_has_waiters(rtm))
+ trace_call__contended_release(rwb);
__rwbase_write_unlock(rwb, WRITER_BIAS, flags);
}
@@ -214,6 +218,8 @@ static inline void rwbase_write_downgrade(struct rwbase_rt *rwb)
unsigned long flags;
raw_spin_lock_irqsave(&rtm->wait_lock, flags);
+ if (trace_contended_release_enabled() && rt_mutex_has_waiters(rtm))
+ trace_call__contended_release(rwb);
/* Release it and account current as reader */
__rwbase_write_unlock(rwb, WRITER_BIAS - 1, flags);
}
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index bf647097369c..b9c180ac1eee 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1387,6 +1387,8 @@ static inline void __up_read(struct rw_semaphore *sem)
rwsem_clear_reader_owned(sem);
tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
DEBUG_RWSEMS_WARN_ON(tmp < 0, sem);
+ if (trace_contended_release_enabled() && (tmp & RWSEM_FLAG_WAITERS))
+ trace_call__contended_release(sem);
if (unlikely((tmp & (RWSEM_LOCK_MASK|RWSEM_FLAG_WAITERS)) ==
RWSEM_FLAG_WAITERS)) {
clear_nonspinnable(sem);
@@ -1413,8 +1415,10 @@ static inline void __up_write(struct rw_semaphore *sem)
preempt_disable();
rwsem_clear_owner(sem);
tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
- if (unlikely(tmp & RWSEM_FLAG_WAITERS))
+ if (unlikely(tmp & RWSEM_FLAG_WAITERS)) {
+ trace_contended_release(sem);
rwsem_wake(sem);
+ }
preempt_enable();
}
@@ -1437,8 +1441,10 @@ static inline void __downgrade_write(struct rw_semaphore *sem)
tmp = atomic_long_fetch_add_release(
-RWSEM_WRITER_LOCKED+RWSEM_READER_BIAS, &sem->count);
rwsem_set_reader_owned(sem);
- if (tmp & RWSEM_FLAG_WAITERS)
+ if (tmp & RWSEM_FLAG_WAITERS) {
+ trace_contended_release(sem);
rwsem_downgrade_wake(sem);
+ }
preempt_enable();
}
diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
index 74d41433ba13..233730c25933 100644
--- a/kernel/locking/semaphore.c
+++ b/kernel/locking/semaphore.c
@@ -230,6 +230,10 @@ void __sched up(struct semaphore *sem)
sem->count++;
else
__up(sem, &wake_q);
+
+ if (trace_contended_release_enabled() && !wake_q_empty(&wake_q))
+ trace_call__contended_release(sem);
+
raw_spin_unlock_irqrestore(&sem->lock, flags);
if (!wake_q_empty(&wake_q))
wake_up_q(&wake_q);
--
2.52.0
^ permalink raw reply related
* [PATCH v6 4/7] locking: Factor out queued_spin_release()
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin,
Paul E. McKenney
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
Introduce queued_spin_release() as an arch-overridable unlock primitive,
and make queued_spin_unlock() a generic wrapper around it.
This is a preparatory refactoring for the next commit, which adds
contended_release tracepoint instrumentation to queued_spin_unlock().
Rename the existing arch-specific queued_spin_unlock() overrides on
x86 (paravirt) and MIPS to queued_spin_release().
No functional change.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
---
arch/mips/include/asm/spinlock.h | 6 +++---
arch/x86/include/asm/paravirt-spinlock.h | 6 +++---
include/asm-generic/qspinlock.h | 15 ++++++++++++---
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index 6ce2117e49f6..c349162f15eb 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -13,12 +13,12 @@
#include <asm-generic/qspinlock_types.h>
-#define queued_spin_unlock queued_spin_unlock
+#define queued_spin_release queued_spin_release
/**
- * queued_spin_unlock - release a queued spinlock
+ * queued_spin_release - release a queued spinlock
* @lock : Pointer to queued spinlock structure
*/
-static inline void queued_spin_unlock(struct qspinlock *lock)
+static inline void queued_spin_release(struct qspinlock *lock)
{
/* This could be optimised with ARCH_HAS_MMIOWB */
mmiowb();
diff --git a/arch/x86/include/asm/paravirt-spinlock.h b/arch/x86/include/asm/paravirt-spinlock.h
index 7beffcb08ed6..ac75e0736198 100644
--- a/arch/x86/include/asm/paravirt-spinlock.h
+++ b/arch/x86/include/asm/paravirt-spinlock.h
@@ -49,9 +49,9 @@ static __always_inline bool pv_vcpu_is_preempted(long cpu)
ALT_NOT(X86_FEATURE_VCPUPREEMPT));
}
-#define queued_spin_unlock queued_spin_unlock
+#define queued_spin_release queued_spin_release
/**
- * queued_spin_unlock - release a queued spinlock
+ * queued_spin_release - release a queued spinlock
* @lock : Pointer to queued spinlock structure
*
* A smp_store_release() on the least-significant byte.
@@ -66,7 +66,7 @@ static inline void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
pv_queued_spin_lock_slowpath(lock, val);
}
-static inline void queued_spin_unlock(struct qspinlock *lock)
+static inline void queued_spin_release(struct qspinlock *lock)
{
kcsan_release();
pv_queued_spin_unlock(lock);
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index bf47cca2c375..df76f34645a0 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -115,12 +115,12 @@ static __always_inline void queued_spin_lock(struct qspinlock *lock)
}
#endif
-#ifndef queued_spin_unlock
+#ifndef queued_spin_release
/**
- * queued_spin_unlock - release a queued spinlock
+ * queued_spin_release - release a queued spinlock
* @lock : Pointer to queued spinlock structure
*/
-static __always_inline void queued_spin_unlock(struct qspinlock *lock)
+static __always_inline void queued_spin_release(struct qspinlock *lock)
{
/*
* unlock() needs release semantics:
@@ -129,6 +129,15 @@ static __always_inline void queued_spin_unlock(struct qspinlock *lock)
}
#endif
+/**
+ * queued_spin_unlock - unlock a queued spinlock
+ * @lock : Pointer to queued spinlock structure
+ */
+static __always_inline void queued_spin_unlock(struct qspinlock *lock)
+{
+ queued_spin_release(lock);
+}
+
#ifndef virt_spin_lock
static __always_inline bool virt_spin_lock(struct qspinlock *lock)
{
--
2.52.0
^ permalink raw reply related
* [PATCH v6 6/7] locking: Factor out __queued_read_unlock()/__queued_write_unlock()
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin,
Paul E. McKenney
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
This is a preparatory refactoring for the next commit, which adds
contended_release tracepoint instrumentation and needs to call the
unlock from both traced and non-traced paths.
No functional change.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
include/asm-generic/qrwlock.h | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 75b8f4601b28..4b627bafba8b 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -101,16 +101,26 @@ static inline void queued_write_lock(struct qrwlock *lock)
queued_write_lock_slowpath(lock);
}
+static __always_inline void __queued_read_unlock(struct qrwlock *lock)
+{
+ /*
+ * Atomically decrement the reader count
+ */
+ (void)atomic_sub_return_release(_QR_BIAS, &lock->cnts);
+}
+
/**
* queued_read_unlock - release read lock of a queued rwlock
* @lock : Pointer to queued rwlock structure
*/
static inline void queued_read_unlock(struct qrwlock *lock)
{
- /*
- * Atomically decrement the reader count
- */
- (void)atomic_sub_return_release(_QR_BIAS, &lock->cnts);
+ __queued_read_unlock(lock);
+}
+
+static __always_inline void __queued_write_unlock(struct qrwlock *lock)
+{
+ smp_store_release(&lock->wlocked, 0);
}
/**
@@ -119,7 +129,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
*/
static inline void queued_write_unlock(struct qrwlock *lock)
{
- smp_store_release(&lock->wlocked, 0);
+ __queued_write_unlock(lock);
}
/**
--
2.52.0
^ permalink raw reply related
* [PATCH v6 5/7] locking: Add contended_release tracepoint to qspinlock
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin,
Paul E. McKenney
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
Use the arch-overridable queued_spin_release(), introduced in the
previous commit, to ensure the tracepoint works correctly across all
architectures, including those with custom unlock implementations (e.g.
x86 paravirt).
When the tracepoint is disabled, the only addition to the hot path is a
single NOP instruction (the static branch). When enabled, the contention
check, trace call, and unlock are combined in an out-of-line function to
minimize hot path impact, avoiding the compiler needing to preserve the
lock pointer in a callee-saved register across the trace call.
Binary size impact (x86_64, defconfig):
uninlined unlock (common case): +680 bytes (+0.00%)
inlined unlock (worst case): +83659 bytes (+0.21%)
The inlined unlock case could not be achieved through Kconfig options on
x86_64 as PREEMPT_BUILD unconditionally selects UNINLINE_SPIN_UNLOCK on
x86_64. The UNINLINE_SPIN_UNLOCK guards were manually inverted to force
inline the unlock path and estimate the worst case binary size increase.
In practice, configurations with UNINLINE_SPIN_UNLOCK=n have already
opted against binary size optimization, so the inlined worst case is
unlikely to be a concern.
Architectures with fully custom qspinlock implementations (e.g.
PowerPC) are not covered by this change.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
include/asm-generic/qspinlock.h | 18 ++++++++++++++++++
kernel/locking/qspinlock.c | 8 ++++++++
2 files changed, 26 insertions(+)
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index df76f34645a0..915a4c2777f6 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -41,6 +41,7 @@
#include <asm-generic/qspinlock_types.h>
#include <linux/atomic.h>
+#include <linux/tracepoint-defs.h>
#ifndef queued_spin_is_locked
/**
@@ -129,12 +130,29 @@ static __always_inline void queued_spin_release(struct qspinlock *lock)
}
#endif
+DECLARE_TRACEPOINT(contended_release);
+
+extern void queued_spin_release_traced(struct qspinlock *lock);
+
/**
* queued_spin_unlock - unlock a queued spinlock
* @lock : Pointer to queued spinlock structure
+ *
+ * Generic tracing wrapper around the arch-overridable
+ * queued_spin_release().
*/
static __always_inline void queued_spin_unlock(struct qspinlock *lock)
{
+ /*
+ * Trace and release are combined in queued_spin_release_traced() so
+ * the compiler does not need to preserve the lock pointer across the
+ * function call, avoiding callee-saved register save/restore on the
+ * hot path.
+ */
+ if (tracepoint_enabled(contended_release)) {
+ queued_spin_release_traced(lock);
+ return;
+ }
queued_spin_release(lock);
}
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index af8d122bb649..649fdca69288 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -104,6 +104,14 @@ static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock,
#define queued_spin_lock_slowpath native_queued_spin_lock_slowpath
#endif
+void __lockfunc queued_spin_release_traced(struct qspinlock *lock)
+{
+ if (queued_spin_is_contended(lock))
+ trace_call__contended_release(lock);
+ queued_spin_release(lock);
+}
+EXPORT_SYMBOL(queued_spin_release_traced);
+
#endif /* _GEN_PV_LOCK_SLOWPATH */
/**
--
2.52.0
^ permalink raw reply related
* [PATCH v6 7/7] locking: Add contended_release tracepoint to qrwlock
From: Dmitry Ilvokhin @ 2026-05-05 17:09 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Waiman Long,
Thomas Bogendoerfer, Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Thomas Gleixner,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Arnd Bergmann,
Dennis Zhou, Tejun Heo, Christoph Lameter, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-kernel, linux-mips, virtualization, linux-arch, linux-mm,
linux-trace-kernel, kernel-team, Dmitry Ilvokhin
In-Reply-To: <cover.1777999826.git.d@ilvokhin.com>
Extend the contended_release tracepoint to queued rwlocks, using the
same out-of-line traced unlock approach as queued spinlocks.
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
---
include/asm-generic/qrwlock.h | 22 ++++++++++++++++++++++
kernel/locking/qrwlock.c | 16 ++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 4b627bafba8b..274c19006125 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -14,6 +14,7 @@
#define __ASM_GENERIC_QRWLOCK_H
#include <linux/atomic.h>
+#include <linux/tracepoint-defs.h>
#include <asm/barrier.h>
#include <asm/processor.h>
@@ -35,6 +36,10 @@
*/
extern void queued_read_lock_slowpath(struct qrwlock *lock);
extern void queued_write_lock_slowpath(struct qrwlock *lock);
+extern void queued_read_unlock_traced(struct qrwlock *lock);
+extern void queued_write_unlock_traced(struct qrwlock *lock);
+
+DECLARE_TRACEPOINT(contended_release);
/**
* queued_read_trylock - try to acquire read lock of a queued rwlock
@@ -115,6 +120,17 @@ static __always_inline void __queued_read_unlock(struct qrwlock *lock)
*/
static inline void queued_read_unlock(struct qrwlock *lock)
{
+ /*
+ * Trace and unlock are combined in the traced unlock variant so
+ * the compiler does not need to preserve the lock pointer across
+ * the function call, avoiding callee-saved register save/restore
+ * on the hot path.
+ */
+ if (tracepoint_enabled(contended_release)) {
+ queued_read_unlock_traced(lock);
+ return;
+ }
+
__queued_read_unlock(lock);
}
@@ -129,6 +145,12 @@ static __always_inline void __queued_write_unlock(struct qrwlock *lock)
*/
static inline void queued_write_unlock(struct qrwlock *lock)
{
+ /* See comment in queued_read_unlock(). */
+ if (tracepoint_enabled(contended_release)) {
+ queued_write_unlock_traced(lock);
+ return;
+ }
+
__queued_write_unlock(lock);
}
diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
index d2ef312a8611..5ae4b0372719 100644
--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -90,3 +90,19 @@ void __lockfunc queued_write_lock_slowpath(struct qrwlock *lock)
trace_contention_end(lock, 0);
}
EXPORT_SYMBOL(queued_write_lock_slowpath);
+
+void __lockfunc queued_read_unlock_traced(struct qrwlock *lock)
+{
+ if (queued_rwlock_is_contended(lock))
+ trace_call__contended_release(lock);
+ __queued_read_unlock(lock);
+}
+EXPORT_SYMBOL(queued_read_unlock_traced);
+
+void __lockfunc queued_write_unlock_traced(struct qrwlock *lock)
+{
+ if (queued_rwlock_is_contended(lock))
+ trace_call__contended_release(lock);
+ __queued_write_unlock(lock);
+}
+EXPORT_SYMBOL(queued_write_unlock_traced);
--
2.52.0
^ permalink raw reply related
* [PATCH v4 0/3] vfio/pci: Request resources and map BARs at enable time
From: Matt Evans @ 2026-05-05 17:38 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
Hi,
These patches fix a potential race for concurrent calls to
vfio_pci_core_setup_barmap(), and a DMABUF missing check for resource
before the export. Discussion on a previous series (different,
replaced by this one) is here:
https://lore.kernel.org/kvm/20260415181423.1008458-1-mattev@meta.com
Responses in that thread indicated there wasn't a strong historical
reason to require the mapping to be performed on-demand at BAR
reference time. It's much simpler to move this earlier, to
vfio_pci_core_enable(), and that then avoids having to deal with
concurrent requests later.
The first patch requests PCI resources and pci_iomap() of the BARs
from vfio_pci_core_enable(), moving this out of
vfio_pci_core_setup_barmap().
Some callers rely on vfio_pci_core_setup_barmap() for its ioremap()
effect, and other callers use it for its resource-acquiring effect.
The function turns into a cheap error check that both these actions
have occurred; that maintains the same error behaviour as before the
fix.
The second patch adds a call to vfio_pci_core_setup_barmap() to VFIO
DMABUF export to check the resource is reserved; previously this was
able to export an unrequested resource. Although patch 1 at first
appears to fix this by requesting resources at enable time, code using
the BAR still needs to check the resource really was acquired.
The third patch refactors vfio_pci_core_setup_barmap() plus the various
vdev->barmap[] accesses into vfio_pci_core_get_iomap() which returns
either a pointer to the mapping or an ERR_PTR() describing why it
doesn't exist. This is used by callers that need the mapping, but
also by other callers to check that the resource/mapping step was
successful.
=== Changes ===
v4:
- Reorder patches to put fixes at the front: First, the early BAR
setup to avoid the race. Then, add DMABUF check. Then,
refactor/tidy.
- Adjust Fixes: of first patch to point to early VFIO PCI commit, and
reduce the patch to only the fix (don't add new error checks).
Use pci_dbg() instead of pci_warn() when setting up BAR
resources. Add barmap[] error checking to vfio_pci_core_disable().
- Add barmap[]/BAR index error checking to vfio_pci_core_get_iomap(),
and use WARN_ON_ONCE() since the conditions truly shouldn't happen.
v3:
https://lore.kernel.org/kvm/20260430100340.2787446-1-mattev@meta.com/
- Remove the separate tracking of the BAR mapping versus the
acquiring its resource. Errors from failing iomap vs resource
reservation are ERR_PTR()-elcoded into barmap[bar].
- Remove the separate test helper, and add vfio_pci_core_get_iomap().
This gets the iomap base or is used check for error/failure to
acquire the resource. Added comments at call sites explaining
whether they want to just ensure the resource is reserved versus
actually use the mapping.
v2:
https://lore.kernel.org/kvm/20260423182517.2286030-1-mattev@meta.com/
- Don't fail if resources can't be requested or iomapped, even for
valid BARs, as this would change the userspace-observable error
behaviour. Specifically, if there was an issue with one particular
BAR which happened to never be used, then userspace would never
encounter an error for it. Track iomap and resource-acquisition
status per BAR.
- Break out the checks for resource success from those for iomap
success, in the form of the two new helpers.
- Third patch to add the check to VFIO DMABUF export, because
init-time requests can now fail.
v1:
https://lore.kernel.org/kvm/20260421174143.3883579-1-mattev@meta.com/
Matt Evans (3):
vfio/pci: Set up BAR resources and maps in vfio_pci_core_enable()
vfio/pci: Check BAR resources before exporting a DMABUF
vfio/pci: Replace vfio_pci_core_setup_barmap() with
vfio_pci_core_get_iomap()
drivers/vfio/pci/nvgrace-gpu/main.c | 11 +++----
drivers/vfio/pci/vfio_pci_core.c | 47 ++++++++++++++++++++++++-----
drivers/vfio/pci/vfio_pci_dmabuf.c | 6 ++--
drivers/vfio/pci/vfio_pci_rdwr.c | 42 +++++---------------------
drivers/vfio/pci/virtio/legacy_io.c | 13 ++++----
include/linux/vfio_pci_core.h | 20 +++++++++++-
6 files changed, 81 insertions(+), 58 deletions(-)
--
2.47.3
^ permalink raw reply
* [PATCH v4 3/3] vfio/pci: Replace vfio_pci_core_setup_barmap() with vfio_pci_core_get_iomap()
From: Matt Evans @ 2026-05-05 17:38 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
In-Reply-To: <20260505173835.2324179-1-mattev@meta.com>
Since "vfio/pci: Set up barmap in vfio_pci_core_enable()", the
resource request and iomap for the BARs was performed early, and
vfio_pci_core_setup_barmap() just checks those actions succeeded.
Move this logic to a new helper that checks success and returns the
iomap address, replacing the various bare vdev->barmap[] lookups.
This maintains the error behaviour of the previous on-demand
vfio_pci_core_setup_barmap() scheme.
Signed-off-by: Matt Evans <mattev@meta.com>
---
drivers/vfio/pci/nvgrace-gpu/main.c | 11 ++++-------
drivers/vfio/pci/vfio_pci_core.c | 11 +++++------
drivers/vfio/pci/vfio_pci_dmabuf.c | 2 +-
drivers/vfio/pci/vfio_pci_rdwr.c | 30 ++++++++---------------------
drivers/vfio/pci/virtio/legacy_io.c | 13 ++++++-------
include/linux/vfio_pci_core.h | 20 ++++++++++++++++++-
6 files changed, 43 insertions(+), 44 deletions(-)
diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c
index fa056b69f899..e153002258ce 100644
--- a/drivers/vfio/pci/nvgrace-gpu/main.c
+++ b/drivers/vfio/pci/nvgrace-gpu/main.c
@@ -184,13 +184,10 @@ static int nvgrace_gpu_open_device(struct vfio_device *core_vdev)
/*
* GPU readiness is checked by reading the BAR0 registers.
- *
- * ioremap BAR0 to ensure that the BAR0 mapping is present before
- * register reads on first fault before establishing any GPU
- * memory mapping.
+ * The BAR map was just set up by vfio_pci_core_enable(), so
+ * check that was successful and bail early if not:
*/
- ret = vfio_pci_core_setup_barmap(vdev, 0);
- if (ret)
+ if (IS_ERR(vfio_pci_core_get_iomap(vdev, 0)))
goto error_exit;
if (nvdev->resmem.memlength) {
@@ -275,7 +272,7 @@ nvgrace_gpu_check_device_ready(struct nvgrace_gpu_pci_core_device *nvdev)
if (!__vfio_pci_memory_enabled(vdev))
return -EIO;
- ret = nvgrace_gpu_wait_device_ready(vdev->barmap[0]);
+ ret = nvgrace_gpu_wait_device_ready(vfio_pci_core_get_iomap(vdev, 0));
if (ret)
return ret;
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 62931dc381d8..5c8bd13f10d0 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1761,7 +1761,7 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
struct pci_dev *pdev = vdev->pdev;
unsigned int index;
u64 phys_len, req_len, pgoff, req_start;
- int ret;
+ void __iomem *bar_io;
index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
@@ -1795,12 +1795,11 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
return -EINVAL;
/*
- * Even though we don't make use of the barmap for the mmap,
- * we need to request the region and the barmap tracks that.
+ * Ensure the BAR resource region is reserved for use.
*/
- ret = vfio_pci_core_setup_barmap(vdev, index);
- if (ret)
- return ret;
+ bar_io = vfio_pci_core_get_iomap(vdev, index);
+ if (IS_ERR(bar_io))
+ return PTR_ERR(bar_io);
vma->vm_private_data = vdev;
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
index 69a5c2d511e6..46cd44b22c9c 100644
--- a/drivers/vfio/pci/vfio_pci_dmabuf.c
+++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
@@ -248,7 +248,7 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
* else. Check that PCI resources have been claimed for it.
*/
if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX ||
- vfio_pci_core_setup_barmap(vdev, get_dma_buf.region_index))
+ IS_ERR(vfio_pci_core_get_iomap(vdev, get_dma_buf.region_index)))
return -ENODEV;
dma_ranges = memdup_array_user(&arg->dma_ranges, get_dma_buf.nr_ranges,
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 3bfbb879a005..7f14dd46de17 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -198,19 +198,6 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
}
EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
-/*
- * The barmap is set up in vfio_pci_core_enable(). Callers use this
- * function to check that the BAR resources are requested or that the
- * pci_iomap() was done.
- */
-int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
-{
- if (IS_ERR(vdev->barmap[bar]))
- return PTR_ERR(vdev->barmap[bar]);
- return 0;
-}
-EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
-
ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
size_t count, loff_t *ppos, bool iswrite)
{
@@ -262,13 +249,11 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
*/
max_width = VFIO_PCI_IO_WIDTH_4;
} else {
- int ret = vfio_pci_core_setup_barmap(vdev, bar);
- if (ret) {
- done = ret;
+ io = vfio_pci_core_get_iomap(vdev, bar);
+ if (IS_ERR(io)) {
+ done = PTR_ERR(io);
goto out;
}
-
- io = vdev->barmap[bar];
}
if (bar == vdev->msix_bar) {
@@ -423,6 +408,7 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset,
loff_t pos = offset & VFIO_PCI_OFFSET_MASK;
int ret, bar = VFIO_PCI_OFFSET_TO_INDEX(offset);
struct vfio_pci_ioeventfd *ioeventfd;
+ void __iomem *io;
/* Only support ioeventfds into BARs */
if (bar > VFIO_PCI_BAR5_REGION_INDEX)
@@ -440,9 +426,9 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset,
if (count == 8)
return -EINVAL;
- ret = vfio_pci_core_setup_barmap(vdev, bar);
- if (ret)
- return ret;
+ io = vfio_pci_core_get_iomap(vdev, bar);
+ if (IS_ERR(io))
+ return PTR_ERR(io);
mutex_lock(&vdev->ioeventfds_lock);
@@ -479,7 +465,7 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset,
}
ioeventfd->vdev = vdev;
- ioeventfd->addr = vdev->barmap[bar] + pos;
+ ioeventfd->addr = io + pos;
ioeventfd->data = data;
ioeventfd->pos = pos;
ioeventfd->bar = bar;
diff --git a/drivers/vfio/pci/virtio/legacy_io.c b/drivers/vfio/pci/virtio/legacy_io.c
index 1ed349a55629..c868b2177310 100644
--- a/drivers/vfio/pci/virtio/legacy_io.c
+++ b/drivers/vfio/pci/virtio/legacy_io.c
@@ -299,19 +299,18 @@ int virtiovf_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
static int virtiovf_set_notify_addr(struct virtiovf_pci_core_device *virtvdev)
{
struct vfio_pci_core_device *core_device = &virtvdev->core_device;
- int ret;
+ void __iomem *io;
/*
* Setup the BAR where the 'notify' exists to be used by vfio as well
* This will let us mmap it only once and use it when needed.
*/
- ret = vfio_pci_core_setup_barmap(core_device,
- virtvdev->notify_bar);
- if (ret)
- return ret;
+ io = vfio_pci_core_get_iomap(core_device,
+ virtvdev->notify_bar);
+ if (IS_ERR(io))
+ return PTR_ERR(io);
- virtvdev->notify_addr = core_device->barmap[virtvdev->notify_bar] +
- virtvdev->notify_offset;
+ virtvdev->notify_addr = io + virtvdev->notify_offset;
return 0;
}
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 2ebba746c18f..ffd67e25bf3f 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -188,7 +188,6 @@ int vfio_pci_core_match_token_uuid(struct vfio_device *core_vdev,
int vfio_pci_core_enable(struct vfio_pci_core_device *vdev);
void vfio_pci_core_disable(struct vfio_pci_core_device *vdev);
void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev);
-int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar);
pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
pci_channel_state_t state);
ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
@@ -234,6 +233,25 @@ static inline bool is_aligned_for_order(struct vm_area_struct *vma,
!IS_ALIGNED(pfn, 1 << order)));
}
+/*
+ * Returns a BAR's iomap base or an ERR_PTR() if, for example, the
+ * BAR isn't valid, its resource wasn't acquired, or its iomap
+ * failed. This shall only be used after vfio_pci_core_enable()
+ * has set up the BAR maps and before vfio_pci_core_disable()
+ * tears them down.
+ */
+static inline void __iomem __must_check *
+vfio_pci_core_get_iomap(struct vfio_pci_core_device *vdev, int bar)
+{
+ if (WARN_ON_ONCE(bar < 0 || bar >= PCI_STD_NUM_BARS))
+ return ERR_PTR(-EINVAL);
+
+ if (WARN_ON_ONCE(!vdev->barmap[bar]))
+ return ERR_PTR(-ENODEV);
+
+ return vdev->barmap[bar];
+}
+
int vfio_pci_dma_buf_iommufd_map(struct dma_buf_attachment *attachment,
struct phys_vec *phys);
--
2.47.3
^ permalink raw reply related
* [PATCH v4 1/3] vfio/pci: Set up BAR resources and maps in vfio_pci_core_enable()
From: Matt Evans @ 2026-05-05 17:38 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
In-Reply-To: <20260505173835.2324179-1-mattev@meta.com>
Previously BAR resource requests and the corresponding pci_iomap()
were performed on-demand and without synchronisation, which was racy.
Rather than add synchronisation, it's simplest to address this by
doing both activities from vfio_pci_core_enable().
The resource allocation and/or pci_iomap() can still fail; their
status is tracked and existing calls to vfio_pci_core_setup_barmap()
will fail in a similar way to before. This keeps the point of failure
as observed by userspace the same, i.e. failures to request/map unused
BARs are benign.
Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
Signed-off-by: Matt Evans <mattev@meta.com>
---
drivers/vfio/pci/vfio_pci_core.c | 36 +++++++++++++++++++++++++++++++-
drivers/vfio/pci/vfio_pci_rdwr.c | 26 +++++++----------------
2 files changed, 42 insertions(+), 20 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 3f8d093aacf8..62931dc381d8 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -482,6 +482,39 @@ static int vfio_pci_core_runtime_resume(struct device *dev)
}
#endif /* CONFIG_PM */
+/*
+ * Eager-request BAR resources, and iomap them. Soft failures are
+ * allowed, and consumers must check the barmap before use in order to
+ * give compatible user-visible behaviour with the previous on-demand
+ * allocation method.
+ */
+static void vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
+{
+ struct pci_dev *pdev = vdev->pdev;
+ int i;
+
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+ int bar = i + PCI_STD_RESOURCES;
+
+ vdev->barmap[bar] = ERR_PTR(-ENODEV);
+
+ if (!pci_resource_len(pdev, i))
+ continue;
+
+ if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
+ pci_dbg(vdev->pdev, "Failed to reserve region %d\n", bar);
+ vdev->barmap[bar] = ERR_PTR(-EBUSY);
+ continue;
+ }
+
+ vdev->barmap[bar] = pci_iomap(pdev, bar, 0);
+ if (!vdev->barmap[bar]) {
+ pci_dbg(vdev->pdev, "Failed to iomap region %d\n", bar);
+ vdev->barmap[bar] = ERR_PTR(-ENOMEM);
+ }
+ }
+}
+
/*
* The pci-driver core runtime PM routines always save the device state
* before going into suspended state. If the device is going into low power
@@ -568,6 +601,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
vdev->has_vga = true;
+ vfio_pci_core_map_bars(vdev);
return 0;
@@ -648,7 +682,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
for (i = 0; i < PCI_STD_NUM_BARS; i++) {
bar = i + PCI_STD_RESOURCES;
- if (!vdev->barmap[bar])
+ if (IS_ERR_OR_NULL(vdev->barmap[bar]))
continue;
pci_iounmap(pdev, vdev->barmap[bar]);
pci_release_selected_regions(pdev, 1 << bar);
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 4251ee03e146..3bfbb879a005 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -198,27 +198,15 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
}
EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
+/*
+ * The barmap is set up in vfio_pci_core_enable(). Callers use this
+ * function to check that the BAR resources are requested or that the
+ * pci_iomap() was done.
+ */
int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
{
- struct pci_dev *pdev = vdev->pdev;
- int ret;
- void __iomem *io;
-
- if (vdev->barmap[bar])
- return 0;
-
- ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
- if (ret)
- return ret;
-
- io = pci_iomap(pdev, bar, 0);
- if (!io) {
- pci_release_selected_regions(pdev, 1 << bar);
- return -ENOMEM;
- }
-
- vdev->barmap[bar] = io;
-
+ if (IS_ERR(vdev->barmap[bar]))
+ return PTR_ERR(vdev->barmap[bar]);
return 0;
}
EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
--
2.47.3
^ permalink raw reply related
* [PATCH v4 2/3] vfio/pci: Check BAR resources before exporting a DMABUF
From: Matt Evans @ 2026-05-05 17:38 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
In-Reply-To: <20260505173835.2324179-1-mattev@meta.com>
A DMABUF exports access to BAR resources and, although they are
requested at startup time, we need to ensure they really were reserved
before exporting. Otherwise, it's possible to access unreserved
resources through the export.
Add a check to the DMABUF-creation path.
Fixes: 5d74781ebc86c ("vfio/pci: Add dma-buf export support for MMIO regions")
Signed-off-by: Matt Evans <mattev@meta.com>
---
drivers/vfio/pci/vfio_pci_dmabuf.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
index f87fd32e4a01..69a5c2d511e6 100644
--- a/drivers/vfio/pci/vfio_pci_dmabuf.c
+++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
@@ -244,9 +244,11 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
return -EINVAL;
/*
- * For PCI the region_index is the BAR number like everything else.
+ * For PCI the region_index is the BAR number like everything
+ * else. Check that PCI resources have been claimed for it.
*/
- if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX)
+ if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX ||
+ vfio_pci_core_setup_barmap(vdev, get_dma_buf.region_index))
return -ENODEV;
dma_ranges = memdup_array_user(&arg->dma_ranges, get_dma_buf.nr_ranges,
--
2.47.3
^ permalink raw reply related
* Re: [PATCH v12 01/13] scsi: aacraid: use block layer helpers to calculate num of queues
From: Aaron Tomlin @ 2026-05-05 18:42 UTC (permalink / raw)
To: axboe, kbusch, hch, sagi, mst
Cc: aacraid, James.Bottomley, martin.petersen, liyihang9,
kashyap.desai, sumit.saxena, shivasharan.srikanteshwara,
chandrakanth.patil, sathya.prakash, sreekanth.reddy,
suganath-prabu.subramani, ranjan.kumar, jinpu.wang, tglx, mingo,
peterz, juri.lelli, vincent.guittot, akpm, maz, ruanjinjie,
bigeasy, yphbchou0911, wagi, frederic, longman, chenridong, hare,
kch, ming.lei, tom.leiming, steve, sean, chjohnst, neelx, mproche,
nick.lange, marco.crivellari, linux-block, linux-kernel,
virtualization, linux-nvme, linux-scsi, megaraidlinux.pdl,
mpi3mr-linuxdrv.pdl, MPT-FusionLinux.pdl
In-Reply-To: <20260422185215.100929-2-atomlin@atomlin.com>
[-- Attachment #1: Type: text/plain, Size: 915 bytes --]
On Wed, Apr 22, 2026 at 02:52:03PM -0400, Aaron Tomlin wrote:
> From: Daniel Wagner <wagi@kernel.org>
>
> The calculation of the upper limit for queues does not depend solely on
> the number of online CPUs; for example, the isolcpus kernel
> command-line option must also be considered.
>
> To account for this, the block layer provides a helper function to
> retrieve the maximum number of queues. Use it to set an appropriate
> upper queue number limit.
>
> Fixes: 94970cfb5f10 ("scsi: use block layer helpers to calculate num of queues")
Commit 94970cfb5f10 was a refactoring patch but it did not modify or break
aacraid. I believe we should drop the "Fixes:" tag and use the following:
This brings aacraid in line with the API migration initiated for other
SCSI drivers in commit 94970cfb5f10 ("scsi: use block layer helpers to
calculate num of queues")
--
Aaron Tomlin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v12 06/13] nvme-pci: use block layer helpers to constrain queue affinity
From: Aaron Tomlin @ 2026-05-05 19:47 UTC (permalink / raw)
To: axboe, kbusch, hch, sagi, mst
Cc: aacraid, James.Bottomley, martin.petersen, liyihang9,
kashyap.desai, sumit.saxena, shivasharan.srikanteshwara,
chandrakanth.patil, sathya.prakash, sreekanth.reddy,
suganath-prabu.subramani, ranjan.kumar, jinpu.wang, tglx, mingo,
peterz, juri.lelli, vincent.guittot, akpm, maz, ruanjinjie,
bigeasy, yphbchou0911, wagi, frederic, longman, chenridong, hare,
kch, ming.lei, tom.leiming, steve, sean, chjohnst, neelx, mproche,
nick.lange, marco.crivellari, linux-block, linux-kernel,
virtualization, linux-nvme, linux-scsi, megaraidlinux.pdl,
mpi3mr-linuxdrv.pdl, MPT-FusionLinux.pdl
In-Reply-To: <20260422185215.100929-7-atomlin@atomlin.com>
[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]
On Wed, Apr 22, 2026 at 02:52:08PM -0400, Aaron Tomlin wrote:
> From: Daniel Wagner <wagi@kernel.org>
>
> Ensure that IRQ affinity setup also respects the queue-to-CPU mapping
> constraints provided by the block layer. This allows the NVMe driver
> to avoid assigning interrupts to CPUs that the block layer has excluded
> (e.g., isolated CPUs).
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
> ---
> drivers/nvme/host/pci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index db5fc9bf6627..daa041d15d3c 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2862,6 +2862,7 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
> .pre_vectors = 1,
> .calc_sets = nvme_calc_irq_sets,
> .priv = dev,
> + .mask = blk_mq_possible_queue_affinity(),
> };
> unsigned int irq_queues, poll_queues;
> unsigned int flags = PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY;
> --
> 2.51.0
Hi Daniel, Martin, Hannes,
I think we can drop this patch, including other similar changes [1][2].
The next iteration of patch 12 [3] in my queue, irq_create_affinity_masks()
has been modified to respect the housekeeping CPU mask. By intersecting the
base affinity mask with the HK_TYPE_IO_QUEUE mask prior to topological
distribution (group_mask_cpus_evenly()), we ensure that managed interrupts
are kept off isolated CPUs.
[1]: https://lore.kernel.org/lkml/20260422185215.100929-8-atomlin@atomlin.com/
[2]: https://lore.kernel.org/lkml/20260422185215.100929-9-atomlin@atomlin.com/
[3]: https://lore.kernel.org/lkml/20260422185215.100929-13-atomlin@atomlin.com/
--
Aaron Tomlin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v12 04/13] genirq/affinity: Add cpumask to struct irq_affinity
From: Aaron Tomlin @ 2026-05-05 20:40 UTC (permalink / raw)
To: axboe, kbusch, hch, sagi, mst
Cc: aacraid, James.Bottomley, martin.petersen, liyihang9,
kashyap.desai, sumit.saxena, shivasharan.srikanteshwara,
chandrakanth.patil, sathya.prakash, sreekanth.reddy,
suganath-prabu.subramani, ranjan.kumar, jinpu.wang, tglx, mingo,
peterz, juri.lelli, vincent.guittot, akpm, maz, ruanjinjie,
bigeasy, yphbchou0911, wagi, frederic, longman, chenridong, hare,
kch, ming.lei, tom.leiming, steve, sean, chjohnst, neelx, mproche,
nick.lange, marco.crivellari, linux-block, linux-kernel,
virtualization, linux-nvme, linux-scsi, megaraidlinux.pdl,
mpi3mr-linuxdrv.pdl, MPT-FusionLinux.pdl
In-Reply-To: <20260422185215.100929-5-atomlin@atomlin.com>
[-- Attachment #1: Type: text/plain, Size: 967 bytes --]
On Wed, Apr 22, 2026 at 02:52:06PM -0400, Aaron Tomlin wrote:
> From: Daniel Wagner <wagi@kernel.org>
>
> Pass a cpumask to irq_create_affinity_masks as an additional constraint
> to consider when creating the affinity masks. This allows the caller to
> exclude specific CPUs, e.g., isolated CPUs (see the 'isolcpus' kernel
> command-line parameter).
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
> ---
> include/linux/interrupt.h | 16 ++++++++++------
> kernel/irq/affinity.c | 12 ++++++++++--
> 2 files changed, 20 insertions(+), 8 deletions(-)
Hi Daniel, Hannes,
Following on from here [1], this patch will be dropped too in the next
iteration. Moving forward, drivers no longer need to pass a custom mask.
[1]: https://lore.kernel.org/lkml/bnklzljfve53m33xdxv4mlu75kqrkpc3xooxgd3pnbvwjst5hr@btomkooj4crh/
--
Aaron Tomlin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v12 05/13] blk-mq: add blk_mq_{online|possible}_queue_affinity
From: Aaron Tomlin @ 2026-05-05 20:55 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: axboe, kbusch, hch, sagi, mst, aacraid, James.Bottomley,
martin.petersen, liyihang9, kashyap.desai, sumit.saxena,
shivasharan.srikanteshwara, chandrakanth.patil, sathya.prakash,
sreekanth.reddy, suganath-prabu.subramani, ranjan.kumar,
jinpu.wang, tglx, mingo, peterz, juri.lelli, vincent.guittot,
akpm, maz, ruanjinjie, yphbchou0911, wagi, frederic, longman,
chenridong, hare, kch, ming.lei, tom.leiming, steve, sean,
chjohnst, neelx, mproche, nick.lange, marco.crivellari,
linux-block, linux-kernel, virtualization, linux-nvme, linux-scsi,
megaraidlinux.pdl, mpi3mr-linuxdrv.pdl, MPT-FusionLinux.pdl
In-Reply-To: <20260427153416.MeVS8yxF@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]
On Mon, Apr 27, 2026 at 05:34:16PM +0200, Sebastian Andrzej Siewior wrote:
> On 2026-04-22 14:52:07 [-0400], Aaron Tomlin wrote:
> > From: Daniel Wagner <wagi@kernel.org>
> >
> > Introduce blk_mq_{online|possible}_queue_affinity, which returns the
> > queue-to-CPU mapping constraints defined by the block layer. This allows
> > other subsystems (e.g., IRQ affinity setup) to respect block layer
> > requirements.
> >
> > It is necessary to provide versions for both the online and possible CPU
> > masks because some drivers want to spread their I/O queues only across
> > online CPUs, while others prefer to use all possible CPUs. And the mask
> > used needs to match with the number of queues requested
> > (see blk_num_{online|possible}_queues).
>
> Which driver uses cpu_possible_mask? This mask is assigned at boot time
> once the kernel figured how many CPUs are possible based on ACPI or
> whatever the system uses. This mask does not change.
>
> I only see drivers/scsi/lpfc/lpfc_init.c using it. Looking at
> cpu_possible_mask might not be the right thing. It is usually the same
> thing as "online" except on system where ACPI thinks that something
> could be added via hotplug _or_ if the admin shuts down a CPU via
> cpuhotplug _or_ boots with less (there a command line option for that).
>
> In case cpu_possible_mask != cpu_online_mask the intention is to
> allocate memory and setup irqs for the offline CPUs?
>
> > Signed-off-by: Daniel Wagner <wagi@kernel.org>
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
>
Hi Sebastian,
In the next iteration, this patch will be dropped. Moving forward, there
will be no more consumers of blk_mq_[online|possible]_queue_affinity().
Please see here [1].
[1]: https://lore.kernel.org/lkml/bnklzljfve53m33xdxv4mlu75kqrkpc3xooxgd3pnbvwjst5hr@btomkooj4crh/
--
Aaron Tomlin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/8] x86/msr: Consolidate rdmsr() definitions
From: Yan Zhao @ 2026-05-06 2:03 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
Juergen Gross, virtualization
In-Reply-To: <20260429184521.7AADEE4C@davehans-spike.ostc.intel.com>
On Wed, Apr 29, 2026 at 11:45:21AM -0700, Dave Hansen wrote:
> +/*
> + * Common paravirt and native helpers:
> + */
> +#define rdmsr(msr, low, high) \
> +do { \
> + u64 __val = paravirt_read_msr((msr)); \
> + (void)((low) = (u32)__val); \
> + (void)((high) = (u32)(__val >> 32)); \
> +} while (0)
Rather than direct all (paravirt and native) invocations of rdmsr() to
paravirt_*(), does it make sense to first introduce the common version of
helpers and direct the common version of helpers to paravirt_* or native_*?
So, we can have
+/*
+ * Common paravirt and native helpers:
+ */
+#define rdmsr(msr, low, high) \
+do { \
+ u64 __val = common_read_msr((msr)); \
+ (void)((low) = (u32)__val); \
+ (void)((high) = (u32)(__val >> 32)); \
+} while (0)
with below changes in patch 1.
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 9c2ea29e12a9..7ba5bc921526 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -171,8 +171,19 @@ static inline u64 native_read_pmc(int counter)
#ifdef CONFIG_PARAVIRT_XXL
#include <asm/paravirt.h>
+#define common_read_msr paravirt_read_msr
+#define common_read_msr_safe paravirt_read_msr_safe
+#define common_write_msr paravirt_write_msr
+#define common_write_msr_safe paravirt_write_msr_safe
#else
#include <linux/errno.h>
+
+/* Short-circuit the paravirt MSR infrastructure when it is disabled: */
+#define common_read_msr native_read_msr
+#define common_read_msr_safe native_read_msr_safe
+#define common_write_msr native_write_msr
+#define common_write_msr_safe native_write_msr_safe
+
/*
* Access to machine-specific registers (available on 586 and better only)
* Note: the rd* operations modify the parameters directly (without using
@@ -181,35 +192,35 @@ static inline u64 native_read_pmc(int counter)
#define rdmsr(msr, low, high) \
do { \
- u64 __val = native_read_msr((msr)); \
+ u64 __val = common_read_msr((msr)); \
(void)((low) = (u32)__val); \
(void)((high) = (u32)(__val >> 32)); \
} while (0)
static inline void wrmsr(u32 msr, u32 low, u32 high)
{
- native_write_msr(msr, (u64)high << 32 | low);
+ common_write_msr(msr, (u64)high << 32 | low);
}
#define rdmsrq(msr, val) \
- ((val) = native_read_msr((msr)))
+ ((val) = common_read_msr((msr)))
static inline void wrmsrq(u32 msr, u64 val)
{
- native_write_msr(msr, val);
+ common_write_msr(msr, val);
}
/* wrmsr with exception handling */
static inline int wrmsrq_safe(u32 msr, u64 val)
{
- return native_write_msr_safe(msr, val);
+ return common_write_msr_safe(msr, val);
}
/* rdmsr with exception handling */
#define rdmsr_safe(msr, low, high) \
({ \
u64 __val; \
- int __err = native_read_msr_safe((msr), &__val); \
+ int __err = common_read_msr_safe((msr), &__val); \
(*low) = (u32)__val; \
(*high) = (u32)(__val >> 32); \
__err; \
@@ -217,7 +228,7 @@ static inline int wrmsrq_safe(u32 msr, u64 val)
static inline int rdmsrq_safe(u32 msr, u64 *p)
{
- return native_read_msr_safe(msr, p);
+ return common_read_msr_safe(msr, p);
}
static __always_inline u64 rdpmc(int counter)
^ permalink raw reply related
* Re: [PATCH RFC 6/6] samples/rust: Add sample virtio-rtc driver [WIP]
From: Manos Pitsidianakis @ 2026-05-06 7:09 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Manos Pitsidianakis, Peter Hilber, Stefano Garzarella,
Stefan Hajnoczi, Viresh Kumar, Michael S. Tsirkin, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, rust-for-linux,
Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, Manos Pitsidianakis
In-Reply-To: <20260505-rust-virtio-v1-6-9563383909e4@pitsidianak.is>
Some stuff that needs to be addressed:
On Tue, 05 May 2026 11:14, Manos Pitsidianakis <manos@pitsidianak.is> wrote:
>While the driver queries clocks and capabilities for each clock, it
>doesn't actually register them yet (TODO).
>
>Until I implement missing functionality, there is some dead code and
>some missing SAFETY comments.
>
>Signed-off-by: Manos Pitsidianakis <manos@pitsidianak.is>
>---
> MAINTAINERS | 1 +
> samples/rust/Kconfig | 15 ++
> samples/rust/Makefile | 1 +
> samples/rust/rust_virtio_rtc.rs | 470 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 487 insertions(+)
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index e8012f708df5d4ee858c82aec3269e615fc8caad..3ed579e8d3cc64d1749cf261cd68f6338a830c4d 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -27937,6 +27937,7 @@ S: Maintained
> F: rust/helpers/virtio.c
> F: rust/kernel/virtio.rs
> F: rust/kernel/virtio/
>+F: samples/rust/rust_virtio_rtc.rs
>
> VIRTIO CRYPTO DRIVER
> M: Gonglei <arei.gonglei@huawei.com>
>diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>index c49ab910634596aea4a1a73dac87585e084f420a..96a16aecc27198fd99f4ffd0ecdf0bc0876860c6 100644
>--- a/samples/rust/Kconfig
>+++ b/samples/rust/Kconfig
>@@ -179,4 +179,19 @@ config SAMPLE_RUST_HOSTPROGS
>
> If unsure, say N.
>
>+config SAMPLE_RUST_VIRTIO_RTC
>+ tristate "Rust Virtio RTC driver"
>+ depends on VIRTIO
>+ depends on PTP_1588_CLOCK_OPTIONAL
>+ help
>+ This driver provides current time from a Virtio RTC device. The driver
>+ provides the time through one or more clocks. The Virtio RTC PTP
>+ clocks and/or the Real Time Clock driver for Virtio RTC must be
>+ enabled to expose the clocks to userspace.
>+
>+ To compile this code as a module, choose M here: the module will be
>+ called rust_virtio_rtc.
>+
>+ If unsure, say M.
>+
> endif # SAMPLES_RUST
>diff --git a/samples/rust/Makefile b/samples/rust/Makefile
>index 6c0aaa58ccccfd12ef019f68ca784f6d977bc668..0142fd8656bb8cdc95b7ef54e3183b5e51358954 100644
>--- a/samples/rust/Makefile
>+++ b/samples/rust/Makefile
>@@ -16,6 +16,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o
> obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY) += rust_driver_auxiliary.o
> obj-$(CONFIG_SAMPLE_RUST_CONFIGFS) += rust_configfs.o
> obj-$(CONFIG_SAMPLE_RUST_SOC) += rust_soc.o
>+obj-$(CONFIG_SAMPLE_RUST_VIRTIO_RTC) += rust_virtio_rtc.o
>
> rust_print-y := rust_print_main.o rust_print_events.o
>
>diff --git a/samples/rust/rust_virtio_rtc.rs b/samples/rust/rust_virtio_rtc.rs
>new file mode 100644
>index 0000000000000000000000000000000000000000..f580ed83a0a57a4b051372a51f56b787d53ed602
>--- /dev/null
>+++ b/samples/rust/rust_virtio_rtc.rs
>@@ -0,0 +1,470 @@
>+// SPDX-License-Identifier: GPL-2.0
>+
>+//! Rust virtio driver sample.
>+
>+use core::{
>+ cell::Cell,
>+ marker::PhantomData,
>+ ptr::NonNull, //
>+};
>+
>+use kernel::{
>+ device::{
>+ Core,
>+ CoreInternal, //
>+ },
>+ new_mutex, new_spinlock, page,
Formatting
>+ prelude::*,
>+ scatterlist::SGEntry,
>+ sync::Completion,
>+ sync::{Mutex, SpinLock},
Ditto
>+ virtio::{
>+ self,
>+ utils::*,
>+ virtqueue::*, //
>+ },
>+};
>+
>+use pin_init::stack_try_pin_init;
>+
>+#[pin_data]
>+struct Token {
>+ resp_actual_size: u32,
>+ #[pin]
>+ responded: Completion,
>+}
>+
>+#[pin_data]
>+struct Message<Request: Zeroable, Response: Zeroable> {
>+ msg_type: u16,
>+ #[pin]
>+ req: KVec<u8>,
>+ #[pin]
>+ resp: KVec<u8>,
>+ req_ptr: NonNull<Request>,
>+ resp_ptr: NonNull<Response>,
>+ #[pin]
>+ token: Token,
>+ _ph_req: PhantomData<Request>,
>+ _ph_resp: PhantomData<Response>,
>+}
>+
>+#[derive(Copy, Clone, Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_req_head")]
>+struct ReqHead {
>+ msg_type: Le16,
>+ reserved: [u8; 6],
>+}
>+
>+#[derive(Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_resp_head")]
>+struct RespHead {
>+ status: u8,
>+ reserved: [u8; 7],
>+}
>+
>+#[derive(Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_resp_cfg")]
>+struct RespCfg {
>+ head: ReqHead,
>+ /** # of clocks -> clock ids < num_clocks are valid */
>+ num_clocks: Le16,
>+ reserved: [u8; 6],
>+}
>+
>+#[derive(Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_req_clock_cap")]
>+struct ReqClockCap {
>+ head: ReqHead,
>+ clock_id: Le16,
>+ reserved: [u8; 6],
>+}
>+
>+#[derive(Copy, Clone, Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_resp_clock_cap")]
>+struct RespClockCap {
>+ head: ReqHead,
>+ clock_type: u8,
>+ leap_second_smearing: u8,
>+ flags: u8,
>+ reserved: [u8; 5],
>+}
>+
>+#[derive(Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_req_read")]
>+struct ReqRead {
>+ head: ReqHead,
>+ clock_id: Le16,
>+ reserved: [u8; 6],
>+}
>+
>+#[derive(Copy, Clone, Debug, Zeroable)]
>+#[repr(C)]
>+#[doc(alias = "virtio_rtc_resp_read")]
>+struct RespRead {
>+ head: ReqHead,
>+ clock_reading: Le64,
>+}
>+
>+#[repr(u8)]
>+enum ClockType {
>+ #[doc(alias = "VIRTIO_RTC_CLOCK_UTC")]
>+ Utc = 0,
>+ #[doc(alias = "VIRTIO_RTC_CLOCK_TAI")]
>+ Tai = 1,
>+ #[doc(alias = "VIRTIO_RTC_CLOCK_MONOTONIC")]
>+ Monotonic = 2,
>+ #[doc(alias = "VIRTIO_RTC_CLOCK_UTC_SMEARED")]
>+ UtcSmeared = 3,
>+ #[doc(alias = "VIRTIO_RTC_CLOCK_UTC_MAYBE_SMEARED")]
>+ UtcMaybeSmeared = 4,
>+}
>+
>+// SAFETY: `Message` is safe to be send to any task.
>+unsafe impl<Request: Zeroable, Response: Zeroable> Send for Message<Request, Response> {}
>+
>+// SAFETY: `Message` is safe to be accessed concurrently.
>+unsafe impl<Request: Zeroable, Response: Zeroable> Sync for Message<Request, Response> {}
>+
>+impl<Request: Zeroable, Response: Zeroable> Message<Request, Response> {
>+ /// Create an initializer for a new [`Message`].
>+ fn new(req_data: Request, msg_type: u16) -> Result<impl PinInit<Self, Error>> {
>+ macro_rules! alloc_buf {
>+ ($t:ty) => {{
>+ let size = (core::mem::size_of::<$t>() / page::PAGE_SIZE + 1) * page::PAGE_SIZE;
>+ KVec::<u8>::with_capacity(size, GFP_KERNEL)
Not sure if KVec is the proper allocation type here...
>+ }};
>+ }
>+ let mut req = alloc_buf!(Request)?;
>+ let mut resp = alloc_buf!(Response)?;
>+ let req_ptr: NonNull<Request> = NonNull::new(req.as_mut_ptr().cast()).unwrap();
>+ let resp_ptr = NonNull::new(resp.as_mut_ptr().cast()).unwrap();
>+ // SAFETY: `req_ptr` is a valid Request allocation
>+ unsafe {
>+ core::ptr::write(req_ptr.as_ptr(), req_data);
>+ }
>+ Ok(pin_init!(Self {
>+ req,
>+ resp,
>+ msg_type,
>+ req_ptr,
>+ resp_ptr,
>+ token <- pin_init!(Token {
>+ resp_actual_size: 0,
>+ responded <- Completion::new(),
>+ }),
>+ _ph_req: PhantomData,
>+ _ph_resp: PhantomData,
>+ }? Error))
>+ }
>+
>+ fn get_response(&self) -> Result<&Response, Error> {
>+ if self.token.resp_actual_size as usize != core::mem::size_of::<Response>() {
>+ if self.token.resp_actual_size as usize >= core::mem::size_of::<RespHead>() {
>+ let head: &RespHead = unsafe { self.resp_ptr.cast().as_ref() };
>+ return match head.status {
>+ 0 | 3 => Err(EINVAL),
>+ 1 => Err(ENOTSUPP),
>+ 2 => Err(ENODEV),
>+ 4 | 5_u8..=u8::MAX => Err(EIO),
>+ };
>+ }
>+ return Err(EINVAL);
>+ }
>+ Ok(unsafe { self.resp_ptr.as_ref() })
>+ }
>+
>+ fn send(&self, vq: &SpinLock<VirtioRtcVq>, timeout_jiffies: c_ulong) -> Result {
>+ let guard = vq.lock();
This is a spinlock, but we don't disable interrupts (spin_lock_irqsave
in C). This method seems to be missing from
rust/kernel/sync/lock/spinlock.rs, I will try adding it.
>+
>+ let mut sg_in = core::mem::MaybeUninit::zeroed();
>+ let mut sg_out = core::mem::MaybeUninit::zeroed();
>+ let req = unsafe {
>+ SGEntry::init_one(
>+ &mut sg_out,
>+ self.req_ptr.cast(),
>+ core::mem::size_of::<Request>() as u32,
>+ )
>+ };
>+ let resp = unsafe {
>+ SGEntry::init_one(
>+ &mut sg_in,
>+ self.resp_ptr.cast(),
>+ core::mem::size_of::<Response>() as u32,
>+ )
>+ };
>+ let sgs = [req, resp];
>+ guard.as_ref().add_sgs(
>+ &sgs,
>+ 1,
>+ 1,
>+ (&raw const self.token).cast_mut().cast(),
>+ GFP_ATOMIC,
>+ )?;
>+
>+ if guard.as_ref().kick_prepare() {
>+ guard.as_ref().notify();
>+ }
>+ drop(guard);
>+
>+ if timeout_jiffies > 0 {
>+ self.token
>+ .responded
>+ .wait_for_completion_interruptible_timeout(timeout_jiffies)?;
>+ } else {
>+ self.token.responded.wait_for_completion_interruptible()?;
>+ }
Failure here should not Drop the message, it should be reference-counted
and dropped when the virtqueue callback gets a reply for that token (or
when the device is reset).
>+ Ok(())
>+ }
>+}
>+
>+// TODO: use a proper enum
>+
>+const VIRTIO_RTC_REQ_READ: u16 = 0x0001;
>+const VIRTIO_RTC_REQ_CFG: u16 = 0x1000;
>+const VIRTIO_RTC_REQ_CLOCK_CAP: u16 = 0x1001;
>+
>+struct VirtioRtcVq {
>+ ptr: NonNull<Virtqueue>,
>+}
>+
>+// SAFETY: `VirtioRtcVq` is safe to be send to any task.
>+unsafe impl Send for VirtioRtcVq {}
>+
>+impl VirtioRtcVq {
>+ fn new(ptr: *mut Virtqueue) -> impl PinInit<SpinLock<Self>> {
>+ let ptr = NonNull::new(ptr).unwrap();
>+ new_spinlock!(Self { ptr })
>+ }
>+
>+ fn as_ref(&self) -> &Virtqueue {
>+ unsafe { self.ptr.as_ref() }
>+ }
>+}
>+
>+struct VirtioRtcDriver {
>+ reqvq: Pin<KBox<SpinLock<VirtioRtcVq>>>,
>+ alarmvq: Option<Pin<KBox<SpinLock<VirtioRtcVq>>>>,
>+ num_clocks: Cell<u16>,
Use atomic u16 here
>+ registered_clocks: Pin<KBox<Mutex<KVec<()>>>>,
>+}
>+
>+impl Drop for VirtioRtcDriver {
>+ fn drop(&mut self) {
>+ pr_info!("Remove Rust virtio driver sample.\n");
>+ }
>+}
>+
>+unsafe extern "C" fn vq_requestq_callback(vq: *mut kernel::bindings::virtqueue) {
>+ let vq = unsafe { Virtqueue::from_raw(vq) };
>+ let dev: &virtio::Device<CoreInternal> = vq.dev();
>+ let data = unsafe { dev.as_ref().drvdata_borrow::<VirtioRtcDriver>() };
>+ data.process_requestq();
>+}
>+
>+impl VirtioRtcDriver {
>+ /// Submit `VIRTIO_RTC_REQ_CFG` and return response (`num_clocks`)
>+ fn req_cfg(&self) -> Result<u16> {
>+ let head = ReqHead {
>+ msg_type: VIRTIO_RTC_REQ_CFG.into(),
>+ reserved: [0; 6],
>+ };
>+ stack_try_pin_init!(
>+ let msg: Message::<ReqHead, RespCfg> =
>+ Message::new(head, VIRTIO_RTC_REQ_CFG)?);
>+ let msg: core::pin::Pin<&mut Message<ReqHead, RespCfg>> = msg?;
>+ msg.send(&self.reqvq, 0)?;
>+ pr_info!("Got response! {:?}\n", msg.get_response());
>+
>+ let response: &RespCfg = msg.get_response()?;
>+ Ok(response.num_clocks.into())
>+ }
>+
>+ fn process_requestq(&self) {
>+ let mut cb_enabled = true;
>+ loop {
>+ let guard = self.reqvq.lock();
>+ if cb_enabled {
>+ guard.as_ref().disable_cb();
>+ cb_enabled = false;
>+ }
>+ if let Some((token, len)) = guard.as_ref().get_buf() {
>+ drop(guard);
>+ pr_info!("process_requestq got buf {len} bytes\n");
>+ let mut token = token.cast::<Token>();
>+
>+ unsafe { token.as_mut().resp_actual_size = len };
>+ unsafe { token.as_mut().responded.complete_all() };
>+ } else {
>+ if guard.as_ref().enable_cb() {
>+ return;
>+ }
>+ cb_enabled = true;
>+ }
>+ }
>+ }
>+
>+ fn clock_cap(&self, clock_id: u16) -> Result<RespClockCap> {
>+ type ClockCapMsg = Message<ReqClockCap, RespClockCap>;
>+
>+ let req = ReqClockCap {
>+ head: ReqHead {
>+ msg_type: VIRTIO_RTC_REQ_CLOCK_CAP.into(),
>+ reserved: [0; 6],
>+ },
>+ clock_id: clock_id.into(),
>+ reserved: [0; 6],
>+ };
>+ stack_try_pin_init!(
>+ let msg: ClockCapMsg = Message::new(req, VIRTIO_RTC_REQ_CLOCK_CAP)?
>+ );
>+ let msg: core::pin::Pin<&mut ClockCapMsg> = msg?;
>+ msg.send(&self.reqvq, 0)?;
>+ pr_info!("Got response! {:?}\n", msg.get_response());
>+ let response: &RespClockCap = msg.get_response()?;
>+ Ok(*response)
>+ }
>+
>+ fn read(&self, clock_id: u16) -> Result<u64> {
>+ type ReadMsg = Message<ReqRead, RespRead>;
>+
>+ let req = ReqRead {
>+ head: ReqHead {
>+ msg_type: VIRTIO_RTC_REQ_READ.into(),
>+ reserved: [0; 6],
>+ },
>+ clock_id: clock_id.into(),
>+ reserved: [0; 6],
>+ };
>+ stack_try_pin_init!(
>+ let msg: ReadMsg = Message::new(req, VIRTIO_RTC_REQ_CLOCK_CAP)?
s/CLOCK_CAP/READ (the Message type is messy, must polish it)
>+ );
>+ let msg: core::pin::Pin<&mut ReadMsg> = msg?;
>+ msg.send(&self.reqvq, 0)?;
>+ pr_info!("Got response! {:?}\n", msg.get_response());
>+ let response: &RespRead = msg.get_response()?;
>+ Ok(response.clock_reading.into())
>+ }
>+}
>+
>+impl virtio::Driver for VirtioRtcDriver {
>+ type IdInfo = ();
>+
>+ /// The table of device ids supported by the driver.
>+ const ID_TABLE: virtio::IdTable<Self::IdInfo> = &VIRTIO_RTC_TABLE;
>+
>+ fn probe(vdev: &virtio::Device<Core>) -> impl PinInit<Self, Error> {
>+ const VQS_INFO: [VirtqueueInfo; 1] = [
>+ VirtqueueInfo::new(c"requestq", false, vq_requestq_callback),
>+ //VirtqueueInfo::new(c"alarmq", false, vq_callback),
>+ ];
>+ let init_fn = move |slot: *mut Self| {
>+ pr_info!("Probe Rust virtio driver sample.\n");
>+ let vqs = match vdev.find_vqs(&VQS_INFO) {
>+ Ok(vqs) => {
>+ pr_info!("Found {} vqs.\n", vqs.len());
>+ vqs
>+ }
>+ Err(err) => {
>+ pr_info!("Could not find vqs: {err:?}.\n");
>+
>+ return Err(err);
>+ }
>+ };
>+ let reqvq = KBox::pin_init(VirtioRtcVq::new(vqs[0]), GFP_ATOMIC)?;
>+ let registered_clocks =
>+ KBox::pin_init(new_mutex!(KVec::with_capacity(0, GFP_KERNEL)?), GFP_KERNEL)?;
>+ unsafe {
>+ core::ptr::write(
>+ slot,
>+ Self {
>+ num_clocks: Cell::new(0),
>+ reqvq,
>+ alarmvq: None,
>+ registered_clocks,
>+ },
>+ )
>+ };
>+ Ok(())
>+ };
>+ unsafe { pin_init::pin_init_from_closure(init_fn) }
>+ }
>+
>+ fn init(&self, vdev: &virtio::Device<Core>) -> Result {
>+ vdev.ready();
>+ self.num_clocks.set(self.req_cfg()?);
>+ for i in 0..(self.num_clocks.get()) {
>+ let mut is_exposed = false;
>+
>+ let resp = self.clock_cap(i)?;
>+ let (clock_type, leap_second_smearing, flags) =
>+ (resp.clock_type, resp.leap_second_smearing, resp.flags);
>+ if cfg!(CONFIG_VIRTIO_RTC_CLASS)
>+ && (clock_type == ClockType::Utc as u8
>+ || clock_type == ClockType::UtcSmeared as u8
>+ || clock_type == ClockType::UtcMaybeSmeared as u8)
>+ {
>+ // TODO:
>+
>+ // ret = viortc_init_rtc_class_clock(viortc, vio_clk_id,
>+ // clock_type, flags);
>+ // if (ret < 0)
>+ // return ret;
>+ // if (ret > 0)
>+ // is_exposed = true;
>+ dev_warn!(vdev.as_ref(), "CONFIG_VIRTIO_RTC_CLASS TODO ");
>+ }
>+
>+ if cfg!(CONFIG_VIRTIO_RTC_PTP) {
>+ // TODO:
>+
>+ // ret = viortc_init_ptp_clock(viortc, vio_clk_id, clock_type,
>+ // leap_second_smearing);
>+ // if (ret < 0)
>+ // return ret;
>+ // if (ret > 0)
>+ // is_exposed = true;
>+ // todo!()
>+ dev_warn!(vdev.as_ref(), "CONFIG_VIRTIO_RTC_PTP TODO ");
>+ }
>+
>+ if !is_exposed {
>+ dev_warn!(
>+ vdev.as_ref(),
>+ "cannot expose clock {i} (type {clock_type}, variant {leap_second_smearing}, \
>+ flags {flags}) to userspace\n"
>+ );
>+ }
>+ let clock_reading = self.read(i)?;
>+ pr_info!("#{i} clock reading = {clock_reading}\n");
>+ }
>+ Ok(())
>+ }
>+
>+ fn remove(vdev: &virtio::Device, _this: Pin<&Self>) {
>+ pr_info!("Removing Rust virtio driver sample.\n");
>+ vdev.reset();
>+ vdev.del_vqs();
>+ }
>+}
>+
>+kernel::virtio_device_table!(
>+ VIRTIO_RTC_TABLE,
>+ MODULE_VIRTIO_RTC_TABLE,
>+ <VirtioRtcDriver as virtio::Driver>::IdInfo,
>+ [(virtio::DeviceId::new(virtio::VirtioID::Clock), ())]
>+);
>+
>+kernel::module_virtio_driver! {
>+ type: VirtioRtcDriver,
>+ name: "rust_virtio_rtc",
>+ authors: ["Manos Pitsidianakis"],
>+ description: "Rust virtio driver",
>+ license: "GPL v2",
>+}
>
>--
>2.47.3
>
^ permalink raw reply
* [PATCH v3] virtio_console: bound __send_to_port() spin loop to prevent watchdog bite
From: Peng Yang @ 2026-05-06 9:03 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman
Cc: kernel, virtualization, linux-kernel, kernel, Peng Yang
__send_to_port() acquires outvq_lock with IRQs disabled via
spin_lock_irqsave(), then spins in virtqueue_get_buf() waiting for the
host to consume the TX descriptor. When the host is slow to respond
(e.g. under heavy load from concurrent virtio_mem plug operations
during secondary VM boot), the spin never exits.
The watchdog bark ISR fires on another CPU and tries to printk(), which
calls hvc_console_print() -> put_chars() -> __send_to_port(), attempting
to acquire outvq_lock. With outvq_lock already held and IRQs disabled,
all CPUs stall and the watchdog cannot be pet, triggering a bite.
Add a 200ms deadline using ktime_get_mono_fast_ns() to break out of the
spin loop as a fallback. Since __send_to_port() may now return before
the host has consumed the TX descriptor, put_chars() is changed to
allocate a struct port_buffer (GFP_ATOMIC) as the virtqueue token
instead of a raw kmemdup() pointer, so that reclaim_consumed_buffers()
can safely call free_buf() on it regardless of whether a timeout
occurred. put_chars() frees the buffer only when
virtqueue_add_outbuf() fails and the token was never submitted.
Signed-off-by: Peng Yang <peng.yang@oss.qualcomm.com>
---
Changes in v3:
- Fix token leak in __send_to_port(): capture virtqueue_get_buf() return
value and call free_buf() after the spin loop exits normally.
- Link to v2: https://patch.msgid.link/20260429-add_timeout_to___send_to_port-v2-1-3d23efd6e388@oss.qualcomm.com
Changes in v2:
- Rework put_chars() to allocate full struct port_buffer (GFP_ATOMIC)
so free_buf() can safely reclaim the token on timeout.
- Transfer buffer ownership to virtqueue on success; free immediately
on virtqueue_add_outbuf() failure.
- Link to v1: https://patch.msgid.link/20260420-add_timeout_to___send_to_port-v1-1-6c32d33bf4f2@oss.qualcomm.com
To: Amit Shah <amit@kernel.org>
To: Arnd Bergmann <arnd@arndb.de>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: kernel@oss.qualcomm.com
Cc: virtualization@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
---
drivers/char/virtio_console.c | 81 +++++++++++++++++++++++++++++++++++--------
1 file changed, 67 insertions(+), 14 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9a33217c68d9..4d6b3e144e7f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -27,6 +27,7 @@
#include <linux/module.h>
#include <linux/dma-mapping.h>
#include <linux/string_choices.h>
+#include <linux/timekeeping.h>
#include "../tty/hvc/hvc_console.h"
#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
@@ -601,6 +602,8 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int err;
unsigned long flags;
unsigned int len;
+ void *token;
+ u64 deadline;
out_vq = port->out_vq;
@@ -632,10 +635,20 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
* buffer and relax the spinning requirement. The downside is
* we need to kmalloc a GFP_ATOMIC buffer each time the
* console driver writes something out.
+ *
+ * To avoid spinning forever if the host stops processing the
+ * TX virtqueue (e.g. during VM shutdown), a 200ms deadline is
+ * used to break out of the loop as a fallback.
*/
- while (!virtqueue_get_buf(out_vq, &len)
- && !virtqueue_is_broken(out_vq))
+ deadline = ktime_get_mono_fast_ns() + 200ULL * NSEC_PER_MSEC;
+ while (!(token = virtqueue_get_buf(out_vq, &len)) &&
+ !virtqueue_is_broken(out_vq)) {
+ if (ktime_get_mono_fast_ns() >= deadline)
+ break;
cpu_relax();
+ }
+ if (token)
+ free_buf(token, false);
done:
spin_unlock_irqrestore(&port->outvq_lock, flags);
@@ -1097,31 +1110,71 @@ static const struct file_operations port_fops = {
};
/*
- * The put_chars() callback is pretty straightforward.
+ * The put_chars() callback writes characters to the virtio console port.
*
- * We turn the characters into a scatter-gather list, add it to the
- * output queue and then kick the Host. Then we sit here waiting for
- * it to finish: inefficient in theory, but in practice
- * implementations will do it immediately.
+ * We allocate a struct port_buffer (with GFP_ATOMIC) to wrap the data so
+ * that reclaim_consumed_buffers() can safely call free_buf() on the token
+ * returned by virtqueue_get_buf(), even if __send_to_port() timed out
+ * before observing the used-ring update.
+ *
+ * On success, ownership of the buffer is transferred to the virtqueue as
+ * the descriptor token; it will be reclaimed by reclaim_consumed_buffers().
+ * On failure (virtqueue_add_outbuf() error), the buffer was never submitted
+ * and must be freed explicitly here.
*/
static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
{
struct port *port;
struct scatterlist sg[1];
- void *data;
- int ret;
+ struct port_buffer *pbuf;
+ ssize_t ret;
+
+ if (!count)
+ return 0;
port = find_port_by_vtermno(vtermno);
if (!port)
return -EPIPE;
- data = kmemdup(buf, count, GFP_ATOMIC);
- if (!data)
+ /*
+ * Allocate a struct port_buffer with GFP_ATOMIC so that
+ * reclaim_consumed_buffers() can safely call free_buf() on the token
+ * returned by virtqueue_get_buf(), whether or not __send_to_port()
+ * timed out. alloc_buf() uses GFP_KERNEL internally, so we open-code
+ * the allocation here.
+ */
+ pbuf = kmalloc(struct_size(pbuf, sg, 0), GFP_ATOMIC);
+ if (!pbuf)
return -ENOMEM;
- sg_init_one(sg, data, count);
- ret = __send_to_port(port, sg, 1, count, data, false);
- kfree(data);
+ pbuf->buf = kmalloc(count, GFP_ATOMIC);
+ if (!pbuf->buf) {
+ kfree(pbuf);
+ return -ENOMEM;
+ }
+ pbuf->dev = NULL;
+ pbuf->sgpages = 0;
+ pbuf->len = count;
+ pbuf->offset = 0;
+ pbuf->size = count;
+ memcpy(pbuf->buf, buf, count);
+
+ sg_init_one(sg, pbuf->buf, count);
+ ret = __send_to_port(port, sg, 1, count, pbuf, false);
+
+ /*
+ * If virtqueue_add_outbuf() failed inside __send_to_port() (ret <= 0),
+ * the token was never submitted to the virtqueue, so reclaim_consumed_
+ * buffers() will never see it. Free pbuf explicitly in that case.
+ *
+ * On success (ret > 0), ownership of pbuf has been transferred to the
+ * virtqueue as the descriptor token. It will be reclaimed and freed
+ * by reclaim_consumed_buffers() -> free_buf() when the host marks the
+ * descriptor as used, even if __send_to_port() timed out before
+ * observing the used-ring update. Do NOT free pbuf here in that case.
+ */
+ if (ret <= 0)
+ free_buf(pbuf, false);
return ret;
}
---
base-commit: 97e797263a5e963da3d1e66e743fd518567dfe37
change-id: 20260420-add_timeout_to___send_to_port-104ce7bcf241
Best regards,
--
Peng Yang <peng.yang@oss.qualcomm.com>
^ permalink raw reply related
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Arseniy Krasnov @ 2026-05-06 9:50 UTC (permalink / raw)
To: Bobby Eshleman, Stefano Garzarella
Cc: Eric Dumazet, Bobby Eshleman, Stefan Hajnoczi, Michael S. Tsirkin,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
netdev, eric.dumazet, Arseniy Krasnov, Jason Wang, Xuan Zhuo,
Eugenio Pérez, kvm, virtualization
In-Reply-To: <afocwkfBHJ5u10rx@devvm29614.prn0.facebook.com>
05.05.2026 19:37, Bobby Eshleman wrote:
> On Tue, May 05, 2026 at 06:11:13PM +0200, Stefano Garzarella wrote:
>> On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
>>> On Tue, May 5, 2026 at 6:52 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>>>
>>>> On Thu, Apr 30, 2026 at 12:26:52PM +0000, Eric Dumazet wrote:
>>>>> virtio_transport_inc_rx_pkt() checks vvs->rx_bytes + len > vvs->buf_alloc.
>>>>>
>>>>> virtio_transport_recv_enqueue() skips coalescing for packets
>>>>> with VIRTIO_VSOCK_SEQ_EOM.
>>>>>
>>>>> If fed with packets with len == 0 and VIRTIO_VSOCK_SEQ_EOM,
>>>>> a very large number of packets can be queued
>>>>> because vvs->rx_bytes stays at 0.
>>>>>
>>>>> Fix this by estimating the skb metadata size:
>>>>>
>>>>> (Number of skbs in the queue) * SKB_TRUESIZE(0)
>>>>>
>>>>> Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
>>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>>>> Cc: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> Cc: Stefano Garzarella <sgarzare@redhat.com>
>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>> Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> Cc: "Eugenio Pérez" <eperezma@redhat.com>
>>>>> Cc: kvm@vger.kernel.org
>>>>> Cc: virtualization@lists.linux.dev
>>>>> ---
>>>>> net/vmw_vsock/virtio_transport_common.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>> index 416d533f493d7b07e9c77c43f741d28cfcd0953e..9b8014516f4fb1130ae184635fbba4dfee58bd64 100644
>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>> @@ -447,7 +447,9 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>>>>> static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
>>>>> u32 len)
>>>>> {
>>>>> - if (vvs->buf_used + len > vvs->buf_alloc)
>>>>> + u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * SKB_TRUESIZE(0);
>>>>> +
>>>>> + if (skb_overhead + vvs->buf_used + len > vvs->buf_alloc)
>>>>> return false;
>>>>
>>>> I'm not sure about this fix, I mean that maybe this is incomplete.
>>>> In virtio-vsock, there is a credit mechanism between the two peers:
>>>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-4850003
>>>>
>>>> This takes only the payload into account, so it’s true that this problem
>>>> exists; however, perhaps we should also inform the other peer of a lower
>>>> credit balance, otherwise the other peer will believe it has much more
>>>> credit than it actually does, send a large payload, and then the packet
>>>> will be discarded and the data lost (there are no retransmissions,
>>>> etc.).
>>>
>>> I dunno, perhaps revert 077706165717 ("virtio/vsock: don't use skbuff
>>> state to account credit")
>>> and find a better fix then?
>>
>> IIRC the same issue was there before the commit fixed by that one (commit
>> 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")), so
>> not sure about reverting it TBH.
>>
>> CCing Arseniy and Bobby.
Thanks!
>>
>>>
>>> There is always a discrepancy between skb->len and skb->truesize.
>>> You will not be able to announce a 1MB window, and accept one milliion
>>> skb of 1-byte each.
>>>
>>> This kind of contract is broken.
>>>
>>
>> Yep, I agree, but before we start discarding data (and losing it), IMHO we
>> should at least inform the other peer that we're out of space.
>>
>> @Stefan, @Michael, do you think we can do something in the spec to avoid
>> this issue and in some way take into account also the metadata in the
>> credit. I mean to avoid the 1-byte packets flooding.
>>
>> Thanks,
>> Stefano
>>
>>
>
> Indeed the old pre-fix skb code would have the same issue.
>
> I can't think of any way around this without extending the spec.
Hi, thanks, agree with Bobby, that accounting metadata (e.g. skb size here) was not implemented "by
design" in credit logic - another side of data exchange knows nothing about that. Also the same
situation was before skb implementation was added by Bobby. So looks like need to update spec may be.
Thanks!
>
> Best,
> Bobby
^ permalink raw reply
* [PATCH v4] virtio_console: bound __send_to_port() spin loop to prevent watchdog bite
From: Peng Yang @ 2026-05-06 10:04 UTC (permalink / raw)
To: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman
Cc: kernel, virtualization, linux-kernel, kernel, Peng Yang
__send_to_port() acquires outvq_lock with IRQs disabled via
spin_lock_irqsave(), then spins in virtqueue_get_buf() waiting for the
host to consume the TX descriptor. When the host is slow to respond
(e.g. under heavy load from concurrent virtio_mem plug operations
during secondary VM boot), the spin never exits.
The watchdog bark ISR fires on another CPU and tries to printk(), which
calls hvc_console_print() -> put_chars() -> __send_to_port(), attempting
to acquire outvq_lock. With outvq_lock already held and IRQs disabled,
all CPUs stall and the watchdog cannot be pet, triggering a bite.
Add a 200ms deadline using ktime_get_mono_fast_ns() to break out of the
spin loop as a fallback. Since __send_to_port() may now return before
the host has consumed the TX descriptor, put_chars() is changed to
allocate a struct port_buffer (GFP_ATOMIC) as the virtqueue token
instead of a raw kmemdup() pointer, so that reclaim_consumed_buffers()
can safely call free_buf() on it regardless of whether a timeout
occurred. put_chars() frees the buffer only when
virtqueue_add_outbuf() fails and the token was never submitted.
Signed-off-by: Peng Yang <peng.yang@oss.qualcomm.com>
---
Changes in v4:
- Rewrite commit message to be more concise and focused.
- Link to v3: https://patch.msgid.link/20260506-add_timeout_to___send_to_port-v3-1-8ad046fc3dc3@oss.qualcomm.com
Changes in v3:
- Fix token leak in __send_to_port(): capture virtqueue_get_buf() return
value and call free_buf() after the spin loop exits normally.
- Link to v2: https://patch.msgid.link/20260429-add_timeout_to___send_to_port-v2-1-3d23efd6e388@oss.qualcomm.com
Changes in v2:
- Rework put_chars() to allocate full struct port_buffer (GFP_ATOMIC)
so free_buf() can safely reclaim the token on timeout.
- Transfer buffer ownership to virtqueue on success; free immediately
on virtqueue_add_outbuf() failure.
- Link to v1: https://patch.msgid.link/20260420-add_timeout_to___send_to_port-v1-1-6c32d33bf4f2@oss.qualcomm.com
To: Amit Shah <amit@kernel.org>
To: Arnd Bergmann <arnd@arndb.de>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: kernel@oss.qualcomm.com
Cc: virtualization@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
---
drivers/char/virtio_console.c | 75 +++++++++++++++++++++++++++++++++++--------
1 file changed, 61 insertions(+), 14 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9a33217c68d9..12674a667fec 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -27,6 +27,7 @@
#include <linux/module.h>
#include <linux/dma-mapping.h>
#include <linux/string_choices.h>
+#include <linux/timekeeping.h>
#include "../tty/hvc/hvc_console.h"
#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
@@ -601,6 +602,8 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int err;
unsigned long flags;
unsigned int len;
+ void *token;
+ u64 deadline;
out_vq = port->out_vq;
@@ -632,10 +635,20 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
* buffer and relax the spinning requirement. The downside is
* we need to kmalloc a GFP_ATOMIC buffer each time the
* console driver writes something out.
+ *
+ * To avoid spinning forever if the host stops processing the
+ * TX virtqueue (e.g. during VM shutdown), a 200ms deadline is
+ * used to break out of the loop as a fallback.
*/
- while (!virtqueue_get_buf(out_vq, &len)
- && !virtqueue_is_broken(out_vq))
+ deadline = ktime_get_mono_fast_ns() + 200ULL * NSEC_PER_MSEC;
+ while (!(token = virtqueue_get_buf(out_vq, &len)) &&
+ !virtqueue_is_broken(out_vq)) {
+ if (ktime_get_mono_fast_ns() >= deadline)
+ break;
cpu_relax();
+ }
+ if (token)
+ free_buf(token, false);
done:
spin_unlock_irqrestore(&port->outvq_lock, flags);
@@ -1097,31 +1110,65 @@ static const struct file_operations port_fops = {
};
/*
- * The put_chars() callback is pretty straightforward.
+ * The put_chars() callback writes characters to the virtio console port.
+ *
+ * We allocate a struct port_buffer (with GFP_ATOMIC) to wrap the data so
+ * that reclaim_consumed_buffers() can safely call free_buf() on the token
+ * returned by virtqueue_get_buf(), even if __send_to_port() timed out
+ * before observing the used-ring update.
*
- * We turn the characters into a scatter-gather list, add it to the
- * output queue and then kick the Host. Then we sit here waiting for
- * it to finish: inefficient in theory, but in practice
- * implementations will do it immediately.
+ * On success, ownership of the buffer is transferred to the virtqueue as
+ * the descriptor token; it will be reclaimed by reclaim_consumed_buffers().
+ * On failure (virtqueue_add_outbuf() error), the buffer was never submitted
+ * and must be freed explicitly here.
*/
static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
{
struct port *port;
struct scatterlist sg[1];
- void *data;
- int ret;
+ struct port_buffer *pbuf;
+ ssize_t ret;
+
+ if (!count)
+ return 0;
port = find_port_by_vtermno(vtermno);
if (!port)
return -EPIPE;
- data = kmemdup(buf, count, GFP_ATOMIC);
- if (!data)
+ pbuf = kmalloc(struct_size(pbuf, sg, 0), GFP_ATOMIC);
+ if (!pbuf)
+ return -ENOMEM;
+
+ pbuf->buf = kmalloc(count, GFP_ATOMIC);
+ if (!pbuf->buf) {
+ kfree(pbuf);
return -ENOMEM;
+ }
+ pbuf->dev = NULL;
+ pbuf->sgpages = 0;
+ pbuf->len = count;
+ pbuf->offset = 0;
+ pbuf->size = count;
+ memcpy(pbuf->buf, buf, count);
- sg_init_one(sg, data, count);
- ret = __send_to_port(port, sg, 1, count, data, false);
- kfree(data);
+ sg_init_one(sg, pbuf->buf, count);
+ ret = __send_to_port(port, sg, 1, count, pbuf, false);
+
+ /*
+ * If virtqueue_add_outbuf() failed inside __send_to_port() (ret <= 0),
+ * the token was never submitted to the virtqueue, so reclaim_consumed_
+ * buffers() will never see it. Free pbuf explicitly in that case.
+ *
+ * On success (ret > 0), __send_to_port() may have already freed pbuf
+ * via free_buf() if virtqueue_get_buf() returned the token before the
+ * deadline. If the spin loop timed out instead, pbuf is still in the
+ * virtqueue and will be reclaimed and freed by
+ * reclaim_consumed_buffers() -> free_buf() on the next call. Either
+ * way, do NOT free pbuf here.
+ */
+ if (ret <= 0)
+ free_buf(pbuf, false);
return ret;
}
---
base-commit: 97e797263a5e963da3d1e66e743fd518567dfe37
change-id: 20260420-add_timeout_to___send_to_port-104ce7bcf241
Best regards,
--
Peng Yang <peng.yang@oss.qualcomm.com>
^ permalink raw reply related
* Re: [PATCH v2 2/8] x86/msr: Consolidate rdmsr() definitions
From: Yan Zhao @ 2026-05-06 10:25 UTC (permalink / raw)
To: Dave Hansen, linux-kernel, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, Juergen Gross, virtualization
In-Reply-To: <afqhbl9pKKwsVjOy@yzhao56-desk.sh.intel.com>
On Wed, May 06, 2026 at 10:03:26AM +0800, Yan Zhao wrote:
> On Wed, Apr 29, 2026 at 11:45:21AM -0700, Dave Hansen wrote:
> > +/*
> > + * Common paravirt and native helpers:
> > + */
> > +#define rdmsr(msr, low, high) \
> > +do { \
> > + u64 __val = paravirt_read_msr((msr)); \
> > + (void)((low) = (u32)__val); \
> > + (void)((high) = (u32)(__val >> 32)); \
> > +} while (0)
> Rather than direct all (paravirt and native) invocations of rdmsr() to
> paravirt_*(), does it make sense to first introduce the common version of
> helpers and direct the common version of helpers to paravirt_* or native_*?
> So, we can have
>
> +/*
> + * Common paravirt and native helpers:
> + */
> +#define rdmsr(msr, low, high) \
> +do { \
> + u64 __val = common_read_msr((msr)); \
> + (void)((low) = (u32)__val); \
> + (void)((high) = (u32)(__val >> 32)); \
> +} while (0)
>
Or maybe s/common/trampoline ?
> with below changes in patch 1.
>
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 9c2ea29e12a9..7ba5bc921526 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -171,8 +171,19 @@ static inline u64 native_read_pmc(int counter)
>
> #ifdef CONFIG_PARAVIRT_XXL
> #include <asm/paravirt.h>
> +#define common_read_msr paravirt_read_msr
> +#define common_read_msr_safe paravirt_read_msr_safe
> +#define common_write_msr paravirt_write_msr
> +#define common_write_msr_safe paravirt_write_msr_safe
> #else
> #include <linux/errno.h>
> +
> +/* Short-circuit the paravirt MSR infrastructure when it is disabled: */
> +#define common_read_msr native_read_msr
> +#define common_read_msr_safe native_read_msr_safe
> +#define common_write_msr native_write_msr
> +#define common_write_msr_safe native_write_msr_safe
> +
>
> /*
> * Access to machine-specific registers (available on 586 and better only)
> * Note: the rd* operations modify the parameters directly (without using
> @@ -181,35 +192,35 @@ static inline u64 native_read_pmc(int counter)
>
> #define rdmsr(msr, low, high) \
> do { \
> - u64 __val = native_read_msr((msr)); \
> + u64 __val = common_read_msr((msr)); \
> (void)((low) = (u32)__val); \
> (void)((high) = (u32)(__val >> 32)); \
> } while (0)
>
> static inline void wrmsr(u32 msr, u32 low, u32 high)
> {
> - native_write_msr(msr, (u64)high << 32 | low);
> + common_write_msr(msr, (u64)high << 32 | low);
> }
>
> #define rdmsrq(msr, val) \
> - ((val) = native_read_msr((msr)))
> + ((val) = common_read_msr((msr)))
>
> static inline void wrmsrq(u32 msr, u64 val)
> {
> - native_write_msr(msr, val);
> + common_write_msr(msr, val);
> }
>
> /* wrmsr with exception handling */
> static inline int wrmsrq_safe(u32 msr, u64 val)
> {
> - return native_write_msr_safe(msr, val);
> + return common_write_msr_safe(msr, val);
> }
>
> /* rdmsr with exception handling */
> #define rdmsr_safe(msr, low, high) \
> ({ \
> u64 __val; \
> - int __err = native_read_msr_safe((msr), &__val); \
> + int __err = common_read_msr_safe((msr), &__val); \
> (*low) = (u32)__val; \
> (*high) = (u32)(__val >> 32); \
> __err; \
> @@ -217,7 +228,7 @@ static inline int wrmsrq_safe(u32 msr, u64 val)
>
> static inline int rdmsrq_safe(u32 msr, u64 *p)
> {
> - return native_read_msr_safe(msr, p);
> + return common_read_msr_safe(msr, p);
> }
>
> static __always_inline u64 rdpmc(int counter)
>
>
^ permalink raw reply
* Re: [PATCH v2 2/8] x86/msr: Consolidate rdmsr() definitions
From: Jürgen Groß @ 2026-05-06 11:09 UTC (permalink / raw)
To: Yan Zhao, Dave Hansen, linux-kernel, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, virtualization
In-Reply-To: <afsXCIUjib70dBYz@yzhao56-desk.sh.intel.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 1220 bytes --]
On 06.05.26 12:25, Yan Zhao wrote:
> On Wed, May 06, 2026 at 10:03:26AM +0800, Yan Zhao wrote:
>> On Wed, Apr 29, 2026 at 11:45:21AM -0700, Dave Hansen wrote:
>>> +/*
>>> + * Common paravirt and native helpers:
>>> + */
>>> +#define rdmsr(msr, low, high) \
>>> +do { \
>>> + u64 __val = paravirt_read_msr((msr)); \
>>> + (void)((low) = (u32)__val); \
>>> + (void)((high) = (u32)(__val >> 32)); \
>>> +} while (0)
>> Rather than direct all (paravirt and native) invocations of rdmsr() to
>> paravirt_*(), does it make sense to first introduce the common version of
>> helpers and direct the common version of helpers to paravirt_* or native_*?
>> So, we can have
>>
>> +/*
>> + * Common paravirt and native helpers:
>> + */
>> +#define rdmsr(msr, low, high) \
>> +do { \
>> + u64 __val = common_read_msr((msr)); \
>> + (void)((low) = (u32)__val); \
>> + (void)((high) = (u32)(__val >> 32)); \
>> +} while (0)
>>
> Or maybe s/common/trampoline ?
I'd suggest __read_msr() and friends.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix potential unbounded skb queue
From: Stefano Garzarella @ 2026-05-06 14:00 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: Bobby Eshleman, Eric Dumazet, Bobby Eshleman, Stefan Hajnoczi,
Michael S. Tsirkin, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, eric.dumazet, Arseniy Krasnov, Jason Wang,
Xuan Zhuo, Eugenio Pérez, kvm, virtualization
In-Reply-To: <e1f32df5-b6a1-47a8-a783-fcc8e3c91f25@salutedevices.com>
On Wed, May 06, 2026 at 12:50:04PM +0300, Arseniy Krasnov wrote:
>
>
>05.05.2026 19:37, Bobby Eshleman wrote:
>> On Tue, May 05, 2026 at 06:11:13PM +0200, Stefano Garzarella wrote:
>>> On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
>>>> On Tue, May 5, 2026 at 6:52 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>>>>
>>>>> On Thu, Apr 30, 2026 at 12:26:52PM +0000, Eric Dumazet wrote:
>>>>>> virtio_transport_inc_rx_pkt() checks vvs->rx_bytes + len > vvs->buf_alloc.
>>>>>>
>>>>>> virtio_transport_recv_enqueue() skips coalescing for packets
>>>>>> with VIRTIO_VSOCK_SEQ_EOM.
>>>>>>
>>>>>> If fed with packets with len == 0 and VIRTIO_VSOCK_SEQ_EOM,
>>>>>> a very large number of packets can be queued
>>>>>> because vvs->rx_bytes stays at 0.
>>>>>>
>>>>>> Fix this by estimating the skb metadata size:
>>>>>>
>>>>>> (Number of skbs in the queue) * SKB_TRUESIZE(0)
>>>>>>
>>>>>> Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
>>>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>>>>> Cc: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>> Cc: Stefano Garzarella <sgarzare@redhat.com>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>> Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>> Cc: "Eugenio Pérez" <eperezma@redhat.com>
>>>>>> Cc: kvm@vger.kernel.org
>>>>>> Cc: virtualization@lists.linux.dev
>>>>>> ---
>>>>>> net/vmw_vsock/virtio_transport_common.c | 4 +++-
>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>>>> index 416d533f493d7b07e9c77c43f741d28cfcd0953e..9b8014516f4fb1130ae184635fbba4dfee58bd64 100644
>>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>>> @@ -447,7 +447,9 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>>>>>> static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
>>>>>> u32 len)
>>>>>> {
>>>>>> - if (vvs->buf_used + len > vvs->buf_alloc)
>>>>>> + u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * SKB_TRUESIZE(0);
>>>>>> +
>>>>>> + if (skb_overhead + vvs->buf_used + len > vvs->buf_alloc)
>>>>>> return false;
>>>>>
>>>>> I'm not sure about this fix, I mean that maybe this is incomplete.
>>>>> In virtio-vsock, there is a credit mechanism between the two peers:
>>>>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-4850003
>>>>>
>>>>> This takes only the payload into account, so it’s true that this problem
>>>>> exists; however, perhaps we should also inform the other peer of a lower
>>>>> credit balance, otherwise the other peer will believe it has much more
>>>>> credit than it actually does, send a large payload, and then the packet
>>>>> will be discarded and the data lost (there are no retransmissions,
>>>>> etc.).
>>>>
>>>> I dunno, perhaps revert 077706165717 ("virtio/vsock: don't use skbuff
>>>> state to account credit")
>>>> and find a better fix then?
>>>
>>> IIRC the same issue was there before the commit fixed by that one (commit
>>> 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")), so
>>> not sure about reverting it TBH.
>>>
>>> CCing Arseniy and Bobby.
>
>Thanks!
>
>>>
>>>>
>>>> There is always a discrepancy between skb->len and skb->truesize.
>>>> You will not be able to announce a 1MB window, and accept one milliion
>>>> skb of 1-byte each.
>>>>
>>>> This kind of contract is broken.
>>>>
>>>
>>> Yep, I agree, but before we start discarding data (and losing it), IMHO we
>>> should at least inform the other peer that we're out of space.
>>>
>>> @Stefan, @Michael, do you think we can do something in the spec to avoid
>>> this issue and in some way take into account also the metadata in the
>>> credit. I mean to avoid the 1-byte packets flooding.
>>>
>>> Thanks,
>>> Stefano
>>>
>>>
>>
>> Indeed the old pre-fix skb code would have the same issue.
>>
>> I can't think of any way around this without extending the spec.
>
>Hi, thanks, agree with Bobby, that accounting metadata (e.g. skb size here) was not implemented "by
>design" in credit logic - another side of data exchange knows nothing about that. Also the same
>situation was before skb implementation was added by Bobby. So looks like need to update spec may be.
>
Even if we change the specifications, we still need to work with older
devices, so we should find a solution for this as well.
My main concern is data loss, so I'm considering the following options:
1. Notify the other peer of a smaller buf_alloc from the start, leave
some room for overhead, and when it's running out, notify them that
buf_alloc = 0. This way, the peer realizes it can’t send anything else.
2. Or update buf_alloc each time by removing the overhead, similar to
what’s currently done in virtio_transport_inc_rx_pkt(), but also do it
in virtio_transport_inc_tx_pkt().
As I said, IMO this patch alone is incomplete; we need to communicate
with the peer somehow regarding space. I don’t think including the
overhead in fwd_cnt is spec compliant, since the other peer has no idea
how much overhead is needed, but reducing buf_alloc should be okay, even
though I’m concerned about packets in flight.
As a quick fix, I think option 2 might be the easiest; I’ll run some
tests and send over a patch.
But in the long run, I think we absolutely need to improve memory
management in vsock, perhaps by avoiding custom solutions.
Thanks,
Stefano
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox