Hi Yi, On Sun, 20 Dec 2020 08:03:51 +0800, Liu Yi L <yi.l....@intel.com> wrote:
> In existing code, if wanting to loop all devices attached to a domain, > current code can only loop the devices which are attached to the domain > via normal manner. While for devices attached via auxiliary manner, this > is subdevice, they are not tracked in the domain. This patch adds struct How about "In the existing code, loop all devices attached to a domain does not include sub-devices attached via iommu_aux_attach_device()." > subdevice_domain_info which is created per domain attachment via auxiliary > manner. So that such devices are also tracked in domain. > > This was found by when I'm working on the belwo patch, There is no device > in domain->devices, thus unable to get the cap and ecap of iommu unit. But > this domain actually has one sub-device which is attached via aux-manner. > This patch fixes the issue. > > https://lore.kernel.org/kvm/1599734733-6431-17-git-send-email-yi.l....@intel.com/ > > But looks like, it doesn't affect me only. Such auxiliary track should be > there for example if wanting to flush device_iotlb for a domain which has > devices attached by auxiliray manner, then this fix is also necessary. Perhaps: This fix goes beyond the patch above, such sub-device tracking is necessary for other cases. For example, flushing device_iotlb for a domain which has sub-devices attached by auxiliary manner. > This issue will also be fixed by another patch in this series with some > additional changes based on the sudevice tracking framework introduced in > this patch. > > Co-developed-by: Xin Zeng <xin.z...@intel.com> > Signed-off-by: Xin Zeng <xin.z...@intel.com> > Co-developed-by: Liu Yi L <yi.l....@intel.com> > Signed-off-by: Liu Yi L <yi.l....@intel.com> > Co-developed-by: Lu Baolu <baolu...@linux.intel.com> > Signed-off-by: Lu Baolu <baolu...@linux.intel.com> > --- > drivers/iommu/intel/iommu.c | 92 ++++++++++++++++++++++++++++++++----- > include/linux/intel-iommu.h | 11 ++++- > 2 files changed, 90 insertions(+), 13 deletions(-) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index a49afa11673c..4274b4acc325 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -1881,6 +1881,7 @@ static struct dmar_domain *alloc_domain(int flags) > domain->flags |= DOMAIN_FLAG_USE_FIRST_LEVEL; > domain->has_iotlb_device = false; > INIT_LIST_HEAD(&domain->devices); > + INIT_LIST_HEAD(&domain->sub_devices); > > return domain; > } > @@ -5172,33 +5173,79 @@ is_aux_domain(struct device *dev, struct > iommu_domain *domain) domain->type == IOMMU_DOMAIN_UNMANAGED; > } > > +static inline > +void _auxiliary_link_device(struct dmar_domain *domain, > + struct subdevice_domain_info *subinfo, > + struct device *dev) > +{ > + subinfo->users++; > +} > + why pass in more arguments than subinfo? the function name does not match what it does, seems just refcount inc. > +static inline > +int _auxiliary_unlink_device(struct dmar_domain *domain, > + struct subdevice_domain_info *subinfo, > + struct device *dev) > +{ > + subinfo->users--; > + return subinfo->users; ditto. why not just return subinfo->users--; > +} > + > static void auxiliary_link_device(struct dmar_domain *domain, > struct device *dev) > { > struct device_domain_info *info = get_domain_info(dev); > + struct subdevice_domain_info *subinfo; > > assert_spin_locked(&device_domain_lock); > if (WARN_ON(!info)) > return; > > + subinfo = kzalloc(sizeof(*subinfo), GFP_ATOMIC); > + if (!subinfo) > + return; > + > + subinfo->domain = domain; > + subinfo->dev = dev; > + list_add(&subinfo->link_domain, &info->auxiliary_domains); > + list_add(&subinfo->link_phys, &domain->sub_devices); > + _auxiliary_link_device(domain, subinfo, dev); or just opencode subinfo->users++? > domain->auxd_refcnt++; > - list_add(&domain->auxd, &info->auxiliary_domains); > } > > -static void auxiliary_unlink_device(struct dmar_domain *domain, > - struct device *dev) > +static struct subdevice_domain_info * > +subdevice_domain_info_lookup(struct dmar_domain *domain, struct device > *dev) +{ > + struct subdevice_domain_info *subinfo; > + > + assert_spin_locked(&device_domain_lock); > + > + list_for_each_entry(subinfo, &domain->sub_devices, link_phys) > + if (subinfo->dev == dev) > + return subinfo; > + > + return NULL; > +} > + > +static int auxiliary_unlink_device(struct dmar_domain *domain, > + struct subdevice_domain_info *subinfo, > + struct device *dev) > { > struct device_domain_info *info = get_domain_info(dev); > + int ret; > > assert_spin_locked(&device_domain_lock); > if (WARN_ON(!info)) > - return; > + return -EINVAL; > > - list_del(&domain->auxd); > + ret = _auxiliary_unlink_device(domain, subinfo, dev); > + if (ret == 0) { > + list_del(&subinfo->link_domain); > + list_del(&subinfo->link_phys); > + kfree(subinfo); > + } > domain->auxd_refcnt--; > > - if (!domain->auxd_refcnt && domain->default_pasid > 0) > - ioasid_free(domain->default_pasid); > + return ret; > } > > static int aux_domain_add_dev(struct dmar_domain *domain, > @@ -5207,6 +5254,8 @@ static int aux_domain_add_dev(struct dmar_domain > *domain, int ret; > unsigned long flags; > struct intel_iommu *iommu; > + struct device_domain_info *info = get_domain_info(dev); > + struct subdevice_domain_info *subinfo; > > iommu = device_to_iommu(dev, NULL, NULL); > if (!iommu) > @@ -5227,6 +5276,12 @@ static int aux_domain_add_dev(struct dmar_domain > *domain, } > > spin_lock_irqsave(&device_domain_lock, flags); > + subinfo = subdevice_domain_info_lookup(domain, dev); > + if (subinfo) { > + _auxiliary_link_device(domain, subinfo, dev); > + spin_unlock_irqrestore(&device_domain_lock, flags); > + return 0; > + } > /* > * iommu->lock must be held to attach domain to iommu and setup > the > * pasid entry for second level translation. > @@ -5270,6 +5325,7 @@ static void aux_domain_remove_dev(struct > dmar_domain *domain, struct device_domain_info *info; > struct intel_iommu *iommu; > unsigned long flags; > + struct subdevice_domain_info *subinfo; > > if (!is_aux_domain(dev, &domain->domain)) > return; > @@ -5278,14 +5334,26 @@ static void aux_domain_remove_dev(struct > dmar_domain *domain, info = get_domain_info(dev); > iommu = info->iommu; > > - auxiliary_unlink_device(domain, dev); > + subinfo = subdevice_domain_info_lookup(domain, dev); > + if (!subinfo) { > + spin_unlock_irqrestore(&device_domain_lock, flags); > + return; > + } > > - spin_lock(&iommu->lock); > - intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid, > false); > - domain_detach_iommu(domain, iommu); > - spin_unlock(&iommu->lock); > + if (auxiliary_unlink_device(domain, subinfo, dev) == 0) { > + spin_lock(&iommu->lock); > + intel_pasid_tear_down_entry(iommu, > + dev, > + domain->default_pasid, > + false); > + domain_detach_iommu(domain, iommu); > + spin_unlock(&iommu->lock); > + } > > spin_unlock_irqrestore(&device_domain_lock, flags); > + > + if (!domain->auxd_refcnt && domain->default_pasid > 0) > + ioasid_free(domain->default_pasid); > } > > static int prepare_domain_attach_device(struct iommu_domain *domain, > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h > index 94522685a0d9..1fb3d6ab719a 100644 > --- a/include/linux/intel-iommu.h > +++ b/include/linux/intel-iommu.h > @@ -537,7 +537,7 @@ struct dmar_domain { > > bool has_iotlb_device; > struct list_head devices; /* all devices' list */ > - struct list_head auxd; /* link to device's > auxiliary list */ > + struct list_head sub_devices; /* all devices' list > attached via aux-attach */ struct iova_domain iovad; /* iova's > that belong to this domain */ > struct dma_pte *pgd; /* virtual address */ > @@ -636,6 +636,15 @@ struct device_domain_info { > struct pasid_table *pasid_table; /* pasid table */ > }; > > +/* Aux attach device domain info */ > +struct subdevice_domain_info { > + struct device *dev; > + struct dmar_domain *domain; > + struct list_head link_phys; /* link to phys device > siblings */ > + struct list_head link_domain; /* link to domain siblings > */ > + int users; > +}; > + > static inline void __iommu_flush_cache( > struct intel_iommu *iommu, void *addr, int size) > { Thanks, Jacob _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
