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

[Dr. David Alan Gilbert]

* 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 
> > > > > 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.
> > > Further analyzing this, on vhost-user.c, I see:
> > > 
> > > Â Â Â Â Â Â Â  if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) 
> > > &&
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  
> > > !(virtio_has_feature(dev->protocol_features,
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  
> > > virtio_has_feature(dev->protocol_features,
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  VHOST_USER_PROTOCOL_F_REPLY_ACK))) 
> > > {
> > > Â Â Â Â Â Â Â Â Â Â Â  error_report("IOMMU support requires reply-ack and 
> > > "
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  "slave-req protocol 
> > > features.");
> > > Â Â Â Â Â Â Â Â Â Â Â  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.

Dave

> Leo
> 
> 
> > 
> > Dave
> > 
> > > Cheers,
> > > 
> > > Leo
> > > 
> > > > Stefan
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK