Re: [Qemu-devel] [PATCH v4 10/10] vhost: iommu: cache static mapping if there is

[Peter Xu]

On Fri, May 19, 2017 at 07:55:26PM +0300, Michael S. Tsirkin wrote:
> On Fri, May 19, 2017 at 11:19:49AM +0800, Peter Xu wrote:
> > This patch pre-heat vhost iotlb cache when passthrough mode enabled.
> > 
> > Sometimes, even if user specified iommu_platform for vhost devices,
> > IOMMU might still be disabled. One case is passthrough mode in VT-d
> > implementation. We can detect this by observing iommu_list. If it's
> > empty, it means IOMMU translation is disabled, then we can actually
> > pre-heat the translation (it'll be static mapping then) by first
> > invalidating all IOTLB, then cache existing memory ranges into vhost
> > backend iotlb using 1:1 mapping.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> 
> I don't really understand. Is this a performance optimization?
> Can you post some #s please?

Yes, it is. Vhost can work even without this patch, but it should be
faster when with this patch.

As mentioned in the commit message and below comment [1], this patch
pre-heat the cache for vhost. Currently the cache entries depends on
the system memory ranges (dev->mem->nregions), and it should be far
smaller than vhost's cache count (currently it is statically defined
as max_iotlb_entries=2048 in kernel). If with current patch, these
cache entries can cover the whole possible DMA ranges that PT mode
would allow, so we won't have any cache miss then.

For the comments, do you have any better suggestion besides commit
message and [1]?

> 
> Also, if it's PT, can't we bypass iommu altogether? That would be
> even faster ...

Yes, but I don't yet know a good way to do it... Any suggestion is
welcomed as well.

Btw, do you have any comment on other patches besides this one? Since
this patch can really be isolated from the whole PT support series.

Thanks,

> 
> > ---
> >  hw/virtio/trace-events |  4 ++++
> >  hw/virtio/vhost.c      | 49 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > index 1f7a7c1..54dcbb3 100644
> > --- a/hw/virtio/trace-events
> > +++ b/hw/virtio/trace-events
> > @@ -24,3 +24,7 @@ virtio_balloon_handle_output(const char *name, uint64_t 
> > gpa) "section name: %s g
> >  virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: 
> > %d actual: %d"
> >  virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d 
> > oldactual: %d"
> >  virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon 
> > target: %"PRIx64" num_pages: %d"
> > +
> > +# hw/virtio/vhost.c
> > +vhost_iommu_commit(void) ""
> > +vhost_iommu_static_preheat(void) ""
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 03a46a7..8069135 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -27,6 +27,7 @@
> >  #include "hw/virtio/virtio-access.h"
> >  #include "migration/blocker.h"
> >  #include "sysemu/dma.h"
> > +#include "trace.h"
> >  
> >  /* enabled until disconnected backend stabilizes */
> >  #define _VHOST_DEBUG 1
> > @@ -730,6 +731,11 @@ static void vhost_iommu_unmap_notify(IOMMUNotifier *n, 
> > IOMMUTLBEntry *iotlb)
> >      }
> >  }
> >  
> > +static bool vhost_iommu_mr_enabled(struct vhost_dev *dev)
> > +{
> > +    return !QLIST_EMPTY(&dev->iommu_list);
> > +}
> > +
> >  static void vhost_iommu_region_add(MemoryListener *listener,
> >                                     MemoryRegionSection *section)
> >  {
> > @@ -782,6 +788,48 @@ static void vhost_iommu_region_del(MemoryListener 
> > *listener,
> >      }
> >  }
> >  
> > +static void vhost_iommu_commit(MemoryListener *listener)
> > +{
> > +    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> > +                                         iommu_listener);
> > +    struct vhost_memory_region *r;
> > +    int i;
> > +
> > +    trace_vhost_iommu_commit();
> > +
> > +    if (!vhost_iommu_mr_enabled(dev)) {
> > +        /*
> > +        * This means iommu_platform is enabled, however iommu memory
> > +        * region is disabled, e.g., when device passthrough is setup.
> > +        * Then, no translation is needed any more.
> > +        *
> > +        * Let's first invalidate the whole IOTLB, then pre-heat the
> > +        * static mapping by looping over vhost memory ranges.
> > +        */

[1]

> > +
> > +        if (dev->vhost_ops->vhost_invalidate_device_iotlb(dev, 0,
> > +                                                          UINT64_MAX)) {
> > +            error_report("%s: flush existing IOTLB failed", __func__);
> > +            return;
> > +        }
> > +
> > +        for (i = 0; i < dev->mem->nregions; i++) {
> > +            r = &dev->mem->regions[i];
> > +            /* Vhost regions are writable RAM, so IOMMU_RW suites. */
> > +            if (dev->vhost_ops->vhost_update_device_iotlb(dev,
> > +                                                          
> > r->guest_phys_addr,
> > +                                                          
> > r->userspace_addr,
> > +                                                          r->memory_size,
> > +                                                          IOMMU_RW)) {
> > +                error_report("%s: pre-heat static mapping failed", 
> > __func__);
> > +                return;
> > +            }
> > +        }
> > +
> > +        trace_vhost_iommu_static_preheat();
> > +    }
> > +}
> > +
> >  static void vhost_region_nop(MemoryListener *listener,
> >                               MemoryRegionSection *section)
> >  {
> > @@ -1298,6 +1346,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
> > *opaque,
> >      hdev->iommu_listener = (MemoryListener) {
> >          .region_add = vhost_iommu_region_add,
> >          .region_del = vhost_iommu_region_del,
> > +        .commit = vhost_iommu_commit,
> >      };
> >  
> >      if (hdev->migration_blocker == NULL) {
> > -- 
> > 2.7.4

-- 
Peter Xu