Patchwork [02/13] Better support for dma_addr_t variables

login
register
mail settings
Submitter David Gibson
Date March 9, 2012, 5:01 a.m.
Message ID <1331269308-22372-3-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/145683/
State New
Headers show

Comments

David Gibson - March 9, 2012, 5:01 a.m.
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.  This lets us correctly convert code which cuurrently
   has DEFINE_PROP_TADDR() for variables which should be dma_addr_t
   instead of target_phys_addr_t.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile.objs |    2 +-
 dma.h         |    1 +
 hw/qdev-dma.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev-dma.h |    8 ++++++
 4 files changed, 87 insertions(+), 1 deletions(-)
 create mode 100644 hw/qdev-dma.c
 create mode 100644 hw/qdev-dma.h
Paolo Bonzini - March 9, 2012, 10 a.m.
Il 09/03/2012 06:01, David Gibson ha scritto:
> 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.  This lets us correctly convert code which cuurrently
>    has DEFINE_PROP_TADDR() for variables which should be dma_addr_t
>    instead of target_phys_addr_t.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  Makefile.objs |    2 +-
>  dma.h         |    1 +
>  hw/qdev-dma.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev-dma.h |    8 ++++++
>  4 files changed, 87 insertions(+), 1 deletions(-)
>  create mode 100644 hw/qdev-dma.c
>  create mode 100644 hw/qdev-dma.h
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 808de6a..59aed69 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -287,7 +287,7 @@ hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
>  hw-obj-$(CONFIG_ESP) += esp.o
>  
>  hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
> -hw-obj-y += qdev-addr.o
> +hw-obj-y += qdev-addr.o qdev-dma.o
>  
>  # VGA
>  hw-obj-$(CONFIG_VGA_PCI) += vga-pci.o
> 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.c b/hw/qdev-dma.c
> new file mode 100644
> index 0000000..2b21bf4
> --- /dev/null
> +++ b/hw/qdev-dma.c
> @@ -0,0 +1,77 @@
> +#include "qdev.h"
> +#include "qdev-dma.h"
> +#include "dma.h"
> +
> +/* --- target physical address --- */
> +
> +static int parse_dmaaddr(DeviceState *dev, Property *prop, const char *str)
> +{
> +    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    *ptr = strtoull(str, NULL, 16);
> +    return 0;
> +}

My fault here.  I didn't update qdev-addr.c in patch
97aa6e9b8f9df37add21d86fac1a9ca6ce7df9b7, and you obviously based your
work on it.  Can you please add an if statement here like this:

+    if (str[0] != '0' || str[1] != 'x') {
+        return -EINVAL;
+    }
+

Paolo

> +static int print_dmaaddr(DeviceState *dev, Property *prop, char *dest,
> +                         size_t len)
> +{
> +    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    return snprintf(dest, len, "0x" DMA_ADDR_FMT, *ptr);
> +}


> +static void get_dmaaddr(Object *obj, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int64_t value;
> +
> +    value = *ptr;
> +    visit_type_int(v, &value, name, errp);
> +}
> +
> +static void set_dmaaddr(Object *obj, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v, &value, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if ((uint64_t)value <= (uint64_t) ~(dma_addr_t)0) {
> +        *ptr = value;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, (uint64_t) 0,
> +                  (uint64_t) ~(dma_addr_t)0);
> +    }
> +}
> +
> +
> +PropertyInfo qdev_prop_dmaaddr = {
> +    .name  = "dmaaddr",
> +    .parse = parse_dmaaddr,
> +    .print = print_dmaaddr,
> +    .get   = get_dmaaddr,
> +    .set   = set_dmaaddr,
> +};
> +
> +void qdev_prop_set_dmaaddr(DeviceState *dev, const char *name, dma_addr_t value)
> +{
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
> +
> +}
> diff --git a/hw/qdev-dma.h b/hw/qdev-dma.h
> new file mode 100644
> index 0000000..8b01fda
> --- /dev/null
> +++ b/hw/qdev-dma.h
> @@ -0,0 +1,8 @@
> +#include "dma.h"
> +
> +#define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)                               \
> +    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_dmaaddr, dma_addr_t)
> +
> +extern PropertyInfo qdev_prop_dmaaddr;
> +void qdev_prop_set_dmaaddr(DeviceState *dev, const char *name,
> +                           dma_addr_t value);

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 808de6a..59aed69 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -287,7 +287,7 @@  hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
 hw-obj-$(CONFIG_ESP) += esp.o
 
 hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
-hw-obj-y += qdev-addr.o
+hw-obj-y += qdev-addr.o qdev-dma.o
 
 # VGA
 hw-obj-$(CONFIG_VGA_PCI) += vga-pci.o
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.c b/hw/qdev-dma.c
new file mode 100644
index 0000000..2b21bf4
--- /dev/null
+++ b/hw/qdev-dma.c
@@ -0,0 +1,77 @@ 
+#include "qdev.h"
+#include "qdev-dma.h"
+#include "dma.h"
+
+/* --- target physical address --- */
+
+static int parse_dmaaddr(DeviceState *dev, Property *prop, const char *str)
+{
+    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    *ptr = strtoull(str, NULL, 16);
+    return 0;
+}
+
+static int print_dmaaddr(DeviceState *dev, Property *prop, char *dest,
+                         size_t len)
+{
+    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+    return snprintf(dest, len, "0x" DMA_ADDR_FMT, *ptr);
+}
+
+static void get_dmaaddr(Object *obj, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+    int64_t value;
+
+    value = *ptr;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void set_dmaaddr(Object *obj, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    dma_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if ((uint64_t)value <= (uint64_t) ~(dma_addr_t)0) {
+        *ptr = value;
+    } else {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+                  dev->id?:"", name, value, (uint64_t) 0,
+                  (uint64_t) ~(dma_addr_t)0);
+    }
+}
+
+
+PropertyInfo qdev_prop_dmaaddr = {
+    .name  = "dmaaddr",
+    .parse = parse_dmaaddr,
+    .print = print_dmaaddr,
+    .get   = get_dmaaddr,
+    .set   = set_dmaaddr,
+};
+
+void qdev_prop_set_dmaaddr(DeviceState *dev, const char *name, dma_addr_t value)
+{
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
+
+}
diff --git a/hw/qdev-dma.h b/hw/qdev-dma.h
new file mode 100644
index 0000000..8b01fda
--- /dev/null
+++ b/hw/qdev-dma.h
@@ -0,0 +1,8 @@ 
+#include "dma.h"
+
+#define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)                               \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_dmaaddr, dma_addr_t)
+
+extern PropertyInfo qdev_prop_dmaaddr;
+void qdev_prop_set_dmaaddr(DeviceState *dev, const char *name,
+                           dma_addr_t value);