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

[Joerg Roedel <jroedel@suse.de>]

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