Re: [Qemu-devel] [PATCH] Better support for dma_addr_t variables

[David Gibson <dwg@au1.ibm.com>]

On Fri, Mar 30, 2012 at 11:32:45AM +0200, Andreas Färber wrote:
> Am 27.03.2012 04:43, schrieb David Gibson:
> > A while back, we introduced the dma_addr_t type, which is supposed to
> > be used for bus visible memory addresses.  At present, this is an
> > alias for target_phys_addr_t, but this will change when we eventually
> > add support for guest visible IOMMUs.
> > 
> > There are some instances of target_phys_addr_t in the code now which
> > should really be dma_addr_t, but can't be trivially converted due to
> > missing features which this patch corrects.
> > 
> >  * We add DMA_ADDR_BITS analagous to TARGET_PHYS_ADDR_BITS.  This is
> >    important where we need to make a compile-time (#if) based on the
> >    size of dma_addr_t.
> > 
> >  * We add a new helper macro to create device properties which take a
> >    dma_addr_t, currently an alias to DEFINE_PROP_TADDR().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  dma.h         |    1 +
> >  hw/qdev-dma.h |    4 ++++
> >  2 files changed, 5 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/qdev-dma.h
> > 
> > diff --git a/dma.h b/dma.h
> > index 05ac325..463095c 100644
> > --- a/dma.h
> > +++ b/dma.h
> > @@ -32,6 +32,7 @@ struct QEMUSGList {
> >  #if defined(TARGET_PHYS_ADDR_BITS)
> >  typedef target_phys_addr_t dma_addr_t;
> >  
> > +#define DMA_ADDR_BITS TARGET_PHYS_ADDR_BITS
> >  #define DMA_ADDR_FMT TARGET_FMT_plx
> >  
> >  struct ScatterGatherEntry {
> > diff --git a/hw/qdev-dma.h b/hw/qdev-dma.h
> > new file mode 100644
> > index 0000000..e407771
> > --- /dev/null
> > +++ b/hw/qdev-dma.h
> > @@ -0,0 +1,4 @@
> > +#include "qdev-addr.h"
> > +
> > +#define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)                               \
> > +    DEFINE_PROP_TADDR(_n, _s, _f, _d)
> 
> Is a new header just for this really needed? It's not being used in this
> patch, so its necessity is hard to judge. ;)
> 
> I would've expected DEFINE_PROP_... in qdev.h along the others.

Well, it's dependent on DEFINE_PROP_TADDR() which is in qdev-addr.h
and not in qdev.h, as well as on dma_addr_t which is in dma.h.  It
will generally be replacing DEFINE_PROP_TADDR too, so making it a
separate header seemed the most straightforward approach.

If someone has a definitive decision that a different way is better, I
can update for that.  But I really hope we can avoid bikeshedding on
this for a month before merging.

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