On Wed, Jan 27, 2021 at 09:30:35AM -0300, Leonardo Augusto Guimarães Garcia wrote: > 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? > > > I don't know if VIRTIO_PCI_FLAG_ATS should depend on > VIRTIO_F_IOMMU_PLATFORM. At least from a code perspective today, they are > completely independent. A user can specify one or the other or both. And if > a user specifies VIRTIO_PCI_FLAG_ATS without specifying > VIRTIO_F_IOMMU_PLATFORM, the same issue described in the original message > will happen inside the guest.
I don't see any PCI ATS-specific code in Linux drivers/virtio/ so I
wonder what the issue is?
Also, I thought PCI ATS is an optional feature. It's still possible to
have IOMMUs without ATS.
Michael: Can you help us understand why enabling ATS on a device in QEMU
would cause issues with a VIRTIO device that does not support
VIRTIO_F_IOMMU_PLATFORM?
> >
> > > +
> > > 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.
>
>
> Sure, I can do that. I wasn't sure whether this restriction was only for
> vhost-user-fs or whether it was generic for all vhost-user devices. I will
> include this in a next version of the patch.
Awesome, thanks!
Stefan
signature.asc
Description: PGP signature
