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

[Michael S. Tsirkin]

On Thu, Apr 28, 2022 at 11:01:10AM +0800, Jason Wang wrote:
> On Wed, Apr 27, 2022 at 8:25 PM Chenyi Qiang <chenyi.qiang@intel.com> wrote:
> >
> >
> >
> > On 4/22/2022 3:11 PM, Chenyi Qiang wrote:
> > >
> > >
> > > On 2/7/2022 7:28 PM, 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") fences _F_ACCESS_PLATFORM from 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")
> > >> Acked-by: Cornelia Huck <cohuck@redhat.com>
> > >> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > >> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > >> Cc: Kevin Wolf <kwolf@redhat.com>
> > >> Cc: qemu-stable@nongnu.org
> > >>
> > >> ---
> > >>
> > >> v4->v5:
> > >> * added back the return; so if somebody were to add code to the end of
> > >>    the function we are still good
> > >> v3->v4:
> > >> * Fixed commit message (thanks Connie)
> > >> * Removed counter-productive initialization (thanks Connie)
> > >> * Added tags
> > >> 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 | 12 +++++++-----
> > >>   1 file changed, 7 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > >> index d23db98c56..0f69d1c742 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;
> > >>       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,15 @@ 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) {
> > >
> > > Hi Pasic,
> > >
> > > When testing the virtio-fs in Intel TDX, I met the error report in this
> > > check. Is it appropriate to compare the dma_as against the
> > > address_space_memory to detect whether the IOMMU is enabled or not? Per
> > > the commit ae4003738f(vhost: correctly detect the enabling IOMMU), we
> > > should call virtio_bus_device_iommu_enabled(vdev) instead here, correct?
> > >
> >
> > Sorry for bothering.
> >
> > Can virtio-fs work properly in AMD SEV?
> >
> > IIUC, If get_dma_as() is implemented and in case of PCI,
> > pci_get_address_space() is used and returns the bus master as. This
> > would fail the check here.
> 
> I think the reason is that the viritio-fs is used without vIOMMU but
> ACCESS_PLATFORM.
> 
> That's why we need to use virtio_bus_device_iommu_enabled() to allow
> this setup to work.
> 
> Thanks

Do you retract your ack then?

> >
> > >> +            error_setg(errp,
> > >> +                       "iommu_platform=true is not supported by the
> > >> device");
> > >> +            return;
> > >> +        }
> > >>       } else {
> > >>           vdev->dma_as = &address_space_memory;
> > >>       }
> > >>
> > >> base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff
> > >
> >