Hi Jacob, > From: Jacob Pan <jacob.jun....@intel.com> > Sent: Wednesday, December 23, 2020 4:21 AM > > 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()."
looks good. will refine accordingly. 😊 > > > 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. yep. Baolu also suggested such refine. will tweak in next version. Regards, Yi Liu > > > 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
