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