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

[Paolo Bonzini]

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 <address@hidden>
> ---
>  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);