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 <m...@redhat.com>
> > Cc: Richard Henderson <r...@twiddle.net>
> >
> > Signed-off-by: Eduard - Gabriel Munteanu <eduard.munte...@linux360.ro>
> > Signed-off-by: David Gibson <da...@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
