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

[Michael S. Tsirkin]

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.

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.

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.

> 
> Alex