Re: [Qemu-devel] [0/12] Preliminary work for IOMMU emulation support (v2)

[David Gibson]

On Sun, Oct 30, 2011 at 07:52:54PM +0200, Michael S. Tsirkin wrote:
> On Sun, Oct 30, 2011 at 06:20:46PM +0100, Alexander Graf wrote:
> > 
> > On 14.10.2011, at 11:20, David Gibson wrote:
> > 
> > > A while back, Eduard - Gabriel Munteanu send a series of patches
> > > implementing support for emulating the AMD IOMMU in conjunction with
> > > qemu emulated PCI devices.  A revised patch series added support for
> > > the Intel IOMMU, and I also send a revised version of this series
> > > which added support for the hypervisor mediated IOMMU on the pseries
> > > machine.
> > > 
> > > Richard Henderson also weighed in on the discussion, and there's still
> > > a cretain amount to be thrashed out in terms of exactly how to set up
> > > an IOMMU / DMA translation subsystem.
> > > 
> > > However, really only 2 or 3 patches in any of these series have
> > > contained anything interesting.  The rest of the series has been
> > > converting existing PCI emulated devices to use the new DMA interface
> > > which worked through the IOMMU translation, whatever it was.  While we
> > > keep working out what we want for the guts of the IOMMU support, these
> > > device conversion patches keep bitrotting against updates to the
> > > various device implementations themselves.
> > > 
> > > Really, regardless of whether we're actually implementing IOMMU
> > > translation, it makes sense that qemu code should distinguish between
> > > when it is really operating in CPU physical addresses and when it is
> > > operating in bus or DMA addresses which might have some kind of
> > > translation into physical addresses.
> > > 
> > > This series, therefore, begins the conversion of existing PCI device
> > > emulation code to use new (stub) pci dma access functions.  These are,
> > > for now, just defined to be untranslated cpu physical memory accesses,
> > > as before, but has three advantages:
> > > 
> > >   * It becomes obvious where the code is working with dma addresses,
> > >     so it's easier to grep for what might be affected by an IOMMU or
> > >     other bus address translation.
> > > 
> > >   * The new stubs take the PCIDevice *, from which any of the various
> > >     suggested IOMMU interfaces should be able to locate the correct
> > >     IOMMU translation context.
> > > 
> > >   * The new pci_dma_{read,write}() functions have a return value.
> > >     When we do have IOMMU support, translation failures could lead to
> > >     these functions failing, so we want a way to report it.
> > > 
> > > This series converts all the easy cases.  It doesn't yet handle
> > > devices which have both PCI and non-PCI variants, such as AHCI, OHCI
> > > and ne2k.  Unlike the earlier version of this series, functions using
> > > scatter gather _are_ covered, though.
> > 
> > Michael, ping?
> 
> It's unfortunate that v2 of the patchset doesn't
> include a changelog, which makes one reread all
> of the patchset before responding.

Ah, sorry, it slipped my mind.  I'll make sure to do a changelog for
the next iteration.

> I still think just making stX_phys/ldX_phys do memcpy is wrong:
> the former guaranteed atomic access, the later doesn't
> give any access order guarantees.
> 
> We do need atomic versions IMO.

Yeah, I think you're right.  I've changed my next version to call
stX_phys/ldX_phys instead of going to the dma_rw function.

> If you think we need the non-ordered unaligned version
> of stl_XXX as well, the atomic versions could get an
> _atomic suffix, or the non-atomic versions _nonatomic suffix.
> 
> I think I showed an example in e1000 which doesn't currently
> use stX_phys/ldX_phys but really should - otherwise
> the valid bit might get flipped before data is written
> (or it could do two cpu_physical_memory_XXX to ensure
> ordering but that's rather inconvenient IMO).
> 
> intel-hda does something similar, I don't know whether ordering
> matters there, but this patch sneaks in a semantic change into
> what should have been a harmless rework.
> 
> Also, could the stub functions be made inline? That'd make
> the intent clearer IMO.

So, I would have preferred to use inlines from the beginning, but I
ran into problems because I needed some of the poisoned functions that
couldn't be used in files included anywhere in libhw.  However, since
then other changes have meant those poisoned functions are no longer
needed even for inlines, but I hadn't realised that until you pointed
it out.  Next version has them as inlines.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson