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? 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 >