Re: [Qemu-devel] [PATCH 08/13] iommu: Introduce IOMMU emulation infrastructure

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2012-05-14 at 21:50 -0500, Anthony Liguori wrote:
> On 05/14/2012 09:32 PM, Benjamin Herrenschmidt wrote:
> > On Mon, 2012-05-14 at 21:03 -0500, Anthony Liguori wrote:
> >> So the CPU thread runs in lock-step with the I/O thread.  Dropping the CPU
> >> thread lock to let the I/O thread run is a dangerous thing to do in a place like
> >> this.
> >>
> >> Also, I think you'd effectively block the CPU until pending DMA operations
> >> complete?  This could be many, many, milliseconds, no?  That's going to make
> >> guests very upset.
> >
> > Do you see any other option ?
> 
> Yes, ignore it.
> 
> I have a hard time believing software depends on changing DMA translation 
> mid-way through a transaction.

It's a correctness issue. It won't happen in normal circumstances but it
can, and thus should be handled gracefully.

Cases where that matter are unloading of a (broken) driver, kexec/kdump
from one guest to another etc... all involve potentially clearing all
iommu tables while a driver might have left a device DMA'ing. The
expectation is that the device will get target aborts from the iommu
until the situation gets "cleaned up" in SW.

> > IE. When the guest invalidates iommu translations, it must have a way to
> > synchronize with anything that may have used such translations. IE. It
> > must have a way to guarantee that
> >
> >   - The translation is no longer used
> 
> You can certainly prevent future uses of the translation.  We're only talking 
> about pending mapped requests.  My assertion is that they don't matter.
> 
> >   - Any physical address obtained as a result of looking at
> >     the translation table/tree/... is no longer used
> 
> Why does this need to be guaranteed?  How can software depend on this in a 
> meaningful way?

The same as TLB invalidations :-)

In real HW, this is a property of the HW itself, ie, whatever MMIO is
used to invalidate the HW TLB provides a way to ensure (usually by
reading back) that any request pending in the iommu pipeline has either
been completed or canned.

When we start having page fault capable iommu's this will be even more
important as faults will be be part of the non-error case.

For now, it's strictly error handling but it still should be done
correctly.

> > This is true regardless of the iommu model. For the PAPR TCE model, we
> > need to provide such synchronization whenever we clear a TCE entry, but
> > if I was to emulate the HW iommu of a G5 for example, I would have to
> > provide similar guarantees in my emulation of MMIO accesses to the iommu
> > TLB invalidate register.
> >
> > This is a problem with devices using the map/unmap calls. This is going
> > to be even more of a problem as we start being more threaded (which from
> > discussions I've read here or there seem to be the goal) since
> > synchronizing with the IO thread isn't going to be enough.
> >
> > As long as the actual data transfer through the iommu is under control
> > of the iommu code, then the iommu implementation can use whatever
> > locking it needs to ensure this synchronization.
> >
> > But map/unmap defeats that.
> >
> > David's approach may not be the best long term, but provided it's not
> > totally broken (I don't know qemu locking well enough to judge how
> > dangerous it is) then it might be a "good enough" first step until we
> > come up with something better ?
> 
> No, it's definitely not good enough.  Dropping the global mutex in random places 
> is asking for worlds of hurt.
> 
> If this is really important, then we need some sort of cancellation API to go 
> along with map/unmap although I doubt that's really possible.
> 
> MMIO/PIO operations cannot block.

Well, there's a truckload of cases in real HW where an MMIO/PIO read is
used to synchronize some sort of HW operation.... I suppose nothing that
involves blocking at this stage in qemu but I would be careful with your
expectations here... writes are usually pipelined but blocking on a read
response does make a lot of sense.

In any case, for the problem at hand, I can just drop the wait for now
and maybe just print a warning if I see an existing map.

We still need some kind of either locking or barrier to simply ensure
that the updates to the TCE table are visible to other processors but
that can be done in the backend.

But I wouldn't just forget about the issue, it's going to come back and
bite...

Cheers,
Ben.

> Regards,
> 
> Anthony Liguori
> 
> >
> > The normal case will be that no map exist, ie, it will almost always be
> > a guest programming error to remove an iommu mapping while a device is
> > actively using it, so having this case be slow is probably a non-issue.
> >
> > Cheers,
> > Ben.
> >
> >