[PATCH v2] IOMMU not supported by vhost-user.

[PATCH v2] IOMMU not supported by vhost-user.

[lagarcia]

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

Re: [PATCH v2] IOMMU not supported by vhost-user.

[Greg Kurz]

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

Re: [PATCH v2] IOMMU not supported by vhost-user.

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] IOMMU not supported by vhost-user.

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]