Re: [PATCH v11 Kernel 6/6] vfio: Selective dirty page tracking if IOMMU backed device pins pages

[Alex Williamson]

On Thu, 9 Jan 2020 02:22:26 +0530
Kirti Wankhede <address@hidden> wrote:

> On 1/8/2020 5:39 AM, Alex Williamson wrote:
> > On Wed, 8 Jan 2020 02:15:01 +0530
> > Kirti Wankhede <address@hidden> wrote:
> >   
> >> On 12/18/2019 5:42 AM, Alex Williamson wrote:  
> >>> On Tue, 17 Dec 2019 22:40:51 +0530
> >>> Kirti Wankhede <address@hidden> wrote:
> >>>      
> >>
> >> <snip>
> >>  
> >>>
> >>> This will fail when there are devices within the IOMMU group that are
> >>> not represented as vfio_devices.  My original suggestion was:
> >>>
> >>> On Thu, 14 Nov 2019 14:06:25 -0700
> >>> Alex Williamson <address@hidden> wrote:  
> >>>> I think it does so by pinning pages.  Is it acceptable that if the
> >>>> vendor driver pins any pages, then from that point forward we consider
> >>>> the IOMMU group dirty page scope to be limited to pinned pages?  There
> >>>> are complications around non-singleton IOMMU groups, but I think we're
> >>>> already leaning towards that being a non-worthwhile problem to solve.
> >>>> So if we require that only singleton IOMMU groups can pin pages and we  
> >>>
> >>> We could tag vfio_groups as singleton at vfio_add_group_dev() time with
> >>> an iommu_group_for_each_dev() walk so that we can cache the value on
> >>> the struct vfio_group.  
> >>
> >> I don't think iommu_group_for_each_dev() is required. Checking
> >> group->device_list in vfio_add_group_dev() if there are more than one
> >> device should work, right?
> >>
> >>           list_for_each_entry(vdev, &group->device_list, group_next) {
> >>                   if (group->is_singleton) {
> >>                           group->is_singleton = false;
> >>                           break;
> >>                   } else {
> >>                           group->is_singleton = true;
> >>                   }
> >>           }  
> > 
> > Hmm, I think you're taking a different approach to this than I was
> > thinking.  Re-reading my previous comments, the fact that both vfio.c
> > and vfio_iommu_type1.c each have their own private struct vfio_group
> > makes things rather unclear.  I was intending to use the struct
> > iommu_group as the object vfio.c provides to type1.c to associate the
> > pinning.  This would require that not only the vfio view of devices in
> > the group to be singleton, but also the actual iommu group to be
> > singleton.  Otherwise the set of devices vfio.c has in the group might
> > only be a subset of the group.  Maybe a merger of the approaches is
> > easier though.
> > 
> > Tracking whether the vfio.c view of a group is singleton is even easier
> > than above, we could simply add a device_count field to vfio_group,
> > increment it in vfio_group_create_device() and decrement it in
> > vfio_device_release().  vfio_pin_pages() could return error if
> > device_count is not 1.  We could still add the iommu_group pointer to
> > the type1 pin_pages callback, but perhaps type1 simply assumes that the
> > group is singleton when pin pages is called and it's vfio.c's
> > responsibility to maintain that group as singleton once pages have been
> > pinned.  vfio.c would therefore also need to set a field on the
> > vfio_group if pages have been pinned such that vfio_add_group_dev()
> > could return error if a new device attempts to join the group.  We'd
> > need to make sure that field is cleared when the group is released from
> > use and pay attention to races that might occur between adding devices
> > to a group and pinning pages.
> >   
> 
> Thinking aloud, will adding singleton check could cause issues in near 
> future? - may be in future support for p2p and direct RDMA will be added 
> for mdev devices. In that case the two devices should be in same 
> iommu_domain, but should be in different iommu_group - is that 
> understanding correct?

The ACS redirection stuff is the only thing that actually changes iommu
grouping relative to p2p/RDMA and that's specifically for untranslated
DMA, aiui.  If we wanted translated p2p DMA to be routed downstream of
the IOMMU we'd need to enable ACS direct translation.  In any case,
those are about shortening the path for p2p between devices.  We
actually don't even need devices to be in the same iommu domain to
allow p2p, we only need the iommu domain for each respective device to
map the mmio spaces of the other device.  I don't think we're doing
anything here that would cause us trouble later in this space, but it's
also just a policy decision, we wouldn't be breaking ABI to change the
implementation later.
 
> >>> vfio_group_nb_add_dev() could update this if
> >>> the IOMMU group composition changes.  
> >>
> >> I don't see vfio_group_nb_add_dev() calls vfio_add_group_dev() (?)
> >> If checking is_singleton is taken care in vfio_group_nb_add_dev(), which
> >> is the only place where vfio_group is allocated, that should work, I 
> >> think.  
> > 
> > This was relative to maintaining that the iommu group itself is
> > singleton, not just the vfio view of the group.  If we use the latter
> > as our basis, then you're right, we should need this, but vfio.c would
> > need to enforce that the group remains singleton if it has pinned
> > pages.  Does that make sense?  Thanks,
> >   
> 
> Which route should be taken - iommu_group view or vfio.c group view?

The latter seems easier, more flexible, and lower overhead as far as I
can see.  Thanks,

Alex