Re: [PATCH 2/2] PCI: Disable ATS for specific Intel IPU E2000 devices

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Sep 06, 2023 at 11:00:19PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 10:59:20PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 02:06:23PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 16, 2023 at 05:21:15PM +0000, Bartosz Pawlowski wrote:
> 
> ...
> 
> > > > +/*
> > > > + * Intel IPU E2000 revisions before C0 implement incorrect endianness
> > > > + * in ATS Invalidate Request message body. Although there is existing software
> > > > + * workaround for this issue, it is not functionally complete (no 5-lvl paging
> > > > + * support) and it requires changes in all IOMMU implementations supporting
> > > > + * ATS. Therefore, disabling ATS seems to be more reasonable.
> > > 
> > > Can we reference the commit that added the existing software
> > > workaround?
> 
> > See below.
> 
> Oh, I meant the second question here, i.e.
> 
> > > It sounds like systems that (a) don't require 5-level paging and (b)
> > > use an IOMMU implementation that include the appropriate changes might
> > > still be able to use ATS?  Is there a way for them to do that?
> 
> ^^^ this one.

Sorry, I'm missing your point here.

The comment mentions an existing partial software workaround.
Presumably that was added by some commit, and it would be helpful to
know which one.

The comment also suggests that if the software workaround were
completed (or if a system didn't require 5-level paging) and it had
related changes to its IOMMU driver, we could still use ATS even on
hardware with this defect.

So I'm wondering if there's a way for an IOMMU driver that has the
required changes and can tell that we're not using 5-level paging can
override this quirk that disables ATS.

Maybe we want to unconditionally disable ATS on these broken devices.
In that case, I think we should just completely drop the comments
about the software workaround and IOMMU driver changes because they
wouldn't be relevant.

> > > > + */
> > > > +static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
> > > > +{
> > 
> > > > +	if (pdev->revision < 0x20)
> > 
> > Isn't it the answer to your question?
> > 
> > > > +		quirk_no_ats(pdev);
> > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>