Re: [PATCH v7 3/5] virtio-iommu: Call iommu notifier for attach/detach

[Bharat Bhushan]

Hi Eric,

On Mon, Mar 16, 2020 at 1:02 PM Auger Eric <address@hidden> wrote:
>
> Hi Bharat,
>
> On 3/16/20 7:41 AM, Bharat Bhushan wrote:
> > Hi Eric,
> >
> > On Fri, Mar 13, 2020 at 8:11 PM Auger Eric <address@hidden> wrote:
> >>
> >> Hi Bharat
> >>
> >> On 3/13/20 8:48 AM, Bharat Bhushan wrote:
> >>> iommu-notifier are called when a device is attached
> >> IOMMU notifiers
> >>> or detached to as address-space.
> >>> This is needed for VFIO.
> >> and vhost for detach
> >>>
> >>> Signed-off-by: Bharat Bhushan <address@hidden>
> >>> ---
> >>>  hw/virtio/virtio-iommu.c | 47 ++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 47 insertions(+)
> >>>
> >>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> >>> index e51344a53e..2006f72901 100644
> >>> --- a/hw/virtio/virtio-iommu.c
> >>> +++ b/hw/virtio/virtio-iommu.c
> >>> @@ -49,6 +49,7 @@ typedef struct VirtIOIOMMUEndpoint {
> >>>      uint32_t id;
> >>>      VirtIOIOMMUDomain *domain;
> >>>      QLIST_ENTRY(VirtIOIOMMUEndpoint) next;
> >>> +    VirtIOIOMMU *viommu;
> >> This needs specal care on post-load. When migrating the EPs, only the id
> >> is migrated. On post-load you need to set viommu as it is done for
> >> domain. migration is allowed with vhost.
> >
> > ok, I have not tried vhost/migration. Below change set viommu when
> > reconstructing endpoint.
>
>
> Yes I think this should be OK.
>
> By the end I did the series a try with vhost/vfio. with vhost it works
> (not with recent kernel though, but the issue may be related to kernel).
> With VFIO however it does not for me.
>
> First issue is: your guest can use 4K page and your host can use 64KB
> pages. In that case VFIO_DMA_MAP will fail with -EINVAL. We must devise
> a way to pass the host settings to the VIRTIO-IOMMU device.
>
> Even with 64KB pages, it did not work for me. I have obviously not the
> storm of VFIO_DMA_MAP failures but I have some, most probably due to
> some wrong notifications somewhere. I will try to investigate on my side.
>
> Did you test with VFIO on your side?

I did not tried with different page sizes, only tested with 4K page size.

Yes it works, I tested with two n/w device assigned to VM, both interfaces works

First I will try with 64k page size.

Thanks
-Bharat

>
> Thanks
>
> Eric
> >
> > @@ -984,6 +973,7 @@ static gboolean reconstruct_endpoints(gpointer
> > key, gpointer value,
> >
> >      QLIST_FOREACH(iter, &d->endpoint_list, next) {
> >          iter->domain = d;
> > +       iter->viommu = s;
> >          g_tree_insert(s->endpoints, GUINT_TO_POINTER(iter->id), iter);
> >      }
> >      return false; /* continue the domain traversal */
> >
> >>>  } VirtIOIOMMUEndpoint;
> >>>
> >>>  typedef struct VirtIOIOMMUInterval {
> >>> @@ -155,8 +156,44 @@ static void 
> >>> virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr iova,
> >>>      memory_region_notify_iommu(mr, 0, entry);
> >>>  }
> >>>
> >>> +static gboolean virtio_iommu_mapping_unmap(gpointer key, gpointer value,
> >>> +                                           gpointer data)
> >>> +{
> >>> +    VirtIOIOMMUInterval *interval = (VirtIOIOMMUInterval *) key;
> >>> +    IOMMUMemoryRegion *mr = (IOMMUMemoryRegion *) data;
> >>> +
> >>> +    virtio_iommu_notify_unmap(mr, interval->low,
> >>> +                              interval->high - interval->low + 1);
> >>> +
> >>> +    return false;
> >>> +}
> >>> +
> >>> +static gboolean virtio_iommu_mapping_map(gpointer key, gpointer value,
> >>> +                                         gpointer data)
> >>> +{
> >>> +    VirtIOIOMMUMapping *mapping = (VirtIOIOMMUMapping *) value;
> >>> +    VirtIOIOMMUInterval *interval = (VirtIOIOMMUInterval *) key;
> >>> +    IOMMUMemoryRegion *mr = (IOMMUMemoryRegion *) data;
> >>> +
> >>> +    virtio_iommu_notify_map(mr, interval->low, mapping->phys_addr,
> >>> +                            interval->high - interval->low + 1);
> >>> +
> >>> +    return false;
> >>> +}
> >>> +
> >>>  static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint 
> >>> *ep)
> >>>  {
> >>> +    VirtioIOMMUNotifierNode *node;
> >>> +    VirtIOIOMMU *s = ep->viommu;
> >>> +    VirtIOIOMMUDomain *domain = ep->domain;
> >>> +
> >>> +    QLIST_FOREACH(node, &s->notifiers_list, next) {
> >>> +        if (ep->id == node->iommu_dev->devfn) {
> >>> +            g_tree_foreach(domain->mappings, virtio_iommu_mapping_unmap,
> >>> +                           &node->iommu_dev->iommu_mr);
> >> I understand this should fo the job for domain removal
> >
> > did not get the comment, are you saying we should do this on domain removal?
> see my reply on 2/5
>
> Note the above code should be moved after the check of !ep->domain below

ohh yes, will move

Thanks
-Bharat

> >
> >>> +        }
> >>> +    }
> >>> +
> >>>      if (!ep->domain) {
> >>>          return;
> >>>      }
> >>> @@ -178,6 +215,7 @@ static VirtIOIOMMUEndpoint 
> >>> *virtio_iommu_get_endpoint(VirtIOIOMMU *s,
> >>>      }
> >>>      ep = g_malloc0(sizeof(*ep));
> >>>      ep->id = ep_id;
> >>> +    ep->viommu = s;
> >>>      trace_virtio_iommu_get_endpoint(ep_id);
> >>>      g_tree_insert(s->endpoints, GUINT_TO_POINTER(ep_id), ep);
> >>>      return ep;
> >>> @@ -272,6 +310,7 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
> >>>  {
> >>>      uint32_t domain_id = le32_to_cpu(req->domain);
> >>>      uint32_t ep_id = le32_to_cpu(req->endpoint);
> >>> +    VirtioIOMMUNotifierNode *node;
> >>>      VirtIOIOMMUDomain *domain;
> >>>      VirtIOIOMMUEndpoint *ep;
> >>>
> >>> @@ -299,6 +338,14 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
> >>>
> >>>      ep->domain = domain;
> >>>
> >>> +    /* Replay existing address space mappings on the associated memory 
> >>> region */
> >> maybe use the "domain" terminology here.
> >
> > ok,
> >
> > Thanks
> > -Bharat
> >
> >>> +    QLIST_FOREACH(node, &s->notifiers_list, next) {
> >>> +        if (ep_id == node->iommu_dev->devfn) {
> >>> +            g_tree_foreach(domain->mappings, virtio_iommu_mapping_map,
> >>> +                           &node->iommu_dev->iommu_mr);
> >>> +        }
> >>> +    }
> >>> +
> >>>      return VIRTIO_IOMMU_S_OK;
> >>>  }
> >>>
> >>>
> >> Thanks
> >>
> >> Eric
> >>
> >
>