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

[David Gibson <david@gibson.dropbear.id.au>]

On Fri, Mar 09, 2012 at 11:23:01AM +0100, Paolo Bonzini wrote:
> Il 09/03/2012 06:01, David Gibson ha scritto:
> > This patch adds the basic infrastructure necessary to emulate an IOMMU
> > visible to the guest.  The DMAContext structure is extended with
> > information and a callback describing the translation, and the various
> > DMA functions used by devices will now perform IOMMU translation using
> > this callback.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > 
> > Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  configure     |   12 ++++
> >  dma-helpers.c |  191 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  dma.h         |  125 +++++++++++++++++++++++++++++++-------
> >  3 files changed, 306 insertions(+), 22 deletions(-)
> > 
> > diff --git a/configure b/configure
> > index a5eb832..e6fba2f 100755
> > --- a/configure
> > +++ b/configure
> > @@ -138,6 +138,7 @@ linux_aio=""
> >  cap_ng=""
> >  attr=""
> >  libattr=""
> > +iommu="yes"
> >  xfs=""
> >  
> >  vhost_net="no"
> > @@ -784,6 +785,10 @@ for opt do
> >    ;;
> >    --enable-vhost-net) vhost_net="yes"
> >    ;;
> > +  --enable-iommu) iommu="yes"
> > +  ;;
> > +  --disable-iommu) iommu="no"
> > +  ;;
> 
> This need not be a configure option.  Just enable it, or it will
> bitrot.

I did wonder about that.  I'd like to hear that suggested by more than
one person before I unconditionally add code which will impose an
overhead on all emulated DMAs.

[snip]
> > +
> > +#ifdef CONFIG_IOMMU
> > +bool __dma_memory_valid(DMAContext *dma, dma_addr_t addr, dma_addr_t len,
> > +                        DMADirection dir)
> 
> No __ identifiers, please.  You could call these
> dma_context_memory_{valid,read,write}.

Ok, I'll think of different names.

[snip]
> > diff --git a/dma.h b/dma.h
> > index a66e3d7..ec06163 100644
> > --- a/dma.h
> > +++ b/dma.h
> > @@ -15,6 +15,7 @@
> >  #include "hw/hw.h"
> >  #include "block.h"
> >  
> > +typedef struct DMAContext DMAContext;
> >  typedef struct ScatterGatherEntry ScatterGatherEntry;
> >  
> >  typedef enum {
> > @@ -31,30 +32,82 @@ struct QEMUSGList {
> >  };
> >  
> >  #if defined(TARGET_PHYS_ADDR_BITS)
> > +
> > +#ifdef CONFIG_IOMMU
> 
> ... which i making dma_addr_t always 64-bit.  This way you can even
> remove the new qdev-dma.c file.  Just make DEFINE_PROP_DMAADDR an alias
> for DEFINE_PROP_HEX64.

Ah, true, I could #define it to DEFINE_PROP_TADDR before this change,
too.  Ok will, revise accordingly.

[snip]
> > +#ifdef CONFIG_IOMMU
> > +
> > +void dma_context_init(DMAContext *dma, DMATranslateFunc fn);
> > +void dma_invalidate_memory_range(DMAContext *dma,
> > +                                 dma_addr_t addr, dma_addr_t len);
> 
> Should dma_invalidate_memory_range be changed to a no-op if dma == NULL?
>  No idea, just asking.

Not really.  Since dma_invalidate_memory_range() would be called from
the IOMMU implementation when translations are removed, it should
never be called with dma == NULL (if there's IOMMU code to do the
calling, then there must be a non-NULL DMAContext to describe that IOMMU).

-- 
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