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

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

[lagarcia]

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;
+    }
+
     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)) {
         return;
     }
-- 
2.29.2

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

[Dr. David Alan Gilbert]

* lagarcia@linux.ibm.com (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;
> +    }
> +
>      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;
> +    }

Yes, I've seen this problem - however, I'm a little confused; isn't the
negotiation of features on virtio supposed to happen automatically?
If so, how come it's managing to set VIRTIO_F_IOMMU_PLATFORM?

Dave

>      if (!vhost_user_init(&fs->vhost_user, &fs->conf.chardev, errp)) {
>          return;
>      }
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Leonardo Augusto Guimarães Garcia]

On 1/26/21 4:45 PM, Dr. David Alan Gilbert wrote:
> * lagarcia@linux.ibm.com (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;
>> +    }
>> +
>>       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;
>> +    }
> Yes, I've seen this problem - however, I'm a little confused; isn't the
> negotiation of features on virtio supposed to happen automatically?
> If so, how come it's managing to set VIRTIO_F_IOMMU_PLATFORM?


It is easy to set by passing iommu_platform=on on the QEMU command line. 
Something like this:

-device 
vhost-user-fs-pci,chardev=chr-vu-fs0,queue-size=512,tag=myfs,iommu_platform=on,ats=on

I understand this is a user error, but QEMU should not allow this 
configuration as it doesn't work.

Cheers,

Leo


>
> Dave
>
>>       if (!vhost_user_init(&fs->vhost_user, &fs->conf.chardev, errp)) {
>>           return;
>>       }
>> -- 
>> 2.29.2
>>

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

[Stefan Hajnoczi]

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

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.

Stefan

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

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

[Leonardo Augusto Guimarães Garcia]

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?


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.


>
> What needs to be added to support ATS?


Unfortunately I don't know the answer for this question. Hopefully 
someone else can help with this one.


>
>> +
>>       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.


Cheers,


Leo


>
> Stefan

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

[Stefan Hajnoczi]

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

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, 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?
> 
> 
> 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

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

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

[Leonardo Augusto Guimarães Garcia]

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.

Cheers,

Leo

>
> Stefan

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

[Dr. David Alan Gilbert]

* 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.

Dave

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

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

[Laszlo Ersek]

On 01/27/21 12:19, 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.

... I had the same thought when a few weeks earlier I tried to use
virtio-fs with an SEV guest (just OVMF), and virtiofsd crashed,
apparently :)

(I didn't report it because, well, SEV wants to prevent sharing between
host and guest, and virtio-fs works precisely in the opposite direction;
so the use case may not have much merit.)

Thanks
Laszlo

> 
> 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.
> 
> Stefan
> 

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

[Leonardo Augusto Guimarães Garcia]

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?

Leo


>
> Dave
>
>> Cheers,
>>
>> Leo
>>
>>> Stefan

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

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

[Leonardo Augusto Guimarães Garcia]

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 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.


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

Leo


>
> Dave
>
>> Leo
>>
>>
>>> Dave
>>>
>>>> Cheers,
>>>>
>>>> Leo
>>>>
>>>>> Stefan

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

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 01/27/21 12:19, 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.
> 
> ... I had the same thought when a few weeks earlier I tried to use
> virtio-fs with an SEV guest (just OVMF), and virtiofsd crashed,
> apparently :)
> 
> (I didn't report it because, well, SEV wants to prevent sharing between
> host and guest, and virtio-fs works precisely in the opposite direction;
> so the use case may not have much merit.)

Yeh I'm not expecting that to currently work; I can see some uses, but
it's much more niche.

Dave

> Thanks
> Laszlo
> 
> > 
> > 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.
> > 
> > Stefan
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Leonardo Augusto Guimarães Garcia]

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.


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

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

[Stefan Hajnoczi]

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

On Thu, Jan 28, 2021 at 12:41:15PM -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, lagarcia@linux.ibm.com wrote:
> > +
> > +   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?

Maybe Michael Tsirkin has an idea. Otherwise let's go with a
vhost-user-fs fix.

Stefan

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

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

[Leonardo Augusto Guimarães Garcia]

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?


As I am working on v2 for this patch, I revisited this. There is no need 
for this check. ATS works fine. The only problem is with the 
iommu_platform flag.


Cheers,


Leo


>
>> +
>>       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.
>
> Stefan