Hi Baolu, > From: Lu Baolu <baolu...@linux.intel.com> > Sent: Tuesday, December 29, 2020 4:38 PM > > Hi Yi, > > On 2020/12/29 11:25, Liu Yi L wrote: > > In the existing code, loop all devices attached to a domain does not > > include sub-devices attached via iommu_aux_attach_device(). > > > > This was found by when I'm working on the belwo patch, There is no > ^^^^^ > below
nice catch. 😉 > > device in the domain->devices list, thus unable to get the cap and > > ecap of iommu unit. But this domain actually has subdevice which is > > attached via aux-manner. But it is tracked by domain. This patch is > > going to fix it. > > > > https://lore.kernel.org/kvm/1599734733-6431-17-git-send-email- > yi.l....@intel.com/ > > > > And 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. > > > > Co-developed-by: Xin Zeng <xin.z...@intel.com> > > Signed-off-by: Xin Zeng <xin.z...@intel.com> > > Signed-off-by: Liu Yi L <yi.l....@intel.com> > > Others look good to me. > > Fixes: 67b8e02b5e761 ("iommu/vt-d: Aux-domain specific domain > attach/detach") > Acked-by: Lu Baolu <baolu...@linux.intel.com> thanks, Regards, Yi Liu > Best regards, > baolu > > > --- > > drivers/iommu/intel/iommu.c | 95 +++++++++++++++++++++++++++-------- > -- > > include/linux/intel-iommu.h | 16 +++++-- > > 2 files changed, 82 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > index 788119c5b021..d7720a836268 100644 > > --- a/drivers/iommu/intel/iommu.c > > +++ b/drivers/iommu/intel/iommu.c > > @@ -1877,6 +1877,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->subdevices); > > > > return domain; > > } > > @@ -2547,7 +2548,7 @@ static struct dmar_domain > *dmar_insert_one_dev_info(struct intel_iommu *iommu, > > info->iommu = iommu; > > info->pasid_table = NULL; > > info->auxd_enabled = 0; > > - INIT_LIST_HEAD(&info->auxiliary_domains); > > + INIT_LIST_HEAD(&info->subdevices); > > > > if (dev && dev_is_pci(dev)) { > > struct pci_dev *pdev = to_pci_dev(info->dev); > > @@ -4475,33 +4476,61 @@ is_aux_domain(struct device *dev, struct > iommu_domain *domain) > > domain->type == IOMMU_DOMAIN_UNMANAGED; > > } > > > > -static void auxiliary_link_device(struct dmar_domain *domain, > > - struct device *dev) > > +static inline struct subdev_domain_info * > > +lookup_subdev_info(struct dmar_domain *domain, struct device *dev) > > +{ > > + struct subdev_domain_info *sinfo; > > + > > + if (!list_empty(&domain->subdevices)) { > > + list_for_each_entry(sinfo, &domain->subdevices, > link_domain) { > > + if (sinfo->pdev == dev) > > + return sinfo; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +static int auxiliary_link_device(struct dmar_domain *domain, > > + struct device *dev) > > { > > struct device_domain_info *info = get_domain_info(dev); > > + struct subdev_domain_info *sinfo = lookup_subdev_info(domain, > dev); > > > > assert_spin_locked(&device_domain_lock); > > if (WARN_ON(!info)) > > - return; > > + return -EINVAL; > > + > > + if (!sinfo) { > > + sinfo = kzalloc(sizeof(*sinfo), GFP_ATOMIC); > > + sinfo->domain = domain; > > + sinfo->pdev = dev; > > + list_add(&sinfo->link_phys, &info->subdevices); > > + list_add(&sinfo->link_domain, &domain->subdevices); > > + } > > > > - domain->auxd_refcnt++; > > - list_add(&domain->auxd, &info->auxiliary_domains); > > + return ++sinfo->users; > > } > > > > -static void auxiliary_unlink_device(struct dmar_domain *domain, > > - struct device *dev) > > +static int auxiliary_unlink_device(struct dmar_domain *domain, > > + struct device *dev) > > { > > struct device_domain_info *info = get_domain_info(dev); > > + struct subdev_domain_info *sinfo = lookup_subdev_info(domain, > dev); > > + int ret; > > > > assert_spin_locked(&device_domain_lock); > > - if (WARN_ON(!info)) > > - return; > > + if (WARN_ON(!info || !sinfo || sinfo->users <= 0)) > > + return -EINVAL; > > > > - list_del(&domain->auxd); > > - domain->auxd_refcnt--; > > + ret = --sinfo->users; > > + if (!ret) { > > + list_del(&sinfo->link_phys); > > + list_del(&sinfo->link_domain); > > + kfree(sinfo); > > + } > > > > - if (!domain->auxd_refcnt && domain->default_pasid > 0) > > - ioasid_put(domain->default_pasid); > > + return ret; > > } > > > > static int aux_domain_add_dev(struct dmar_domain *domain, > > @@ -4530,6 +4559,19 @@ static int aux_domain_add_dev(struct > dmar_domain *domain, > > } > > > > spin_lock_irqsave(&device_domain_lock, flags); > > + ret = auxiliary_link_device(domain, dev); > > + if (ret <= 0) > > + goto link_failed; > > + > > + /* > > + * Subdevices from the same physical device can be attached to the > > + * same domain. For such cases, only the first subdevice attachment > > + * needs to go through the full steps in this function. So if ret > > > + * 1, just goto out. > > + */ > > + if (ret > 1) > > + goto out; > > + > > /* > > * iommu->lock must be held to attach domain to iommu and setup > the > > * pasid entry for second level translation. > > @@ -4548,10 +4590,9 @@ static int aux_domain_add_dev(struct > dmar_domain *domain, > > domain->default_pasid); > > if (ret) > > goto table_failed; > > - spin_unlock(&iommu->lock); > > - > > - auxiliary_link_device(domain, dev); > > > > + spin_unlock(&iommu->lock); > > +out: > > spin_unlock_irqrestore(&device_domain_lock, flags); > > > > return 0; > > @@ -4560,8 +4601,10 @@ static int aux_domain_add_dev(struct > dmar_domain *domain, > > domain_detach_iommu(domain, iommu); > > attach_failed: > > spin_unlock(&iommu->lock); > > + auxiliary_unlink_device(domain, dev); > > +link_failed: > > spin_unlock_irqrestore(&device_domain_lock, flags); > > - if (!domain->auxd_refcnt && domain->default_pasid > 0) > > + if (list_empty(&domain->subdevices) && domain->default_pasid > 0) > > ioasid_put(domain->default_pasid); > > > > return ret; > > @@ -4581,14 +4624,18 @@ static void aux_domain_remove_dev(struct > dmar_domain *domain, > > info = get_domain_info(dev); > > iommu = info->iommu; > > > > - auxiliary_unlink_device(domain, dev); > > - > > - 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, dev)) { > > + 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 (list_empty(&domain->subdevices) && domain->default_pasid > 0) > > + ioasid_put(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..09c6a0bf3892 100644 > > --- a/include/linux/intel-iommu.h > > +++ b/include/linux/intel-iommu.h > > @@ -533,11 +533,10 @@ struct dmar_domain { > > /* Domain ids per IOMMU. Use u16 > since > > * domain ids are 16 bit wide > according > > * to VT-d spec, section 9.3 */ > > - unsigned int auxd_refcnt; /* Refcount of auxiliary attaching */ > > > > bool has_iotlb_device; > > struct list_head devices; /* all devices' list */ > > - struct list_head auxd; /* link to device's auxiliary list */ > > + struct list_head subdevices; /* all subdevices' list */ > > struct iova_domain iovad; /* iova's that belong to this domain > */ > > > > struct dma_pte *pgd; /* virtual address */ > > @@ -610,14 +609,21 @@ struct intel_iommu { > > struct dmar_drhd_unit *drhd; > > }; > > > > +/* Per subdevice private data */ > > +struct subdev_domain_info { > > + struct list_head link_phys; /* link to phys device siblings */ > > + struct list_head link_domain; /* link to domain siblings */ > > + struct device *pdev; /* physical device derived from */ > > + struct dmar_domain *domain; /* aux-domain */ > > + int users; /* user count */ > > +}; > > + > > /* PCI domain-device relationship */ > > struct device_domain_info { > > struct list_head link; /* link to domain siblings */ > > struct list_head global; /* link to global list */ > > struct list_head table; /* link to pasid table */ > > - struct list_head auxiliary_domains; /* auxiliary domains > > - * attached to this device > > - */ > > + struct list_head subdevices; /* subdevices sibling */ > > u32 segment; /* PCI segment number */ > > u8 bus; /* PCI bus number */ > > u8 devfn; /* PCI devfn number */ > >
