[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

From: Leonardo Augusto Guimarães Garcia
Subject: Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.
Date: Wed, 27 Jan 2021 16:54:04 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

On 1/27/21 4:40 PM, Dr. David Alan Gilbert wrote:
* Leonardo Augusto Guimarães Garcia (lagarcia@br.ibm.com) wrote:
On 1/27/21 1:34 PM, Dr. David Alan Gilbert wrote:
* Leonardo Augusto Guimarães Garcia (lagarcia@linux.ibm.com) wrote:
On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:
On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagarcia@linux.ibm.com wrote:
From: Leonardo Garcia <lagarcia@br.ibm.com>

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com>
    hw/virtio/vhost-user-fs-pci.c | 7 +++++++
    hw/virtio/vhost-user-fs.c     | 5 +++++
    2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
     * top-level directory.
+#include "qemu/osdep.h"
+#include "qapi/error.h"
    #include "qemu/osdep.h"
    #include "hw/qdev-properties.h"
    #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
            vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
+    if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {
+        error_setg(errp, "ATS is currently not supported with 
+        return;
+    }
Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?

What needs to be added to support ATS?

        qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
+    if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+        error_setg(errp, "IOMMU is currently not supported with 
+        return;
+    }
        if (!vhost_user_init(&fs->vhost_user, &fs->conf.chardev, errp)) {
I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

       ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user,
                            VHOST_BACKEND_TYPE_USER, 0);
       if (ret < 0) {
           error_setg_errno(errp, -ret, "vhost_dev_init failed");
           goto err_virtio;
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+       !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+       error_setg(errp, "IOMMU is not supported by the vhost-user device 
+       goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.
Further analyzing this, on vhost-user.c, I see:

        if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) &&
                    VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
                    VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
            error_report("IOMMU support requires reply-ack and "
                         "slave-req protocol 
            return -1;

This code was included by commit 6dcdd06. It included support for
VIRTIO_F_IOMMU_PLATFORM with vhost_net devices. So, the restriction is not
generic for all vhost-user devices.
That test is a slightly different test; that's checking that the
vhost-user device has two underlying features that are needed to make
iommu work; it's not a full test though; it doesn't actually check the
vhost-user device also wants to do iommu.

Right... but Stefan was suggesting to move the check I proposed to avoid
VIRTIO_F_IOMMU_PLATFORM on vhost-user-fs up to vhost-user. What I understand
from the above code excerpt and the rest of commit 6dcdd06 is that IOMMU
support has been added into vhost-user already. However, it seems
vhost-user-fs (maybe all other devices on top of vhost-user) doesn't support
it yet. If I move the check up to vhost-user, does it make sense to still
have all the IOMMU code support there?
Libvhost-user isn't just used by virtiofs; so it can have IOMMU code in
(I'm not convinced it was complete though); it just needs to make sure
that the device is also happy to do IOMMU, as Stefan's code did.

Oh, I think I finally got what Stefan and you are saying. Thanks for the additional clarification!








reply via email to

[Prev in Thread] Current Thread [Next in Thread]