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

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

[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)
-- 
1.7.9.1

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

[David Gibson]

Uh.. please apply?  Anyone?

On Tue, Mar 27, 2012 at 01:43:21PM +1100, David Gibson wrote:
> 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)

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

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

[Andreas Färber]

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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Andreas Färber]

Am 30.03.2012 11:32, schrieb Andreas Färber:
> 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. ;)

Additionally it's missing a license notice.

> I would've expected DEFINE_PROP_... in qdev.h along the others.
> 
> Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[David Gibson]

On Fri, Mar 30, 2012 at 11:34:25AM +0200, Andreas Färber wrote:
> Am 30.03.2012 11:32, schrieb Andreas Färber:
> > 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. ;)
> 
> Additionally it's missing a license notice.

Just like qdev-addr.h.  And qdev.h for that matter.

You seriously want a license notice for two lines of trivial macro?

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

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

[Andreas Färber]

Am 31.03.2012 10:50, schrieb David Gibson:
> On Fri, Mar 30, 2012 at 11:34:25AM +0200, Andreas Färber wrote:
>> Am 30.03.2012 11:32, schrieb Andreas Färber:
>>> Am 27.03.2012 04:43, schrieb David Gibson:
>>>> 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. ;)
>>
>> Additionally it's missing a license notice.
> 
> Just like qdev-addr.h.  And qdev.h for that matter.
> 
> You seriously want a license notice for two lines of trivial macro?

Yes, the issue here is under what license the file is. It's a new file,
so in lack of a license statement is it under GPLv2 because QEMU as a
whole currently is?  Thus a header explicitly saying that it's under
GPLv2+ (or BSD or MIT/X11 or ...) would be appreciated to avoid further
complications. Compare our GPLv2+ relicensing page:

http://wiki.qemu.org/Relicensing

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[David Gibson]

On Mon, Apr 02, 2012 at 09:49:12AM +0200, Andreas Färber wrote:
> Am 31.03.2012 10:50, schrieb David Gibson:
> > On Fri, Mar 30, 2012 at 11:34:25AM +0200, Andreas Färber wrote:
> >> Am 30.03.2012 11:32, schrieb Andreas Färber:
> >>> Am 27.03.2012 04:43, schrieb David Gibson:
> >>>> 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. ;)
> >>
> >> Additionally it's missing a license notice.
> > 
> > Just like qdev-addr.h.  And qdev.h for that matter.
> > 
> > You seriously want a license notice for two lines of trivial macro?
> 
> Yes, the issue here is under what license the file is. It's a new file,
> so in lack of a license statement is it under GPLv2 because QEMU as a
> whole currently is?  Thus a header explicitly saying that it's under
> GPLv2+ (or BSD or MIT/X11 or ...) would be appreciated to avoid further
> complications. Compare our GPLv2+ relicensing page:
> 
> http://wiki.qemu.org/Relicensing

It's 4 trivial lines.  Well under the copyrightability threshold even
by the paranoid estimates of IBM Legal.

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

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

[Peter Maydell]

On 3 April 2012 01:51, David Gibson <dwg@au1.ibm.com> wrote:
> On Mon, Apr 02, 2012 at 09:49:12AM +0200, Andreas Färber wrote:
>> Yes, the issue here is under what license the file is. It's a new file,
>> so in lack of a license statement is it under GPLv2 because QEMU as a
>> whole currently is?  Thus a header explicitly saying that it's under
>> GPLv2+ (or BSD or MIT/X11 or ...) would be appreciated to avoid further
>> complications. Compare our GPLv2+ relicensing page:
>>
>> http://wiki.qemu.org/Relicensing
>
> It's 4 trivial lines.  Well under the copyrightability threshold even
> by the paranoid estimates of IBM Legal.

(a) other legal departments may be more paranoid still
(b) how about when somebody else adds more code to the header later
(c) if a file has a clear license statement then it's immediately
obvious what the situation is. Why be ambigious when you can be clear?

-- PMM

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

[Andreas Färber]

Am 03.04.2012 02:51, schrieb David Gibson:
> On Mon, Apr 02, 2012 at 09:49:12AM +0200, Andreas Färber wrote:
>> Am 31.03.2012 10:50, schrieb David Gibson:
>>> On Fri, Mar 30, 2012 at 11:34:25AM +0200, Andreas Färber wrote:
>>>> Am 30.03.2012 11:32, schrieb Andreas Färber:
>>>>> Am 27.03.2012 04:43, schrieb David Gibson:
>>>>>> 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. ;)
>>>>
>>>> Additionally it's missing a license notice.
>>>
>>> Just like qdev-addr.h.  And qdev.h for that matter.
>>>
>>> You seriously want a license notice for two lines of trivial macro?
>>
>> Yes, the issue here is under what license the file is. It's a new file,
>> so in lack of a license statement is it under GPLv2 because QEMU as a
>> whole currently is?  Thus a header explicitly saying that it's under
>> GPLv2+ (or BSD or MIT/X11 or ...) would be appreciated to avoid further
>> complications. Compare our GPLv2+ relicensing page:
>>
>> http://wiki.qemu.org/Relicensing
> 
> It's 4 trivial lines.  Well under the copyrightability threshold even
> by the paranoid estimates of IBM Legal.

Tell that to your IBM colleague. We had an argument about 1 line of
trivial code replacement (not even new code) that kept us from
relicensing target-unicore32/helper.c just recently.
This is not me who's being paranoid about lines of code, really. I've
even heard that header files may not even be the subject of licenses at
all in some legislations. I'm just picky and sometimes spot the odd sock
from afar. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[David Gibson]

On Tue, Apr 03, 2012 at 10:53:10AM +0200, Andreas Färber wrote:
> Am 03.04.2012 02:51, schrieb David Gibson:
> > On Mon, Apr 02, 2012 at 09:49:12AM +0200, Andreas Färber wrote:
> >> Am 31.03.2012 10:50, schrieb David Gibson:
> >>> On Fri, Mar 30, 2012 at 11:34:25AM +0200, Andreas Färber wrote:
> >>>> Am 30.03.2012 11:32, schrieb Andreas Färber:
> >>>>> Am 27.03.2012 04:43, schrieb David Gibson:
> >>>>>> 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. ;)
> >>>>
> >>>> Additionally it's missing a license notice.
> >>>
> >>> Just like qdev-addr.h.  And qdev.h for that matter.
> >>>
> >>> You seriously want a license notice for two lines of trivial macro?
> >>
> >> Yes, the issue here is under what license the file is. It's a new file,
> >> so in lack of a license statement is it under GPLv2 because QEMU as a
> >> whole currently is?  Thus a header explicitly saying that it's under
> >> GPLv2+ (or BSD or MIT/X11 or ...) would be appreciated to avoid further
> >> complications. Compare our GPLv2+ relicensing page:
> >>
> >> http://wiki.qemu.org/Relicensing
> > 
> > It's 4 trivial lines.  Well under the copyrightability threshold even
> > by the paranoid estimates of IBM Legal.
> 
> Tell that to your IBM colleague. We had an argument about 1 line of
> trivial code replacement (not even new code) that kept us from
> relicensing target-unicore32/helper.c just recently.
> This is not me who's being paranoid about lines of code, really. I've
> even heard that header files may not even be the subject of licenses at
> all in some legislations. I'm just picky and sometimes spot the odd sock
> from afar. ;)

Sigh.  Okay.

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

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

[David Gibson]

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

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

[Michael S. Tsirkin]

On Tue, Mar 27, 2012 at 01:43:21PM +1100, David Gibson wrote:
> 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>

I don't have any issues with this but let's apply together
with the patch converting the devices that you mention.

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

Headers need preprocessor guards so that multiple
inclusions don't trigger errors.
It does not matter much but did you consider sticking this
in dma.h instead of adding a new header?

> -- 
> 1.7.9.1

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

[David Gibson]

On Sun, Apr 01, 2012 at 12:17:28PM +0300, Michael S. Tsirkin wrote:
> On Tue, Mar 27, 2012 at 01:43:21PM +1100, David Gibson wrote:
> > 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>
> 
> I don't have any issues with this but let's apply together
> with the patch converting the devices that you mention.

Ok, I'll resend as a two patch series with the xhci conversion, which
is the main one.

> > 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)
> 
> Headers need preprocessor guards so that multiple
> inclusions don't trigger errors.

Again, a flaw copied from qdev-addr.h

> It does not matter much but did you consider sticking this
> in dma.h instead of adding a new header?

I'm pretty sure I tried that and ran into some sort of hairy circular
dependency problem which this seemed amongst the easier ways to work
around.

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