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