On 12/18/2019 5:42 AM, Alex Williamson wrote:
On Tue, 17 Dec 2019 22:40:51 +0530
Kirti Wankhede <kwankhede@xxxxxxxxxx> 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 <alex.williamson@xxxxxxxxxx> 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;
}
}
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.
vfio_pin_pages() could return
-EINVAL if (!group->is_singleton).
pass the IOMMU group as a parameter to
vfio_iommu_driver_ops.pin_pages(), then the type1 backend can set a
flag on its local vfio_group struct to indicate dirty page scope is
limited to pinned pages.
ie. vfio_iommu_type1_unpin_pages() calls find_iommu_group() on each
domain in domain_list and the external_domain using the struct
iommu_group pointer provided by vfio-core. We set a new attribute on
the vfio_group to indicate that vfio_group has (at some point) pinned
pages.
We might want to keep a flag on the
vfio_iommu struct to indicate if all of the vfio_groups for each
vfio_domain in the vfio_iommu.domain_list dirty page scope limited to
pinned pages as an optimization to avoid walking lists too often. Then
we could test if vfio_iommu.domain_list is not empty and this new flag
does not limit the dirty page scope, then everything within each
vfio_dma is considered dirty.
So at the point where we change vfio_group.has_pinned_pages from false
to true, or a group is added or removed, we walk all the groups in the
vfio_iommu and if they all have has_pinned_pages set, we can set a
vfio_iommu.pinned_page_dirty_scope flag to true. If that flag is
already true on page pinning, we can skip the lookup.
I still like this approach better, it doesn't require a callback from
type1 to vfio-core and it doesn't require a heavy weight walking for
group devices and vfio data structures every time we fill a bitmap.
Did you run into issues trying to implement this approach?
Thanks for elaborative steps.
This works. Changing this last commit.
Thanks,
Kirti
