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
