On Wed, Nov 27, 2019 at 05:09:41PM +0000, Robin Murphy wrote:
> On 27/11/2019 2:16 pm, Thierry Reding wrote:
> [...]
> > Nevermind that, I figured out that I was missingthe initialization of
> > some of the S2CR variables. I've got something that I think is working
> > now, though I don't know yet how to go about cleaning up the initial
> > mapping and "recycling" it.
> > 
> > I'll clean things up a bit, run some more tests and post a new patch
> > that can serve as a basis for discussion.
> 
> I'm a little puzzled by the smmu->identity domain - disregarding the fact
> that it's not actually used by the given diff ;) - if isolation is the
> reason for not simply using a bypass S2CR for the window between reset and
> the relevant .add_device call where the default domain proper comes in[1],
> then surely exposing the union of memory regions to the union of all
> associated devices isn't all that desirable either.

A bypass S2CR was what I had originally in mind, but Will objected to
that because it "leaves the thing wide open if we don't subsequently
probe the master."[0]

Will went on to suggest setting up a page-table early for stream IDs
with reserved regions, so that's what I implemented. It ends up working
fairly nicely (see attached patch).

I suppose putting all the masters into the same bucket isn't an ideal
solution, but it's pretty simple and straightforward. Also, I don't
expect this to be a very common use-case. In fact, the only place where
I'm aware that this is needed is for display controllers scanning out a
splash screen. So the worst that could happen here is if they somehow
got the addresses mixed up and read each others' framebuffers, which
would really only be possible if they were already doing so before the
SMMU was initialized. Any harm from that would already be done.

I don't think there's a real risk here. Before the ARM SMMU driver takes
over and configures all contexts as fault by default all of these
devices are reading from physical memory without any isolation. Setting
up this identity domain will allow them to keep accessing the regions
that they were meant to access, while still faulting when access happens
outside.

> Either way, I'll give you the pre-emptive warning that this is the SMMU in
> the way of my EFI framebuffer ;)
> 
> ...
> arm-smmu 7fb20000.iommu: 	1 context banks (1 stage-2 only)
> ...

Interesting. How did you avoid getting the faults by default? Do you
just enable bypass by default?

If I understand correctly, this would mean that you can have only a
single IOMMU domain in that case, right? In that case it would perhaps
be better to keep a list of identity IOMMU domains and later on somehow
pass them on when the driver takes over. Basically these would have to
become the IOMMU groups' default domains.

> Robin.
> 
> [1] the fact that it currently depends on probe order whether getting that
> .add_device call requires a driver probing for the device is an error as
> discussed elsewhere, and will get fixed separately, I promise.

I'm not sure I understand how that would fix anything. You'd still need
to program the SMMU first before calling the ->add_device() for all the
masters, in which case you're still going to run into faults.

Thierry

[0]: https://lkml.org/lkml/2019/9/17/745