On Thu, Jun 16, 2022 at 07:08:10AM +0000, Tian, Kevin wrote: > ... > > +static struct vfio_domain * > > +vfio_iommu_alloc_attach_domain(struct bus_type *bus, struct vfio_iommu > > *iommu, > > + struct vfio_iommu_group *group) > > +{ > > + struct iommu_domain *new_domain; > > + struct vfio_domain *domain; > > + int ret = 0; > > + > > + /* Try to match an existing compatible domain */ > > + list_for_each_entry (domain, &iommu->domain_list, next) { > > + ret = iommu_attach_group(domain->domain, group- > > >iommu_group); > > + if (ret == -EMEDIUMTYPE) > > + continue; > > Probably good to add one line comment here for what EMEDIUMTYPE > represents. It's not a widely-used retry type like EAGAIN. A comment > can save the time of digging out the fact by jumping to iommu file.
Sure. I can add that. > ... > > - if (resv_msi) { > > + if (resv_msi && !domain->msi_cookie) { > > ret = iommu_get_msi_cookie(domain->domain, > > resv_msi_base); > > if (ret && ret != -ENODEV) > > goto out_detach; > > + domain->msi_cookie = true; > > } > > why not moving to alloc_attach_domain() then no need for the new > domain field? It's required only when a new domain is allocated. When reusing an existing domain that doesn't have an msi_cookie, we can do iommu_get_msi_cookie() if resv_msi is found. So it is not limited to a new domain. > ... > > - if (list_empty(&domain->group_list)) { > > - if (list_is_singular(&iommu->domain_list)) { > > - if (list_empty(&iommu- > > >emulated_iommu_groups)) { > > - WARN_ON(iommu->notifier.head); > > - > > vfio_iommu_unmap_unpin_all(iommu); > > - } else { > > - > > vfio_iommu_unmap_unpin_reaccount(iommu); > > - } > > - } > > - iommu_domain_free(domain->domain); > > - list_del(&domain->next); > > - kfree(domain); > > - vfio_iommu_aper_expand(iommu, &iova_copy); > > Previously the aperture is adjusted when a domain is freed... > > > - vfio_update_pgsize_bitmap(iommu); > > - } > > - /* > > - * Removal of a group without dirty tracking may allow > > - * the iommu scope to be promoted. > > - */ > > - if (!group->pinned_page_dirty_scope) { > > - iommu->num_non_pinned_groups--; > > - if (iommu->dirty_page_tracking) > > - vfio_iommu_populate_bitmap_full(iommu); > > - } > > + vfio_iommu_detach_destroy_domain(domain, iommu, > > group); > > kfree(group); > > break; > > } > > > > + vfio_iommu_aper_expand(iommu, &iova_copy); > > but now it's done for every group detach. The aperture is decided > by domain geometry which is not affected by attached groups. Yea, I've noticed this part. Actually Jason did this change for simplicity, and I think it'd be safe to do so? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu