hw/virtio/vhost-user-fs.c | 5 +++++ hw/virtio/vhost-user.c | 7 +++++++ hw/virtio/vhost.c | 1 - 3 files changed, 12 insertions(+), 1 deletion(-)
From: Leonardo Garcia <lagarcia@br.ibm.com>
Currently, as IOMMU is not supported, if a user mistakenly set it and
tries to mount the vhost-user filesystem (or use any other vhost-user
based device) inside the guest, whenever the user tries to access the
device, the system will hang forever (because virtiofsd will not be
running anymore).
Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com>
---
hw/virtio/vhost-user-fs.c | 5 +++++
hw/virtio/vhost-user.c | 7 +++++++
hw/virtio/vhost.c | 1 -
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..059b0d2ef9 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -222,6 +222,11 @@ static void vuf_device_realize(DeviceState *dev, Error **errp)
/* 1 high prio queue, plus the number configured */
fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues;
fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs);
+ /*
+ * vhost_user_backend_init does additional checks if VirtIODevice is defined
+ * in vhost_dev
+ */
+ fs->vhost_dev.vdev = vdev;
ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user,
VHOST_BACKEND_TYPE_USER, 0);
if (ret < 0) {
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2fdd5daf74..6ab760364b 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1849,6 +1849,13 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
}
}
+ if ((dev->vdev != NULL) &&
+ virtio_host_has_feature(dev->vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+ !(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
+ error_report("IOMMU is currently not supported with vhost-user");
+ return -1;
+ }
+
if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) &&
!(virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6e17d631f7..78e58d2148 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1293,7 +1293,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
int i, r, n_initialized_vqs = 0;
Error *local_err = NULL;
- hdev->vdev = NULL;
hdev->migration_blocker = NULL;
r = vhost_set_backend_type(hdev, backend_type);
--
2.29.2
On Wed, Feb 17, 2021 at 04:55:12PM -0300, lagarcia@linux.ibm.com wrote: > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 2fdd5daf74..6ab760364b 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -1849,6 +1849,13 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > } > } > > + if ((dev->vdev != NULL) && > + virtio_host_has_feature(dev->vdev, VIRTIO_F_IOMMU_PLATFORM) && > + !(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) { > + error_report("IOMMU is currently not supported with vhost-user"); I think IOMMU *is* supported by vhost-user. It's just that specific vhost-user backends might not implement it. This error message can be made more specific: error_report("IOMMU is not supported by the connected vhost-user backend"); Stefan
Hi Leo, Please use my personal groug@kaod.org address for upstream matters. On Wed, 17 Feb 2021 16:55:12 -0300 lagarcia@linux.ibm.com wrote: > From: Leonardo Garcia <lagarcia@br.ibm.com> > > Currently, as IOMMU is not supported, if a user mistakenly set it and > tries to mount the vhost-user filesystem (or use any other vhost-user > based device) inside the guest, whenever the user tries to access the > device, the system will hang forever (because virtiofsd will not be > running anymore). > > Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> > --- > hw/virtio/vhost-user-fs.c | 5 +++++ > hw/virtio/vhost-user.c | 7 +++++++ > hw/virtio/vhost.c | 1 - > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c > index ac4fc34b36..059b0d2ef9 100644 > --- a/hw/virtio/vhost-user-fs.c > +++ b/hw/virtio/vhost-user-fs.c > @@ -222,6 +222,11 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) > /* 1 high prio queue, plus the number configured */ > fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; > fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); > + /* > + * vhost_user_backend_init does additional checks if VirtIODevice is defined > + * in vhost_dev > + */ > + fs->vhost_dev.vdev = vdev; This looks a bit like a software boundary violation to me. Only places where ->vdev is modified are all in hw/vhost/vhost.c : - vhost_dev_init() = NULL - vhost_dev_start() = vdev - vhost_dev_stop() = NULL - vhost_dev_prepare_inflight() = vdev The last one is interesting because it seems to be doing something vaguely similar to what you're trying to achieve, i.e. setting ->vdev before vhost_dev_start() is called for some reason. commit 1b0063b3048af65dfaae6422a572c87db8575a92 Author: Jin Yu <jin.yu@intel.com> Date: Tue Nov 3 20:36:17 2020 +0800 vhost-blk: set features before setting inflight feature > ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user, > VHOST_BACKEND_TYPE_USER, 0); > if (ret < 0) { > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 2fdd5daf74..6ab760364b 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -1849,6 +1849,13 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > } > } > > + if ((dev->vdev != NULL) && > + virtio_host_has_feature(dev->vdev, VIRTIO_F_IOMMU_PLATFORM) && > + !(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) { > + error_report("IOMMU is currently not supported with vhost-user"); > + return -1; > + } > + > if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) && > !(virtio_has_feature(dev->protocol_features, > VHOST_USER_PROTOCOL_F_SLAVE_REQ) && > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 6e17d631f7..78e58d2148 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1293,7 +1293,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > int i, r, n_initialized_vqs = 0; > Error *local_err = NULL; > > - hdev->vdev = NULL; With this change, vhost_dev_init() no longer offers the guarantee that ->vdev is valid, which might cause problems for any device that doesn't set ->vdev itself like this patch does for vhost-user-fs. I suggest you to turn vhost_dev_init() into a vhost_dev_init_vdev() with an extra vdev argument and use it for all vhost-user devices. Introduce a vhost_dev_init() inline that does vhost_dev_init_vdev(NULL) and call it everywhere else. > hdev->migration_blocker = NULL; > > r = vhost_set_backend_type(hdev, backend_type);
On Tue, Feb 23, 2021 at 04:55:41PM +0100, Greg Kurz wrote: > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 6e17d631f7..78e58d2148 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1293,7 +1293,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > > int i, r, n_initialized_vqs = 0; > > Error *local_err = NULL; > > > > - hdev->vdev = NULL; > > With this change, vhost_dev_init() no longer offers the guarantee > that ->vdev is valid, which might cause problems for any device > that doesn't set ->vdev itself like this patch does for vhost-user-fs. > > I suggest you to turn vhost_dev_init() into a vhost_dev_init_vdev() > with an extra vdev argument and use it for all vhost-user devices. > Introduce a vhost_dev_init() inline that does vhost_dev_init_vdev(NULL) > and call it everywhere else. Yes, I agree. Passing vdev in as an argument is a clean solution. Stefan
© 2016 - 2024 Red Hat, Inc.