hw/virtio/vhost-user-fs-pci.c | 7 +++++++ hw/virtio/vhost-user-fs.c | 5 +++++ 2 files changed, 12 insertions(+)
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
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
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
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
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
* 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
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
* 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
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
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
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
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
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
>
* 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
* 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
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
>>
© 2016 - 2024 Red Hat, Inc.