Re: [PATCH v2 01/12] iommu/vt-d: debugfs: Remove device_domain_lock usage

[Baolu Lu]

On 2022/6/15 14:13, Tian, Kevin wrote:
> > From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, June 15, 2022 9:54 AM
> >
> > On 2022/6/14 14:43, Tian, Kevin wrote:
> > > > From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx>
> > ---
> >    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.

Yes. This is what it was. Ideally we could walk the iommu groups and
dump the device names belonging to the group and it's domain mappings,
but I was not willing to add any helpers in the iommu core just for a
debugfs use.

---
Best regards,
baolu