lkml.org 
[lkml]   [2021]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [GIT PULL] (swiotlb) stable/for-linus-5.15
From
Date
Hi Linus,

On 2021-09-02 19:54, Linus Torvalds wrote:
> On Wed, Sep 1, 2021 at 2:09 PM Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
>>
>> which has a new feature called restricted DMA pools. It allows SWIOTLB to utilize
>> per-device (or per-platform) allocated memory pools instead of using the global one.
>>
>> The first big user of this is ARM Confidential Computing where the memory for DMA
>> operations can be set per platform.
>
> Hmm.
>
> I was going to merge this, but I had already merged the dma-mapping
> updates from Christoph Hellwig first.
>
> And it turns out that the conflicts are trivial to do the
> straightforward way, that's not a problem.
>
> But I think that straightforward way is actually buggy, because it
> makes the is_swiotlb_for_alloc() type allocations then use that new
> global pool for allocations.
>
> Just to check, I looked into what linux-next did, in case this had
> caused any discussion there. Nope. Linux-next does the obvious
> straightforward merge.
>
> So I'm going to hold off merging until I can clarify what the rules
> for these restricted DMA pools are, because I get the feeling that
> people didn't actually think about the interaction with the new global
> pool very much.

Not to defend the state things have got into - frankly I've found it
increasingly hard to follow for some time now too - but I believe the
unwritten story is of the one true dma-direct trying to be all things to
all users, and there are several aspects at play here which are mutually
exclusive in practice.

> Or if they did, it didn't get communicated to me. Christoph mentioned
> the merge conflict, but he implied that I should just merge it the
> obvious way. And the more I look at that code, the less I think that's
> the right thing to do.
>
> So just to make this very concrete, look at dma_direct_alloc(). The
> straightforward merge is to do this:
>
> if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
> !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
> !IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
> !dev_is_dma_coherent(dev) &&
> !is_swiotlb_for_alloc(dev))
> return arch_dma_alloc(dev, size, dma_handle, gfp, attrs);
>
> if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
> !dev_is_dma_coherent(dev))
> return dma_alloc_from_global_coherent(dev, size, dma_handle);
>
> and that seems to be what linux-next did too.
>
> But that means that a is_swiotlb_for_alloc(dev) allocation can use
> that new global coherent pool (see that second case).
>
> Maybe that's ok. But I get the feeling that it is not. It sure as heck
> isn't obvious. If it's ok to use the global pool, then fine - but I
> want to know why (and I want the code to say why).

The global pool is essentially for noMMU, so should not be expected to
overlap with SWIOTLB at all, much less systems using restricted DMA.

> I think that the second conditional should probably get an all-new added
>
> && !is_swiotlb_for_alloc(dev)
>
> in it, but that's not what happened in linux-next. And all this code
> is messy and illogical and crazy.
>
> Btw, what's the actual logic with the
>
> !dev_is_dma_coherent(dev)
>
> tests anyway? Those are pre-existing, but seem odd and senseless. The
> function is called "dma_alloc_from_global_coherent()", but it is *not*
> called when "dev_is_dma_coherent()".

Heh, good terminology for this is hard to find. One way to make sense of
it is to consider that either the device itself is coherent - i.e. can
snoop caches such that we don't have to much in the way of special
treatment - or otherwise it's the memory which has to be made coherent,
i.e. remapped non-cacheable, such that CPUs and non-snooping devices can
still maintain a consistent view of it (note: I'm focusing on caches
here for simplicity, but in practice there's other stuff like memory
encryption in the mix too). Hence a "coherent pool" is actually a pool
of non-cacheably-mapped memory from which to allocate for
non-hardware-coherent devices in contexts where we can't remap regular
page allocations on-the-fly (also called an "atomic pool" in various
places for that reason). For noMMU with caches, that's effectively all
the time, which is where the global pool came from - unlike the atomic
pool(s) for MMU systems which can just be allocated and remapped at
boot, the global pool is often subject to platform-specific restrictions
like being at a special physical alias, so starts life in a very
different way.

[ And the reason it's called the global coherent pool is from the
implementation growing out of per-device coherent pools, which are yet
another similar-but-slightly-different thing. For added fun, those may
also intersect with restricted DMA, but that all forks off down the
dma_alloc_from_dev_coherent() path before we even get to dma-direct. ]

After the initial setup, though, I guess it might be possible with a bit
more refactoring to streamline the global pool bits into the
dma_alloc_from_pool() path, since by that point it's functionally
equivalent to the atomic pool(s), just under different conditions.

> Is it just me, or does anybody else also get the feeling that too many
> recreational drugs may have been involved in the creation of this code
> over the years? People possibly self-medicating to take away the pain?
>
> Anyway - I'm going to punt on this pull request, because I want
> clarification that didn't happen despite the conflict in linux-next.
>
> I also have to say, looking at that code, you guys write collectively
> some disgusting code.
>
> I think individually each change has been probably fairly small, but
> taken together it's just too nasty for words. That code should be
> taken out behind a shed and shot in the head, because it's entirely
> impossible to read all those nasty conditionals over multiple lines.
>
> So regardless of what the merge resolution should be, I think this
> code needs reorganizing. Because those entirely random conditionals
> with several different config tests that DO NOT MAKE SENSE TOGETHER
> should not exist.
>
> The merge conflict is just a symptom of the disease in this code, not
> the problem itself.
>
> That code needs to be split up, and the logic needs to be made
> understandable. Preferably *obvious*. Which it isn't now.

I suppose one logical option would be to split out dedicated versions of
dma-direct for at least the major differences - noMMU,
CONFIG_DMA_DIRECT_REMAP, etc. - but that does run the risk of ending up
as a lot of very similar looking code and being a rich source of bugs
where things get added/fixed in one place but not the others.

> In particular, if the rule is that "is_swiotlb_for_alloc()" means that
> you *have* to use swiotlb_alloc() and absolutely nothing else", then
> that should be the logic, and it should be at the top, and that should
> be all it is. No m ixing with the other tests that are about entirely
> different issues.

Sadly it doesn't entirely mean that. A device using restricted DMA
*should* usually end up allocating from its own dedicated SWIOTLB pool,
but there may be cases where falling back to
dma_direct_alloc_from_pool() is still OK, or maybe even necessary.

> Because right now, that code is literally set up to use completely
> random different allocation choices depending on completely random and
> illogical combinations of config rules mixed with random device rules.
>
> Make each conditional be about *one* thing, and one thing only. Not
> about three unrelated things that may or may have the same end result
> rule.
>
> See what I'm saying?
>
> The solution may be something like
>
> if (is_swiotlb_for_alloc(dev))
> goto swiotlb_alloc_only;
>
> at the top of the allocation routines, or whatever. Don't make the
> other conditionals - that have nothing to do with this thing - get
> mixed up in the fundamental rule.
>
> Also, that's an odd name. "is_swiotlb_for_alloc()"? Those four words
> do not make sense together in that order. Shouldn't it be
> "use_swiotlb_for_alloc()"?
>
> So make the actual rules individual, and make them make sense. It's ok
> to have five (or more) of those kinds of things, but they need to be
> individually sensible, and not randomly tied to each other as one big
> broken non-sensible thing.
>
> Please no more of this completely crazy "let's mix it all together in
> a blender, and write code that makes no sense".
>
> In the immediate short term, please
>
> (a) explain what the rules actually are

I hope some of the background above has helped a bit.

> (b) so that I can do a merge that actually does the right thing (ie I
> _think_ I should add that extra test above, but I want this thought
> about and confirmed or be told why that isn't the case).
>
> and then the rewrite should be a future step. But that rewrite of
> those disgusting "rules" absolutely NEEDS to happen, because looking
> at that spaghetti of random config options and other random
> conditionals, that's simply not acceptable.

FWIW, no argument from me there - I believe it's actually ChromeOS and
Android pKVM having the initial use-cases for getting this new SWIOTLB
functionality landed, but in terms of Arm CCA we certainly still have
some interest in it at the moment, so I might be able to wrangle some
time to take on a bit of cleanup from that angle.

Cheers,
Robin.

> It's particularly not acceptable since I suspect that it all probably
> *works* even if you allocate from the wrong pool, but it then
> presumably violates some security rule that it was set up so that
> allocations would *not* preferably get mixed up in some global pool.
>
> Pls advise. I'm not asking anybody to do the merge for me - I'm
> literally asking for that code and the rules to be clarified.
>
> Linus
>

\
 
 \ /
  Last update: 2021-09-02 23:56    [W:0.050 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site