Message ID | 20220207112857.607829-1-pasic@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v5,1/1] virtio: fix the condition for iommu_platform not supported | expand |
On Mon, Feb 7, 2022 at 7:29 PM 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") 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 Acked-by: Jason Wang <jasowang@redhat.com> > > --- > > 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 >
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? > + error_setg(errp, > + "iommu_platform=true is not supported by the device"); > + return; > + } > } else { > vdev->dma_as = &address_space_memory; > } > > base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff
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. >> + error_setg(errp, >> + "iommu_platform=true is not supported by the >> device"); >> + return; >> + } >> } else { >> vdev->dma_as = &address_space_memory; >> } >> >> base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff >
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 > > >> + error_setg(errp, > >> + "iommu_platform=true is not supported by the > >> device"); > >> + return; > >> + } > >> } else { > >> vdev->dma_as = &address_space_memory; > >> } > >> > >> base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff > > >
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 > > > > >
On Thu, Apr 28, 2022 at 12:57 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > 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? Somehow, we need a fix on top. Thanks > > > > > > > >> + error_setg(errp, > > > >> + "iommu_platform=true is not supported by the > > > >> device"); > > > >> + return; > > > >> + } > > > >> } else { > > > >> vdev->dma_as = &address_space_memory; > > > >> } > > > >> > > > >> base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff > > > > > > > >
On Thu, Apr 28, 2022 at 01:52:46PM +0800, Jason Wang wrote: > On Thu, Apr 28, 2022 at 12:57 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > 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? > > Somehow, we need a fix on top. > > Thanks OK ... Halil what is your take here? I'd rather have it all lined up not applied piecemeal ... > > > > > > > > > > >> + error_setg(errp, > > > > >> + "iommu_platform=true is not supported by the > > > > >> device"); > > > > >> + return; > > > > >> + } > > > > >> } else { > > > > >> vdev->dma_as = &address_space_memory; > > > > >> } > > > > >> > > > > >> base-commit: 0d564a3e32ba8494014c67cdd2ebf0fb71860dff > > > > > > > > > > >
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; }