Re: [PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported
On Fri, 4 Feb 2022 20:15:27 -0500 "Michael S. Tsirkin" wrote: > On Sat, Feb 05, 2022 at 01:02:05AM +0100, Halil Pasic wrote: > > On Fri, 4 Feb 2022 08:05:25 -0500 > > "Michael S. Tsirkin" wrote: > > > > > On Thu, Feb 03, 2022 at 05:06:35PM +0100, Halil Pasic wrote: > > > > On Wed, 2 Feb 2022 20:54:38 +0100 > > > > Halil Pasic wrote: > > > > > > > > > } > > > > > @@ -82,9 +78,14 @@ 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(>host_features, > > > > > VIRTIO_F_IOMMU_PLATFORM); > > > > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > > > > +if (!vdev_has_iommu && vdev->dma_as != > > > > > _space_memory) { > > > > > +error_setg(errp, > > > > > + "iommu_platform=true is not supported by the > > > > > device"); > > > > > +} > > > > > > > > I'm wondering, would it be wise to change the message? Since this is now > > > > dependent on the VM/bus the device is plugged into the message might be > > > > a > > > > little misleading: i.e. the very same device could work perfectly fine > > > > with iommu_platform=true if the "surroundings" are different. > > > > > > > > Maybe "the device has no IOMMU support (iommu_platform=true)" would be a > > > > better option. On the other hand changing the message has its downsides > > > > as well. > > > > > > I personally don't care much frankly. > > > > > > > Also I realized that keeping the return after error_setg() might have > > > > been a good idea for the case more logic is added at the end of the > > > > function. > > > > > > OK so you are sending v5 with this change then? > > > > As stated below, I would prefer to get this merged. If I change the > > message, I guess I have to drop the r-b's and the I'm sure the if > > somebody decides to add logic to the end of the function that person > > will be careful enough. > > yes but you wrote about return after error_setg above. > I've sent a v5 with the extra return. Please pick v5 or v4 whichever you prefer :D Cheers, Halil
Re: [PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported
On Sat, Feb 05, 2022 at 01:02:05AM +0100, Halil Pasic wrote: > On Fri, 4 Feb 2022 08:05:25 -0500 > "Michael S. Tsirkin" wrote: > > > On Thu, Feb 03, 2022 at 05:06:35PM +0100, Halil Pasic wrote: > > > On Wed, 2 Feb 2022 20:54:38 +0100 > > > Halil Pasic wrote: > > > > > > > } > > > > @@ -82,9 +78,14 @@ 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(>host_features, > > > > VIRTIO_F_IOMMU_PLATFORM); > > > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > > > +if (!vdev_has_iommu && vdev->dma_as != _space_memory) { > > > > +error_setg(errp, > > > > + "iommu_platform=true is not supported by the > > > > device"); > > > > +} > > > > > > I'm wondering, would it be wise to change the message? Since this is now > > > dependent on the VM/bus the device is plugged into the message might be a > > > little misleading: i.e. the very same device could work perfectly fine > > > with iommu_platform=true if the "surroundings" are different. > > > > > > Maybe "the device has no IOMMU support (iommu_platform=true)" would be a > > > better option. On the other hand changing the message has its downsides > > > as well. > > > > I personally don't care much frankly. > > > > > Also I realized that keeping the return after error_setg() might have > > > been a good idea for the case more logic is added at the end of the > > > function. > > > > OK so you are sending v5 with this change then? > > As stated below, I would prefer to get this merged. If I change the > message, I guess I have to drop the r-b's and the I'm sure the if > somebody decides to add logic to the end of the function that person > will be careful enough. yes but you wrote about return after error_setg above. > > > > > In any case I would like to address these, if necessary with a separate > > > patch. I don't want the fix to experience any further delays. > > > > > > Regards, > > > Halil > >
Re: [PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported
On Fri, 4 Feb 2022 08:05:25 -0500 "Michael S. Tsirkin" wrote: > On Thu, Feb 03, 2022 at 05:06:35PM +0100, Halil Pasic wrote: > > On Wed, 2 Feb 2022 20:54:38 +0100 > > Halil Pasic wrote: > > > > > } > > > @@ -82,9 +78,14 @@ 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(>host_features, > > > VIRTIO_F_IOMMU_PLATFORM); > > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > > +if (!vdev_has_iommu && vdev->dma_as != _space_memory) { > > > +error_setg(errp, > > > + "iommu_platform=true is not supported by the > > > device"); > > > +} > > > > I'm wondering, would it be wise to change the message? Since this is now > > dependent on the VM/bus the device is plugged into the message might be a > > little misleading: i.e. the very same device could work perfectly fine > > with iommu_platform=true if the "surroundings" are different. > > > > Maybe "the device has no IOMMU support (iommu_platform=true)" would be a > > better option. On the other hand changing the message has its downsides > > as well. > > I personally don't care much frankly. > > > Also I realized that keeping the return after error_setg() might have > > been a good idea for the case more logic is added at the end of the > > function. > > OK so you are sending v5 with this change then? As stated below, I would prefer to get this merged. If I change the message, I guess I have to drop the r-b's and the I'm sure the if somebody decides to add logic to the end of the function that person will be careful enough. > > > In any case I would like to address these, if necessary with a separate > > patch. I don't want the fix to experience any further delays. > > > > Regards, > > Halil >
Re: [PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported
On Thu, Feb 03, 2022 at 05:06:35PM +0100, Halil Pasic wrote: > On Wed, 2 Feb 2022 20:54:38 +0100 > Halil Pasic wrote: > > > } > > @@ -82,9 +78,14 @@ 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(>host_features, VIRTIO_F_IOMMU_PLATFORM); > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > +if (!vdev_has_iommu && vdev->dma_as != _space_memory) { > > +error_setg(errp, > > + "iommu_platform=true is not supported by the > > device"); > > +} > > I'm wondering, would it be wise to change the message? Since this is now > dependent on the VM/bus the device is plugged into the message might be a > little misleading: i.e. the very same device could work perfectly fine > with iommu_platform=true if the "surroundings" are different. > > Maybe "the device has no IOMMU support (iommu_platform=true)" would be a > better option. On the other hand changing the message has its downsides > as well. I personally don't care much frankly. > Also I realized that keeping the return after error_setg() might have > been a good idea for the case more logic is added at the end of the > function. OK so you are sending v5 with this change then? > In any case I would like to address these, if necessary with a separate > patch. I don't want the fix to experience any further delays. > > Regards, > Halil
Re: [PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported
On Wed, 2 Feb 2022 20:54:38 +0100 Halil Pasic wrote: > } > @@ -82,9 +78,14 @@ 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(>host_features, VIRTIO_F_IOMMU_PLATFORM); > vdev->dma_as = klass->get_dma_as(qbus->parent); > +if (!vdev_has_iommu && vdev->dma_as != _space_memory) { > +error_setg(errp, > + "iommu_platform=true is not supported by the device"); > +} I'm wondering, would it be wise to change the message? Since this is now dependent on the VM/bus the device is plugged into the message might be a little misleading: i.e. the very same device could work perfectly fine with iommu_platform=true if the "surroundings" are different. Maybe "the device has no IOMMU support (iommu_platform=true)" would be a better option. On the other hand changing the message has its downsides as well. Also I realized that keeping the return after error_setg() might have been a good idea for the case more logic is added at the end of the function. In any case I would like to address these, if necessary with a separate patch. I don't want the fix to experience any further delays. Regards, Halil
[PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported
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 Reported-by: Jakob Naucke Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but unsupported") Acked-by: Cornelia Huck Reviewed-by: Daniel Henrique Barboza Tested-by: Daniel Henrique Barboza Cc: Kevin Wolf Cc: qemu-sta...@nongnu.org --- 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 | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index d23db98c56..fbf0dd14b8 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, _err); } @@ -82,9 +78,14 @@ 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(>host_features, VIRTIO_F_IOMMU_PLATFORM); vdev->dma_as = klass->get_dma_as(qbus->parent); +if (!vdev_has_iommu && vdev->dma_as != _space_memory) { +error_setg(errp, + "iommu_platform=true is not supported by the device"); +} } else { vdev->dma_as = _space_memory; } base-commit: 6621441db50d5bae7e34dbd04bf3c57a27a71b32 -- 2.32.0