Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH v2 01/12] iommu/vt-d: debugfs: Remove device_domain_lock usage | Date | Wed, 15 Jun 2022 06:13:17 +0000 |
| |
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, June 15, 2022 9:54 AM > > On 2022/6/14 14:43, Tian, Kevin wrote: > >> From: Lu Baolu<baolu.lu@linux.intel.com> > >> Sent: Tuesday, June 14, 2022 10:51 AM > >> > >> The domain_translation_struct debugfs node is used to dump the DMAR > >> page > >> tables for the PCI devices. It potentially races with setting domains to > >> devices. The existing code uses a global spinlock device_domain_lock to > >> avoid the races, but this is problematical as this lock is only used to > >> protect the device tracking lists of each domain. > > is it really problematic at this point? Before following patches are applied > > using device_domain_lock should have similar effect as holding the group > > lock. > > > > Here it might make more sense to just focus on removing the use of > > device_domain_lock outside of iommu.c. Just that using group lock is > > cleaner and more compatible to following cleanups. > > > > and it's worth mentioning that racing with page table updates is out > > of the scope of this series. Probably also add a comment in the code > > to clarify this point. > > > > Hi Kevin, > > How do you like below updated patch?
Yes, this is better.
> > From cecc9a0623780a11c4ea4d0a15aa6187f01541c4 Mon Sep 17 00:00:00 > 2001 > From: Lu Baolu <baolu.lu@linux.intel.com> > Date: Sun, 29 May 2022 10:18:56 +0800 > Subject: [PATCH 1/1] iommu/vt-d: debugfs: Remove device_domain_lock > usage > > The domain_translation_struct debugfs node is used to dump the DMAR > page > tables for the PCI devices. It potentially races with setting domains to > devices. The existing code uses the global spinlock device_domain_lock to > avoid the races. > > This removes the use of device_domain_lock outside of iommu.c by replacing > it with the group mutex lock. Using the group mutex lock is cleaner and > more compatible to following cleanups. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/iommu/intel/debugfs.c | 42 +++++++++++++++++++++++++---------- > drivers/iommu/intel/iommu.c | 2 +- > drivers/iommu/intel/iommu.h | 1 - > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c > index d927ef10641b..f4acd8993f60 100644 > --- a/drivers/iommu/intel/debugfs.c > +++ b/drivers/iommu/intel/debugfs.c > @@ -342,13 +342,13 @@ static void pgtable_walk_level(struct seq_file *m, > struct dma_pte *pde, > } > } > > -static int show_device_domain_translation(struct device *dev, void *data) > +static int __show_device_domain_translation(struct device *dev, void *data) > { > - struct device_domain_info *info = dev_iommu_priv_get(dev); > - struct dmar_domain *domain = info->domain; > + struct dmar_domain *domain; > struct seq_file *m = data; > u64 path[6] = { 0 }; > > + domain = to_dmar_domain(iommu_get_domain_for_dev(dev)); > if (!domain) > return 0; > > @@ -359,20 +359,38 @@ static int show_device_domain_translation(struct > device *dev, void *data) > pgtable_walk_level(m, domain->pgd, domain->agaw + 2, 0, path); > seq_putc(m, '\n'); > > - return 0; > + return 1; > } > > -static int domain_translation_struct_show(struct seq_file *m, void *unused) > +static int show_device_domain_translation(struct device *dev, void *data) > { > - unsigned long flags; > - int ret; > + struct iommu_group *group; > > - spin_lock_irqsave(&device_domain_lock, flags); > - ret = bus_for_each_dev(&pci_bus_type, NULL, m, > - show_device_domain_translation); > - spin_unlock_irqrestore(&device_domain_lock, flags); > + group = iommu_group_get(dev); > + if (group) { > + /* > + * The group->mutex is held across the callback, which will > + * block calls to iommu_attach/detach_group/device. Hence, > + * the domain of the device will not change during traversal. > + * > + * All devices in an iommu group share a single domain, > hence > + * we only dump the domain of the first device. Even though,
bus_for_each_dev() will still lead to duplicated dump in the same group but probably we can leave with it for a debug interface.
> + * this code still possibly races with the iommu_unmap() > + * interface. This could be solved by RCU-freeing the page > + * table pages in the iommu_unmap() path. > + */ > + iommu_group_for_each_dev(group, data, > + __show_device_domain_translation); > + iommu_group_put(group); > + } > > - return ret; > + return 0; > +} > + > +static int domain_translation_struct_show(struct seq_file *m, void *unused) > +{ > + return bus_for_each_dev(&pci_bus_type, NULL, m, > + show_device_domain_translation); > } > DEFINE_SHOW_ATTRIBUTE(domain_translation_struct); > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 19024dc52735..a39d72a9d1cf 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -314,7 +314,7 @@ static int iommu_skip_te_disable; > #define IDENTMAP_GFX 2 > #define IDENTMAP_AZALIA 4 > > -DEFINE_SPINLOCK(device_domain_lock); > +static DEFINE_SPINLOCK(device_domain_lock); > static LIST_HEAD(device_domain_list); > > /* > diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h > index a22adfbdf870..8a6d64d726c0 100644 > --- a/drivers/iommu/intel/iommu.h > +++ b/drivers/iommu/intel/iommu.h > @@ -480,7 +480,6 @@ enum { > #define VTD_FLAG_SVM_CAPABLE (1 << 2) > > extern int intel_iommu_sm; > -extern spinlock_t device_domain_lock; > > #define sm_supported(iommu) (intel_iommu_sm && > ecap_smts((iommu)->ecap)) > #define pasid_supported(iommu) (sm_supported(iommu) && > \ > -- > 2.25.1 > > Best regards, > baolu
| |