On Mon, Feb 7, 2022 at 7:29 PM Halil Pasic <[email protected]> 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 <[email protected]> > Reported-by: Jakob Naucke <[email protected]> > Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but > unsupported") > Acked-by: Cornelia Huck <[email protected]> > Reviewed-by: Daniel Henrique Barboza <[email protected]> > Tested-by: Daniel Henrique Barboza <[email protected]> > Cc: Kevin Wolf <[email protected]> > Cc: [email protected]
Acked-by: Jason Wang <[email protected]> > > --- > > 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) { > + error_setg(errp, > + "iommu_platform=true is not supported by the device"); > + return; > + } > } else { > vdev->dma_as = &address_space_memory; > } > > base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff > -- > 2.32.0 >
