Re: [PATCH v3 1/2] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

[Stefano Garzarella <sgarzare@redhat.com>]

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
>