* [PATCH v2] virtio_console: fix race between hvc put_chars and virtqueue teardown on freeze
From: Sungho Bae @ 2026-05-19 16:22 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
From: Sungho Bae <baver.bae@lge.com>
With no_console_suspend enabled, hvc console output can continue while
virtio_console is freezing. In that window, put_chars can still enqueue
buffers to the output virtqueue while virtcons_freeze is tearing queues
down, triggering a BUG_ON in virtqueue_detach_unused_buf_split:
BUG_ON(vq->vq.num_free != vq->split.vring.num)
Add a pm_freezing flag to ports_device. Set it via smp_store_release()
at the start of virtcons_freeze(); put_chars() and __send_to_port() drop
output while the flag is set, checked via smp_load_acquire().
The check in __send_to_port() is placed under outvq_lock, making it
atomic with remove_port_data() which also acquires outvq_lock. Once
remove_port_data() returns for a given port, no concurrent
__send_to_port() can add buffers before remove_vqs() tears down the vq.
After setting pm_freezing, acquire and release outvq_lock for each port
before calling virtio_reset_device(). A TX thread that already passed
the pm_freezing check may still hold outvq_lock while spinning for host
acknowledgment (the nonblock=false hvc path); the drain loop ensures
all such threads have completed before the device is reset.
Clear pm_freezing in virtcons_restore() only after all port->out_vq
pointers have been reassigned to the newly allocated virtqueues,
preventing TX paths from dereferencing freed vqs during restore.
Also fix two races in __send_to_port() uncovered by this work: load
port->portdev via READ_ONCE() and check for NULL to guard against
concurrent hot-unplug, and move out_vq = port->out_vq inside
outvq_lock after the pm_freezing check to avoid a stale pointer.
Link: https://sashiko.dev/#/patchset/20260515225259.1054-1-baver.bae%40gmail.com
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/char/virtio_console.c | 80 ++++++++++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 2 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9a33217c68d9..fad673f733a8 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -157,6 +157,12 @@ struct ports_device {
/* Major number for this device. Ports will be created as minors. */
int chr_major;
+
+ /*
+ * Set to true during PM freeze to block TX paths that may race
+ * with virtqueue teardown (e.g. hvc put_chars with no_console_suspend).
+ */
+ bool pm_freezing;
};
struct port_stats {
@@ -601,11 +607,30 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int err;
unsigned long flags;
unsigned int len;
-
- out_vq = port->out_vq;
+ struct ports_device *portdev;
spin_lock_irqsave(&port->outvq_lock, flags);
+ portdev = READ_ONCE(port->portdev);
+
+ if (!portdev) {
+ in_count = 0;
+ goto done;
+ }
+
+ /*
+ * Check freeze flag under the lock so that the flag check and
+ * virtqueue_add_outbuf() are atomic with respect to
+ * remove_port_data() which also takes outvq_lock. This
+ * guarantees that once remove_port_data() returns, no new
+ * buffers can be added before remove_vqs() tears down the vq.
+ * Pairs with smp_store_release() in virtcons_freeze/restore.
+ */
+ if (smp_load_acquire(&portdev->pm_freezing)) /* pairs with freeze/restore */
+ goto done;
+
+ out_vq = port->out_vq;
+
reclaim_consumed_buffers(port);
err = virtqueue_add_outbuf(out_vq, sg, nents, data, GFP_ATOMIC);
@@ -1110,11 +1135,36 @@ static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
struct scatterlist sg[1];
void *data;
int ret;
+ struct ports_device *portdev;
port = find_port_by_vtermno(vtermno);
if (!port)
return -EPIPE;
+ /*
+ * Silently drop output in two cases, both by returning count so
+ * that the hvc layer does not spin-retry:
+ *
+ * 1. Device hot-unplug (!portdev): portdev was NULLed by
+ * unplug_port() after hvc_remove() was already called, so
+ * the hvc layer will stop invoking put_chars() very soon.
+ * Returning count avoids a pointless retry loop in the
+ * interim.
+ *
+ * 2. PM freeze (pm_freezing): the hvc console stays active
+ * under no_console_suspend but virtqueues are being torn
+ * down. Drop the output silently so the hvc layer does not
+ * stall suspend.
+ *
+ * This early check avoids a pointless GFP_ATOMIC allocation;
+ * __send_to_port() rechecks under outvq_lock for correctness.
+ * Pairs with smp_store_release() in virtcons_freeze/restore.
+ */
+ portdev = READ_ONCE(port->portdev);
+ if (!portdev ||
+ smp_load_acquire(&portdev->pm_freezing)) /* pairs with freeze/restore */
+ return count;
+
data = kmemdup(buf, count, GFP_ATOMIC);
if (!data)
return -ENOMEM;
@@ -1972,6 +2022,7 @@ static int virtcons_probe(struct virtio_device *vdev)
/* Attach this portdev to this virtio_device, and vice-versa. */
portdev->vdev = vdev;
vdev->priv = portdev;
+ portdev->pm_freezing = false;
portdev->chr_major = register_chrdev(0, "virtio-portsdev",
&portdev_fops);
@@ -2092,6 +2143,24 @@ static int virtcons_freeze(struct virtio_device *vdev)
portdev = vdev->priv;
+ /*
+ * Block TX paths (put_chars, __send_to_port) before resetting the
+ * device and tearing down virtqueues. This prevents races with
+ * hvc console writes that remain active under no_console_suspend.
+ */
+ smp_store_release(&portdev->pm_freezing, true);
+
+ /*
+ * Synchronize with any concurrent __send_to_port() that may have
+ * passed the pm_freezing check. By acquiring and releasing the
+ * outvq_lock for each port, we ensure all active TX paths have
+ * completed before we reset the device.
+ */
+ list_for_each_entry(port, &portdev->ports, list) {
+ spin_lock_irq(&port->outvq_lock);
+ spin_unlock_irq(&port->outvq_lock);
+ }
+
virtio_reset_device(vdev);
if (use_multiport(portdev))
@@ -2153,6 +2222,13 @@ static int virtcons_restore(struct virtio_device *vdev)
if (port->guest_connected)
send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
}
+
+ /*
+ * Allow TX paths only after all port->out_vq pointers have
+ * been reassigned to the newly allocated virtqueues.
+ */
+ smp_store_release(&portdev->pm_freezing, false);
+
return 0;
}
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH net v2] vsock: keep poll shutdown state consistent
From: Ziyu Zhang @ 2026-05-19 16:56 UTC (permalink / raw)
To: Stefano Garzarella, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Simon Horman, Andy King, George Zhang, Dmitry Torokhov,
K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Stefan Hajnoczi, Michael S . Tsirkin, Jason Wang, Xuan Zhuo,
Eugenio Pérez, Bryan Tan, Vishnu Dasa,
bcm-kernel-feedback-list, virtualization, netdev, linux-kernel,
linux-hyperv, kvm, baijiaju1990, r33s3n6, gality369,
zhenghaoran154, hanguidong02, zzzccc427, Ziyu Zhang
vsock_poll() reads vsk->peer_shutdown before taking the socket lock
to set EPOLLHUP and EPOLLRDHUP, then reads it again after taking
the lock to report EOF readability. A shutdown packet can update
peer_shutdown while poll is waiting for the lock, so one poll invocation
can report EOF readability without the corresponding HUP/RDHUP bits.
For connectible sockets, take one peer_shutdown snapshot after
lock_sock() and use it for all peer-shutdown-derived poll bits. For
datagram sockets, which do not take lock_sock() in poll(), take one
lockless READ_ONCE() snapshot and pair it with WRITE_ONCE() on the
writer side.
This keeps the peer-shutdown-derived bits internally consistent for each
poll pass.
Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
---
Link: https://lore.kernel.org/netdev/20260516034745.260442-1-ziyuzhang201@gmail.com/
v2:
- Pair lockless READ_ONCE() users with WRITE_ONCE() on peer_shutdown writers.
- Move datagram shutdown handling into the SOCK_DGRAM branch and add a comment.
- Keep one connectible peer_shutdown snapshot after lock_sock() and
restore the previous shutdown-derived mask ordering.
net/vmw_vsock/af_vsock.c | 49 ++++++++++++++++---------
net/vmw_vsock/hyperv_transport.c | 9 +++--
net/vmw_vsock/virtio_transport_common.c | 14 ++++---
net/vmw_vsock/vmci_transport.c | 8 ++--
4 files changed, 52 insertions(+), 28 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index adcba1b7b..789b00f6e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -523,7 +523,7 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
*/
sock_reset_flag(sk, SOCK_DONE);
sk->sk_state = TCP_CLOSE;
- vsk->peer_shutdown = 0;
+ WRITE_ONCE(vsk->peer_shutdown, 0);
}
if (sk->sk_type == SOCK_SEQPACKET) {
@@ -814,7 +814,7 @@ static struct sock *__vsock_create(struct net *net,
vsk->rejected = false;
vsk->sent_request = false;
vsk->ignore_connecting_rst = false;
- vsk->peer_shutdown = 0;
+ WRITE_ONCE(vsk->peer_shutdown, 0);
INIT_DELAYED_WORK(&vsk->connect_work, vsock_connect_timeout);
INIT_DELAYED_WORK(&vsk->pending_work, vsock_pending_work);
@@ -1122,6 +1122,25 @@ static int vsock_shutdown(struct socket *sock, int mode)
return err;
}
+static __poll_t vsock_poll_shutdown(struct sock *sk, u32 peer_shutdown)
+{
+ __poll_t mask = 0;
+
+ /* INET sockets treat local write shutdown and peer write shutdown as a
+ * case of EPOLLHUP set.
+ */
+ if (sk->sk_shutdown == SHUTDOWN_MASK ||
+ ((sk->sk_shutdown & SEND_SHUTDOWN) &&
+ (peer_shutdown & SEND_SHUTDOWN)))
+ mask |= EPOLLHUP;
+
+ if (sk->sk_shutdown & RCV_SHUTDOWN ||
+ peer_shutdown & SEND_SHUTDOWN)
+ mask |= EPOLLRDHUP;
+
+ return mask;
+}
+
static __poll_t vsock_poll(struct file *file, struct socket *sock,
poll_table *wait)
{
@@ -1139,24 +1158,17 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
/* Signify that there has been an error on this socket. */
mask |= EPOLLERR;
- /* INET sockets treat local write shutdown and peer write shutdown as a
- * case of EPOLLHUP set.
- */
- if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
- ((sk->sk_shutdown & SEND_SHUTDOWN) &&
- (vsk->peer_shutdown & SEND_SHUTDOWN))) {
- mask |= EPOLLHUP;
- }
-
- if (sk->sk_shutdown & RCV_SHUTDOWN ||
- vsk->peer_shutdown & SEND_SHUTDOWN) {
- mask |= EPOLLRDHUP;
- }
-
if (sk_is_readable(sk))
mask |= EPOLLIN | EPOLLRDNORM;
if (sock->type == SOCK_DGRAM) {
+ u32 peer_shutdown = READ_ONCE(vsk->peer_shutdown);
+
+ /* DGRAM sockets do not take lock_sock() in poll(), so use one
+ * lockless snapshot for all shutdown-derived mask bits.
+ */
+ mask |= vsock_poll_shutdown(sk, peer_shutdown);
+
/* For datagram sockets we can read if there is something in
* the queue and write as long as the socket isn't shutdown for
* sending.
@@ -1171,6 +1183,7 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
} else if (sock_type_connectible(sk->sk_type)) {
const struct vsock_transport *transport;
+ u32 peer_shutdown;
lock_sock(sk);
@@ -1203,8 +1216,10 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
* terminated should also be considered read, and we check the
* shutdown flag for that.
*/
+ peer_shutdown = READ_ONCE(vsk->peer_shutdown);
+ mask |= vsock_poll_shutdown(sk, peer_shutdown);
if (sk->sk_shutdown & RCV_SHUTDOWN ||
- vsk->peer_shutdown & SEND_SHUTDOWN) {
+ peer_shutdown & SEND_SHUTDOWN) {
mask |= EPOLLIN | EPOLLRDNORM;
}
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 432fcbbd1..16b981566 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -264,7 +264,7 @@ static void hvs_do_close_lock_held(struct vsock_sock *vsk,
struct sock *sk = sk_vsock(vsk);
sock_set_flag(sk, SOCK_DONE);
- vsk->peer_shutdown = SHUTDOWN_MASK;
+ WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
if (vsock_stream_has_data(vsk) <= 0)
sk->sk_state = TCP_CLOSING;
sk->sk_state_change(sk);
@@ -593,7 +593,9 @@ static int hvs_update_recv_data(struct hvsock *hvs)
return -EIO;
if (payload_len == 0)
- hvs->vsk->peer_shutdown |= SEND_SHUTDOWN;
+ WRITE_ONCE(hvs->vsk->peer_shutdown,
+ READ_ONCE(hvs->vsk->peer_shutdown) |
+ SEND_SHUTDOWN);
hvs->recv_data_len = payload_len;
hvs->recv_data_off = 0;
@@ -715,7 +717,8 @@ static s64 hvs_stream_has_data(struct vsock_sock *vsk)
return ret;
return hvs->recv_data_len;
case 0:
- vsk->peer_shutdown |= SEND_SHUTDOWN;
+ WRITE_ONCE(vsk->peer_shutdown,
+ READ_ONCE(vsk->peer_shutdown) | SEND_SHUTDOWN);
ret = 0;
break;
default: /* -1 */
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index dcc8a1d58..71d8eac82 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1220,7 +1220,7 @@ static void virtio_transport_do_close(struct vsock_sock *vsk,
struct sock *sk = sk_vsock(vsk);
sock_set_flag(sk, SOCK_DONE);
- vsk->peer_shutdown = SHUTDOWN_MASK;
+ WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
if (vsock_stream_has_data(vsk) <= 0)
sk->sk_state = TCP_CLOSING;
sk->sk_state_change(sk);
@@ -1411,12 +1411,15 @@ virtio_transport_recv_connected(struct sock *sk,
case VIRTIO_VSOCK_OP_CREDIT_UPDATE:
sk->sk_write_space(sk);
break;
- case VIRTIO_VSOCK_OP_SHUTDOWN:
+ case VIRTIO_VSOCK_OP_SHUTDOWN: {
+ u32 peer_shutdown = READ_ONCE(vsk->peer_shutdown);
+
if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SHUTDOWN_RCV)
- vsk->peer_shutdown |= RCV_SHUTDOWN;
+ peer_shutdown |= RCV_SHUTDOWN;
if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SHUTDOWN_SEND)
- vsk->peer_shutdown |= SEND_SHUTDOWN;
- if (vsk->peer_shutdown == SHUTDOWN_MASK) {
+ peer_shutdown |= SEND_SHUTDOWN;
+ WRITE_ONCE(vsk->peer_shutdown, peer_shutdown);
+ if (peer_shutdown == SHUTDOWN_MASK) {
if (vsock_stream_has_data(vsk) <= 0 && !sock_flag(sk, SOCK_DONE)) {
(void)virtio_transport_reset(vsk, NULL);
virtio_transport_do_close(vsk, true);
@@ -1431,6 +1434,7 @@ virtio_transport_recv_connected(struct sock *sk,
if (le32_to_cpu(virtio_vsock_hdr(skb)->flags))
sk->sk_state_change(sk);
break;
+ }
case VIRTIO_VSOCK_OP_RST:
virtio_transport_do_close(vsk, true);
break;
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 7eccd6708..c2231c402 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -811,7 +811,7 @@ static void vmci_transport_handle_detach(struct sock *sk)
/* On a detach the peer will not be sending or receiving
* anymore.
*/
- vsk->peer_shutdown = SHUTDOWN_MASK;
+ WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
/* We should not be sending anymore since the peer won't be
* there to receive, but we can still receive if there is data
@@ -1534,7 +1534,9 @@ static int vmci_transport_recv_connected(struct sock *sk,
if (pkt->u.mode) {
vsk = vsock_sk(sk);
- vsk->peer_shutdown |= pkt->u.mode;
+ WRITE_ONCE(vsk->peer_shutdown,
+ READ_ONCE(vsk->peer_shutdown) |
+ pkt->u.mode);
sk->sk_state_change(sk);
}
break;
@@ -1551,7 +1553,7 @@ static int vmci_transport_recv_connected(struct sock *sk,
* a clean shutdown.
*/
sock_set_flag(sk, SOCK_DONE);
- vsk->peer_shutdown = SHUTDOWN_MASK;
+ WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
if (vsock_stream_has_data(vsk) <= 0)
sk->sk_state = TCP_CLOSING;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Dmitry Osipenko @ 2026-05-20 6:50 UTC (permalink / raw)
To: Christian König, Deepanshu Kartikey, airlied, kraxel,
gurchetansingh, olvaffe, maarten.lankhorst, mripard, tzimmermann,
simona, sumit.semwal
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <2e23513c-9d59-4891-acfe-9f1fbcbce778@amd.com>
On 5/19/26 11:27, Christian König wrote:
> On 5/19/26 10:22, Deepanshu Kartikey wrote:
>> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
>> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
>> ignore its return value. The function can fail with -EINTR from
>> dma_resv_lock_interruptible() (signal during lock wait) or with
>> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
>> leaving the resv lock not held. The queue path then walks the object
>> array and calls dma_resv_add_fence(), which requires the lock held;
>> with lockdep enabled this trips dma_resv_assert_held():
>>
>> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
>> Call Trace:
>> virtio_gpu_array_add_fence
>> virtio_gpu_queue_ctrl_sgs
>> virtio_gpu_queue_fenced_ctrl_buffer
>> virtio_gpu_cursor_plane_update
>> drm_atomic_helper_commit_planes
>> drm_atomic_helper_commit_tail
>> commit_tail
>> drm_atomic_helper_commit
>> drm_atomic_commit
>> drm_atomic_helper_update_plane
>> __setplane_atomic
>> drm_mode_cursor_universal
>> drm_mode_cursor_common
>> drm_mode_cursor_ioctl
>> drm_ioctl
>> __x64_sys_ioctl
>>
>> Beyond the WARN, mutating the dma_resv fence list without the lock
>> races with concurrent readers/writers and can corrupt the list.
>
> Well why are you trying to add a fence on an atomic mode set in the first place?
>
> That is usually an illegal operation here.
That is pre-existing in the driver. It performs draw operation and in
some cases waits for the completion during atomic. Whether all that
syncing is correct is hard to say immediately as some of it may be
historical edge cases.
--
Best regards,
Dmitry
^ permalink raw reply
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Christian König @ 2026-05-20 7:05 UTC (permalink / raw)
To: Dmitry Osipenko, Deepanshu Kartikey, airlied, kraxel,
gurchetansingh, olvaffe, maarten.lankhorst, mripard, tzimmermann,
simona, sumit.semwal
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <f6bcef23-5510-4aad-bf6a-4e1ecfc8d474@collabora.com>
On 5/20/26 08:50, Dmitry Osipenko wrote:
> On 5/19/26 11:27, Christian König wrote:
>> On 5/19/26 10:22, Deepanshu Kartikey wrote:
>>> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
>>> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
>>> ignore its return value. The function can fail with -EINTR from
>>> dma_resv_lock_interruptible() (signal during lock wait) or with
>>> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
>>> leaving the resv lock not held. The queue path then walks the object
>>> array and calls dma_resv_add_fence(), which requires the lock held;
>>> with lockdep enabled this trips dma_resv_assert_held():
>>>
>>> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
>>> Call Trace:
>>> virtio_gpu_array_add_fence
>>> virtio_gpu_queue_ctrl_sgs
>>> virtio_gpu_queue_fenced_ctrl_buffer
>>> virtio_gpu_cursor_plane_update
>>> drm_atomic_helper_commit_planes
>>> drm_atomic_helper_commit_tail
>>> commit_tail
>>> drm_atomic_helper_commit
>>> drm_atomic_commit
>>> drm_atomic_helper_update_plane
>>> __setplane_atomic
>>> drm_mode_cursor_universal
>>> drm_mode_cursor_common
>>> drm_mode_cursor_ioctl
>>> drm_ioctl
>>> __x64_sys_ioctl
>>>
>>> Beyond the WARN, mutating the dma_resv fence list without the lock
>>> races with concurrent readers/writers and can corrupt the list.
>>
>> Well why are you trying to add a fence on an atomic mode set in the first place?
>>
>> That is usually an illegal operation here.
> That is pre-existing in the driver. It performs draw operation and in
> some cases waits for the completion during atomic. Whether all that
> syncing is correct is hard to say immediately as some of it may be
> historical edge cases.
I'm not not so deeply in the atomic mode setting stuff but it strongly sounds like that this is seriously broken.
The background is that the atomic mode set framework allows an output dma_fence which is signaled when the commit is finished.
So when you allocate a fence slot and add a new fence to finish the atomic commit it is trivially possible that this cycles back and waits for the atomic commit to finish. In other words you have a deadlock.
You probably need specially crafted userspace with the right timing to trigger that, but such issues are usually a rather big no-no and need to be fixed in the long term.
Try to add dma_fence_begin_signaling() and dma_fence_end_signaling() annotation and enable lockdep, the tool should be able to point out if and what exactly goes wrong.
The usual fix is to prepare everything before commit_tail is called (alloc memory, create, reserve slot, add dma_fence etc....) and then just send out the prepared commands later on.
Regards,
Christian.
^ permalink raw reply
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Dmitry Osipenko @ 2026-05-20 8:12 UTC (permalink / raw)
To: Christian König, Deepanshu Kartikey, airlied, kraxel,
gurchetansingh, olvaffe, maarten.lankhorst, mripard, tzimmermann,
simona, sumit.semwal
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <a0f2cfd5-d4df-4e50-a52b-d5befbc2e481@amd.com>
On 5/20/26 10:05, Christian König wrote:
> On 5/20/26 08:50, Dmitry Osipenko wrote:
>> On 5/19/26 11:27, Christian König wrote:
>>> On 5/19/26 10:22, Deepanshu Kartikey wrote:
>>>> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
>>>> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
>>>> ignore its return value. The function can fail with -EINTR from
>>>> dma_resv_lock_interruptible() (signal during lock wait) or with
>>>> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
>>>> leaving the resv lock not held. The queue path then walks the object
>>>> array and calls dma_resv_add_fence(), which requires the lock held;
>>>> with lockdep enabled this trips dma_resv_assert_held():
>>>>
>>>> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
>>>> Call Trace:
>>>> virtio_gpu_array_add_fence
>>>> virtio_gpu_queue_ctrl_sgs
>>>> virtio_gpu_queue_fenced_ctrl_buffer
>>>> virtio_gpu_cursor_plane_update
>>>> drm_atomic_helper_commit_planes
>>>> drm_atomic_helper_commit_tail
>>>> commit_tail
>>>> drm_atomic_helper_commit
>>>> drm_atomic_commit
>>>> drm_atomic_helper_update_plane
>>>> __setplane_atomic
>>>> drm_mode_cursor_universal
>>>> drm_mode_cursor_common
>>>> drm_mode_cursor_ioctl
>>>> drm_ioctl
>>>> __x64_sys_ioctl
>>>>
>>>> Beyond the WARN, mutating the dma_resv fence list without the lock
>>>> races with concurrent readers/writers and can corrupt the list.
>>>
>>> Well why are you trying to add a fence on an atomic mode set in the first place?
>>>
>>> That is usually an illegal operation here.
>> That is pre-existing in the driver. It performs draw operation and in
>> some cases waits for the completion during atomic. Whether all that
>> syncing is correct is hard to say immediately as some of it may be
>> historical edge cases.
>
> I'm not not so deeply in the atomic mode setting stuff but it strongly sounds like that this is seriously broken.
>
> The background is that the atomic mode set framework allows an output dma_fence which is signaled when the commit is finished.
>
> So when you allocate a fence slot and add a new fence to finish the atomic commit it is trivially possible that this cycles back and waits for the atomic commit to finish. In other words you have a deadlock.
>
> You probably need specially crafted userspace with the right timing to trigger that, but such issues are usually a rather big no-no and need to be fixed in the long term.
>
> Try to add dma_fence_begin_signaling() and dma_fence_end_signaling() annotation and enable lockdep, the tool should be able to point out if and what exactly goes wrong.
>
> The usual fix is to prepare everything before commit_tail is called (alloc memory, create, reserve slot, add dma_fence etc....) and then just send out the prepared commands later on.
We tried with moving resv alloc to prepare_fb() in a previous patch
version, it resulted in a non-trivial deadlocks. The goal of this patch
is to fix immediate problem with a minimal code change.
What you're saying is correct, but it may require a rather big
refactoring of the code. In general, everything works okay today, so not
really an urgent problem.
--
Best regards,
Dmitry
^ permalink raw reply
* [PATCH] virtio_net: drop redundant err assignment in virtnet_probe()
From: Li Wang @ 2026-05-20 10:23 UTC (permalink / raw)
To: Michael S . Tsirkin, Jason Wang
Cc: Xuan Zhuo, Eugenio Pérez, netdev, virtualization,
linux-kernel, Li Wang
err is initialized to -ENOMEM at the start of virtnet_probe(), and no
code path between that and the devm_kzalloc() failure can change
err. Assigning -ENOMEM again before goto free therefore is redundant.
Signed-off-by: Li Wang <liwang@kylinos.cn>
---
drivers/net/virtio_net.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f4adcfee7a80..1fbafca90e4c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -6853,10 +6853,8 @@ static int virtnet_probe(struct virtio_device *vdev)
rss_max_indirection_table_length));
}
vi->rss_hdr = devm_kzalloc(&vdev->dev, virtnet_rss_hdr_size(vi), GFP_KERNEL);
- if (!vi->rss_hdr) {
- err = -ENOMEM;
+ if (!vi->rss_hdr)
goto free;
- }
if (vi->has_rss || vi->has_rss_hash_report) {
key_sz = virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Christian König @ 2026-05-20 11:00 UTC (permalink / raw)
To: Dmitry Osipenko, Deepanshu Kartikey, airlied, kraxel,
gurchetansingh, olvaffe, maarten.lankhorst, mripard, tzimmermann,
simona, sumit.semwal
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <f37bdc63-3575-49e5-aa5b-7b93428b293d@collabora.com>
On 5/20/26 10:12, Dmitry Osipenko wrote:
> On 5/20/26 10:05, Christian König wrote:
>> On 5/20/26 08:50, Dmitry Osipenko wrote:
>>> On 5/19/26 11:27, Christian König wrote:
>>>> On 5/19/26 10:22, Deepanshu Kartikey wrote:
>>>>> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
>>>>> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
>>>>> ignore its return value. The function can fail with -EINTR from
>>>>> dma_resv_lock_interruptible() (signal during lock wait) or with
>>>>> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
>>>>> leaving the resv lock not held. The queue path then walks the object
>>>>> array and calls dma_resv_add_fence(), which requires the lock held;
>>>>> with lockdep enabled this trips dma_resv_assert_held():
>>>>>
>>>>> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
>>>>> Call Trace:
>>>>> virtio_gpu_array_add_fence
>>>>> virtio_gpu_queue_ctrl_sgs
>>>>> virtio_gpu_queue_fenced_ctrl_buffer
>>>>> virtio_gpu_cursor_plane_update
>>>>> drm_atomic_helper_commit_planes
>>>>> drm_atomic_helper_commit_tail
>>>>> commit_tail
>>>>> drm_atomic_helper_commit
>>>>> drm_atomic_commit
>>>>> drm_atomic_helper_update_plane
>>>>> __setplane_atomic
>>>>> drm_mode_cursor_universal
>>>>> drm_mode_cursor_common
>>>>> drm_mode_cursor_ioctl
>>>>> drm_ioctl
>>>>> __x64_sys_ioctl
>>>>>
>>>>> Beyond the WARN, mutating the dma_resv fence list without the lock
>>>>> races with concurrent readers/writers and can corrupt the list.
>>>>
>>>> Well why are you trying to add a fence on an atomic mode set in the first place?
>>>>
>>>> That is usually an illegal operation here.
>>> That is pre-existing in the driver. It performs draw operation and in
>>> some cases waits for the completion during atomic. Whether all that
>>> syncing is correct is hard to say immediately as some of it may be
>>> historical edge cases.
>>
>> I'm not not so deeply in the atomic mode setting stuff but it strongly sounds like that this is seriously broken.
>>
>> The background is that the atomic mode set framework allows an output dma_fence which is signaled when the commit is finished.
>>
>> So when you allocate a fence slot and add a new fence to finish the atomic commit it is trivially possible that this cycles back and waits for the atomic commit to finish. In other words you have a deadlock.
>>
>> You probably need specially crafted userspace with the right timing to trigger that, but such issues are usually a rather big no-no and need to be fixed in the long term.
>>
>> Try to add dma_fence_begin_signaling() and dma_fence_end_signaling() annotation and enable lockdep, the tool should be able to point out if and what exactly goes wrong.
>>
>> The usual fix is to prepare everything before commit_tail is called (alloc memory, create, reserve slot, add dma_fence etc....) and then just send out the prepared commands later on.
>
> We tried with moving resv alloc to prepare_fb() in a previous patch
> version, it resulted in a non-trivial deadlocks. The goal of this patch
> is to fix immediate problem with a minimal code change.
Yeah, totally fine with me to get that fixed first.
> What you're saying is correct, but it may require a rather big
> refactoring of the code. In general, everything works okay today, so not
> really an urgent problem.
It's just a potential issue and when the AI bots keep evolving like they already do they will sooner or later start to point that out as well.
Regards,
Christian.
^ permalink raw reply
* Re: [PATCH net v4] vsock/vmci: fix UAF when peer resets connection during handshake
From: Bryan Tan @ 2026-05-20 13:51 UTC (permalink / raw)
To: Minh Nguyen
Cc: pabeni, sgarzare, vishnu.dasa, davem, edumazet, kuba, horms,
bcm-kernel-feedback-list, netdev, virtualization, linux-kernel,
stable
In-Reply-To: <20260519102310.237181-1-minhnguyen.080505@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3772 bytes --]
On Tue, May 19, 2026 at 11:23 AM Minh Nguyen
<minhnguyen.080505@gmail.com> wrote:
>
> vmci_transport_recv_connecting_server() returned err = 0 for a peer
> RST in its default switch arm:
>
> err = pkt->type == VMCI_TRANSPORT_PACKET_TYPE_RST ? 0 : -EINVAL;
>
> That made vmci_transport_recv_listen() skip vsock_remove_pending(),
> leaving the pending socket on the listener's pending_links with
> sk_state = TCP_CLOSE while destroy: still dropped the explicit
> reference taken before schedule_delayed_work().
>
> One second later vsock_pending_work() observed is_pending=true and
> performed full cleanup: vsock_remove_pending() then the two trailing
> sock_put(sk) calls -- the first reached refcount 0 and __sk_freed
> the socket, and the second wrote into the freed object:
>
> BUG: KASAN: slab-use-after-free in refcount_warn_saturate
> Write of size 4 at addr ffff88800b1cac80 by task kworker
> Workqueue: events vsock_pending_work
>
> Treat peer RST like any other unexpected packet type (err = -EINVAL).
> All destroy: arms now return err < 0, so vmci_transport_recv_listen()
> removes pending from pending_links synchronously and
> vsock_pending_work() takes the is_pending=false / !rejected branch,
> dropping only its own work reference. This also closes the
> multi-packet race Sashiko reported on v2: pending is removed from
> the list before any subsequent packet can find it.
>
> The pre-existing sk_acceptq_removed() gap on the err < 0 path of
> vmci_transport_recv_listen() that Sashiko also noted is not
> introduced or changed by this patch.
>
> Tested on lts-6.12.79 with KASAN: 52/100 unpatched -> 0/100 patched.
>
> Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
> Cc: stable@vger.kernel.org
> Signed-off-by: Minh Nguyen <minhnguyen.080505@gmail.com>
> Assisted-by: Claude:claude-opus-4-7
> ---
> v4:
> - Resend as an independent thread per netdev workflow (v3 was
> incorrectly posted in-reply-to the v2 thread).
> - Drop the inline comment expansion; keep the original
> /* Close and cleanup the connection. */. No functional change.
>
> v3:
> - Different approach to Sashiko/Paolo's "trading UAF for leak"
> concern: normalize RST to err = -EINVAL so all destroy: arms
> take the same err < 0 cleanup path -- no special case, no
> multi-packet race.
> - Sashiko's secondary observation ("while not introduced by this
> patch, does this error path leak sk_ack_backlog slots on failed
> handshakes?") is correct: the sk_acceptq_removed() gap on the
> err < 0 branch of vmci_transport_recv_listen() is pre-existing
> and is not introduced or changed by this patch. A separate fix
> for that gap is needed and would be welcome.
>
> v2: https://lore.kernel.org/netdev/20260512025851.189140-1-minhnguyen.080505@gmail.com/
>
> v1 was sent to security@kernel.org on 2026-05-10 (not on lore).
>
> net/vmw_vsock/vmci_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Bryan Tan <bryan-bt.tan@broadcom.com>
>
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 4296ca1..d257938 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1164,7 +1164,7 @@ vmci_transport_recv_connecting_server(struct sock *listener,
> /* Close and cleanup the connection. */
> vmci_transport_send_reset(pending, pkt);
> skerr = EPROTO;
> - err = pkt->type == VMCI_TRANSPORT_PACKET_TYPE_RST ? 0 : -EINVAL;
> + err = -EINVAL;
> goto destroy;
> }
>
>
> base-commit: be48e5fe51a5864566307998286a699d6b986934
> --
> 2.54.0
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5417 bytes --]
^ permalink raw reply
* Re: [PATCH net v2] vsock/vmci: fix UAF when peer resets connection during handshake
From: Bryan Tan @ 2026-05-20 13:55 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Paolo Abeni, Minh Nguyen, Vishnu Dasa, David S . Miller,
Eric Dumazet, Jakub Kicinski, Simon Horman,
bcm-kernel-feedback-list, netdev, virtualization, linux-kernel,
stable
In-Reply-To: <agwv3YkxYIC7mvyj@sgarzare-redhat>
[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]
On Tue, May 19, 2026 at 10:41 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, May 14, 2026 at 03:26:28PM +0200, Paolo Abeni wrote:
> >On 5/12/26 4:58 AM, Minh Nguyen wrote:
> >> vmci_transport_recv_connecting_server() jumps to its destroy: label
> >> and performs an unconditional sock_put(pending) to release the
> >> explicit sock_hold() taken by vmci_transport_recv_listen() before
> >> schedule_delayed_work(). The existing comment claimed this was safe
> >> because the listen handler removes pending from the pending list on
> >> the way out, which would prevent vsock_pending_work() from dropping
> >> the same reference later.
> >
>
> [...]
>
> >Sashiko says:
> >
> >---
> >Could this change lead to a socket memory leak if another packet arrives
> >before vsock_pending_work() executes?
> >If a peer RST is received (err == 0), the socket stays on the
> >pending_links list with its state set to TCP_CLOSE, and the base
> >reference is kept.
> >If the peer then sends another packet (such as another RST) within the
> >delay window before vsock_pending_work() runs,
> >vmci_transport_get_pending() might find this same socket.
> >Since its state is TCP_CLOSE, vmci_transport_recv_listen() would hit the
> >default switch case, set err = -EINVAL, and call vsock_remove_pending().
> >This removes the socket from the list and drops the list reference, but
> >it bypasses vmci_transport_recv_connecting_server(), meaning the base
> >reference is never dropped.
> >When vsock_pending_work() runs later, vsock_is_pending() evaluates to false.
> >This sets cleanup = false and bypasses the sock_put(sk) call, leaking
> >the pending socket.
> >While not introduced by this patch, does this error path leak
> >sk_ack_backlog slots on failed handshakes?
> >If a handshake fails due to an error, vmci_transport_recv_listen()
> >handles it by calling vsock_remove_pending(). This removes the socket
> >from the pending_links list but does not call sk_acceptq_removed(sk).
> >When vsock_pending_work() runs later, vsock_is_pending() evaluates to
> >false because the socket is no longer in the list. This causes the work
> >function to skip its own sk_acceptq_removed(listener) call, meaning the
> >listener's sk_ack_backlog is never decremented.
> >---
> >
> >it looks like the above is trading an UaF for a leak ?!?
> >
>
> @Minh @Bryan can you check this report?
> It seems a real issue, so the patch was not applied.
Thanks Paolo, Sashiko. We'll fix the sk_ack_backlog handling.
>
> Thanks,
> Stefano
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5417 bytes --]
^ permalink raw reply
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Dmitry Osipenko @ 2026-05-20 15:04 UTC (permalink / raw)
To: Deepanshu Kartikey, airlied, kraxel, gurchetansingh, olvaffe,
maarten.lankhorst, mripard, tzimmermann, simona, sumit.semwal,
christian.koenig
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <20260519082247.34470-1-kartikey406@gmail.com>
On 5/19/26 11:22, Deepanshu Kartikey wrote:
> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
> ignore its return value. The function can fail with -EINTR from
> dma_resv_lock_interruptible() (signal during lock wait) or with
> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
> leaving the resv lock not held. The queue path then walks the object
> array and calls dma_resv_add_fence(), which requires the lock held;
> with lockdep enabled this trips dma_resv_assert_held():
>
> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
> Call Trace:
> virtio_gpu_array_add_fence
> virtio_gpu_queue_ctrl_sgs
> virtio_gpu_queue_fenced_ctrl_buffer
> virtio_gpu_cursor_plane_update
> drm_atomic_helper_commit_planes
> drm_atomic_helper_commit_tail
> commit_tail
> drm_atomic_helper_commit
> drm_atomic_commit
> drm_atomic_helper_update_plane
> __setplane_atomic
> drm_mode_cursor_universal
> drm_mode_cursor_common
> drm_mode_cursor_ioctl
> drm_ioctl
> __x64_sys_ioctl
>
> Beyond the WARN, mutating the dma_resv fence list without the lock
> races with concurrent readers/writers and can corrupt the list.
>
> Both call sites run inside the .atomic_update plane callback, which
> DRM atomic helpers do not allow to fail (by the time it runs, the
> commit has been signed off to userspace and there is no clean
> rollback path). Moving the lock acquisition to .prepare_fb was
> rejected because the broader lock scope deadlocks against other BO
> locking paths in the same atomic commit.
>
> Introduce virtio_gpu_lock_one_resv_uninterruptible() that uses
> dma_resv_lock() instead of dma_resv_lock_interruptible(). This
> eliminates the -EINTR failure mode -- the realistic syzbot trigger
> -- without extending the lock hold across the commit. The helper
> locks a single BO and rejects nents > 1 with -EINVAL; both fix
> sites lock exactly one BO.
>
> Use it from virtio_gpu_cursor_plane_update() and
> virtio_gpu_resource_flush(); check the return value to handle the
> remaining -ENOMEM case from dma_resv_reserve_fences() by freeing
> the objs and skipping the plane update for that frame. The
> framebuffer BOs touched here are not shared with other contexts
> and lock contention is expected to be brief, so the loss of
> signal-interruptibility is acceptable.
>
> Other callers of virtio_gpu_array_lock_resv() (the ioctl paths)
> continue to use the interruptible variant.
>
> The bug was reported by syzbot, triggered via fault injection
> (fail_nth) on the DRM_IOCTL_MODE_CURSOR path, which forces the
> -ENOMEM branch in dma_resv_reserve_fences().
>
> Reported-by: syzbot+72bd3dd3a5d5f39a0271@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=72bd3dd3a5d5f39a0271
> Fixes: 5cfd31c5b3a3 ("drm/virtio: fix virtio_gpu_cursor_plane_update().")
> Cc: stable@vger.kernel.org
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
> v4: Rename the helper to virtio_gpu_lock_one_resv_uninterruptible()
> and reject objs->nents > 1 with -EINVAL. The v3 helper's
> multi-object branch used drm_gem_lock_reservations(), which is
> interruptible, contradicting the "uninterruptible" name; both
> fix sites lock a single BO so the multi-object path is dropped.
> (Dmitry Osipenko)
> v3: Drop the prepare_fb/cleanup_fb approach from v2 (it deadlocked
> against virtio_gpu_resource_flush(), which also locks the BO in
> the same atomic commit). Instead add an uninterruptible variant
> of the resv lock helper and use it in both
> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush().
> (Dmitry Osipenko)
> v2: Move resv lock acquisition from .atomic_update (which must not
> fail) to .prepare_fb (which may), per maintainer review of v1.
> The v1 approach of silently skipping the cursor update on lock
> failure violated the atomic-commit contract with userspace.
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
> drivers/gpu/drm/virtio/virtgpu_gem.c | 17 +++++++++++++++++
> drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++--
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index f17660a71a3e..2f3531950aa4 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -317,6 +317,7 @@ virtio_gpu_array_from_handles(struct drm_file *drm_file, u32 *handles, u32 nents
> void virtio_gpu_array_add_obj(struct virtio_gpu_object_array *objs,
> struct drm_gem_object *obj);
> int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs);
> +int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs);
> void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs);
> void virtio_gpu_array_add_fence(struct virtio_gpu_object_array *objs,
> struct dma_fence *fence);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index f22dc5c21cd4..435d37d36034 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -238,6 +238,23 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
> return ret;
> }
>
> +int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs)
> +{
> + int ret;
> +
> + if (objs->nents != 1)
> + return -EINVAL;
> +
> + dma_resv_lock(objs->objs[0]->resv, NULL);
> +
> + ret = dma_resv_reserve_fences(objs->objs[0]->resv, 1);
> + if (ret) {
> + virtio_gpu_array_unlock_resv(objs);
> + return ret;
> + }
> + return 0;
> +}
> +
> void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs)
> {
> if (objs->nents == 1) {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index a126d1b25f46..652352424744 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -215,7 +215,10 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
> if (!objs)
> return;
> virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
> - virtio_gpu_array_lock_resv(objs);
> + if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
> + virtio_gpu_array_put_free(objs);
> + return;
> + }
> virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
> width, height, objs,
> vgplane_st->fence);
> @@ -459,7 +462,10 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
> if (!objs)
> return;
> virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
> - virtio_gpu_array_lock_resv(objs);
> + if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
> + virtio_gpu_array_put_free(objs);
> + return;
> + }
> virtio_gpu_cmd_transfer_to_host_2d
> (vgdev, 0,
> plane->state->crtc_w,
Applied to misc-next, thanks
--
Best regards,
Dmitry
^ permalink raw reply
* Re: [PATCH v2 0/3] drm/virtio: introduce F_BLOB_ALIGNMENT support
From: Dmitry Osipenko @ 2026-05-20 15:05 UTC (permalink / raw)
To: Sergio Lopez, Chia-I Wu, Jason Wang, Michael S. Tsirkin,
Eugenio Pérez, Xuan Zhuo, linux-kernel, Simona Vetter,
Thomas Zimmermann, David Airlie, Gurchetan Singh, Gerd Hoffmann,
virtualization, dri-devel, Maxime Ripard, Maarten Lankhorst
In-Reply-To: <20260428194450.518296-1-slp@redhat.com>
On 4/28/26 22:44, Sergio Lopez wrote:
> There's an increasing number of machines supporting multiple page sizes
> and on these machines the host and a guest can be running, each one,
> with a different page size.
>
> For what pertains to virtio-gpu, this is not a problem if the page size
> of the guest happens to be bigger or equal than the host, but will
> potentially lead to failures in memory allocations and/or mappings
> otherwise.
>
> To deal with this, the virtio-spec was extended to introduce with the
> VIRTIO_GPU_F_BLOB_ALIGNMENT feature [1]. If this feature is negotiated,
> we must use the "blob_alignment" field in the config to ensure every
> CREATE_BLOB and MAP_BLOB is properly aligned before sending it to the
> device.
>
> We also introduce the VIRTGPU_PARAM_BLOB_ALIGNMENT parameter to allow
> userspace to query the alignment restrictions for blobs.
>
> This supersedes "drm/virtio: introduce the HOST_PAGE_SIZE feature" [2].
>
> [1] https://github.com/oasis-tcs/virtio-spec/commit/f9abfd55cb663837dda1153b826216dcf4d25b84
> [2] https://lkml.org/lkml/2024/7/23/438
>
> Changes in v2:
> - Rebased.
> - Moved blob size alignment validation to verify_blob(), rejecting
> misaligned sizes early in the ioctl path instead of checking in
> virtio_gpu_cmd_resource_create_blob() and virtio_gpu_cmd_map()
> (Dmitry Osipenko).
>
> Sergio Lopez (3):
> drm/virtio: support VIRTIO_GPU_F_BLOB_ALIGNMENT
> drm/virtio: honor blob_alignment requirements
> drm/virtio: add VIRTGPU_PARAM_BLOB_ALIGNMENT to params
>
> drivers/gpu/drm/virtio/virtgpu_drv.c | 1 +
> drivers/gpu/drm/virtio/virtgpu_drv.h | 2 ++
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 10 ++++++++++
> drivers/gpu/drm/virtio/virtgpu_kms.c | 14 +++++++++++---
> include/uapi/drm/virtgpu_drm.h | 1 +
> include/uapi/linux/virtio_gpu.h | 9 +++++++++
> 6 files changed, 34 insertions(+), 3 deletions(-)
>
Applied to misc-next, thanks
--
Best regards,
Dmitry
^ permalink raw reply
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Dmitry Osipenko @ 2026-05-20 15:18 UTC (permalink / raw)
To: Deepanshu Kartikey, airlied, kraxel, gurchetansingh, olvaffe,
maarten.lankhorst, mripard, tzimmermann, simona, sumit.semwal,
christian.koenig
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <43ecdd2f-5faf-432a-a814-77190b3ef239@collabora.com>
On 5/20/26 18:04, Dmitry Osipenko wrote:
> On 5/19/26 11:22, Deepanshu Kartikey wrote:
>> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
>> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
>> ignore its return value. The function can fail with -EINTR from
>> dma_resv_lock_interruptible() (signal during lock wait) or with
>> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
>> leaving the resv lock not held. The queue path then walks the object
>> array and calls dma_resv_add_fence(), which requires the lock held;
>> with lockdep enabled this trips dma_resv_assert_held():
>>
>> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
>> Call Trace:
>> virtio_gpu_array_add_fence
>> virtio_gpu_queue_ctrl_sgs
>> virtio_gpu_queue_fenced_ctrl_buffer
>> virtio_gpu_cursor_plane_update
>> drm_atomic_helper_commit_planes
>> drm_atomic_helper_commit_tail
>> commit_tail
>> drm_atomic_helper_commit
>> drm_atomic_commit
>> drm_atomic_helper_update_plane
>> __setplane_atomic
>> drm_mode_cursor_universal
>> drm_mode_cursor_common
>> drm_mode_cursor_ioctl
>> drm_ioctl
>> __x64_sys_ioctl
>>
>> Beyond the WARN, mutating the dma_resv fence list without the lock
>> races with concurrent readers/writers and can corrupt the list.
>>
>> Both call sites run inside the .atomic_update plane callback, which
>> DRM atomic helpers do not allow to fail (by the time it runs, the
>> commit has been signed off to userspace and there is no clean
>> rollback path). Moving the lock acquisition to .prepare_fb was
>> rejected because the broader lock scope deadlocks against other BO
>> locking paths in the same atomic commit.
>>
>> Introduce virtio_gpu_lock_one_resv_uninterruptible() that uses
>> dma_resv_lock() instead of dma_resv_lock_interruptible(). This
>> eliminates the -EINTR failure mode -- the realistic syzbot trigger
>> -- without extending the lock hold across the commit. The helper
>> locks a single BO and rejects nents > 1 with -EINVAL; both fix
>> sites lock exactly one BO.
>>
>> Use it from virtio_gpu_cursor_plane_update() and
>> virtio_gpu_resource_flush(); check the return value to handle the
>> remaining -ENOMEM case from dma_resv_reserve_fences() by freeing
>> the objs and skipping the plane update for that frame. The
>> framebuffer BOs touched here are not shared with other contexts
>> and lock contention is expected to be brief, so the loss of
>> signal-interruptibility is acceptable.
>>
>> Other callers of virtio_gpu_array_lock_resv() (the ioctl paths)
>> continue to use the interruptible variant.
>>
>> The bug was reported by syzbot, triggered via fault injection
>> (fail_nth) on the DRM_IOCTL_MODE_CURSOR path, which forces the
>> -ENOMEM branch in dma_resv_reserve_fences().
>>
>> Reported-by: syzbot+72bd3dd3a5d5f39a0271@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=72bd3dd3a5d5f39a0271
>> Fixes: 5cfd31c5b3a3 ("drm/virtio: fix virtio_gpu_cursor_plane_update().")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
>> ---
>> v4: Rename the helper to virtio_gpu_lock_one_resv_uninterruptible()
>> and reject objs->nents > 1 with -EINVAL. The v3 helper's
>> multi-object branch used drm_gem_lock_reservations(), which is
>> interruptible, contradicting the "uninterruptible" name; both
>> fix sites lock a single BO so the multi-object path is dropped.
>> (Dmitry Osipenko)
>> v3: Drop the prepare_fb/cleanup_fb approach from v2 (it deadlocked
>> against virtio_gpu_resource_flush(), which also locks the BO in
>> the same atomic commit). Instead add an uninterruptible variant
>> of the resv lock helper and use it in both
>> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush().
>> (Dmitry Osipenko)
>> v2: Move resv lock acquisition from .atomic_update (which must not
>> fail) to .prepare_fb (which may), per maintainer review of v1.
>> The v1 approach of silently skipping the cursor update on lock
>> failure violated the atomic-commit contract with userspace.
>> ---
>> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
>> drivers/gpu/drm/virtio/virtgpu_gem.c | 17 +++++++++++++++++
>> drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++--
>> 3 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>> index f17660a71a3e..2f3531950aa4 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>> @@ -317,6 +317,7 @@ virtio_gpu_array_from_handles(struct drm_file *drm_file, u32 *handles, u32 nents
>> void virtio_gpu_array_add_obj(struct virtio_gpu_object_array *objs,
>> struct drm_gem_object *obj);
>> int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs);
>> +int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs);
>> void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs);
>> void virtio_gpu_array_add_fence(struct virtio_gpu_object_array *objs,
>> struct dma_fence *fence);
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
>> index f22dc5c21cd4..435d37d36034 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
>> @@ -238,6 +238,23 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
>> return ret;
>> }
>>
>> +int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs)
>> +{
>> + int ret;
>> +
>> + if (objs->nents != 1)
>> + return -EINVAL;
>> +
>> + dma_resv_lock(objs->objs[0]->resv, NULL);
>> +
>> + ret = dma_resv_reserve_fences(objs->objs[0]->resv, 1);
>> + if (ret) {
>> + virtio_gpu_array_unlock_resv(objs);
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs)
>> {
>> if (objs->nents == 1) {
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> index a126d1b25f46..652352424744 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> @@ -215,7 +215,10 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
>> if (!objs)
>> return;
>> virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
>> - virtio_gpu_array_lock_resv(objs);
>> + if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
>> + virtio_gpu_array_put_free(objs);
>> + return;
>> + }
>> virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
>> width, height, objs,
>> vgplane_st->fence);
>> @@ -459,7 +462,10 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
>> if (!objs)
>> return;
>> virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
>> - virtio_gpu_array_lock_resv(objs);
>> + if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
>> + virtio_gpu_array_put_free(objs);
>> + return;
>> + }
>> virtio_gpu_cmd_transfer_to_host_2d
>> (vgdev, 0,
>> plane->state->crtc_w,
>
> Applied to misc-next, thanks
Realized patche should go to -fixes, applied to misc-fixes too
--
Best regards,
Dmitry
^ permalink raw reply
* Re: [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines
From: Sean Christopherson @ 2026-05-20 17:56 UTC (permalink / raw)
To: David Woodhouse
Cc: tglx@kernel.org, longli@microsoft.com, luto@kernel.org,
alexey.makhalov@broadcom.com, jstultz@google.com,
dave.hansen@linux.intel.com, ajay.kaher@broadcom.com,
jan.kiszka@siemens.com, haiyangz@microsoft.com, kas@kernel.org,
pbonzini@redhat.com, kys@microsoft.com, decui@microsoft.com,
daniel.lezcano@kernel.org, wei.liu@kernel.org,
peterz@infradead.org, jgross@suse.com, boris.ostrovsky@oracle.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org,
mhklinux@outlook.com, thomas.lendacky@amd.com,
linux-kernel@vger.kernel.org,
bcm-kernel-feedback-list@broadcom.com, tglx@linutronix.de,
nikunj@amd.com, xen-devel@lists.xenproject.org,
linux-hyperv@vger.kernel.org, vkuznets@redhat.com,
rick.p.edgecombe@intel.com, virtualization@lists.linux.dev,
sboyd@kernel.org, x86@kernel.org
In-Reply-To: <949e39aec749f019b18fa41c2a42bcc9231288b9.camel@amazon.co.uk>
On Mon, May 18, 2026, David Woodhouse wrote:
> On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> >
> > --- a/arch/x86/xen/time.c
> > +++ b/arch/x86/xen/time.c
> > @@ -569,7 +569,7 @@ static void __init xen_init_time_common(void)
> > static_call_update(pv_steal_clock, xen_steal_clock);
> > paravirt_set_sched_clock(xen_sched_clock);
> >
> > - x86_platform.calibrate_tsc = xen_tsc_khz;
> > + tsc_register_calibration_routines(xen_tsc_khz, NULL);
> > x86_platform.get_wallclock = xen_get_wallclock;
> > }
> >
>
> xen_tsc_khz() doesn't use CPUID but really *should*.
>
> Care to pull in
> https://lore.kernel.org/all/20260509224824.3264567-31-dwmw2@infradead.org/
> to your next round please?
>
> (Without the misplaced changes in kvm/x86.c that should have been in
> two different prior commits, and are now folded into those correctly in
> my kvmclock5 branch ready for the next posting of that).
Ya, will do. What's one more patch...
^ permalink raw reply
* Re: [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC
From: Sean Christopherson @ 2026-05-20 17:59 UTC (permalink / raw)
To: David Woodhouse
Cc: Kiryl Shutsemau, Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Juergen Gross, Daniel Lezcano, Thomas Gleixner, John Stultz,
Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <7260682b21c28d1299e58400b9a2f4b8d23bd434.camel@infradead.org>
On Tue, May 19, 2026, David Woodhouse wrote:
> On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> > Dave/Thomas/Peter/Boris, what's the going rate for bribes to take something
> > like this through the tip tree?
> >
> > The bulk of the changes are in kvmclock and TSC, but pretty much every
> > hypervisor's guest-side code gets touched at some point. I am reaonsably
> > confident in the correctness of the KVM changes. Michael tested Hyper-V in
> > v2, and while there were conflicts when rebasing, they were largely
> > superficial (and I've just jinxed myself). For all other hypervisors, assume
> > the code is compile-tested only, but those changes are all quite small and
> > straightforward.
> >
> > The only changes that are questionable/contentious are the last two patches,
> > which have KVM-as-a-guest use CPUID 0x16 to get the CPU frequency, even on
> > AMD (that's the dubious part). I very deliberately put them last, so that
> > they can be dropped at will (I don't care terribly if those patches land).
> > To merge them, I would want explicit Acks from Paolo and David W.
> >
> > So, except for the last two patches, to get the stuff I really care about
> > landed, I think/hope it's just the TSC and guest-side CoCo changes that need
> > reviews/acks?
> >
> > The primary goal of this series is (or at least was, when I started) to
> > fix flaws with SNP and TDX guests where a PV clock provided by the untrusted
> > hypervisor is used instead of the secure/trusted TSC that is controlled by
> > trusted firmware.
> >
> > The secondary goal is to draft off of the SNP and TDX changes to slightly
> > modernize running under KVM. Currently, KVM guests will use TSC for
> > clocksource, but not sched_clock. And they ignore Intel's CPUID-based TSC
> > and CPU frequency enumeration, even when using the TSC instead of kvmclock.
> > And if the host provides the core crystal frequency in CPUID.0x15, then KVM
> > guests can use that for the APIC timer period instead of manually calibrating
> > the frequency.
> >
> > The tertiary goal is to clean up all of the PV clock code to deduplicate logic
> > across hypervisors, and to hopefully make it all easier to maintain going
> > forward.
>
> I booted this in qemu with -cpu host,+invtsc,+vmware-cpuid-freq
>
> I was expecting to see it eschew the kvmclock and use *only* the TSC.
> Is there even any need for 'tsc-early' given that it's *told* the TSC
> frequency in CPUID? Shouldn't it have detected that the TSC is known
> before init_tsc_clocksource() runs?
>
> And then it even spent some time at boot actually using the kvmclock as
> clocksource... when ideally I don't think it would even have *enabled*
> it at all?
Yeah, that's definitely the ideal state. And I had all the same expectations and
observations as you when digging in and testing this. But unless this series
makes things worse, I want punt on achieving the ideal state for the moment, as
it's proving to be a big lift just to get to a not-awful state.
> [ 0.000000] clocksource: kvm-clock: mask: 0xffffffffffffffff max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns
> [ 0.000000] tsc: Detected 2400.000 MHz processor
> [ 0.008205] TSC deadline timer available
> [ 0.008270] clocksource: refined-jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1910969940391419 ns
> [ 0.159085] clocksource: hpet: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604467 ns
> [ 0.164074] clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles: 0x22983777dd9, max_idle_ns: 440795300422 ns
> [ 0.229087] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275000 ns
> [ 0.337095] clocksource: Switched to clocksource kvm-clock
> [ 0.345246] clocksource: acpi_pm: mask: 0xffffff max_cycles: 0xffffff, max_idle_ns: 2085701024 ns
> [ 0.356201] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x22983777dd9, max_idle_ns: 440795300422 ns
> [ 0.360560] clocksource: Switched to clocksource tsc
>
^ permalink raw reply
* Re: [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC
From: David Woodhouse @ 2026-05-20 18:30 UTC (permalink / raw)
To: Sean Christopherson
Cc: Kiryl Shutsemau, Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Juergen Gross, Daniel Lezcano, Thomas Gleixner, John Stultz,
Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <ag32mwLvHpWn2vCt@google.com>
[-- Attachment #1: Type: text/plain, Size: 648 bytes --]
On Wed, 2026-05-20 at 10:59 -0700, Sean Christopherson wrote:
>
> > And then it even spent some time at boot actually using the kvmclock as
> > clocksource... when ideally I don't think it would even have *enabled*
> > it at all?
>
> Yeah, that's definitely the ideal state. And I had all the same expectations and
> observations as you when digging in and testing this. But unless this series
> makes things worse, I want punt on achieving the ideal state for the moment, as
> it's proving to be a big lift just to get to a not-awful state.
Ack. Just checking my understanding was correct. Baby steps... 40 patches at a time :)
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 40/41] x86/tsc: Add standalone helper for getting CPU frequency from CPUID
From: David Woodhouse @ 2026-05-20 18:50 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Kiryl Shutsemau,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <3cb683b4-973a-4b3e-a5d5-a8baa8a70eb0@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 907 bytes --]
On Sat, 2026-05-16 at 09:42 +0200, Paolo Bonzini wrote:
> On 5/15/26 21:19, Sean Christopherson wrote:
> > Extract the guts of cpu_khz_from_cpuid() to a standalone helper that
> > doesn't restrict the usage to Intel CPUs. This will allow sharing the
> > core logic with kvmclock, as (a) CPUID.0x16 may be enumerated alongside
> > kvmclock, and (b) KVM generally doesn't restrict CPUID based on vendor.
>
> Even for native there's no real reason to restrict to Intel, I think.
> native_calibrate_tsc() only limits itself because historically (prior to
> commit 604dc9170f24, "x86/tsc: Use CPUID.0x16 to calculate missing
> crystal frequency", 2019-05-09) it used a hardcoded table of crystal
> frequencies.
>
> Of course paranoia applies, but for virtualization, if the leaf exists
> there is no reason not to trust it.
Agreed.
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 41/41] x86/kvmclock: Get CPU base frequency from CPUID when it's available
From: David Woodhouse @ 2026-05-20 18:52 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-42-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> If CPUID.0x16 is present and valid, use the CPU frequency provided by
> CPUID instead of assuming that the virtual CPU runs at the same
> frequency as TSC and/or kvmclock. Back before constant TSCs were a
> thing, treating the TSC and CPU frequencies as one and the same was
> somewhat reasonable, but now it's nonsensical, especially if the
> hypervisor explicitly enumerates the CPU frequency.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kernel/kvmclock.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 62c8ea2e6769..7607920ae386 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -190,6 +190,20 @@ void kvmclock_cpu_action(enum kvm_guest_cpu_action action)
> }
> }
>
> +static unsigned long kvm_get_cpu_khz(void)
> +{
> + unsigned int cpu_khz;
> +
> + /*
> + * Prefer CPUID over kvmclock when possible, as the base CPU frequency
> + * isn't necessarily the same as the kvmlock "TSC" frequency.
> + */
> + if (!cpuid_get_cpu_freq(&cpu_khz))
> + return cpu_khz;
> +
> + return pvclock_tsc_khz(this_cpu_pvti());
I'm fine with this in principle but shouldn't the fallback be calling
kvm_get_tsc_khz() instead of directly calling pvclock_tsc_khz()?
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 01/41] x86/tsc: Add a standalone helpers for getting TSC info from CPUID.0x15
From: David Woodhouse @ 2026-05-20 18:59 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-2-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 755 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Extract retrieval of TSC frequency information from CPUID into standalone
> helpers so that TDX guest support can reuse the logic. Provide a version
> that includes the multiplier math as TDX does NOT want to use
> native_calibrate_tsc()'s fallback logic that derives the TSC frequency
> based on CPUID.0x16, when the core crystal frequency isn't known.
>
> Opportunsitically drop native_calibrate_tsc()'s "== 0" and "!= 0" checks
> in favor of the kernel's preferred style.
"Opportunistically" ? Now that looks wrong too...
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines
From: David Woodhouse @ 2026-05-20 19:04 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-3-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Add a helper to register non-native, i.e. PV and CoCo, CPU and TSC
> frequency calibration routines. This will allow consolidating handling
> of common TSC properties that are forced by hypervisor (PV routines),
> and will also allow adding sanity checks to guard against overriding a
> TSC calibration routine with a routine that is less robust/trusted.
>
> Make the CPU calibration routine optional, as Xen (very sanely) doesn't
> assume the CPU runs as the same frequency as the TSC.
>
> Wrap the helper in an #ifdef to document that the kernel overrides
> the native routines when running as a VM, and to guard against unwanted
> usage. Add a TODO to call out that AMD_MEM_ENCRYPT is a mess and doesn't
> depend on HYPERVISOR_GUEST because it gates both guest and host code.
>
> No functional change intended.
>
> Reviewed-by: Michael Kelley <mhklinux@outlook.com>
> Tested-by: Michael Kelley <mhklinux@outlook.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Mildly concerned that we might want to support multiple options — does
it have CPUID 0x15? Does it have 0x40000x10? Does it have a pvclock?
There are various permutations of those which are perhaps best handled
by *trying* each one, in some order, and populating a struct with the
answers?
But on the basis that perfect is the enemy of good,
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index b5991d53fc0e..e9e7394140dd 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -321,8 +321,8 @@ void __init kvmclock_init(void)
> flags = pvclock_read_flags(&hv_clock_boot[0].pvti);
> kvm_sched_clock_init(flags & PVCLOCK_TSC_STABLE_BIT);
>
> - x86_platform.calibrate_tsc = kvm_get_tsc_khz;
> - x86_platform.calibrate_cpu = kvm_get_tsc_khz;
> + tsc_register_calibration_routines(kvm_get_tsc_khz, kvm_get_tsc_khz);
> +
> x86_platform.get_wallclock = kvm_get_wallclock;
> x86_platform.set_wallclock = kvm_set_wallclock;
> #ifdef CONFIG_X86_LOCAL_APIC
Can we move those (and maybe everything in the context there too) up
*before* the check for no-kvmclock at the top of the function? Probably
in a separate patch.
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 41/41] x86/kvmclock: Get CPU base frequency from CPUID when it's available
From: Sean Christopherson @ 2026-05-20 19:06 UTC (permalink / raw)
To: David Woodhouse
Cc: Kiryl Shutsemau, Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Juergen Gross, Daniel Lezcano, Thomas Gleixner, John Stultz,
Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <0a3aa07a1d3c4bec2b89f8026093969155b73caa.camel@infradead.org>
On Wed, May 20, 2026, David Woodhouse wrote:
> On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> > If CPUID.0x16 is present and valid, use the CPU frequency provided by
> > CPUID instead of assuming that the virtual CPU runs at the same
> > frequency as TSC and/or kvmclock. Back before constant TSCs were a
> > thing, treating the TSC and CPU frequencies as one and the same was
> > somewhat reasonable, but now it's nonsensical, especially if the
> > hypervisor explicitly enumerates the CPU frequency.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > arch/x86/kernel/kvmclock.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index 62c8ea2e6769..7607920ae386 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> > @@ -190,6 +190,20 @@ void kvmclock_cpu_action(enum kvm_guest_cpu_action action)
> > }
> > }
> >
> > +static unsigned long kvm_get_cpu_khz(void)
> > +{
> > + unsigned int cpu_khz;
> > +
> > + /*
> > + * Prefer CPUID over kvmclock when possible, as the base CPU frequency
> > + * isn't necessarily the same as the kvmlock "TSC" frequency.
> > + */
> > + if (!cpuid_get_cpu_freq(&cpu_khz))
> > + return cpu_khz;
> > +
> > + return pvclock_tsc_khz(this_cpu_pvti());
>
> I'm fine with this in principle but shouldn't the fallback be calling
> kvm_get_tsc_khz() instead of directly calling pvclock_tsc_khz()?
Oh, yeah, for this patch, definitely yes, so that there's no side effects. The
question really should be answered in the context of "x86/kvmclock: Obtain TSC
frequency from CPUID if present", which subtly impacts the CPU frequency, but I
think the answer is "yes" there as well.
^ permalink raw reply
* Re: [PATCH v3 03/41] x86/sev: Mark TSC as reliable when configuring Secure TSC
From: David Woodhouse @ 2026-05-20 19:06 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-4-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 653 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Move the code to mark the TSC as reliable from sme_early_init() to
> snp_secure_tsc_init(). The only reader of TSC_RELIABLE is the aptly
> named check_system_tsc_reliable(), which runs in tsc_init(), i.e.
> after snp_secure_tsc_init().
>
> This will allow consolidating the handling of TSC_KNOWN_FREQ and
> TSC_RELIABLE when overriding the TSC calibration routine.
>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 04/41] x86/sev: Move check for SNP Secure TSC support to tsc_early_init()
From: David Woodhouse @ 2026-05-20 19:16 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-5-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 572 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Move the check on having a Secure TSC to the common tsc_early_init() so
> that it's obvious that having a Secure TSC is conditional, and to prepare
> for adding TDX to the mix (blindly initializing *both* SNP and TDX TSC
> logic looks especially weird).
>
> No functional change intended.
>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 05/41] x86/tdx: Override PV calibration routines with CPUID-based calibration
From: David Woodhouse @ 2026-05-20 19:52 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-6-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> When running as a TDX guest, explicitly override the TSC frequency
> calibration routine with CPUID-based calibration instead of potentially
> relying on a hypervisor-controlled PV routine. For TDX guests, CPUID.0x15
> is always emulated by the TDX-Module, i.e. the information from CPUID is
> more trustworthy than the information provided by the hypervisor.
>
> To maintain backwards compatibility with TDX guest kernels that use native
> calibration, and because it's the least awful option, retain
> native_calibrate_tsc()'s stuffing of the local APIC bus period using the
> core crystal frequency. While it's entirely possible for the hypervisor
> to emulate the APIC timer at a different frequency than the core crystal
> frequency, the commonly accepted interpretation of Intel's SDM is that APIC
> timer runs at the core crystal frequency when that latter is enumerated via
> CPUID:
>
> The APIC timer frequency will be the processor’s bus clock or core
> crystal clock frequency (when TSC/core crystal clock ratio is enumerated
> in CPUID leaf 0x15).
>
> If the hypervisor is malicious and deliberately runs the APIC timer at the
> wrong frequency, nothing would stop the hypervisor from modifying the
> frequency at any time, i.e. attempting to manually calibrate the frequency
> out of paranoia would be futile.
>
> Deliberately leave the CPU frequency calibration routine as is, since the
> TDX-Module doesn't provide any guarantees with respect to CPUID.0x16.
>
> Opportunistically add a comment explaining that CoCo TSC initialization
> needs to come after hypervisor specific initialization.
>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
I don't much like stuffing the lapic_timer_period... but I'll give you
'least awful option'. For now.
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 06/41] x86/acrn: Mark TSC frequency as known when using ACRN for calibration
From: David Woodhouse @ 2026-05-20 20:01 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-7-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Mark the TSC frequency as known when using ACRN's PV CPUID information.
> Per commit 81a71f51b89e ("x86/acrn: Set up timekeeping") and common sense,
> the TSC freq is explicitly provided by the hypervisor.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kernel/cpu/acrn.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
> index c1506cb87d8c..2da3de4d470e 100644
> --- a/arch/x86/kernel/cpu/acrn.c
> +++ b/arch/x86/kernel/cpu/acrn.c
> @@ -29,6 +29,7 @@ static void __init acrn_init_platform(void)
> /* Install system interrupt handler for ACRN hypervisor callback */
> sysvec_install(HYPERVISOR_CALLBACK_VECTOR, sysvec_acrn_hv_callback);
>
> + setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> tsc_register_calibration_routines(acrn_get_tsc_khz,
> acrn_get_tsc_khz);
I'd feel slightly happier doing that from within acrn_get_tsc_khz()
itself.... which I note is 'static inline'. I'm vaguely surprised that
even builds (although it does). Probably nicer to move it explicitly
out of line in acrn.c though.
Most of that should be a comment on patch 2 of the series, I guess?
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines
From: Sean Christopherson @ 2026-05-20 20:44 UTC (permalink / raw)
To: David Woodhouse
Cc: Kiryl Shutsemau, Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Juergen Gross, Daniel Lezcano, Thomas Gleixner, John Stultz,
Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <44e0d60548d317fd59895f18bd17220dfb2f834b.camel@infradead.org>
On Wed, May 20, 2026, David Woodhouse wrote:
> On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index b5991d53fc0e..e9e7394140dd 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> > @@ -321,8 +321,8 @@ void __init kvmclock_init(void)
> > flags = pvclock_read_flags(&hv_clock_boot[0].pvti);
> > kvm_sched_clock_init(flags & PVCLOCK_TSC_STABLE_BIT);
> >
> > - x86_platform.calibrate_tsc = kvm_get_tsc_khz;
> > - x86_platform.calibrate_cpu = kvm_get_tsc_khz;
> > + tsc_register_calibration_routines(kvm_get_tsc_khz, kvm_get_tsc_khz);
> > +
> > x86_platform.get_wallclock = kvm_get_wallclock;
> > x86_platform.set_wallclock = kvm_set_wallclock;
> > #ifdef CONFIG_X86_LOCAL_APIC
>
> Can we move those (and maybe everything in the context there too) up
> *before* the check for no-kvmclock at the top of the function?
Oof, I was going to say "no", but disabling kvmclock is exactly the workaround
I've told people to use to get the kernel to use the TSC instead of kvmclock.
> Probably in a separate patch.
Ya. I think this?
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 08ee4bc304c8..92a1ebf31e4d 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -27,6 +27,7 @@ static int kvmclock_vsyscall __initdata = 1;
static int msr_kvm_system_time __ro_after_init;
static int msr_kvm_wall_clock __ro_after_init;
static u64 kvm_sched_clock_offset __ro_after_init;
+static unsigned int kvm_tsc_khz_cpuid __ro_after_init;
static int __init parse_no_kvmclock(char *arg)
{
@@ -207,7 +208,7 @@ static unsigned long kvm_get_tsc_khz(void)
lapic_timer_period = apic_khz * 1000 / HZ;
#endif
- return kvm_para_tsc_khz() ? : pvclock_tsc_khz(this_cpu_pvti());
+ return kvm_tsc_khz_cpuid ? : pvclock_tsc_khz(this_cpu_pvti());
}
static unsigned long kvm_get_cpu_khz(void)
@@ -387,9 +388,39 @@ void __init kvmclock_init(void)
enum tsc_properties tsc_properties = TSC_FREQUENCY_KNOWN;
bool stable = false;
- if (!kvm_para_available() || !kvmclock)
+ if (!kvm_para_available())
return;
+ /*
+ * If the TSC counts at a constant frequency across P/T states, counts
+ * in deep C-states, and the TSC hasn't been marked unstable, treat the
+ * TSC reliable, as guaranteed by KVM. Note, the TSC unstable check
+ * exists purely to honor the TSC being marked unstable via command
+ * line, any runtime detection of an unstable will happen after this.
+ */
+ if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
+ boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
+ !check_tsc_unstable())
+ tsc_properties = TSC_FREQ_KNOWN_AND_RELIABLE;
+
+ kvm_tsc_khz_cpuid = kvm_para_tsc_khz();
+
+ /*
+ * If provided, use the TSC (and APIC bus) frequency provided in KVM's
+ * PV CPUID leaf even if kvmclock itself is disabled via command line.
+ * The PV CPUID information isn't dependent on kvmclock in any way, and
+ * in fact using the precise information is *more* important when the
+ * user has explicitly disabled kvmclock to force the kernel to use the
+ * TSC as its clocksource.
+ */
+ if (!kvmclock) {
+ if (kvm_tsc_khz_cpuid)
+ tsc_register_calibration_routines(kvm_get_tsc_khz,
+ kvm_get_cpu_khz,
+ tsc_properties);
+ return;
+ }
+
if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE2)) {
msr_kvm_system_time = MSR_KVM_SYSTEM_TIME_NEW;
msr_kvm_wall_clock = MSR_KVM_WALL_CLOCK_NEW;
@@ -424,21 +455,14 @@ void __init kvmclock_init(void)
}
/*
- * If the TSC counts at a constant frequency across P/T states, counts
- * in deep C-states, and the TSC hasn't been marked unstable, prefer
- * the TSC over kvmclock for sched_clock and drop kvmclock's rating so
- * that TSC is chosen as the clocksource. Note, the TSC unstable check
- * exists purely to honor the TSC being marked unstable via command
- * line, any runtime detection of an unstable will happen after this.
+ * If the TSC is reliable (see above), prefer the TSC over kvmclock for
+ * sched_clock and drop kvmclock's rating so that TSC is chosen as the
+ * clocksource.
*/
- if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
- boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
- !check_tsc_unstable()) {
+ if (tsc_properties & TSC_RELIABLE)
kvm_clock.rating = 299;
- tsc_properties = TSC_FREQ_KNOWN_AND_RELIABLE;
- } else {
+ else
kvm_sched_clock_init(stable);
- }
tsc_register_calibration_routines(kvm_get_tsc_khz, kvm_get_cpu_khz,
tsc_properties);
^ permalink raw reply related
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