* Re: [PATCH v3 1/2] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared
[not found] ` <20260810134018.143973-2-physicalmtea@gmail.com>
@ 2026-08-13 9:39 ` Stefano Garzarella
0 siblings, 0 replies; 2+ messages in thread
From: Stefano Garzarella @ 2026-08-13 9:39 UTC (permalink / raw)
To: Jia Jia
Cc: stefanha, mst, jasowangio, eperezma, kvm, virtualization, netdev,
linux-kernel
On Mon, Aug 10, 2026 at 09:40:17PM +0800, Jia Jia wrote:
>vhost_vsock_set_features() leaves the device IOTLB attached when
>userspace clears VIRTIO_F_ACCESS_PLATFORM. Descriptors can therefore
>continue to use translations installed before the feature change,
>including HVAs made stale by a later memory table update.
>
>Detach the device IOTLB before acknowledging a feature mask without
>ACCESS_PLATFORM. Serialize each virtqueue handoff with its own mutex
>while clearing its IOTLB pointer, resetting its metadata cache, and
>updating its acknowledged features. Keep the old IOTLB alive until all
>virtqueues have dropped their references, then free it.
>
>Also drop queued IOTLB miss messages and wake readers now that the
>device no longer accepts IOTLB updates.
>
>Fixes: e13a6915a03f ("vhost/vsock: add IOTLB API support")
>Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>Signed-off-by: Jia Jia <physicalmtea@gmail.com>
>---
> drivers/vhost/vsock.c | 39 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 34 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index 9aaab6bb8061..7372c22691de 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -851,6 +851,30 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
> return 0;
> }
>
>+/* Caller must hold the device mutex. */
>+static void vhost_vsock_clear_iotlb(struct vhost_vsock *vsock, u64 features)
>+{
>+ struct vhost_iotlb *iotlb;
>+ struct vhost_virtqueue *vq;
>+ int i;
>+
>+ iotlb = vsock->dev.iotlb;
>+ vsock->dev.iotlb = NULL;
>+
>+ for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
>+ mutex_lock(&vsock->vqs[i].mutex);
>+ vq = &vsock->vqs[i];
You can assing vq before the mutex_lock() and use it there too (and in
mutex_unlock()), as we do in all other places in this file.
>+ vq->iotlb = NULL;
>+ memset(vq->meta_iotlb, 0, sizeof(vq->meta_iotlb));
>+ vq->acked_features = features;
>+ mutex_unlock(&vsock->vqs[i].mutex);
>+ }
>+
>+ vhost_clear_msg(&vsock->dev);
>+ vhost_iotlb_free(iotlb);
>+ wake_up_interruptible_poll(&vsock->dev.wait, EPOLLIN | EPOLLRDNORM);
>+}
I don't see anything vsock specific here. Would it be better to move
this to vhost.c and reuse some of the functions we have there?
I mean something like this (untested and may be incomplete):
void vhost_clear_device_iotlb(struct vhost_dev *d)
{
struct vhost_iotlb *iotlb;
int i;
iotlb = d->iotlb;
d->iotlb = NULL;
for (i = 0; i < d->nvqs; ++i) {
struct vhost_virtqueue *vq = d->vqs[i];
mutex_lock(&vq->mutex);
vq->iotlb = NULL;
__vhost_vq_meta_reset(vq);
mutex_unlock(&vq->mutex);
}
vhost_clear_msg(d);
vhost_iotlb_free(iotlb);
wake_up_interruptible_poll(&d->wait, EPOLLIN | EPOLLRDNORM);
}
EXPORT_SYMBOL_GPL(vhost_clear_device_iotlb);
>+
> static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
> {
> struct vhost_virtqueue *vq;
>@@ -872,11 +896,16 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>
> vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET);
>
>- for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
>- vq = &vsock->vqs[i];
>- mutex_lock(&vq->mutex);
>- vq->acked_features = features;
>- mutex_unlock(&vq->mutex);
>+ if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)) &&
>+ vsock->dev.iotlb) {
>+ vhost_vsock_clear_iotlb(vsock, features);
>+ } else {
>+ for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
>+ vq = &vsock->vqs[i];
>+ mutex_lock(&vq->mutex);
>+ vq->acked_features = features;
>+ mutex_unlock(&vq->mutex);
>+ }
TBH I don't like this mix.
Why assigning acked_features inside vhost_vsock_clear_iotlb()?
IMO it's confusing. I see that you're saving another loop around the
VQs, but this code is not easy to understand IMO.
I think we have 2 options:
1. leave the loop for acked_features and don't set it in
vhost_vsock_clear_iotlb() (less code touched by this patch).
This is also what to do if we move the clear_iotlb() function
in vhost.c.
2. change the code to have a single for loop with if block inside to
reset IOTLB stuff if needed. In this case maybe better to avoid the
function and move the entire code here.
I prefer 1 with clear_iotlb() in vhost.c, but I'm not fully against 2.
Thanks,
Stefano
> }
> mutex_unlock(&vsock->dev.mutex);
> return 0;
>--
>2.34.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread