[Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Peter Maydell]

There is currently only one user of the qdev taddr (target addr,
now hwaddr) property code, and that user is wrong anyway, since the
property it's trying to set isn't actually a taddr property.
Fix the erroneous use in sm501 and drop all the taddr property code.

The justification for dropping the support is that a device should
generally not be exposing properties whose width (conceptually)
depends on the target CPU type. (This is the same rationale for
not supporting hwaddr fields in migration state.) Instead the
device should expose either a 32 bit or 64 bit property, depending
on what the actual hardware does.

Aside: I may try to get rid of the DMAADDR property too at some
point, because what the sysbus-ohci device should actually be doing
is taking a MemoryRegion* [or maybe a DMAContext*] representing what
it should be DMAing into, rather than the current "DMA into the
system address space at addr + some constant offset" hack. One
thing at a time, though.

Peter Maydell (2):
  hw/sm501: Use correct setter for sysbus-ohci dma-address property
  qdev: Drop taddr properties

 hw/Makefile.objs        |  1 -
 hw/fdc.c                |  1 -
 hw/milkymist-hw.h       |  1 -
 hw/milkymist-minimac2.c |  1 -
 hw/qdev-addr.c          | 78 -------------------------------------------------
 hw/qdev-addr.h          | 10 -------
 hw/sm501.c              |  3 +-
 hw/sparc/sun4m.c        |  1 -
 hw/tcx.c                |  1 -
 hw/xilinx_axidma.c      |  1 -
 10 files changed, 1 insertion(+), 97 deletions(-)
 delete mode 100644 hw/qdev-addr.c
 delete mode 100644 hw/qdev-addr.h

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/2] hw/sm501: Use correct setter for sysbus-ohci dma-address property

[Peter Maydell]

The sysbus-ohci dma-address property is declared as a HEX64
property, not a TADDR, so use the correct setter for it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sm501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sm501.c b/hw/sm501.c
index 93a06c9..0ef6b8a 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -1430,7 +1430,7 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
     /* bridge to usb host emulation module */
     dev = qdev_create(NULL, "sysbus-ohci");
     qdev_prop_set_uint32(dev, "num-ports", 2);
-    qdev_prop_set_taddr(dev, "dma-offset", base);
+    qdev_prop_set_uint64(dev, "dma-offset", base);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
                     base + MMIO_BASE_OFFSET + SM501_USB_HOST);
-- 
1.7.11.4

[Qemu-devel] [PATCH 2/2] qdev: Drop taddr properties

[Peter Maydell]

Drop all the infrastructure for taddr properties (ie ones which
are 'hwaddr' sized). These are now unused, and any further desired
use would be rather questionable since device properties shouldn't
generally depend on a type that is conceptually variable based on
the target CPU. 32 or 64 bit integer properties should be used instead
as appropriate for the specific device.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/Makefile.objs        |  1 -
 hw/fdc.c                |  1 -
 hw/milkymist-hw.h       |  1 -
 hw/milkymist-minimac2.c |  1 -
 hw/qdev-addr.c          | 78 -------------------------------------------------
 hw/qdev-addr.h          | 10 -------
 hw/sm501.c              |  1 -
 hw/sparc/sun4m.c        |  1 -
 hw/tcx.c                |  1 -
 hw/xilinx_axidma.c      |  1 -
 10 files changed, 96 deletions(-)
 delete mode 100644 hw/qdev-addr.c
 delete mode 100644 hw/qdev-addr.h

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index d0b2ecb..bee5736 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -134,7 +134,6 @@ common-obj-$(CONFIG_ESP) += esp.o
 common-obj-$(CONFIG_ESP_PCI) += esp-pci.o
 
 common-obj-y += sysbus.o isa-bus.o
-common-obj-y += qdev-addr.o
 
 # VGA
 common-obj-$(CONFIG_VGA_PCI) += vga-pci.o
diff --git a/hw/fdc.c b/hw/fdc.c
index a4bb129..3220538 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -33,7 +33,6 @@
 #include "qemu/timer.h"
 #include "hw/isa.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-addr.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
 #include "qemu/log.h"
diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index 4e86c4e..5317ce6 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -2,7 +2,6 @@
 #define QEMU_HW_MILKYMIST_H
 
 #include "hw/qdev.h"
-#include "hw/qdev-addr.h"
 #include "net/net.h"
 
 static inline DeviceState *milkymist_uart_create(hwaddr base,
diff --git a/hw/milkymist-minimac2.c b/hw/milkymist-minimac2.c
index 29618e8..4ef6318 100644
--- a/hw/milkymist-minimac2.c
+++ b/hw/milkymist-minimac2.c
@@ -27,7 +27,6 @@
 #include "trace.h"
 #include "net/net.h"
 #include "qemu/error-report.h"
-#include "hw/qdev-addr.h"
 
 #include <zlib.h>
 
diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
deleted file mode 100644
index 80a38bb..0000000
--- a/hw/qdev-addr.c
+++ /dev/null
@@ -1,78 +0,0 @@
-#include "hw/qdev.h"
-#include "hw/qdev-addr.h"
-#include "exec/hwaddr.h"
-#include "qapi/qmp/qerror.h"
-#include "qapi/visitor.h"
-
-/* --- target physical address --- */
-
-static int parse_taddr(DeviceState *dev, Property *prop, const char *str)
-{
-    hwaddr *ptr = qdev_get_prop_ptr(dev, prop);
-
-    *ptr = strtoull(str, NULL, 16);
-    return 0;
-}
-
-static int print_taddr(DeviceState *dev, Property *prop, char *dest, size_t len)
-{
-    hwaddr *ptr = qdev_get_prop_ptr(dev, prop);
-    return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
-}
-
-static void get_taddr(Object *obj, Visitor *v, void *opaque,
-                      const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    hwaddr *ptr = qdev_get_prop_ptr(dev, prop);
-    int64_t value;
-
-    value = *ptr;
-    visit_type_int64(v, &value, name, errp);
-}
-
-static void set_taddr(Object *obj, Visitor *v, void *opaque,
-                      const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    hwaddr *ptr = qdev_get_prop_ptr(dev, prop);
-    Error *local_err = NULL;
-    int64_t value;
-
-    if (dev->realized) {
-        qdev_prop_set_after_realize(dev, name, errp);
-        return;
-    }
-
-    visit_type_int64(v, &value, name, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    if ((uint64_t)value <= (uint64_t) ~(hwaddr)0) {
-        *ptr = value;
-    } else {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-                  dev->id?:"", name, value, (uint64_t) 0,
-                  (uint64_t) ~(hwaddr)0);
-    }
-}
-
-
-PropertyInfo qdev_prop_taddr = {
-    .name  = "taddr",
-    .parse = parse_taddr,
-    .print = print_taddr,
-    .get   = get_taddr,
-    .set   = set_taddr,
-};
-
-void qdev_prop_set_taddr(DeviceState *dev, const char *name, hwaddr value)
-{
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert(!errp);
-
-}
diff --git a/hw/qdev-addr.h b/hw/qdev-addr.h
deleted file mode 100644
index 79708e6..0000000
--- a/hw/qdev-addr.h
+++ /dev/null
@@ -1,10 +0,0 @@
-#ifndef HW_QDEV_ADDR_H
-#define HW_QDEV_ADDR_H 1
-
-#define DEFINE_PROP_TADDR(_n, _s, _f, _d)                               \
-    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_taddr, hwaddr)
-
-extern PropertyInfo qdev_prop_taddr;
-void qdev_prop_set_taddr(DeviceState *dev, const char *name, hwaddr value);
-
-#endif
diff --git a/hw/sm501.c b/hw/sm501.c
index 0ef6b8a..5f89fb2 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -28,7 +28,6 @@
 #include "ui/console.h"
 #include "hw/devices.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-addr.h"
 #include "qemu/range.h"
 #include "ui/pixel_ops.h"
 
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 9ebda02..fc282e2 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -37,7 +37,6 @@
 #include "hw/fw_cfg.h"
 #include "hw/escc.h"
 #include "hw/empty_slot.h"
-#include "hw/qdev-addr.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "sysemu/blockdev.h"
diff --git a/hw/tcx.c b/hw/tcx.c
index c44068e..55abaee 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -26,7 +26,6 @@
 #include "ui/console.h"
 #include "ui/pixel_ops.h"
 #include "hw/sysbus.h"
-#include "hw/qdev-addr.h"
 
 #define MAXX 1024
 #define MAXY 768
diff --git a/hw/xilinx_axidma.c b/hw/xilinx_axidma.c
index 8db1a74..997b06e 100644
--- a/hw/xilinx_axidma.c
+++ b/hw/xilinx_axidma.c
@@ -26,7 +26,6 @@
 #include "qemu/timer.h"
 #include "hw/ptimer.h"
 #include "qemu/log.h"
-#include "hw/qdev-addr.h"
 
 #include "hw/stream.h"
 
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Paolo Bonzini]

Il 02/04/2013 18:10, Peter Maydell ha scritto:
> There is currently only one user of the qdev taddr (target addr,
> now hwaddr) property code, and that user is wrong anyway, since the
> property it's trying to set isn't actually a taddr property.
> Fix the erroneous use in sm501 and drop all the taddr property code.
> 
> The justification for dropping the support is that a device should
> generally not be exposing properties whose width (conceptually)
> depends on the target CPU type. (This is the same rationale for
> not supporting hwaddr fields in migration state.) Instead the
> device should expose either a 32 bit or 64 bit property, depending
> on what the actual hardware does.
> 
> Aside: I may try to get rid of the DMAADDR property too at some
> point, because what the sysbus-ohci device should actually be doing
> is taking a MemoryRegion* [or maybe a DMAContext*]

Avi had patches to unify DMAContext and MemoryRegion.  I should revive
them perhaps.

> representing what
> it should be DMAing into, rather than the current "DMA into the
> system address space at addr + some constant offset" hack. One
> thing at a time, though.

That's a general problem with sysbus.  I guess if you need another
address space you should define your own bus, like PCI does.

Paolo

> Peter Maydell (2):
>   hw/sm501: Use correct setter for sysbus-ohci dma-address property
>   qdev: Drop taddr properties
> 
>  hw/Makefile.objs        |  1 -
>  hw/fdc.c                |  1 -
>  hw/milkymist-hw.h       |  1 -
>  hw/milkymist-minimac2.c |  1 -
>  hw/qdev-addr.c          | 78 -------------------------------------------------
>  hw/qdev-addr.h          | 10 -------
>  hw/sm501.c              |  3 +-
>  hw/sparc/sun4m.c        |  1 -
>  hw/tcx.c                |  1 -
>  hw/xilinx_axidma.c      |  1 -
>  10 files changed, 1 insertion(+), 97 deletions(-)
>  delete mode 100644 hw/qdev-addr.c
>  delete mode 100644 hw/qdev-addr.h
> 

Re: [Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Peter Maydell]

On 2 April 2013 17:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/04/2013 18:10, Peter Maydell ha scritto:
>> Aside: I may try to get rid of the DMAADDR property too at some
>> point, because what the sysbus-ohci device should actually be doing
>> is taking a MemoryRegion* [or maybe a DMAContext*]
>
> Avi had patches to unify DMAContext and MemoryRegion.  I should revive
> them perhaps.

Yes please.

>> representing what
>> it should be DMAing into, rather than the current "DMA into the
>> system address space at addr + some constant offset" hack. One
>> thing at a time, though.
>
> That's a general problem with sysbus.  I guess if you need another
> address space you should define your own bus, like PCI does.

It's not a problem with sysbus, it's a problem with people
being lazy about implementing things that do DMA. For instance
PCI doesn't take a MemoryRegion* for DMA, it just assumes it
can DMA into the system address space. (it does let you pass
a DMAContext, but maybe that goes away with the patches you
mention above; it's only used for spapr.)

NB that "the PCI address space", ie the view of the world that
the host controller sees when it makes a PCI access, is not
the same as the address space that another bus mastering PCI
device sees when it does a DMA access. Chances are good that
the former shouldn't have any of the system's RAM in it and
the latter should. (This is easy to handle though, you just
create container MemoryRegions for each and pass them to the
PCI code and make sure that you're using the right one at
any particular point.)

-- PMM

Re: [Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Paolo Bonzini]

Il 02/04/2013 19:07, Peter Maydell ha scritto:
> On 2 April 2013 17:42, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 02/04/2013 18:10, Peter Maydell ha scritto:
>>> Aside: I may try to get rid of the DMAADDR property too at some
>>> point, because what the sysbus-ohci device should actually be doing
>>> is taking a MemoryRegion* [or maybe a DMAContext*]
>>
>> Avi had patches to unify DMAContext and MemoryRegion.  I should revive
>> them perhaps.
> 
> Yes please.
> 
>>> representing what
>>> it should be DMAing into, rather than the current "DMA into the
>>> system address space at addr + some constant offset" hack. One
>>> thing at a time, though.
>>
>> That's a general problem with sysbus.  I guess if you need another
>> address space you should define your own bus, like PCI does.
> 
> It's not a problem with sysbus, it's a problem with people
> being lazy about implementing things that do DMA. For instance
> PCI doesn't take a MemoryRegion* for DMA, it just assumes it
> can DMA into the system address space.

PCI does provide its own DMAContext*, which includes an AddressSpace.
This address space ("view of the world") is different for each device.

> (it does let you pass
> a DMAContext, but maybe that goes away with the patches you
> mention above; it's only used for spapr.)

It is extended so that it is also used for spapr with those patches.

Paolo

> NB that "the PCI address space", ie the view of the world that
> the host controller sees when it makes a PCI access, is not
> the same as the address space that another bus mastering PCI
> device sees when it does a DMA access. Chances are good that
> the former shouldn't have any of the system's RAM in it and
> the latter should. (This is easy to handle though, you just
> create container MemoryRegions for each and pass them to the
> PCI code and make sure that you're using the right one at
> any particular point.)
> 
> -- PMM
> 
> 

Re: [Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Peter Maydell]

On 2 April 2013 20:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/04/2013 19:07, Peter Maydell ha scritto:
>> It's not a problem with sysbus, it's a problem with people
>> being lazy about implementing things that do DMA. For instance
>> PCI doesn't take a MemoryRegion* for DMA, it just assumes it
>> can DMA into the system address space.
>
> PCI does provide its own DMAContext*, which includes an AddressSpace.
> This address space ("view of the world") is different for each device.

Yes, but (unless you're using the legacy stuff spapr uses)
it generates it via get_system_memory(), which is always
wrong (though often close enough that Linux will work right).

>> (it does let you pass
>> a DMAContext, but maybe that goes away with the patches you
>> mention above; it's only used for spapr.)
>
> It is extended so that it is also used for spapr with those patches.

I'm confused. Do you mean that it's changed so that spapr passes
a MemoryRegion* rather than a DMAContext*, or so that everything
uses a DMAContext* ? (The comments in pci.c suggest that the former
is the intended approach.)

-- PMM

Re: [Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Peter Maydell" <peter.maydell@linaro.org>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Anthony Liguori" <aliguori@us.ibm.com>, patches@linaro.org, qemu-devel@nongnu.org, "David Gibson"
> <david@gibson.dropbear.id.au>
> Inviato: Martedì, 2 aprile 2013 22:33:48
> Oggetto: Re: [PATCH 0/2] Drop support for qdev taddr properties
> 
> On 2 April 2013 20:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 02/04/2013 19:07, Peter Maydell ha scritto:
> >> It's not a problem with sysbus, it's a problem with people
> >> being lazy about implementing things that do DMA. For instance
> >> PCI doesn't take a MemoryRegion* for DMA, it just assumes it
> >> can DMA into the system address space.
> >
> > PCI does provide its own DMAContext*, which includes an AddressSpace.
> > This address space ("view of the world") is different for each device.
> 
> Yes, but (unless you're using the legacy stuff spapr uses)
> it generates it via get_system_memory(), which is always
> wrong (though often close enough that Linux will work right).

It also respects the bus master enable bit:

        memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus master",
                                 get_system_memory(), 0,
                                 memory_region_size(get_system_memory()));
        memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);

> >> (it does let you pass
> >> a DMAContext, but maybe that goes away with the patches you
> >> mention above; it's only used for spapr.)
> >
> > It is extended so that it is also used for spapr with those patches.
> 
> I'm confused. Do you mean that it's changed so that spapr passes
> a MemoryRegion* rather than a DMAContext*, or so that everything
> uses a DMAContext* ? (The comments in pci.c suggest that the former
> is the intended approach.)

Yes, the former.  More precisely, everything uses an AddressSpace*,
in fact (that was the part that Avi didn't write) dma-helpers.c can
use AddressSpace* instead of DMAContext*.

spapr right now needs a special translate pointer embedded in the
DMAContext.  With the iommu MemoryRegion patches, it can put the
translate function pointer into a translation MemoryRegion.  Then
the AddressSpace points to that MemoryRegion.

Paolo

Re: [Qemu-devel] [PATCH 0/2] Drop support for qdev taddr properties

[Peter Maydell]

On 2 April 2013 21:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> "Peter Maydell" <peter.maydell@linaro.org> wrote:
>> I'm confused. Do you mean that it's changed so that spapr passes
>> a MemoryRegion* rather than a DMAContext*, or so that everything
>> uses a DMAContext* ? (The comments in pci.c suggest that the former
>> is the intended approach.)
>
> Yes, the former.  More precisely, everything uses an AddressSpace*,
> in fact (that was the part that Avi didn't write) dma-helpers.c can
> use AddressSpace* instead of DMAContext*.
>
> spapr right now needs a special translate pointer embedded in the
> DMAContext.  With the iommu MemoryRegion patches, it can put the
> translate function pointer into a translation MemoryRegion.  Then
> the AddressSpace points to that MemoryRegion.

Ah, right, good. It would certainly be good to see those patches;
there's a potential followup to the versatile_pci patches that
implements the device-to-system access windows properly which
should probably be based on top of them.

-- PMM