Re: [PATCH v4 1/1] virtio: fix the condition for iommu_platform not supported

[Michael S. Tsirkin]

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