[PATCH 1/1] virtio: fix the condition for iommu_platform not supported

[PATCH 1/1] virtio: fix the condition for iommu_platform not supported

[Halil Pasic]

The commit 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
unsupported") claims to fail the device hotplug when iommu_platform
is requested, but not supported by the (vhost) device. On the first
glance the condition for detecting that situation looks perfect, but
because a certain peculiarity of virtio_platform it ain't.

In fact the aforementioned commit introduces a regression. It breaks
virtio-fs support for Secure Execution, and most likely also for AMD SEV
or any other confidential guest scenario that relies encrypted guest
memory.  The same also applies to any other vhost device that does not
negotiate _F_ACCESS_PLATFORM.

The peculiarity is that iommu_platform and _F_ACCESS_PLATFORM collates
"device can not access all of the guest ram" and "iova != gpa, thus
device needs to translate iova".

Confidential guest technologies currently rely on the device/hypervisor
offering _F_ACCESS_PLATFORM to grant access to whatever the device needs
to see, because of the first. But, generally, they don't care for the
second.

This is the very reason for which commit 7ef7e6e3b ("vhost: correctly
turn on VIRTIO_F_IOMMU_PLATFORM") for, which fences _F_ACCESS_PLATFORM
form the vhost device that does not need it, because on the vhost
interface it only means "I/O address translation is needed".

This patch takes inspiration from 7ef7e6e3b ("vhost: correctly turn on
VIRTIO_F_IOMMU_PLATFORM"), and uses the same condition for detecting the
situation when _F_ACCESS_PLATFORM is requested, but no I/O translation
by the device, and thus no device capability is needed. In this
situation claiming that the device does not support iommu_plattform=on
is counter-productive. So let us stop doing that!

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reported-by: Jakob Naucke <Jakob.Naucke@ibm.com>
Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
unsupported")
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-stable@nongnu.org

---

@Kevin: Can you please verify, that I don't break your fix?
---
 hw/virtio/virtio-bus.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index d23db98c56..c1578f3de2 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -69,11 +69,6 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
         return;
     }
 
-    if (has_iommu && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
-        error_setg(errp, "iommu_platform=true is not supported by the device");
-        return;
-    }
-
     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent, &local_err);
     }
@@ -88,6 +83,12 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
     } else {
         vdev->dma_as = &address_space_memory;
     }
+
+    if (has_iommu && vdev->dma_as != &address_space_memory
+                  && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+        error_setg(errp, "iommu_platform=true is not supported by the device");
+        return;
+    }
 }
 
 /* Reset the virtio_bus */

base-commit: f8d75e10d3e0033a0a29a7a7e4777a4fbc17a016
-- 
2.32.0

Re: [PATCH 1/1] virtio: fix the condition for iommu_platform not supported

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 05:51:31PM +0100, Halil Pasic wrote:
> The commit 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
> unsupported") claims to fail the device hotplug when iommu_platform
> is requested, but not supported by the (vhost) device. On the first
> glance the condition for detecting that situation looks perfect, but
> because a certain peculiarity of virtio_platform it ain't.
> 
> In fact the aforementioned commit introduces a regression. It breaks
> virtio-fs support for Secure Execution, and most likely also for AMD SEV
> or any other confidential guest scenario that relies encrypted guest
> memory.  The same also applies to any other vhost device that does not
> negotiate _F_ACCESS_PLATFORM.
> 
> The peculiarity is that iommu_platform and _F_ACCESS_PLATFORM collates
> "device can not access all of the guest ram" and "iova != gpa, thus
> device needs to translate iova".
> 
> Confidential guest technologies currently rely on the device/hypervisor
> offering _F_ACCESS_PLATFORM to grant access to whatever the device needs
> to see, because of the first. But, generally, they don't care for the
> second.
> 
> This is the very reason for which commit 7ef7e6e3b ("vhost: correctly
> turn on VIRTIO_F_IOMMU_PLATFORM") for, which fences _F_ACCESS_PLATFORM
> form the vhost device that does not need it, because on the vhost
> interface it only means "I/O address translation is needed".
> 
> This patch takes inspiration from 7ef7e6e3b ("vhost: correctly turn on
> VIRTIO_F_IOMMU_PLATFORM"),

Strange, I could not find this commit. Did you mean f7ef7e6e3b?


> and uses the same condition for detecting the
> situation when _F_ACCESS_PLATFORM is requested, but no I/O translation
> by the device, and thus no device capability is needed.
>
> In this
> situation claiming that the device does not support iommu_plattform=on
> is counter-productive. So let us stop doing that!
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Jakob Naucke <Jakob.Naucke@ibm.com>
> Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
> unsupported")
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: qemu-stable@nongnu.org
> 
> ---
> 
> @Kevin: Can you please verify, that I don't break your fix?

So which configurations did you test for this?

> ---
>  hw/virtio/virtio-bus.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index d23db98c56..c1578f3de2 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -69,11 +69,6 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>          return;
>      }
>  
> -    if (has_iommu && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> -        error_setg(errp, "iommu_platform=true is not supported by the device");
> -        return;
> -    }
> -
>      if (klass->device_plugged != NULL) {
>          klass->device_plugged(qbus->parent, &local_err);
>      }
> @@ -88,6 +83,12 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>      } else {
>          vdev->dma_as = &address_space_memory;
>      }
> +
> +    if (has_iommu && vdev->dma_as != &address_space_memory
> +                  && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> +        error_setg(errp, "iommu_platform=true is not supported by the device");
> +        return;
> +    }
>  }
>  /* Reset the virtio_bus */
> 
> base-commit: f8d75e10d3e0033a0a29a7a7e4777a4fbc17a016
> -- 
> 2.32.0

Re: [PATCH 1/1] virtio: fix the condition for iommu_platform not supported

[Halil Pasic]

On Thu, 13 Jan 2022 12:11:42 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 13, 2022 at 05:51:31PM +0100, Halil Pasic wrote:
> > The commit 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
> > unsupported") claims to fail the device hotplug when iommu_platform
> > is requested, but not supported by the (vhost) device. On the first
> > glance the condition for detecting that situation looks perfect, but
> > because a certain peculiarity of virtio_platform it ain't.
> > 
> > In fact the aforementioned commit introduces a regression. It breaks
> > virtio-fs support for Secure Execution, and most likely also for AMD SEV
> > or any other confidential guest scenario that relies encrypted guest
> > memory.  The same also applies to any other vhost device that does not
> > negotiate _F_ACCESS_PLATFORM.
> > 
> > The peculiarity is that iommu_platform and _F_ACCESS_PLATFORM collates
> > "device can not access all of the guest ram" and "iova != gpa, thus
> > device needs to translate iova".
> > 
> > Confidential guest technologies currently rely on the device/hypervisor
> > offering _F_ACCESS_PLATFORM to grant access to whatever the device needs
> > to see, because of the first. But, generally, they don't care for the
> > second.
> > 
> > This is the very reason for which commit 7ef7e6e3b ("vhost: correctly
> > turn on VIRTIO_F_IOMMU_PLATFORM") for, which fences _F_ACCESS_PLATFORM
> > form the vhost device that does not need it, because on the vhost
> > interface it only means "I/O address translation is needed".
> > 
> > This patch takes inspiration from 7ef7e6e3b ("vhost: correctly turn on
> > VIRTIO_F_IOMMU_PLATFORM"),  
> 
> Strange, I could not find this commit. Did you mean f7ef7e6e3b?
> 

Right! Copy-paste error.


 
 static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
@@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
     if (enable_log) {
         features |= 0x1ULL << VHOST_F_LOG_ALL;
     }
+    if (!vhost_dev_has_iommu(dev)) {
+        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
+    }
     r = dev->vhost_ops->vhost_set_features(dev, features);
     if (r < 0) {
         VHOST_OPS_DEBUG("vhost_set_features failed");

> > and uses the same condition for detecting the
> > situation when _F_ACCESS_PLATFORM is requested, but no I/O translation
> > by the device, and thus no device capability is needed.
> >
> > In this
> > situation claiming that the device does not support iommu_plattform=on
> > is counter-productive. So let us stop doing that!
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Reported-by: Jakob Naucke <Jakob.Naucke@ibm.com>
> > Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
> > unsupported")
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > 
> > ---
> > 
> > @Kevin: Can you please verify, that I don't break your fix?  
> 
> So which configurations did you test for this?

I tested it with virtio-fs and qemu-system-s390x with -device
vhost-user-fs-ccw,iommu_platform=on,...

Regards,
Halil

> 
> > ---
> >  hw/virtio/virtio-bus.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > index d23db98c56..c1578f3de2 100644
> > --- a/hw/virtio/virtio-bus.c
> > +++ b/hw/virtio/virtio-bus.c
> > @@ -69,11 +69,6 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> >          return;
> >      }
> >  
> > -    if (has_iommu && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > -        error_setg(errp, "iommu_platform=true is not supported by the device");
> > -        return;
> > -    }
> > -
> >      if (klass->device_plugged != NULL) {
> >          klass->device_plugged(qbus->parent, &local_err);
> >      }
> > @@ -88,6 +83,12 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> >      } else {
> >          vdev->dma_as = &address_space_memory;
> >      }
> > +
> > +    if (has_iommu && vdev->dma_as != &address_space_memory
> > +                  && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > +        error_setg(errp, "iommu_platform=true is not supported by the device");
> > +        return;
> > +    }
> >  }
> >  /* Reset the virtio_bus */
> > 
> > base-commit: f8d75e10d3e0033a0a29a7a7e4777a4fbc17a016
> > -- 
> > 2.32.0  
> 
> 

Re: [PATCH 1/1] virtio: fix the condition for iommu_platform not supported

[Halil Pasic]

On Thu, 13 Jan 2022 20:54:52 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> > > This is the very reason for which commit 7ef7e6e3b ("vhost: correctly
> > > turn on VIRTIO_F_IOMMU_PLATFORM") for, which fences _F_ACCESS_PLATFORM
> > > form the vhost device that does not need it, because on the vhost
> > > interface it only means "I/O address translation is needed".
> > > 
> > > This patch takes inspiration from 7ef7e6e3b ("vhost: correctly turn on
> > > VIRTIO_F_IOMMU_PLATFORM"),    
> > 
> > Strange, I could not find this commit. Did you mean f7ef7e6e3b?
> >   
> 
> Right! Copy-paste error.
> 
> 

Should I spin a v2 to correct this?


Sorry for the hunk below. I wanted to post the  whole patch in question,
then deleted it, but left some leftovers. Another copy-paste error. Grrr

>  
>  static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
> @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
>      if (enable_log) {
>          features |= 0x1ULL << VHOST_F_LOG_ALL;
>      }
> +    if (!vhost_dev_has_iommu(dev)) {
> +        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> +    }
>      r = dev->vhost_ops->vhost_set_features(dev, features);
>      if (r < 0) {
>          VHOST_OPS_DEBUG("vhost_set_features failed");
> 
> > > and uses the same condition for detecting the

Re: [PATCH 1/1] virtio: fix the condition for iommu_platform not supported

[Michael S. Tsirkin]

On Fri, Jan 14, 2022 at 05:05:56PM +0100, Halil Pasic wrote:
> On Thu, 13 Jan 2022 20:54:52 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > > > This is the very reason for which commit 7ef7e6e3b ("vhost: correctly
> > > > turn on VIRTIO_F_IOMMU_PLATFORM") for, which fences _F_ACCESS_PLATFORM
> > > > form the vhost device that does not need it, because on the vhost
> > > > interface it only means "I/O address translation is needed".
> > > > 
> > > > This patch takes inspiration from 7ef7e6e3b ("vhost: correctly turn on
> > > > VIRTIO_F_IOMMU_PLATFORM"),    
> > > 
> > > Strange, I could not find this commit. Did you mean f7ef7e6e3b?
> > >   
> > 
> > Right! Copy-paste error.
> > 
> > 
> 
> Should I spin a v2 to correct this?
> 
> 
> Sorry for the hunk below. I wanted to post the  whole patch in question,
> then deleted it, but left some leftovers. Another copy-paste error. Grrr

Yes pls.

> >  
> >  static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
> > @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
> >      if (enable_log) {
> >          features |= 0x1ULL << VHOST_F_LOG_ALL;
> >      }
> > +    if (!vhost_dev_has_iommu(dev)) {
> > +        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> > +    }
> >      r = dev->vhost_ops->vhost_set_features(dev, features);
> >      if (r < 0) {
> >          VHOST_OPS_DEBUG("vhost_set_features failed");
> > 
> > > > and uses the same condition for detecting the