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@xxxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
