On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:
On Tue, Jan 26, 2021 at 03:23:38PM -0300, [email protected] wrote:
From: Leonardo Garcia <[email protected]>
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 <[email protected]>
---
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
vhost-user-fs-pci");
+ 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
**errp)
return;
}
+ if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+ error_setg(errp, "IOMMU is currently not supported with
vhost-user-fs");
+ 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
backend");
+ 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.
I am afraid I will need some additional guidance on how to do that. If I
am reading the code correctly, the vhost-user devices don't follow any
specific pattern. Looking at the vhost-user-fs code path, we have:
vuf_device_realize -> vhost_dev_init -> vhost_user_backend_init
So, I thought that naturally, if the check was in vuf_device_realize on
my previous patch, I should move it to vhost_user_backend_init. However,
in order to check if the VirtIODevice has been specified with the
VIRTIO_F_IOMMU_PLATFORM option, I would need to have access to the
VirtIODevice inside vhost_user_backend_init, which currently is not
available and not one of the arguments of vhost_backend_init virtual
function it implements. vhost_dev_init doesn't have access to
VirIODevices as well. Looking at other device types that call
vhost_dev_init, not all of them initialized the VirtIODevice by the time
vhost_dev_init is called (e.g. cryptodev-host). Hence, adding a
VirtIODevice as parameter to vhost_dev_init and vhost_backedn_init is
not a feasible solution. vhost_dev_init does receive structu vhost_dev
which has a field VirtIODevice vdev. But as the VirtIODevice hasn't been
initialized by the time vhost_dev_init is called on all vhost-user
devices today also makes this not a solution.
Any ideas on this? Is it correct for a virtio-user devices to call
vhost_dev_init before having VirtIODevice ready?
Leo
Stefan
