Message ID | e76462fdcfaa07208380e2a7df9b281b6e6717b8.1611685180.git.lagarcia@br.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | IOMMU and ATS not supported by vhost-user filesystem. | expand |
* 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 >
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 >>
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 >
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 >
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 >
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
* 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 > > >
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
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; }