[PATCH v2 00/11] PCI: Fix ATS deadlock

[PATCH v2 00/11] PCI: Fix ATS deadlock

[Bjorn Helgaas]

Gregor reported a deadlock [1] when enabling a VF that supports ATS.
This series is intended to fix that.  The second patch should be enough to
fix the deadlock; the rest are simplification and cleanup.

These are based on v4.2-rc2.

[1] http://permalink.gmane.org/gmane.linux.kernel.iommu/9433

Changes between v1 and v2:
  - Remove use of pci_ats_enabled() (intel-iommu.c)
  - Call pci_ats_queue_depth() only once per device and cache result
    (intel-iommu.c)
  - Remove pci_ats_enabled() interface
  - Stop caching queue depth in pci_dev to save space
  - Add PF refcount of how many associated VFs have ATS enabled
  - Add comment that ATS must be enabled on PF before on VFs
  - Require ATS to be disabled on all VFs and PF before changing STU


---

Bjorn Helgaas (11):
      iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
      PCI: Allocate ATS struct during enumeration
      PCI: Embed ATS info directly into struct pci_dev
      PCI: Reduce size of ATS structure elements
      PCI: Rationalize pci_ats_queue_depth() error checking
      PCI: Inline the ATS setup code into pci_ats_init()
      PCI: Use pci_physfn() rather than looking up physfn by hand
      PCI: Clean up ATS error handling
      PCI: Move ATS declarations to linux/pci.h so they're all together
      PCI: Stop caching ATS Invalidate Queue Depth
      PCI: Remove pci_ats_enabled()


 drivers/iommu/intel-iommu.c |   26 +++++----
 drivers/pci/ats.c           |  131 +++++++++++++++----------------------------
 drivers/pci/probe.c         |    3 +
 include/linux/pci-ats.h     |   49 ----------------
 include/linux/pci.h         |   18 ++++++
 5 files changed, 82 insertions(+), 145 deletions(-)

[PATCH v2 01/11] iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

[Bjorn Helgaas]

We check the ATS state (enabled/disabled) and fetch the PCI ATS Invalidate
Queue Depth in performance-sensitive paths.  It's easy to cache these,
which removes dependencies on PCI.

Remember the ATS enabled state.  When enabling, read the queue depth once
and cache it in the device_domain_info struct.  This is similar to what
amd_iommu.c does.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/iommu/intel-iommu.c |   26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a98a7b2..50832f1 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -408,6 +408,10 @@ struct device_domain_info {
 	struct list_head global; /* link to global list */
 	u8 bus;			/* PCI bus number */
 	u8 devfn;		/* PCI devfn number */
+	struct {
+		int enabled:1;
+		u8 qdep;
+	} ats;			/* ATS state */
 	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
 	struct intel_iommu *iommu; /* IOMMU used by this device */
 	struct dmar_domain *domain; /* pointer to domain */
@@ -1391,19 +1395,26 @@ iommu_support_dev_iotlb (struct dmar_domain *domain, struct intel_iommu *iommu,
 
 static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 {
+	struct pci_dev *pdev;
+
 	if (!info || !dev_is_pci(info->dev))
 		return;
 
-	pci_enable_ats(to_pci_dev(info->dev), VTD_PAGE_SHIFT);
+	pdev = to_pci_dev(info->dev);
+	if (pci_enable_ats(pdev, VTD_PAGE_SHIFT))
+		return;
+
+	info->ats.enabled = 1;
+	info->ats.qdep = pci_ats_queue_depth(pdev);
 }
 
 static void iommu_disable_dev_iotlb(struct device_domain_info *info)
 {
-	if (!info->dev || !dev_is_pci(info->dev) ||
-	    !pci_ats_enabled(to_pci_dev(info->dev)))
+	if (!info->ats.enabled)
 		return;
 
 	pci_disable_ats(to_pci_dev(info->dev));
+	info->ats.enabled = 0;
 }
 
 static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
@@ -1415,16 +1426,11 @@ static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
 
 	spin_lock_irqsave(&device_domain_lock, flags);
 	list_for_each_entry(info, &domain->devices, link) {
-		struct pci_dev *pdev;
-		if (!info->dev || !dev_is_pci(info->dev))
-			continue;
-
-		pdev = to_pci_dev(info->dev);
-		if (!pci_ats_enabled(pdev))
+		if (!info->ats.enabled)
 			continue;
 
 		sid = info->bus << 8 | info->devfn;
-		qdep = pci_ats_queue_depth(pdev);
+		qdep = info->ats.qdep;
 		qi_flush_dev_iotlb(info->iommu, sid, qdep, addr, mask);
 	}
 	spin_unlock_irqrestore(&device_domain_lock, flags);

Re: [PATCH v2 01/11] iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:13:57PM -0500, Bjorn Helgaas wrote:
> We check the ATS state (enabled/disabled) and fetch the PCI ATS Invalidate
> Queue Depth in performance-sensitive paths.  It's easy to cache these,
> which removes dependencies on PCI.
> 
> Remember the ATS enabled state.  When enabling, read the queue depth once
> and cache it in the device_domain_info struct.  This is similar to what
> amd_iommu.c does.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/intel-iommu.c |   26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index a98a7b2..50832f1 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -408,6 +408,10 @@ struct device_domain_info {
>  	struct list_head global; /* link to global list */
>  	u8 bus;			/* PCI bus number */
>  	u8 devfn;		/* PCI devfn number */
> +	struct {
> +		int enabled:1;
> +		u8 qdep;
> +	} ats;			/* ATS state */
>  	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
>  	struct intel_iommu *iommu; /* IOMMU used by this device */
>  	struct dmar_domain *domain; /* pointer to domain */
> @@ -1391,19 +1395,26 @@ iommu_support_dev_iotlb (struct dmar_domain *domain, struct intel_iommu *iommu,
>  
>  static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>  {
> +	struct pci_dev *pdev;
> +
>  	if (!info || !dev_is_pci(info->dev))
>  		return;
>  
> -	pci_enable_ats(to_pci_dev(info->dev), VTD_PAGE_SHIFT);
> +	pdev = to_pci_dev(info->dev);
> +	if (pci_enable_ats(pdev, VTD_PAGE_SHIFT))
> +		return;
> +
> +	info->ats.enabled = 1;
> +	info->ats.qdep = pci_ats_queue_depth(pdev);

Hmm, this is a place where the relaxed error handling in
pci_enable_ats() can get problematic. If ATS is (by accident or a bug
elsewhere) already enabled an the function returns -EINVAL, the IOMMU
driver considers ATS disabled and doesn't flush the IO/TLBs of the
device. This can cause data corruption later on, so we should make sure
that info->ats.enabled is consistent with pdev->ats_enabled.


	Joerg

Re: [PATCH v2 01/11] iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:13:57PM -0500, Bjorn Helgaas wrote:
> We check the ATS state (enabled/disabled) and fetch the PCI ATS Invalidate
> Queue Depth in performance-sensitive paths.  It's easy to cache these,
> which removes dependencies on PCI.
> 
> Remember the ATS enabled state.  When enabling, read the queue depth once
> and cache it in the device_domain_info struct.  This is similar to what
> amd_iommu.c does.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/iommu/intel-iommu.c |   26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index a98a7b2..50832f1 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -408,6 +408,10 @@ struct device_domain_info {
>  	struct list_head global; /* link to global list */
>  	u8 bus;			/* PCI bus number */
>  	u8 devfn;		/* PCI devfn number */
> +	struct {
> +		int enabled:1;
> +		u8 qdep;
> +	} ats;			/* ATS state */
>  	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
>  	struct intel_iommu *iommu; /* IOMMU used by this device */
>  	struct dmar_domain *domain; /* pointer to domain */
> @@ -1391,19 +1395,26 @@ iommu_support_dev_iotlb (struct dmar_domain *domain, struct intel_iommu *iommu,
>  
>  static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>  {
> +	struct pci_dev *pdev;
> +
>  	if (!info || !dev_is_pci(info->dev))
>  		return;
>  
> -	pci_enable_ats(to_pci_dev(info->dev), VTD_PAGE_SHIFT);
> +	pdev = to_pci_dev(info->dev);
> +	if (pci_enable_ats(pdev, VTD_PAGE_SHIFT))
> +		return;
> +
> +	info->ats.enabled = 1;
> +	info->ats.qdep = pci_ats_queue_depth(pdev);

Hmm, this is a place where the relaxed error handling in
pci_enable_ats() can get problematic. If ATS is (by accident or a bug
elsewhere) already enabled an the function returns -EINVAL, the IOMMU
driver considers ATS disabled and doesn't flush the IO/TLBs of the
device. This can cause data corruption later on, so we should make sure
that info->ats.enabled is consistent with pdev->ats_enabled.


	Joerg

Re: [PATCH v2 01/11] iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

[Bjorn Helgaas]

Hi Joerg,

Thanks for all your help reviewing this!

On Mon, Jul 27, 2015 at 03:08:10PM +0200, Joerg Roedel wrote:
> On Mon, Jul 20, 2015 at 07:13:57PM -0500, Bjorn Helgaas wrote:
> > We check the ATS state (enabled/disabled) and fetch the PCI ATS Invalidate
> > Queue Depth in performance-sensitive paths.  It's easy to cache these,
> > which removes dependencies on PCI.
> > 
> > Remember the ATS enabled state.  When enabling, read the queue depth once
> > and cache it in the device_domain_info struct.  This is similar to what
> > amd_iommu.c does.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/iommu/intel-iommu.c |   26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index a98a7b2..50832f1 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -408,6 +408,10 @@ struct device_domain_info {
> >  	struct list_head global; /* link to global list */
> >  	u8 bus;			/* PCI bus number */
> >  	u8 devfn;		/* PCI devfn number */
> > +	struct {
> > +		int enabled:1;
> > +		u8 qdep;
> > +	} ats;			/* ATS state */
> >  	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
> >  	struct intel_iommu *iommu; /* IOMMU used by this device */
> >  	struct dmar_domain *domain; /* pointer to domain */
> > @@ -1391,19 +1395,26 @@ iommu_support_dev_iotlb (struct dmar_domain *domain, struct intel_iommu *iommu,
> >  
> >  static void iommu_enable_dev_iotlb(struct device_domain_info *info)
> >  {
> > +	struct pci_dev *pdev;
> > +
> >  	if (!info || !dev_is_pci(info->dev))
> >  		return;
> >  
> > -	pci_enable_ats(to_pci_dev(info->dev), VTD_PAGE_SHIFT);
> > +	pdev = to_pci_dev(info->dev);
> > +	if (pci_enable_ats(pdev, VTD_PAGE_SHIFT))
> > +		return;
> > +
> > +	info->ats.enabled = 1;
> > +	info->ats.qdep = pci_ats_queue_depth(pdev);
> 
> Hmm, this is a place where the relaxed error handling in
> pci_enable_ats() can get problematic. 

By "relaxed error handling," do you mean the fact that in v4.1,
pci_enable_ats() has a BUG_ON(is_enabled), while now it merely
returns -EINVAL?

(BTW, I did change it to add a WARN_ON and return -EBUSY as you suggested.)

> If ATS is (by accident or a bug
> elsewhere) already enabled an the function returns -EINVAL, the IOMMU
> driver considers ATS disabled and doesn't flush the IO/TLBs of the
> device. This can cause data corruption later on, so we should make sure
> that info->ats.enabled is consistent with pdev->ats_enabled.

I'm not quite sure I understand this.  In the patch above, we only set
"info->ats.enabled = 1" if pci_enable_ats() has succeeded.  The amd_iommu.c
code is similar.

Are you concerned about the case where future code enables ATS before
intel-iommu, the pci_enable_ats() in intel-iommu fails, intel-iommu
believes ATS is disabled, intel-iommu calls iommu_flush_dev_iotlb(), but it
doesn't flush the IOTLB?

I guess I could make intel-iommu handle -EBUSY differently from -EINVAL.
Would that help?  It seems sort of clumsy, but ...?

Bjorn

Re: [PATCH v2 01/11] iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

[Joerg Roedel]

Hi Bjorn,

On Mon, Jul 27, 2015 at 05:54:53PM -0500, Bjorn Helgaas wrote:
> > Hmm, this is a place where the relaxed error handling in
> > pci_enable_ats() can get problematic. 
> 
> By "relaxed error handling," do you mean the fact that in v4.1,
> pci_enable_ats() has a BUG_ON(is_enabled), while now it merely
> returns -EINVAL?
> 
> (BTW, I did change it to add a WARN_ON and return -EBUSY as you suggested.)

Okay, great.

> > If ATS is (by accident or a bug
> > elsewhere) already enabled an the function returns -EINVAL, the IOMMU
> > driver considers ATS disabled and doesn't flush the IO/TLBs of the
> > device. This can cause data corruption later on, so we should make sure
> > that info->ats.enabled is consistent with pdev->ats_enabled.
> 
> I'm not quite sure I understand this.  In the patch above, we only set
> "info->ats.enabled = 1" if pci_enable_ats() has succeeded.  The amd_iommu.c
> code is similar.
> 
> Are you concerned about the case where future code enables ATS before
> intel-iommu, the pci_enable_ats() in intel-iommu fails, intel-iommu
> believes ATS is disabled, intel-iommu calls iommu_flush_dev_iotlb(), but it
> doesn't flush the IOTLB?
> 
> I guess I could make intel-iommu handle -EBUSY differently from -EINVAL.
> Would that help?  It seems sort of clumsy, but ...?

I was concerned that it was harder now to spot bugs in ATS
enabling/disabling, when pci_enable_ats just returns -EINVAL when ATS is
already enabled.

But with the WARN_ON now we will notice these bugs early again, thanks
for adding it.



	Joerg

Re: [PATCH v2 01/11] iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

[Joerg Roedel]

Hi Bjorn,

On Mon, Jul 27, 2015 at 05:54:53PM -0500, Bjorn Helgaas wrote:
> > Hmm, this is a place where the relaxed error handling in
> > pci_enable_ats() can get problematic. 
> 
> By "relaxed error handling," do you mean the fact that in v4.1,
> pci_enable_ats() has a BUG_ON(is_enabled), while now it merely
> returns -EINVAL?
> 
> (BTW, I did change it to add a WARN_ON and return -EBUSY as you suggested.)

Okay, great.

> > If ATS is (by accident or a bug
> > elsewhere) already enabled an the function returns -EINVAL, the IOMMU
> > driver considers ATS disabled and doesn't flush the IO/TLBs of the
> > device. This can cause data corruption later on, so we should make sure
> > that info->ats.enabled is consistent with pdev->ats_enabled.
> 
> I'm not quite sure I understand this.  In the patch above, we only set
> "info->ats.enabled = 1" if pci_enable_ats() has succeeded.  The amd_iommu.c
> code is similar.
> 
> Are you concerned about the case where future code enables ATS before
> intel-iommu, the pci_enable_ats() in intel-iommu fails, intel-iommu
> believes ATS is disabled, intel-iommu calls iommu_flush_dev_iotlb(), but it
> doesn't flush the IOTLB?
> 
> I guess I could make intel-iommu handle -EBUSY differently from -EINVAL.
> Would that help?  It seems sort of clumsy, but ...?

I was concerned that it was harder now to spot bugs in ATS
enabling/disabling, when pci_enable_ats just returns -EINVAL when ATS is
already enabled.

But with the WARN_ON now we will notice these bugs early again, thanks
for adding it.



	Joerg

[PATCH v2 02/11] PCI: Allocate ATS struct during enumeration

[Bjorn Helgaas]

Previously, we allocated pci_ats structures when an IOMMU driver called
pci_enable_ats().  An SR-IOV VF shares the STU setting with its PF, so when
enabling ATS on the VF, we allocated a pci_ats struct for the PF if it
didn't already have one.  We held the sriov->lock to serialize threads
concurrently enabling ATS on several VFS so only one would allocate the PF
pci_ats.

Gregor reported a deadlock here:

  pci_enable_sriov
    sriov_enable
      virtfn_add
        mutex_lock(dev->sriov->lock)      # acquire sriov->lock
        pci_device_add
          device_add
            BUS_NOTIFY_ADD_DEVICE notifier chain
            iommu_bus_notifier
              amd_iommu_add_device        # iommu_ops.add_device
                init_iommu_group
                  iommu_group_get_for_dev
                    iommu_group_add_device
                      __iommu_attach_device
                        amd_iommu_attach_device  # iommu_ops.attach_device
                          attach_device
                            pci_enable_ats
                              mutex_lock(dev->sriov->lock) # deadlock

There's no reason to delay allocating the pci_ats struct, and if we
allocate it for each device at enumeration-time, there's no need for
locking in pci_enable_ats().

Allocate pci_ats struct during enumeration, when we initialize other
capabilities.

Note that this implementation requires ATS to be enabled on the PF first,
before on any of the VFs because the PF controls the STU for all the VFs.

Link: http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
Reported-by: Gregor Dick <gdick@solarflare.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c       |   98 ++++++++++++++++++++---------------------------
 drivers/pci/probe.c     |    3 +
 drivers/pci/remove.c    |    1 
 include/linux/pci-ats.h |    2 -
 include/linux/pci.h     |    9 ++++
 5 files changed, 56 insertions(+), 57 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index a8099d4..2026f53 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -17,7 +17,7 @@
 
 #include "pci.h"
 
-static int ats_alloc_one(struct pci_dev *dev, int ps)
+static void ats_alloc_one(struct pci_dev *dev)
 {
 	int pos;
 	u16 cap;
@@ -25,20 +25,19 @@ static int ats_alloc_one(struct pci_dev *dev, int ps)
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
 	if (!pos)
-		return -ENODEV;
+		return;
 
 	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
-	if (!ats)
-		return -ENOMEM;
+	if (!ats) {
+		dev_warn(&dev->dev, "can't allocate space for ATS state\n");
+		return;
+	}
 
 	ats->pos = pos;
-	ats->stu = ps;
 	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
 	ats->qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
 					    PCI_ATS_MAX_QDEP;
 	dev->ats = ats;
-
-	return 0;
 }
 
 static void ats_free_one(struct pci_dev *dev)
@@ -47,6 +46,16 @@ static void ats_free_one(struct pci_dev *dev)
 	dev->ats = NULL;
 }
 
+void pci_ats_init(struct pci_dev *dev)
+{
+	ats_alloc_one(dev);
+}
+
+void pci_ats_free(struct pci_dev *dev)
+{
+	ats_free_one(dev);
+}
+
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -56,43 +65,35 @@ static void ats_free_one(struct pci_dev *dev)
  */
 int pci_enable_ats(struct pci_dev *dev, int ps)
 {
-	int rc;
 	u16 ctrl;
 
 	BUG_ON(dev->ats && dev->ats->is_enabled);
 
+	if (!dev->ats)
+		return -EINVAL;
+
 	if (ps < PCI_ATS_MIN_STU)
 		return -EINVAL;
 
-	if (dev->is_physfn || dev->is_virtfn) {
-		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
+	/*
+	 * Note that enabling ATS on a VF fails unless it's already enabled
+	 * with the same STU on the PF.
+	 */
+	ctrl = PCI_ATS_CTRL_ENABLE;
+	if (dev->is_virtfn) {
+		struct pci_dev *pdev = dev->physfn;
 
-		mutex_lock(&pdev->sriov->lock);
-		if (pdev->ats)
-			rc = pdev->ats->stu == ps ? 0 : -EINVAL;
-		else
-			rc = ats_alloc_one(pdev, ps);
+		if (pdev->ats->stu != ps)
+			return -EINVAL;
 
-		if (!rc)
-			pdev->ats->ref_cnt++;
-		mutex_unlock(&pdev->sriov->lock);
-		if (rc)
-			return rc;
-	}
-
-	if (!dev->is_physfn) {
-		rc = ats_alloc_one(dev, ps);
-		if (rc)
-			return rc;
+		atomic_inc(&pdev->ats->ref_cnt);  /* count enabled VFs */
+	} else {
+		dev->ats->stu = ps;
+		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
 	}
-
-	ctrl = PCI_ATS_CTRL_ENABLE;
-	if (!dev->is_virtfn)
-		ctrl |= PCI_ATS_CTRL_STU(ps - PCI_ATS_MIN_STU);
 	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
 
 	dev->ats->is_enabled = 1;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_enable_ats);
@@ -107,24 +108,20 @@ void pci_disable_ats(struct pci_dev *dev)
 
 	BUG_ON(!dev->ats || !dev->ats->is_enabled);
 
+	if (atomic_read(&dev->ats->ref_cnt))
+		return;		/* VFs still enabled */
+
+	if (dev->is_virtfn) {
+		struct pci_dev *pdev = dev->physfn;
+
+		atomic_dec(&pdev->ats->ref_cnt);
+	}
+
 	pci_read_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, &ctrl);
 	ctrl &= ~PCI_ATS_CTRL_ENABLE;
 	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
 
 	dev->ats->is_enabled = 0;
-
-	if (dev->is_physfn || dev->is_virtfn) {
-		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
-
-		mutex_lock(&pdev->sriov->lock);
-		pdev->ats->ref_cnt--;
-		if (!pdev->ats->ref_cnt)
-			ats_free_one(pdev);
-		mutex_unlock(&pdev->sriov->lock);
-	}
-
-	if (!dev->is_physfn)
-		ats_free_one(dev);
 }
 EXPORT_SYMBOL_GPL(pci_disable_ats);
 
@@ -140,7 +137,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)
 		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
-
 	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
 }
 EXPORT_SYMBOL_GPL(pci_restore_ats_state);
@@ -159,23 +155,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
  */
 int pci_ats_queue_depth(struct pci_dev *dev)
 {
-	int pos;
-	u16 cap;
-
 	if (dev->is_virtfn)
 		return 0;
 
 	if (dev->ats)
 		return dev->ats->qdep;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
-	if (!pos)
-		return -ENODEV;
-
-	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
-
-	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
-				       PCI_ATS_MAX_QDEP;
+	return -ENODEV;
 }
 EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..c206398 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1540,6 +1540,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	/* Single Root I/O Virtualization */
 	pci_iov_init(dev);
 
+	/* Address Translation Services */
+	pci_ats_init(dev);
+
 	/* Enable ACS P2P upstream forwarding */
 	pci_enable_acs(dev);
 }
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8a280e9..27617b8 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -26,6 +26,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 		dev->is_added = 0;
 	}
 
+	pci_ats_free(dev);
 	if (dev->bus->self)
 		pcie_aspm_exit_link_state(dev);
 }
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 7203178..e2dcc2f 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -8,7 +8,7 @@ struct pci_ats {
 	int pos;        /* capability position */
 	int stu;        /* Smallest Translation Unit */
 	int qdep;       /* Invalidate Queue Depth */
-	int ref_cnt;    /* Physical Function reference count */
+	atomic_t ref_cnt; /* number of VFs with ATS enabled */
 	unsigned int is_enabled:1;      /* Enable bit is set */
 };
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..1817819 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1294,6 +1294,15 @@ int  ht_create_irq(struct pci_dev *dev, int idx);
 void ht_destroy_irq(unsigned int irq);
 #endif /* CONFIG_HT_IRQ */
 
+#ifdef CONFIG_PCI_ATS
+/* Address Translation Service */
+void pci_ats_init(struct pci_dev *dev);
+void pci_ats_free(struct pci_dev *dev);
+#else
+static inline void pci_ats_init(struct pci_dev *dev) { }
+static inline void pci_ats_free(struct pci_dev *dev) { }
+#endif
+
 void pci_cfg_access_lock(struct pci_dev *dev);
 bool pci_cfg_access_trylock(struct pci_dev *dev);
 void pci_cfg_access_unlock(struct pci_dev *dev);

Re: [PATCH v2 02/11] PCI: Allocate ATS struct during enumeration

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:14:05PM -0500, Bjorn Helgaas wrote:
> Previously, we allocated pci_ats structures when an IOMMU driver called
> pci_enable_ats().  An SR-IOV VF shares the STU setting with its PF, so when
> enabling ATS on the VF, we allocated a pci_ats struct for the PF if it
> didn't already have one.  We held the sriov->lock to serialize threads
> concurrently enabling ATS on several VFS so only one would allocate the PF
> pci_ats.
> 
> Gregor reported a deadlock here:
> 
>   pci_enable_sriov
>     sriov_enable
>       virtfn_add
>         mutex_lock(dev->sriov->lock)      # acquire sriov->lock
>         pci_device_add
>           device_add
>             BUS_NOTIFY_ADD_DEVICE notifier chain
>             iommu_bus_notifier
>               amd_iommu_add_device        # iommu_ops.add_device
>                 init_iommu_group
>                   iommu_group_get_for_dev
>                     iommu_group_add_device
>                       __iommu_attach_device
>                         amd_iommu_attach_device  # iommu_ops.attach_device
>                           attach_device
>                             pci_enable_ats
>                               mutex_lock(dev->sriov->lock) # deadlock
> 
> There's no reason to delay allocating the pci_ats struct, and if we
> allocate it for each device at enumeration-time, there's no need for
> locking in pci_enable_ats().
> 
> Allocate pci_ats struct during enumeration, when we initialize other
> capabilities.
> 
> Note that this implementation requires ATS to be enabled on the PF first,
> before on any of the VFs because the PF controls the STU for all the VFs.
> 
> Link: http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
> Reported-by: Gregor Dick <gdick@solarflare.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c       |   98 ++++++++++++++++++++---------------------------
>  drivers/pci/probe.c     |    3 +
>  drivers/pci/remove.c    |    1 
>  include/linux/pci-ats.h |    2 -
>  include/linux/pci.h     |    9 ++++
>  5 files changed, 56 insertions(+), 57 deletions(-)

Looks good to me now.


Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 03/11] PCI: Embed ATS info directly into struct pci_dev

[Bjorn Helgaas]

The pci_ats struct is small and will get smaller, so I don't think it's
worth allocating it separately from the pci_dev struct.

Embed the ATS fields directly into struct pci_dev.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c       |   61 ++++++++++++++++-------------------------------
 drivers/pci/remove.c    |    1 -
 include/linux/pci-ats.h |   10 +-------
 include/linux/pci.h     |    8 ++++--
 4 files changed, 27 insertions(+), 53 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 2026f53..690ae6e 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -21,29 +21,15 @@ static void ats_alloc_one(struct pci_dev *dev)
 {
 	int pos;
 	u16 cap;
-	struct pci_ats *ats;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
 	if (!pos)
 		return;
 
-	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
-	if (!ats) {
-		dev_warn(&dev->dev, "can't allocate space for ATS state\n");
-		return;
-	}
-
-	ats->pos = pos;
-	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
-	ats->qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
+	dev->ats_cap = pos;
+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
+	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
 					    PCI_ATS_MAX_QDEP;
-	dev->ats = ats;
-}
-
-static void ats_free_one(struct pci_dev *dev)
-{
-	kfree(dev->ats);
-	dev->ats = NULL;
 }
 
 void pci_ats_init(struct pci_dev *dev)
@@ -51,11 +37,6 @@ void pci_ats_init(struct pci_dev *dev)
 	ats_alloc_one(dev);
 }
 
-void pci_ats_free(struct pci_dev *dev)
-{
-	ats_free_one(dev);
-}
-
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -67,9 +48,9 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 {
 	u16 ctrl;
 
-	BUG_ON(dev->ats && dev->ats->is_enabled);
+	BUG_ON(dev->ats_cap && dev->ats_enabled);
 
-	if (!dev->ats)
+	if (!dev->ats_cap)
 		return -EINVAL;
 
 	if (ps < PCI_ATS_MIN_STU)
@@ -83,17 +64,17 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	if (dev->is_virtfn) {
 		struct pci_dev *pdev = dev->physfn;
 
-		if (pdev->ats->stu != ps)
+		if (pdev->ats_stu != ps)
 			return -EINVAL;
 
-		atomic_inc(&pdev->ats->ref_cnt);  /* count enabled VFs */
+		atomic_inc(&pdev->ats_ref_cnt);  /* count enabled VFs */
 	} else {
-		dev->ats->stu = ps;
-		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
+		dev->ats_stu = ps;
+		ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
 	}
-	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
 
-	dev->ats->is_enabled = 1;
+	dev->ats_enabled = 1;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_enable_ats);
@@ -106,22 +87,22 @@ void pci_disable_ats(struct pci_dev *dev)
 {
 	u16 ctrl;
 
-	BUG_ON(!dev->ats || !dev->ats->is_enabled);
+	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
 
-	if (atomic_read(&dev->ats->ref_cnt))
+	if (atomic_read(&dev->ats_ref_cnt))
 		return;		/* VFs still enabled */
 
 	if (dev->is_virtfn) {
 		struct pci_dev *pdev = dev->physfn;
 
-		atomic_dec(&pdev->ats->ref_cnt);
+		atomic_dec(&pdev->ats_ref_cnt);
 	}
 
-	pci_read_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, &ctrl);
+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl);
 	ctrl &= ~PCI_ATS_CTRL_ENABLE;
-	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
 
-	dev->ats->is_enabled = 0;
+	dev->ats_enabled = 0;
 }
 EXPORT_SYMBOL_GPL(pci_disable_ats);
 
@@ -136,8 +117,8 @@ void pci_restore_ats_state(struct pci_dev *dev)
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)
-		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
-	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
+		ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
 }
 EXPORT_SYMBOL_GPL(pci_restore_ats_state);
 
@@ -158,8 +139,8 @@ int pci_ats_queue_depth(struct pci_dev *dev)
 	if (dev->is_virtfn)
 		return 0;
 
-	if (dev->ats)
-		return dev->ats->qdep;
+	if (dev->ats_cap)
+		return dev->ats_qdep;
 
 	return -ENODEV;
 }
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 27617b8..8a280e9 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -26,7 +26,6 @@ static void pci_stop_dev(struct pci_dev *dev)
 		dev->is_added = 0;
 	}
 
-	pci_ats_free(dev);
 	if (dev->bus->self)
 		pcie_aspm_exit_link_state(dev);
 }
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index e2dcc2f..5d81d47 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -4,14 +4,6 @@
 #include <linux/pci.h>
 
 /* Address Translation Service */
-struct pci_ats {
-	int pos;        /* capability position */
-	int stu;        /* Smallest Translation Unit */
-	int qdep;       /* Invalidate Queue Depth */
-	atomic_t ref_cnt; /* number of VFs with ATS enabled */
-	unsigned int is_enabled:1;      /* Enable bit is set */
-};
-
 #ifdef CONFIG_PCI_ATS
 
 int pci_enable_ats(struct pci_dev *dev, int ps);
@@ -26,7 +18,7 @@ int pci_ats_queue_depth(struct pci_dev *dev);
  */
 static inline int pci_ats_enabled(struct pci_dev *dev)
 {
-	return dev->ats && dev->ats->is_enabled;
+	return dev->ats_cap && dev->ats_enabled;
 }
 
 #else /* CONFIG_PCI_ATS */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1817819..8bc16b5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -343,6 +343,7 @@ struct pci_dev {
 	unsigned int	msi_enabled:1;
 	unsigned int	msix_enabled:1;
 	unsigned int	ari_enabled:1;	/* ARI forwarding */
+	unsigned int	ats_enabled:1;	/* Address Translation Service */
 	unsigned int	is_managed:1;
 	unsigned int    needs_freset:1; /* Dev requires fundamental reset */
 	unsigned int	state_saved:1;
@@ -375,7 +376,10 @@ struct pci_dev {
 		struct pci_sriov *sriov;	/* SR-IOV capability related */
 		struct pci_dev *physfn;	/* the PF this VF is associated with */
 	};
-	struct pci_ats	*ats;	/* Address Translation Service */
+	int		ats_cap;	/* ATS Capability offset */
+	int		ats_stu;	/* ATS Smallest Translation Unit */
+	int		ats_qdep;	/* ATS Invalidate Queue Depth */
+	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
 	size_t romlen; /* Length of ROM if it's not from the BAR */
@@ -1297,10 +1301,8 @@ void ht_destroy_irq(unsigned int irq);
 #ifdef CONFIG_PCI_ATS
 /* Address Translation Service */
 void pci_ats_init(struct pci_dev *dev);
-void pci_ats_free(struct pci_dev *dev);
 #else
 static inline void pci_ats_init(struct pci_dev *dev) { }
-static inline void pci_ats_free(struct pci_dev *dev) { }
 #endif
 
 void pci_cfg_access_lock(struct pci_dev *dev);

Re: [PATCH v2 03/11] PCI: Embed ATS info directly into struct pci_dev

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:14:13PM -0500, Bjorn Helgaas wrote:
> The pci_ats struct is small and will get smaller, so I don't think it's
> worth allocating it separately from the pci_dev struct.
> 
> Embed the ATS fields directly into struct pci_dev.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/pci/ats.c       |   61 ++++++++++++++++-------------------------------
>  drivers/pci/remove.c    |    1 -
>  include/linux/pci-ats.h |   10 +-------
>  include/linux/pci.h     |    8 ++++--
>  4 files changed, 27 insertions(+), 53 deletions(-)

Reviewed-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH v2 03/11] PCI: Embed ATS info directly into struct pci_dev

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:14:13PM -0500, Bjorn Helgaas wrote:
> The pci_ats struct is small and will get smaller, so I don't think it's
> worth allocating it separately from the pci_dev struct.
> 
> Embed the ATS fields directly into struct pci_dev.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c       |   61 ++++++++++++++++-------------------------------
>  drivers/pci/remove.c    |    1 -
>  include/linux/pci-ats.h |   10 +-------
>  include/linux/pci.h     |    8 ++++--
>  4 files changed, 27 insertions(+), 53 deletions(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 04/11] PCI: Reduce size of ATS structure elements

[Bjorn Helgaas]

The extended capabilities list is linked with 12-bit pointers, and the ATS
Smallest Translation Unit and Invalidate Queue Depth fields are both 5
bits.

Use u16 and u8 to hold the extended capability address and the stu and qdep
values.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 include/linux/pci.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8bc16b5..238b77e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,9 +376,9 @@ struct pci_dev {
 		struct pci_sriov *sriov;	/* SR-IOV capability related */
 		struct pci_dev *physfn;	/* the PF this VF is associated with */
 	};
-	int		ats_cap;	/* ATS Capability offset */
-	int		ats_stu;	/* ATS Smallest Translation Unit */
-	int		ats_qdep;	/* ATS Invalidate Queue Depth */
+	u16		ats_cap;	/* ATS Capability offset */
+	u8		ats_stu;	/* ATS Smallest Translation Unit */
+	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
 	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */

[PATCH v2 04/11] PCI: Reduce size of ATS structure elements

[Bjorn Helgaas]

The extended capabilities list is linked with 12-bit pointers, and the ATS
Smallest Translation Unit and Invalidate Queue Depth fields are both 5
bits.

Use u16 and u8 to hold the extended capability address and the stu and qdep
values.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 include/linux/pci.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8bc16b5..238b77e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,9 +376,9 @@ struct pci_dev {
 		struct pci_sriov *sriov;	/* SR-IOV capability related */
 		struct pci_dev *physfn;	/* the PF this VF is associated with */
 	};
-	int		ats_cap;	/* ATS Capability offset */
-	int		ats_stu;	/* ATS Smallest Translation Unit */
-	int		ats_qdep;	/* ATS Invalidate Queue Depth */
+	u16		ats_cap;	/* ATS Capability offset */
+	u8		ats_stu;	/* ATS Smallest Translation Unit */
+	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
 	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */

[PATCH v2 07/11] PCI: Use pci_physfn() rather than looking up physfn by hand

[Bjorn Helgaas]

Use the pci_physfn() helper rather than looking up physfn by hand.
No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/pci/ats.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 95905f3..0b5b0ed 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,6 +42,7 @@ void pci_ats_init(struct pci_dev *dev)
 int pci_enable_ats(struct pci_dev *dev, int ps)
 {
 	u16 ctrl;
+	struct pci_dev *pdev;
 
 	BUG_ON(dev->ats_cap && dev->ats_enabled);
 
@@ -57,8 +58,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	 */
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (dev->is_virtfn) {
-		struct pci_dev *pdev = dev->physfn;
-
+		pdev = pci_physfn(dev);
 		if (pdev->ats_stu != ps)
 			return -EINVAL;
 
@@ -80,6 +80,7 @@ EXPORT_SYMBOL_GPL(pci_enable_ats);
  */
 void pci_disable_ats(struct pci_dev *dev)
 {
+	struct pci_dev *pdev;
 	u16 ctrl;
 
 	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
@@ -88,8 +89,7 @@ void pci_disable_ats(struct pci_dev *dev)
 		return;		/* VFs still enabled */
 
 	if (dev->is_virtfn) {
-		struct pci_dev *pdev = dev->physfn;
-
+		pdev = pci_physfn(dev);
 		atomic_dec(&pdev->ats_ref_cnt);
 	}
 

[PATCH v2 07/11] PCI: Use pci_physfn() rather than looking up physfn by hand

[Bjorn Helgaas]

Use the pci_physfn() helper rather than looking up physfn by hand.
No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/pci/ats.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 95905f3..0b5b0ed 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,6 +42,7 @@ void pci_ats_init(struct pci_dev *dev)
 int pci_enable_ats(struct pci_dev *dev, int ps)
 {
 	u16 ctrl;
+	struct pci_dev *pdev;
 
 	BUG_ON(dev->ats_cap && dev->ats_enabled);
 
@@ -57,8 +58,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	 */
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (dev->is_virtfn) {
-		struct pci_dev *pdev = dev->physfn;
-
+		pdev = pci_physfn(dev);
 		if (pdev->ats_stu != ps)
 			return -EINVAL;
 
@@ -80,6 +80,7 @@ EXPORT_SYMBOL_GPL(pci_enable_ats);
  */
 void pci_disable_ats(struct pci_dev *dev)
 {
+	struct pci_dev *pdev;
 	u16 ctrl;
 
 	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
@@ -88,8 +89,7 @@ void pci_disable_ats(struct pci_dev *dev)
 		return;		/* VFs still enabled */
 
 	if (dev->is_virtfn) {
-		struct pci_dev *pdev = dev->physfn;
-
+		pdev = pci_physfn(dev);
 		atomic_dec(&pdev->ats_ref_cnt);
 	}
 

[PATCH v2 08/11] PCI: Clean up ATS error handling

[Bjorn Helgaas]

There's no need to BUG() if we enable ATS when it's already enabled.  We
don't need to BUG() when disabling ATS on a device that doesn't support ATS
or if it's already disabled.  If ATS is enabled, certainly we found an ATS
capability in the past, so it should still be there now.

Clean up these error paths.

Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/ats.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 0b5b0ed..67524a7 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -44,11 +44,12 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	u16 ctrl;
 	struct pci_dev *pdev;
 
-	BUG_ON(dev->ats_cap && dev->ats_enabled);
-
 	if (!dev->ats_cap)
 		return -EINVAL;
 
+	if (pci_ats_enabled(dev))
+		return -EINVAL;
+
 	if (ps < PCI_ATS_MIN_STU)
 		return -EINVAL;
 
@@ -83,7 +84,8 @@ void pci_disable_ats(struct pci_dev *dev)
 	struct pci_dev *pdev;
 	u16 ctrl;
 
-	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
+	if (!pci_ats_enabled(dev))
+		return;
 
 	if (atomic_read(&dev->ats_ref_cnt))
 		return;		/* VFs still enabled */
@@ -107,8 +109,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
 
 	if (!pci_ats_enabled(dev))
 		return;
-	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS))
-		BUG();
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)

[PATCH v2 08/11] PCI: Clean up ATS error handling

[Bjorn Helgaas]

There's no need to BUG() if we enable ATS when it's already enabled.  We
don't need to BUG() when disabling ATS on a device that doesn't support ATS
or if it's already disabled.  If ATS is enabled, certainly we found an ATS
capability in the past, so it should still be there now.

Clean up these error paths.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 0b5b0ed..67524a7 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -44,11 +44,12 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	u16 ctrl;
 	struct pci_dev *pdev;
 
-	BUG_ON(dev->ats_cap && dev->ats_enabled);
-
 	if (!dev->ats_cap)
 		return -EINVAL;
 
+	if (pci_ats_enabled(dev))
+		return -EINVAL;
+
 	if (ps < PCI_ATS_MIN_STU)
 		return -EINVAL;
 
@@ -83,7 +84,8 @@ void pci_disable_ats(struct pci_dev *dev)
 	struct pci_dev *pdev;
 	u16 ctrl;
 
-	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
+	if (!pci_ats_enabled(dev))
+		return;
 
 	if (atomic_read(&dev->ats_ref_cnt))
 		return;		/* VFs still enabled */
@@ -107,8 +109,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
 
 	if (!pci_ats_enabled(dev))
 		return;
-	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS))
-		BUG();
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)

Re: [PATCH v2 08/11] PCI: Clean up ATS error handling

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:14:54PM -0500, Bjorn Helgaas wrote:
> There's no need to BUG() if we enable ATS when it's already enabled.  We
> don't need to BUG() when disabling ATS on a device that doesn't support ATS
> or if it's already disabled.  If ATS is enabled, certainly we found an ATS
> capability in the past, so it should still be there now.
> 
> Clean up these error paths.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 0b5b0ed..67524a7 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -44,11 +44,12 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  	u16 ctrl;
>  	struct pci_dev *pdev;
>  
> -	BUG_ON(dev->ats_cap && dev->ats_enabled);
> -
>  	if (!dev->ats_cap)
>  		return -EINVAL;
>  
> +	if (pci_ats_enabled(dev))
> +		return -EINVAL;
> +

Better return -EBUSY here? It would be good to find out if we are trying
to enable ATS while it is already enabled. A WARN_ON around this would
also be good, so that we spot bugs in the iommu drivers early when they
use this interface.



	Joerg

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Bjorn Helgaas]

On Mon, Jul 20, 2015 at 07:13:49PM -0500, Bjorn Helgaas wrote:
> Gregor reported a deadlock [1] when enabling a VF that supports ATS.
> This series is intended to fix that.  The second patch should be enough to
> fix the deadlock; the rest are simplification and cleanup.
> 
> These are based on v4.2-rc2.
> 
> [1] http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
> 
> Changes between v1 and v2:
>   - Remove use of pci_ats_enabled() (intel-iommu.c)
>   - Call pci_ats_queue_depth() only once per device and cache result
>     (intel-iommu.c)
>   - Remove pci_ats_enabled() interface
>   - Stop caching queue depth in pci_dev to save space
>   - Add PF refcount of how many associated VFs have ATS enabled
>   - Add comment that ATS must be enabled on PF before on VFs
>   - Require ATS to be disabled on all VFs and PF before changing STU
> 
> 
> ---
> 
> Bjorn Helgaas (11):
>       iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
>       PCI: Allocate ATS struct during enumeration
>       PCI: Embed ATS info directly into struct pci_dev
>       PCI: Reduce size of ATS structure elements
>       PCI: Rationalize pci_ats_queue_depth() error checking
>       PCI: Inline the ATS setup code into pci_ats_init()
>       PCI: Use pci_physfn() rather than looking up physfn by hand
>       PCI: Clean up ATS error handling
>       PCI: Move ATS declarations to linux/pci.h so they're all together
>       PCI: Stop caching ATS Invalidate Queue Depth
>       PCI: Remove pci_ats_enabled()

I applied these to a pci/iommu branch for v4.3.  Let me know if you see any
issues.

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Bjorn Helgaas]

On Mon, Jul 20, 2015 at 07:13:49PM -0500, Bjorn Helgaas wrote:
> Gregor reported a deadlock [1] when enabling a VF that supports ATS.
> This series is intended to fix that.  The second patch should be enough to
> fix the deadlock; the rest are simplification and cleanup.
> 
> These are based on v4.2-rc2.
> 
> [1] http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
> 
> Changes between v1 and v2:
>   - Remove use of pci_ats_enabled() (intel-iommu.c)
>   - Call pci_ats_queue_depth() only once per device and cache result
>     (intel-iommu.c)
>   - Remove pci_ats_enabled() interface
>   - Stop caching queue depth in pci_dev to save space
>   - Add PF refcount of how many associated VFs have ATS enabled
>   - Add comment that ATS must be enabled on PF before on VFs
>   - Require ATS to be disabled on all VFs and PF before changing STU
> 
> 
> ---
> 
> Bjorn Helgaas (11):
>       iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
>       PCI: Allocate ATS struct during enumeration
>       PCI: Embed ATS info directly into struct pci_dev
>       PCI: Reduce size of ATS structure elements
>       PCI: Rationalize pci_ats_queue_depth() error checking
>       PCI: Inline the ATS setup code into pci_ats_init()
>       PCI: Use pci_physfn() rather than looking up physfn by hand
>       PCI: Clean up ATS error handling
>       PCI: Move ATS declarations to linux/pci.h so they're all together
>       PCI: Stop caching ATS Invalidate Queue Depth
>       PCI: Remove pci_ats_enabled()

I applied these to a pci/iommu branch for v4.3.  Let me know if you see any
issues.

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Yinghai Lu]

On Wed, Jul 29, 2015 at 9:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Jul 20, 2015 at 07:13:49PM -0500, Bjorn Helgaas wrote:
>> Gregor reported a deadlock [1] when enabling a VF that supports ATS.
>> This series is intended to fix that.  The second patch should be enough to
>> fix the deadlock; the rest are simplification and cleanup.
>>
>> These are based on v4.2-rc2.
>>
>> [1] http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
>>
>> Changes between v1 and v2:
>>   - Remove use of pci_ats_enabled() (intel-iommu.c)
>>   - Call pci_ats_queue_depth() only once per device and cache result
>>     (intel-iommu.c)
>>   - Remove pci_ats_enabled() interface
>>   - Stop caching queue depth in pci_dev to save space
>>   - Add PF refcount of how many associated VFs have ATS enabled
>>   - Add comment that ATS must be enabled on PF before on VFs
>>   - Require ATS to be disabled on all VFs and PF before changing STU
>>
>>
>> ---
>>
>> Bjorn Helgaas (11):
>>       iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
>>       PCI: Allocate ATS struct during enumeration
>>       PCI: Embed ATS info directly into struct pci_dev
>>       PCI: Reduce size of ATS structure elements
>>       PCI: Rationalize pci_ats_queue_depth() error checking
>>       PCI: Inline the ATS setup code into pci_ats_init()
>>       PCI: Use pci_physfn() rather than looking up physfn by hand
>>       PCI: Clean up ATS error handling
>>       PCI: Move ATS declarations to linux/pci.h so they're all together
>>       PCI: Stop caching ATS Invalidate Queue Depth
>>       PCI: Remove pci_ats_enabled()
>
> I applied these to a pci/iommu branch for v4.3.  Let me know if you see any
> issues.

looks like this branch causes hang on system with qlogic/emulex cards.

exclude the branch, will make kernel work again.

Yinghai

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Yinghai Lu]

On Thu, Aug 6, 2015 at 9:03 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Jul 29, 2015 at 9:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>
>>> Bjorn Helgaas (11):
>>>       iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
>>>       PCI: Allocate ATS struct during enumeration
>>>       PCI: Embed ATS info directly into struct pci_dev
>>>       PCI: Reduce size of ATS structure elements
>>>       PCI: Rationalize pci_ats_queue_depth() error checking
>>>       PCI: Inline the ATS setup code into pci_ats_init()
>>>       PCI: Use pci_physfn() rather than looking up physfn by hand
>>>       PCI: Clean up ATS error handling
>>>       PCI: Move ATS declarations to linux/pci.h so they're all together
>>>       PCI: Stop caching ATS Invalidate Queue Depth
>>>       PCI: Remove pci_ats_enabled()
>>
>> I applied these to a pci/iommu branch for v4.3.  Let me know if you see any
>> issues.
>
> looks like this branch causes hang on system with qlogic/emulex cards.
>
> exclude the branch, will make kernel work again.

first patch has problem:

7b98d2d02887f8d422e05323241a5fa36b2a371e is the first bad commit
commit 7b98d2d02887f8d422e05323241a5fa36b2a371e
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Mon Jul 20 09:10:36 2015 -0500

    iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth

    We check the ATS state (enabled/disabled) and fetch the PCI ATS Invalidate
    Queue Depth in performance-sensitive paths.  It's easy to cache these,
    which removes dependencies on PCI.

    Remember the ATS enabled state.  When enabling, read the queue depth once
    and cache it in the device_domain_info struct.  This is similar to what
    amd_iommu.c does.

    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

:040000 040000 e8597ffc4955b097ff3c80cbfa0e074c71761521
33b5491120a2aa89437b43d8020c7cce5339ab41 M    drivers

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Bjorn Helgaas]

On Thu, Aug 06, 2015 at 06:06:34PM -0700, Yinghai Lu wrote:
> On Thu, Aug 6, 2015 at 9:03 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Wed, Jul 29, 2015 at 9:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>>
> >>> Bjorn Helgaas (11):
> >>>       iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
> >>>       PCI: Allocate ATS struct during enumeration
> >>>       PCI: Embed ATS info directly into struct pci_dev
> >>>       PCI: Reduce size of ATS structure elements
> >>>       PCI: Rationalize pci_ats_queue_depth() error checking
> >>>       PCI: Inline the ATS setup code into pci_ats_init()
> >>>       PCI: Use pci_physfn() rather than looking up physfn by hand
> >>>       PCI: Clean up ATS error handling
> >>>       PCI: Move ATS declarations to linux/pci.h so they're all together
> >>>       PCI: Stop caching ATS Invalidate Queue Depth
> >>>       PCI: Remove pci_ats_enabled()
> >>
> >> I applied these to a pci/iommu branch for v4.3.  Let me know if you see any
> >> issues.
> >
> > looks like this branch causes hang on system with qlogic/emulex cards.
> >
> > exclude the branch, will make kernel work again.
> 
> first patch has problem:
> 
> 7b98d2d02887f8d422e05323241a5fa36b2a371e is the first bad commit
> commit 7b98d2d02887f8d422e05323241a5fa36b2a371e
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Mon Jul 20 09:10:36 2015 -0500
> 
>     iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
> 
>     We check the ATS state (enabled/disabled) and fetch the PCI ATS Invalidate
>     Queue Depth in performance-sensitive paths.  It's easy to cache these,
>     which removes dependencies on PCI.
> 
>     Remember the ATS enabled state.  When enabling, read the queue depth once
>     and cache it in the device_domain_info struct.  This is similar to what
>     amd_iommu.c does.
> 
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Huh, I don't see anything obvious there.  Where does it hang?  Boot?
Driver attach?  Can you give me any hints, or maybe try the attached patch?
Is there anything funny about those particular devices?  Maybe some
"lspci -vv" output?


diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6603448..988d9b5 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1406,10 +1406,13 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 
 	info->ats.enabled = 1;
 	info->ats.qdep = pci_ats_queue_depth(pdev);
+	dev_info(info->dev, "enabled ATS, qdep %d\n", info->ats.qdep);
 }
 
 static void iommu_disable_dev_iotlb(struct device_domain_info *info)
 {
+	dev_info(info->dev, "disabling ATS, current %d\n", info->ats.enabled):
+
 	if (!info->ats.enabled)
 		return;
 
@@ -1426,6 +1429,13 @@ static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
 
 	spin_lock_irqsave(&device_domain_lock, flags);
 	list_for_each_entry(info, &domain->devices, link) {
+		struct pci_dev *pdev;
+		if (!info->dev || !dev_is_pci(info->dev))
+			continue;
+
+		dev_info(info->dev, "flush; cached ena %d qdep %d, current ena %d qdep %d\n",
+			info->ats.enabled, info->ats.qdep,
+			pci_ats_enabled(pdev), pci_ats_queue_depth(pdev));
 		if (!info->ats.enabled)
 			continue;
 

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Yinghai Lu]

On Mon, Aug 10, 2015 at 10:33 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
...
> Huh, I don't see anything obvious there.  Where does it hang?  Boot?
> Driver attach?  Can you give me any hints, or maybe try the attached patch?
> Is there anything funny about those particular devices?  Maybe some
> "lspci -vv" output?

with debug patch:

[   79.691788] calling  qla2x00_module_init+0x0/0x21c @ 1
[   79.711206] qla2xxx [0000:00:00.0]-0005: : QLogic Fibre Channel HBA
Driver: 8.07.00.18-k.
[   79.731379] qla2xxx [0000:45:00.0]-001d: : Found an ISP2532 irq 30
iobase 0xffffc900311ae000.
[   79.733945] qla2xxx 0000:45:00.0: enabling Mem-Wr-Inval
[   82.381363] qla2xxx 0000:45:00.0: flush; cached ena 0 qdep 0,
current ena 0 qdep -19
[   82.397008] qla2xxx 0000:45:00.0: flush; cached ena 0 qdep 0,
current ena 0 qdep -19
[   82.621261] qla2xxx 0000:45:00.0: flush; cached ena 0 qdep 0,
current ena 0 qdep -19
[   82.625717] scsi host0: qla2xxx
[   87.646633] qla2xxx [0000:45:00.0]-00fb:0: QLogic QEM3572 -
SG-XPCIEFCGBE-Q8-Z Sun StorageTek Dual 8Gb FC Dual GbE HBA.
[   87.647145] qla2xxx [0000:45:00.0]-00fc:0: ISP2532: PCIe (5.0GT/s
x8) @ 0000:45:00.0 hdma+ host#=0 fw=5.10.00 (90d5).
[   87.663408] qla2xxx [0000:45:00.1]-001d: : Found an ISP2532 irq 31
iobase 0xffffc9003120e000.
[   87.684329] qla2xxx 0000:45:00.1: enabling Mem-Wr-Inval
[   90.331021] qla2xxx 0000:45:00.1: flush; cached ena 1 qdep 121,
current ena 0 qdep -19
...


Looks like ats in the device_domain_info is not initialized to 0.
after set it 0, it will work.


Index: linux-2.6/drivers/iommu/intel-iommu.c
===================================================================
--- linux-2.6.orig/drivers/iommu/intel-iommu.c
+++ linux-2.6/drivers/iommu/intel-iommu.c
@@ -2281,6 +2292,8 @@ static struct dmar_domain *dmar_insert_d
        info->dev = dev;
        info->domain = domain;
        info->iommu = iommu;
+       info->ats.enabled = 0;
+       info->ats.qdep = 0;

        spin_lock_irqsave(&device_domain_lock, flags);
        if (dev)

[PATCH v2 05/11] PCI: Rationalize pci_ats_queue_depth() error checking

[Bjorn Helgaas]

We previously returned -ENODEV for devices that don't support ATS (except
that we always returned 0 for VFs, whether or not they support ATS).

For consistency, always return -EINVAL (not -ENODEV) if the device doesn't
support ATS.  Return zero for VFs that support ATS.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/pci/ats.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 690ae6e..9a98b3a 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -136,13 +136,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
  */
 int pci_ats_queue_depth(struct pci_dev *dev)
 {
+	if (!dev->ats_cap)
+		return -EINVAL;
+
 	if (dev->is_virtfn)
 		return 0;
 
-	if (dev->ats_cap)
-		return dev->ats_qdep;
-
-	return -ENODEV;
+	return dev->ats_qdep;
 }
 EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
 

[PATCH v2 06/11] PCI: Inline the ATS setup code into pci_ats_init()

[Bjorn Helgaas]

The ATS setup code in ats_alloc_one() is only used by pci_ats_init(), so
inline it there.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/pci/ats.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 9a98b3a..95905f3 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -17,7 +17,7 @@
 
 #include "pci.h"
 
-static void ats_alloc_one(struct pci_dev *dev)
+void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
 	u16 cap;
@@ -32,11 +32,6 @@ static void ats_alloc_one(struct pci_dev *dev)
 					    PCI_ATS_MAX_QDEP;
 }
 
-void pci_ats_init(struct pci_dev *dev)
-{
-	ats_alloc_one(dev);
-}
-
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device

[PATCH v2 09/11] PCI: Move ATS declarations to linux/pci.h so they're all together

[Bjorn Helgaas]

Move ATS declarations to linux/pci.h so they're all in one place.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 include/linux/pci-ats.h |   41 -----------------------------------------
 include/linux/pci.h     |   10 +++++++++-
 2 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 5d81d47..57e0b82 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -3,47 +3,6 @@
 
 #include <linux/pci.h>
 
-/* Address Translation Service */
-#ifdef CONFIG_PCI_ATS
-
-int pci_enable_ats(struct pci_dev *dev, int ps);
-void pci_disable_ats(struct pci_dev *dev);
-int pci_ats_queue_depth(struct pci_dev *dev);
-
-/**
- * pci_ats_enabled - query the ATS status
- * @dev: the PCI device
- *
- * Returns 1 if ATS capability is enabled, or 0 if not.
- */
-static inline int pci_ats_enabled(struct pci_dev *dev)
-{
-	return dev->ats_cap && dev->ats_enabled;
-}
-
-#else /* CONFIG_PCI_ATS */
-
-static inline int pci_enable_ats(struct pci_dev *dev, int ps)
-{
-	return -ENODEV;
-}
-
-static inline void pci_disable_ats(struct pci_dev *dev)
-{
-}
-
-static inline int pci_ats_queue_depth(struct pci_dev *dev)
-{
-	return -ENODEV;
-}
-
-static inline int pci_ats_enabled(struct pci_dev *dev)
-{
-	return 0;
-}
-
-#endif /* CONFIG_PCI_ATS */
-
 #ifdef CONFIG_PCI_PRI
 
 int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 238b77e..307f96a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1301,8 +1301,16 @@ void ht_destroy_irq(unsigned int irq);
 #ifdef CONFIG_PCI_ATS
 /* Address Translation Service */
 void pci_ats_init(struct pci_dev *dev);
+int pci_enable_ats(struct pci_dev *dev, int ps);
+void pci_disable_ats(struct pci_dev *dev);
+int pci_ats_queue_depth(struct pci_dev *dev);
+static inline int pci_ats_enabled(struct pci_dev *dev) { return dev->ats_cap && dev->ats_enabled; }
 #else
-static inline void pci_ats_init(struct pci_dev *dev) { }
+static inline void pci_ats_init(struct pci_dev *d) { }
+static inline int pci_enable_ats(struct pci_dev *d, int ps) { return -ENODEV; }
+static inline void pci_disable_ats(struct pci_dev *d) { }
+static inline int pci_ats_queue_depth(struct pci_dev *d) { return -ENODEV; }
+static inline int pci_ats_enabled(struct pci_dev *d) { return 0; }
 #endif
 
 void pci_cfg_access_lock(struct pci_dev *dev);

[PATCH v2 10/11] PCI: Stop caching ATS Invalidate Queue Depth

[Bjorn Helgaas]

Stop caching the Invalidate Queue Depth in struct pci_dev.
pci_ats_queue_depth() is typically called only once per device, and it
returns a fixed value per-device, so callers who need the value frequently
can cache it themselves.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c   |    9 ++++-----
 include/linux/pci.h |    1 -
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 67524a7..bdb1383 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -20,16 +20,12 @@
 void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
-	u16 cap;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
 	if (!pos)
 		return;
 
 	dev->ats_cap = pos;
-	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
-	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
-					    PCI_ATS_MAX_QDEP;
 }
 
 /**
@@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
  */
 int pci_ats_queue_depth(struct pci_dev *dev)
 {
+	u16 cap;
+
 	if (!dev->ats_cap)
 		return -EINVAL;
 
 	if (dev->is_virtfn)
 		return 0;
 
-	return dev->ats_qdep;
+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
+	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
 }
 EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 307f96a..4b484fd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -378,7 +378,6 @@ struct pci_dev {
 	};
 	u16		ats_cap;	/* ATS Capability offset */
 	u8		ats_stu;	/* ATS Smallest Translation Unit */
-	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
 	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */

Re: [PATCH v2 10/11] PCI: Stop caching ATS Invalidate Queue Depth

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:15:11PM -0500, Bjorn Helgaas wrote:
> Stop caching the Invalidate Queue Depth in struct pci_dev.
> pci_ats_queue_depth() is typically called only once per device, and it
> returns a fixed value per-device, so callers who need the value frequently
> can cache it themselves.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c   |    9 ++++-----
>  include/linux/pci.h |    1 -
>  2 files changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH v2 10/11] PCI: Stop caching ATS Invalidate Queue Depth

[Don Dutile]

On 07/20/2015 08:15 PM, Bjorn Helgaas wrote:
> Stop caching the Invalidate Queue Depth in struct pci_dev.
> pci_ats_queue_depth() is typically called only once per device, and it
> returns a fixed value per-device, so callers who need the value frequently
> can cache it themselves.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/ats.c   |    9 ++++-----
>   include/linux/pci.h |    1 -
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 67524a7..bdb1383 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -20,16 +20,12 @@
>   void pci_ats_init(struct pci_dev *dev)
>   {
>   	int pos;
> -	u16 cap;
>
>   	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
>   	if (!pos)
>   		return;
>
>   	dev->ats_cap = pos;
> -	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> -	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
> -					    PCI_ATS_MAX_QDEP;
>   }
>
>   /**
> @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
>    */
>   int pci_ats_queue_depth(struct pci_dev *dev)
>   {
> +	u16 cap;
> +
>   	if (!dev->ats_cap)
>   		return -EINVAL;
>
>   	if (dev->is_virtfn)
>   		return 0;
>
hmmm, isn't one of the fixes in this patch set to
change the caching of ats queue depth ?  won't above
make it 0 for all VF's ?  is that correct, or desired (virtual) value?

> -	return dev->ats_qdep;
> +	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> +	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
>   }
>   EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 307f96a..4b484fd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -378,7 +378,6 @@ struct pci_dev {
>   	};
>   	u16		ats_cap;	/* ATS Capability offset */
>   	u8		ats_stu;	/* ATS Smallest Translation Unit */
> -	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
>   	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
>   #endif
>   	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2 10/11] PCI: Stop caching ATS Invalidate Queue Depth

[Bjorn Helgaas]

Hi Don,

On Mon, Jul 27, 2015 at 10:00:53AM -0400, Don Dutile wrote:
> On 07/20/2015 08:15 PM, Bjorn Helgaas wrote:
> >Stop caching the Invalidate Queue Depth in struct pci_dev.
> >pci_ats_queue_depth() is typically called only once per device, and it
> >returns a fixed value per-device, so callers who need the value frequently
> >can cache it themselves.
> >
> >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >---
> >  drivers/pci/ats.c   |    9 ++++-----
> >  include/linux/pci.h |    1 -
> >  2 files changed, 4 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> >index 67524a7..bdb1383 100644
> >--- a/drivers/pci/ats.c
> >+++ b/drivers/pci/ats.c
> >@@ -20,16 +20,12 @@
> >  void pci_ats_init(struct pci_dev *dev)
> >  {
> >  	int pos;
> >-	u16 cap;
> >
> >  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> >  	if (!pos)
> >  		return;
> >
> >  	dev->ats_cap = pos;
> >-	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> >-	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
> >-					    PCI_ATS_MAX_QDEP;
> >  }
> >
> >  /**
> >@@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
> >   */
> >  int pci_ats_queue_depth(struct pci_dev *dev)
> >  {
> >+	u16 cap;
> >+
> >  	if (!dev->ats_cap)
> >  		return -EINVAL;
> >
> >  	if (dev->is_virtfn)
> >  		return 0;
> >
> hmmm, isn't one of the fixes in this patch set to
> change the caching of ats queue depth ?  won't above
> make it 0 for all VF's ?  is that correct, or desired (virtual) value?

Per spec (SR-IOV r1.1., sec 3.7.4), the ATS queue depth register on a VF
always contains zero.

Here's the v4.1 code:

    int pci_ats_queue_depth(struct pci_dev *dev)
    {
        if (dev->is_virtfn)
                return 0;

        if (dev->ats)
                return dev->ats->qdep;

	...


In v4.1, pci_ats_queue_depth() always returned 0 for a VF.  For VFs, we
didn't look at the dev->ats->qdep cache.  So I don't think this changes
anything for the caller.

A previous patch changed this path so we return -EINVAL instead of 0 for
VFs that don't support ATS.  I think the previous behavior there was wrong,
but I doubt anybody noticed.

Bjorn

> >-	return dev->ats_qdep;
> >+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
> >+	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
> >
> >diff --git a/include/linux/pci.h b/include/linux/pci.h
> >index 307f96a..4b484fd 100644
> >--- a/include/linux/pci.h
> >+++ b/include/linux/pci.h
> >@@ -378,7 +378,6 @@ struct pci_dev {
> >  	};
> >  	u16		ats_cap;	/* ATS Capability offset */
> >  	u8		ats_stu;	/* ATS Smallest Translation Unit */
> >-	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
> >  	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
> >  #endif
> >  	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 

Re: [PATCH v2 10/11] PCI: Stop caching ATS Invalidate Queue Depth

[Don Dutile]

On 07/27/2015 06:27 PM, Bjorn Helgaas wrote:
> Hi Don,
>
> On Mon, Jul 27, 2015 at 10:00:53AM -0400, Don Dutile wrote:
>> On 07/20/2015 08:15 PM, Bjorn Helgaas wrote:
>>> Stop caching the Invalidate Queue Depth in struct pci_dev.
>>> pci_ats_queue_depth() is typically called only once per device, and it
>>> returns a fixed value per-device, so callers who need the value frequently
>>> can cache it themselves.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>   drivers/pci/ats.c   |    9 ++++-----
>>>   include/linux/pci.h |    1 -
>>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>> index 67524a7..bdb1383 100644
>>> --- a/drivers/pci/ats.c
>>> +++ b/drivers/pci/ats.c
>>> @@ -20,16 +20,12 @@
>>>   void pci_ats_init(struct pci_dev *dev)
>>>   {
>>>   	int pos;
>>> -	u16 cap;
>>>
>>>   	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
>>>   	if (!pos)
>>>   		return;
>>>
>>>   	dev->ats_cap = pos;
>>> -	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
>>> -	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
>>> -					    PCI_ATS_MAX_QDEP;
>>>   }
>>>
>>>   /**
>>> @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
>>>    */
>>>   int pci_ats_queue_depth(struct pci_dev *dev)
>>>   {
>>> +	u16 cap;
>>> +
>>>   	if (!dev->ats_cap)
>>>   		return -EINVAL;
>>>
>>>   	if (dev->is_virtfn)
>>>   		return 0;
>>>
>> hmmm, isn't one of the fixes in this patch set to
>> change the caching of ats queue depth ?  won't above
>> make it 0 for all VF's ?  is that correct, or desired (virtual) value?
>
> Per spec (SR-IOV r1.1., sec 3.7.4), the ATS queue depth register on a VF
> always contains zero.
>
> Here's the v4.1 code:
>
>      int pci_ats_queue_depth(struct pci_dev *dev)
>      {
>          if (dev->is_virtfn)
>                  return 0;
>
>          if (dev->ats)
>                  return dev->ats->qdep;
>
> 	...
>
>
> In v4.1, pci_ats_queue_depth() always returned 0 for a VF.  For VFs, we
> didn't look at the dev->ats->qdep cache.  So I don't think this changes
> anything for the caller.
>
> A previous patch changed this path so we return -EINVAL instead of 0 for
> VFs that don't support ATS.  I think the previous behavior there was wrong,
> but I doubt anybody noticed.
>
> Bjorn
>
ok. thanks for the sanity check.

>>> -	return dev->ats_qdep;
>>> +	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
>>> +	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP;
>>>   }
>>>   EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
>>>
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 307f96a..4b484fd 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -378,7 +378,6 @@ struct pci_dev {
>>>   	};
>>>   	u16		ats_cap;	/* ATS Capability offset */
>>>   	u8		ats_stu;	/* ATS Smallest Translation Unit */
>>> -	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
>>>   	atomic_t	ats_ref_cnt;	/* number of VFs with ATS enabled */
>>>   #endif
>>>   	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>

[PATCH v2 11/11] PCI: Remove pci_ats_enabled()

[Bjorn Helgaas]

Remove pci_ats_enabled().  There are no callers outside the ATS code
itself.  We don't need to check ats_cap, because if we don't find an ATS
capability, we'll never set ats_enabled.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c   |    6 +++---
 include/linux/pci.h |    2 --
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index bdb1383..7a1c88f 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -43,7 +43,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	if (!dev->ats_cap)
 		return -EINVAL;
 
-	if (pci_ats_enabled(dev))
+	if (dev->ats_enabled)
 		return -EINVAL;
 
 	if (ps < PCI_ATS_MIN_STU)
@@ -80,7 +80,7 @@ void pci_disable_ats(struct pci_dev *dev)
 	struct pci_dev *pdev;
 	u16 ctrl;
 
-	if (!pci_ats_enabled(dev))
+	if (!dev->ats_enabled)
 		return;
 
 	if (atomic_read(&dev->ats_ref_cnt))
@@ -103,7 +103,7 @@ void pci_restore_ats_state(struct pci_dev *dev)
 {
 	u16 ctrl;
 
-	if (!pci_ats_enabled(dev))
+	if (!dev->ats_enabled)
 		return;
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4b484fd..806da76 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1303,13 +1303,11 @@ void pci_ats_init(struct pci_dev *dev);
 int pci_enable_ats(struct pci_dev *dev, int ps);
 void pci_disable_ats(struct pci_dev *dev);
 int pci_ats_queue_depth(struct pci_dev *dev);
-static inline int pci_ats_enabled(struct pci_dev *dev) { return dev->ats_cap && dev->ats_enabled; }
 #else
 static inline void pci_ats_init(struct pci_dev *d) { }
 static inline int pci_enable_ats(struct pci_dev *d, int ps) { return -ENODEV; }
 static inline void pci_disable_ats(struct pci_dev *d) { }
 static inline int pci_ats_queue_depth(struct pci_dev *d) { return -ENODEV; }
-static inline int pci_ats_enabled(struct pci_dev *d) { return 0; }
 #endif
 
 void pci_cfg_access_lock(struct pci_dev *dev);

Re: [PATCH v2 11/11] PCI: Remove pci_ats_enabled()

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:15:19PM -0500, Bjorn Helgaas wrote:
> Remove pci_ats_enabled().  There are no callers outside the ATS code
> itself.  We don't need to check ats_cap, because if we don't find an ATS
> capability, we'll never set ats_enabled.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/pci/ats.c   |    6 +++---
>  include/linux/pci.h |    2 --
>  2 files changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Re: [PATCH v2 11/11] PCI: Remove pci_ats_enabled()

[Joerg Roedel]

On Mon, Jul 20, 2015 at 07:15:19PM -0500, Bjorn Helgaas wrote:
> Remove pci_ats_enabled().  There are no callers outside the ATS code
> itself.  We don't need to check ats_cap, because if we don't find an ATS
> capability, we'll never set ats_enabled.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c   |    6 +++---
>  include/linux/pci.h |    2 --
>  2 files changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH v2 00/11] PCI: Fix ATS deadlock

[Joerg Roedel]

Hi Bjorn,

On Mon, Jul 20, 2015 at 07:13:49PM -0500, Bjorn Helgaas wrote:
> Bjorn Helgaas (11):
>       iommu/vt-d: Cache PCI ATS state and Invalidate Queue Depth
>       PCI: Allocate ATS struct during enumeration
>       PCI: Embed ATS info directly into struct pci_dev
>       PCI: Reduce size of ATS structure elements
>       PCI: Rationalize pci_ats_queue_depth() error checking
>       PCI: Inline the ATS setup code into pci_ats_init()
>       PCI: Use pci_physfn() rather than looking up physfn by hand
>       PCI: Clean up ATS error handling
>       PCI: Move ATS declarations to linux/pci.h so they're all together
>       PCI: Stop caching ATS Invalidate Queue Depth
>       PCI: Remove pci_ats_enabled()

So while you are at working on ATS, could we probably also introduce a
way to globally disable ATS on a box from the kernel command line
(pci=noats?)? ATS is basically a way for a device to tunnel through the
IOMMU without access checking (because pre-translated requests are not
checked again).

For security reasons it would be good to have a way to disable ATS
completly if desired.



	Joerg