Re: [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 02:20:36AM -0500, Jason Wang wrote:
> 
> 
> ----- Original Message -----
> > On Wed, Feb 26, 2020 at 03:06:47PM +0800, Jason Wang wrote:
> > > We turn on device IOTLB via VIRTIO_F_IOMMU_PLATFORM unconditionally on
> > > platform without IOMMU support. This can lead unnecessary IOTLB
> > > transactions which will damage the performance.
> > > 
> > > Fixing this by check whether the device is backed by IOMMU and disable
> > > device IOTLB.
> > > 
> > > Reported-by: Halil Pasic <address@hidden>
> > > Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> > > Signed-off-by: Jason Wang <address@hidden>
> > > ---
> > >  hw/virtio/vhost.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 9edfadc81d..6e12c3d2de 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -290,7 +290,14 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
> > >  {
> > >      VirtIODevice *vdev = dev->vdev;
> > >  
> > > -    return virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> > > +    /*
> > > +     * For vhost, VIRTIO_F_IOMMU_PLATFORM means the backend support
> > > +     * incremental memory mapping API via IOTLB API. For platform that
> > > +     * does not have IOMMU, there's no need to enable this feature
> > > +     * which may cause unnecessary IOTLB miss/update trnasactions.
> > > +     */
> > > +    return vdev->dma_as != &address_space_memory &&
> > > +           virtio_has_feature(dev->acked_features,
> > > VIRTIO_F_IOMMU_PLATFORM);
> > >  }
> > >  
> > >  static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
> > 
> > Why check acked_features and not host features here?
> > I'd worry that if we do it like this, userspace driver
> > within guest can clear the feature and make device access
> > memory directly.
> 
> Right, host_features should be more than enough.
> 
> > 
> > > @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev
> > > *dev,
> > >      if (enable_log) {
> > >          features |= 0x1ULL << VHOST_F_LOG_ALL;
> > >      }
> > > +    if (dev->vdev->dma_as == &address_space_memory) {
> > > +        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
> > > +    }
> > 
> > 
> > That's a guest visible change. Which seems at best unnecessary.
> >
> 
> I don't get how this can be visible from guest? It works as F_LOG_ALL.
> 
> Thanks

Oh you are right.
So just call vhost_dev_has_iommu here too?

> > >      r = dev->vhost_ops->vhost_set_features(dev, features);
> > >      if (r < 0) {
> > >          VHOST_OPS_DEBUG("vhost_set_features failed");
> > > --
> > > 2.19.1
> > 
> >