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

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 2 Feb 2022 02:06:12 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

[..]
> > In my opinion not forcing the guest to negotiate IOMMU_PLATFORM when  
> > ->get_dma_as() is not set is at least unfortunate. Please observe, that  
> > virtio-pci is not affected by this omission because for virtio-pci
> > devices ->get_dma_as != NULL always holds. And what is the deal for
> > devices that don't implement get_dma_as() (and don't need address
> > translation)? If iommu_platform=on is justified (no user error) then
> > the device does not have access to the entire guest memory. Which
> > means it more than likely needs cooperation form the guest (driver).
> > So detecting that the guest does not support IOMMU_PLATFORM and failing
> > gracefully via virtio_validate_features() instead of carrying on
> > in good faith and failing in ugly ways when the host attempts to access
> > guest memory to which it does not have access to. If we assume user
> > error, that is the host can access at least all the memory it needs
> > to access to make that device work, then it is probably still a
> > good idea to fail the device and thus help the user correct his
> > error.
> > 
> > IMHO the best course of action is
> > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > index 34f5a0a664..1d0eb16d1c 100644
> > --- a/hw/virtio/virtio-bus.c
> > +++ b/hw/virtio/virtio-bus.c
> > @@ -80,7 +80,6 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> >  
> >      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,
> > @@ -89,6 +88,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> >      } else {
> >          vdev->dma_as = &address_space_memory;
> >      }
> > +    virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> >  }
> > 
> > which would be a separate patch, as this is a separate issue. Jason,
> > Michael, Connie, what do you think?  
> 
> Do you mean just force VIRTIO_F_IOMMU_PLATFORM for everyone?
> Or am I misreading the patch?

Yes. Where force means: prevent the driver from setting FEATURES_OK
if it cleared VIRTIO_F_IOMMU_PLATFORM. I really don't see the case
where the device offering but the driver not accepting
VIRTIO_F_IOMMU_PLATFORM is good and useful.

Regards,
Halil


> 
> 
> > Regards,
> > Halil  
> 
>