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

[Daniel Henrique Barboza]

On 2/1/22 10:39, 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
> support _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, so that, after the feature has been
> negotiated, the guest  grants access to the portions of memory the
> device needs to see. So in for confidential guests, generally,
> _F_ACCESS_PLATFORM is about the restricted access to memory, but not
> about the addresses used being something else than guest physical
> addresses.
>
> This is the very reason for which commit f7ef7e6e3b ("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 f7ef7e6e3b ("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
>
> ---

This version solved the case I mentioned in the v2 about the false positive with
iommu_platform=on and and the vhost-user-fs-pci device. Now I get:


$ sudo ./qemu-system-ppc64 -machine pseries,accel=kvm (...)
-chardev socket,id=char0,path=/tmp/vhostqemu -device 
vhost-user-fs-pci,chardev=char0,tag=myfs,iommu_platform=on
qemu-system-ppc64: -device 
vhost-user-fs-pci,chardev=char0,tag=myfs,iommu_platform=on: iommu_platform=true 
is not supported by the device


Which is the expected result.

I didn't find the opportunity to test this patch with the PowerPC secure VM 
tech (papr-pef) but I
don't see the point of delaying this fix because of that.


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>


One small suggestion down below in case a v4 is needed:

> v2->v3:
> * Caught a bug: I tired to check if vdev has the feature
>     ACCESS_PLATFORM after we have forced it. Moved the check
>     to a better place
> v1->v2:
> * Commit message tweaks. Most notably fixed commit SHA (Michael)
>
> ---
> ---
>   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..34f5a0a664 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -48,6 +48,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
> **errp)
>       VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
>       VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>       bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> +    bool vdev_has_iommu = false;
>       Error *local_err = NULL;
>   
>       DPRINTF("%s: plug device.\n", qbus->name);
> @@ -69,11 +70,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);
>       }
> @@ -82,9 +78,14 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
> **errp)
>           return;
>       }
>   
> +    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>       if (klass->get_dma_as != NULL && has_iommu) {
>           virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>           vdev->dma_as = klass->get_dma_as(qbus->parent);
> +        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> +            error_setg(errp,
> +                       "iommu_platform=true is not supported by the device");
> +        }


>       } else {
>           vdev->dma_as = &address_space_memory;
>       }


I struggled to understand what this 'else' clause was doing and I assumed that 
it was
wrong. Searching through the ML I learned that this 'else' clause is intended 
to handle
legacy virtio devices that doesn't support the DMA API (introduced in 
8607f5c3072caeebb)
and thus shouldn't set  VIRTIO_F_IOMMU_PLATFORM.


My suggestion, if a v4 is required for any other reason, is to add a small 
comment in this
'else' clause explaining that this is the legacy virtio devices condition and 
those devices
don't set F_IOMMU_PLATFORM. This would make the code easier to read for a 
virtio casual like
myself.



Thanks,


Daniel


> base-commit: 6621441db50d5bae7e34dbd04bf3c57a27a71b32