On Fri, Jan 28, 2022 at 03:29:11AM +0100, Halil Pasic wrote: > On Thu, 27 Jan 2022 18:34:23 -0300 > Daniel Henrique Barboza <danielhb...@gmail.com> wrote: > > > 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) > > > > > > > Does your VM have an IOMMU and does your guest see it? If yes does > vdev->dma_as != &address_space_memory hold for your virtio device? If no why > not? > > My understanding is that your guest wants to do translated addresses, > because it sees the ACCESS_PLATFORM feature, and probably thinks that > your device is indeed behind an IOMMU, from what I assume, at least it > sees that there is an IOMMU. But then I would expect your virtio device > to have its vdev->dma_as set to something different than > &address_space_memory. Conversely if your dma address space is > address_space_memory, then you don't need address translation because > your dma addresses are the same as your guest physical addresses. > > > > > 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. > > We don't have this problem on s390, so it ain't entirely 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. > > Can you help me with this? IMHO the big problem is that iommu_platform > is used for two distinct things. I've described that in the commit > message. > > We may be able to differentiate between the two using ->dma_as, but for > that it needs to be set up correctly: whenever you require translation > it should be something different than address_space_memory. The question > is why do you require translation but don't have your ->dma_as set up > properly? It can be a guest thing, i.e. guest just assumes it has to do > bus addresses, while it actually does not have to, or we indeed do have > an IOMMU which polices the devices access to the guest memory, but for > some strange reason we failed to set up ->dma_as to reflect that. > > @Michael: what is your opinion?
Right, I am puzzled too. > > > > > > 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 <pa...@linux.ibm.com> wrote: > > > > > >> ping > > >> > > >> On Mon, 17 Jan 2022 13:02:38 +0100 > > >> Halil Pasic <pa...@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 <pa...@linux.ibm.com> > > >>> Reported-by: Jakob Naucke <jakob.nau...@ibm.com> > > >>> Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but > > >>> unsupported") > > >>> Cc: Kevin Wolf <kw...@redhat.com> > > >>> Cc: qemu-sta...@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 > > >> > > > > > > > >