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

[Halil Pasic]

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