Re: Who understand the IOMMU code?

Re: Who understand the IOMMU code?

[Andrew Cooks]

On 31 January 2014 04:12, George Spelvin <linux@horizon.com> wrote:
>
> 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.

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

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

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?

> 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.

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?

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


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.

Re: Who understand the IOMMU code?

[George Spelvin]

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

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

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.

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

> Which additional mapping?

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

> 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?

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.

> 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.

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.

> 2) You complain that "this stuff" (presumably the stuff you didn't
>    learn from the ugly patch) is hard to understand.

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.

> 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?

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 don't think you understood the relevance of Alex's patch set for a
> "PCIe requester ID interface", that I referred you to.

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.

> 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.

I may have to ask Joerg.