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

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

[Stefano Garzarella]

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
>

Re: [PATCH v3 2/2] vhost/vsock: keep IOTLB across feature updates

[Stefano Garzarella]

On Mon, Aug 10, 2026 at 09:40:18PM +0800, Jia Jia wrote:
>VHOST_SET_FEATURES is also used to update logging while a device is
>running. When ACCESS_PLATFORM stays enabled, allocating a new empty
>IOTLB on every call drops valid translations and forces avoidable
>misses.
>
>Initialize the device IOTLB only when one does not already exist.
>

Do we need a Fixes tag here?

>Signed-off-by: Jia Jia <physicalmtea@gmail.com>
>---
> drivers/vhost/vsock.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index 7372c22691de..1beae7f5b9a9 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -889,7 +889,8 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
> 		goto err;
> 	}
>
>-	if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) {
>+	if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)) &&
>+	    !vsock->dev.iotlb) {

Do we need to do the same in vhost-net too?

Thanks,
Stefano

> 		if (vhost_init_device_iotlb(&vsock->dev))
> 			goto err;
> 	}
>-- 
>2.34.1
>

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

[Jia Jia]

> 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.

Sure, will do.

> 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);

Thanks for the detailed feedback. There has been some evolution behind
this patch. The work originally started from a vhost-scsi bug. After
Stefan Hajnoczi pointed out that vhost-net and vhost-vsock might have the
same underlying issue, the first version used a feature-change rejection
approach. The vhost-net patch from that stage is:

https://lore.kernel.org/all/20260726141158.1652386-1-physicalmtea@gmail.com/

Michael S. Tsirkin then suggested that simply rejecting the change was
not the best way to handle the IOTLB transition, and suggested discarding
the existing IOTLB instead. That led to the current vhost-vsock patch.

The vhost-net patch already preserves an existing IOTLB when
ACCESS_PLATFORM remains enabled, as in patch 2 here. For clearing
ACCESS_PLATFORM, the two versions currently use different policies:
vhost-net rejects the live change with -EBUSY, while this vhost-vsock
series accepts the change and tears down the old IOTLB. The vhost-net
patch also addresses a separate live IN_ORDER transition issue.

I initially kept the helper in vsock.c because I was unsure whether
changing the common vhost code was appropriate. Looking at the code,
however, the IOTLB teardown has no dependency on vsock- or net-specific
state. Unless there are other concerns, I will move the helper to
vhost.c in the next revision and apply the same IOTLB teardown handling
to vhost-net as well. I will keep the acked_features updates in each
backend's own loop, while retaining the vhost-net-specific checks for
IN_ORDER and other unsafe feature changes.

> 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 for understanding that the original intent was to avoid an extra loop.
I agree that assigning acked_features in vhost_vsock_clear_iotlb() is confusing,
so I'll follow option 1 in the next version.

Thanks,
Jia

Re: [PATCH v3 2/2] vhost/vsock: keep IOTLB across feature updates

[Jia Jia]

> Do we need a Fixes tag here?

I overlooked the Fixes tag. Thanks for pointing that out. I will add:

Fixes: e13a6915a03f ("vhost/vsock: add IOTLB API support")

> Do we need to do the same in vhost-net too?

Yes, vhost-net has the same IOTLB lifecycle issue. I previously posted a
separate patch for vhost-net:

https://lore.kernel.org/all/20260726141158.1652386-1-physicalmtea@gmail.com/

I will update that patch in the next version to use the common IOTLB
teardown handling for the ACCESS_PLATFORM transition, while retaining
the vhost-net-specific checks for IN_ORDER and other unsafe feature
changes.

Thanks,
Jia