Who understand the IOMMU code?

I'm trying to add a quirk to the IOMMU code to handle certain buggy devices which use the wrong requester id (devfn) for PCIe requests. In my case, there are a number of Marvell SATA controllers, popular with motherboard manufacturers, which generate DMA from functions 0 and 1, even though they only support function 0.

Andrew cooks wrote a working (but, to my eyes, ugly) patch; see
https://bugzilla.kernel.org/show_bug.cgi?id=42679#c22

I'm trying to improve it, but even limiting myself to intel-iommu.c, it's definitely hairy.

Right now, I'm trying to figure out where to remove the additional mapping.

Currently I'm looking at adding another clear_context_table() call before iommu_detach_dev() in domain_remove_one_dev_info(); my goal is to piggyback on iommu_detach_dev's flush calls.

But I'm having trouble figuring out the whole device_domain_info list business.

In particular, does the comparison of domain, bus, and devinfo ever produce a different result than just comparing info->dev to pdev?

I have a flag in the pci_device warning about the quirk, but I'm having trouble finding the function that's the opposite of domain_context_mapping().

Does anyone actually understand this stuff well enough to review my patches when I finish? May I ask for a bit of guidance on the way? Right now I don't even understand the Intel iommu code, much less all ther others I should be patching for a full fix.

The same code points would also be a good place to add phantom functions support some day.

Thank you!

What's hairy? The patch, the problem, or the iommu code?

Which additional mapping? Are you implying that the patch adds redundant context entries or are you talking about folding the quirk_map_multi_requester_ids()
and quirk_map_requester_id() functions into the main code path?

Let me see if I understand you correctly.
1) You don't like the simple patch that clearly shows the problem (and offers a simple work-around), because it's too ugly.
2) You complain that "this stuff" (presumably the stuff you didn't learn from the
ugly patch) is hard to understand.
3) You question whether anyone else understands "this stuff".
4) You want to know whether anyone is qualified to review your patch when

    you 'finish it' ...

5) ... as long as someone just help you a little bit?

It that really what you meant?

I don't think you understood the relevance of Alex's patch set for a "PCIe requester ID interface", that I referred you to.

Firstly, finding the full set of requester ids that an iommu might see for every pdev below it is a bigger problem than just quirky Marvell SATA controllers and phantom functions (which may or may not actually be used by anyone). PCIe bridges, even spec compliant ones, introduce similar issues.

Secondly, that patch tries to unify the quirky and non quirky cases under the pcie_for_each_requester() function, which seems like the prefered approach.

Thirdly, that patch was written and reviewed by people who do actually understand how "this stuff" works, so there is much to learn from it if you want to write the solution that is ultimately accepted into mainline and convince them that you can (and will) maintain it.

Here's the link to the patch posting and review again:
https://lists.linuxfoundation.org/pipermail/iommu/2013-July/006083.html

When you've read and understood that discussion, have another look at the current domain_context_mapping() and try to see the resemblance between the handling of bridging, dependant devices and quirky devices.

Ultimately, this isn't going anywhere until David Woodhouse, the Intel iommu maintainer, gets involved. I have not yet found the magical incantation to achieve that.

May the odds be ever in your favour!

a.

The iommu code. The problem is pretty straightforward, it's just the existing code I'm trying to patch that's already large and complex and figuring out how to fit in as neatly as possible is a challenge.

The additional requester_id -> IOMMU domain mapping that is set up for the additional devfn(s).

I'm working on a complete rewrite, using the existing PCI quirk system, although with yours as a base, and I'm having a hard time convincing myself that the quirk_unmap_* functions are in the right place.

Yes. I want something that can get merged upstream, and I don't think the existing one has much of a chance. If nothing else, the linear search through exception lists each time a mapping is set up will quickly become a bottleneck.

Well, the target of the complaint is my own knowledge. I'm not claiming the complexity is *unnecessary*, just that it is hard to follow.

Pretty much. Figuring out whose blessing I need and at least making initial contact seems like an obvious prerequisite to getting a patch merged,

The help I need is primarily not breaking the existing code, which requires a clear understanding of what it's supposed to do.

Really, I just want to find *someone* with authority in this area who responds to e-mail. I can escalate to someone higher on the patch merge hierarchy, but I want to make it very clear that I tried before going that route.

I was hoping that chatting to you on appropriate public mailing lists would attract some attention, but maybe I have to be more direct.

I think I didn't understand it as well the first itme I read it. Certainly it's a nice cleanup, and would be a better place to put the relevant quirks. If, as you say, it gets un-stalled.

I've just now been re-reading the discussion, and it's a lot more comprehensible now that I've read a number of PCIe and IOMMU specification documents.

I still have to wrap my head around ACS, though. That also affects the code heavily.

I may have to ask Joerg.