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

[Daniel Henrique Barboza]

On 1/27/22 10:28, Halil Pasic wrote:
> ping^2
>
> Also adding Brijesh and Daniel, as I believe you guys should be
> interested in this, and I'm yet to receive review.
>
> @Brijesh, Daniel: Can you confirm that AMD (SEV) and Power are affected
> too, and that the fix works for your platforms as well?

I failed to find a host that has Power secure execution support. I'll keep 
looking.


Meanwhile I have to mention that this patch re-introduced the problem that 
Kevin's
commit fixed.


With current upstream, if you start a regular guest with the following command 
line:

qemu-system-ppc64 (....)
-chardev socket,id=char0,path=/tmp/vhostqemu
-device vhost-user-fs-pci,chardev=char0,tag=myfs,iommu_platform=on

i.e. a guest with a vhost-user-fs-pci device that claims to have iommu support,
but it doesn't, this is the error message:


qemu-system-ppc64: -device 
vhost-user-fs-pci,chardev=char0,tag=myfs,iommu_platform=on: iommu_platform=true 
is not supported by the device


With this patch, that command line above starts the guest. virtiofsd fails 
during boot:

sudo ~/qemu/build/tools/virtiofsd/virtiofsd --socket-path=/tmp/vhostqemu -o 
source=~/linux-L1
[sudo] password for danielhb:
virtio_session_mount: Waiting for vhost-user socket connection...
virtio_session_mount: Received vhost-user socket connection
virtio_loop: Entry
fv_panic: libvhost-user: Invalid vring_addr message


And inside the guest, if you attempt to mount and use the virtiofs filesystem, 
the guest
hangs:

[root@localhost ~]# mount -t virtiofs myfs /mnt
[root@localhost ~]# cd /mnt

(hangs)

Exiting QEMU throws several vhost related errors:


QEMU 6.2.50 monitor - type 'help' for more information
(qemu) quit
qemu-system-ppc64: Failed to set msg fds.
qemu-system-ppc64: vhost VQ 0 ring restore failed: -22: Invalid argument (22)
qemu-system-ppc64: Failed to set msg fds.
qemu-system-ppc64: vhost VQ 1 ring restore failed: -22: Invalid argument (22)
qemu-system-ppc64: Failed to set msg fds.
qemu-system-ppc64: vhost_set_vring_call failed: Invalid argument (22)
qemu-system-ppc64: Failed to set msg fds.
qemu-system-ppc64: vhost_set_vring_call failed: Invalid argument (22)



I made a little experiment with upstream and reverting Kevin's patch and the 
result is
the same, meaning that this is the original bug [1] Kevin fixed back then. Note 
that [1]
was reported on x86, meaning that this particular issue seems to be arch 
agnostic.


My point here is that your patch fixes the situation for s390x, and Brijesh 
already chimed
in claiming that it fixed for AMD SEV, but it reintroduced a bug. I believe you 
should
include this test case with vhost-user in your testing to figure out a way to 
fix what
is needed without adding this particular regression.


In fact, I have a feeling that this is not the first time this kind of 
situation is discussed
around here. This reminds me of [2] and a discussion about the order virtiofs 
features
are negotiated versus when/how QEMU inits the devices.



[1] https://bugzilla.redhat.com/show_bug.cgi?id=1935019
[2] https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05644.html


Thanks,


Daniel


> Regards,
> Halil
>
> On Tue, 25 Jan 2022 11:21:12 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
> > ping
> >
> > On Mon, 17 Jan 2022 13:02:38 +0100
> > Halil Pasic <pasic@linux.ibm.com> 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
> > >
> > > ---
> > >
> > > 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..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: 6621441db50d5bae7e34dbd04bf3c57a27a71b32