Message ID | 20220202195438.379753-1-pasic@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v4,1/1] virtio: fix the condition for iommu_platform not supported | expand |
On Wed, 2 Feb 2022 20:54:38 +0100 Halil Pasic <pasic@linux.ibm.com> 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(&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"); > + } 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
On Thu, Feb 03, 2022 at 05:06:35PM +0100, Halil Pasic wrote: > On Wed, 2 Feb 2022 20:54:38 +0100 > Halil Pasic <pasic@linux.ibm.com> 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(&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"); > > + } > > 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
On Fri, 4 Feb 2022 08:05:25 -0500 "Michael S. Tsirkin" <mst@redhat.com> 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 <pasic@linux.ibm.com> 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(&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"); > > > + } > > > > 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 >
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" <mst@redhat.com> 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 <pasic@linux.ibm.com> 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(&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"); > > > > + } > > > > > > 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 > >
On Fri, 4 Feb 2022 20:15:27 -0500 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 <pasic@linux.ibm.com> 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(&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"); > > > > > + } > > > > > > > > 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
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, &local_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(&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"); + } } else { vdev->dma_as = &address_space_memory; }