Patchwork [RFC,v1,3/3] device_tree: qemu_fdt_setprop: Fixup error reporting

login
register
mail settings
Submitter Peter Crosthwaite
Date July 12, 2013, 4:29 a.m.
Message ID <21d3151db1cf00430db82f67ea6f66e93197e2c9.1373603020.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/258687/
State New
Headers show

Comments

Peter Crosthwaite - July 12, 2013, 4:29 a.m.
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

There are a mix of usages of the qemu_fdt_* API calls, some which
wish to assert and abort QEMU on failure and some of which wish to do
their own error handling. The latter in more correct, but the former
is in the majority and more pragmatic, so facilitate both schemes by
creating asserting and non asserting variants. This patch does this
for qemu fdt_setprop and its wrapping users:

 * qemu_fdt_setprop
 * qemu_fdt_setprop_u64
 * qemu_fdt_setprop_cells

Error reporting is stylistically udpated to use Error ** instead of
integer return codes and exit(1).

Users of these APIs that ignore the return code are converted to using
the _assert variants. Users that check the return code are converted to
use Error ** for their error detection paths.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
If this is ok, I will apply the change pattern to the entire
qemu_fdt API

 device_tree.c                | 35 ++++++++++++----
 hw/arm/boot.c                |  7 ++--
 hw/ppc/e500.c                | 66 +++++++++++++++---------------
 hw/ppc/e500plat.c            |  6 +--
 hw/ppc/mpc8544ds.c           |  6 +--
 hw/ppc/ppc440_bamboo.c       |  8 ++--
 include/sysemu/device_tree.h | 97 +++++++++++++++++++++++++++++++++++++++++---
 7 files changed, 166 insertions(+), 59 deletions(-)
Peter Crosthwaite - July 20, 2013, 2:36 a.m.
Ping!

If theres not objections to the change pattern id like to proceed with the full
change.

Regards,
Peter

On Fri, Jul 12, 2013 at 2:29 PM,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> There are a mix of usages of the qemu_fdt_* API calls, some which
> wish to assert and abort QEMU on failure and some of which wish to do
> their own error handling. The latter in more correct, but the former
> is in the majority and more pragmatic, so facilitate both schemes by
> creating asserting and non asserting variants. This patch does this
> for qemu fdt_setprop and its wrapping users:
>
>  * qemu_fdt_setprop
>  * qemu_fdt_setprop_u64
>  * qemu_fdt_setprop_cells
>
> Error reporting is stylistically udpated to use Error ** instead of
> integer return codes and exit(1).
>
> Users of these APIs that ignore the return code are converted to using
> the _assert variants. Users that check the return code are converted to
> use Error ** for their error detection paths.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> If this is ok, I will apply the change pattern to the entire
> qemu_fdt API
>
>  device_tree.c                | 35 ++++++++++++----
>  hw/arm/boot.c                |  7 ++--
>  hw/ppc/e500.c                | 66 +++++++++++++++---------------
>  hw/ppc/e500plat.c            |  6 +--
>  hw/ppc/mpc8544ds.c           |  6 +--
>  hw/ppc/ppc440_bamboo.c       |  8 ++--
>  include/sysemu/device_tree.h | 97 +++++++++++++++++++++++++++++++++++++++++---
>  7 files changed, 166 insertions(+), 59 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 048b8e1..ca2a88d 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -4,8 +4,10 @@
>   * interface.
>   *
>   * Copyright 2008 IBM Corporation.
> + * Copyright 2013 Xilinx Inc.
>   * Authors: Jerone Young <jyoung5@us.ibm.com>
>   *          Hollis Blanchard <hollisb@us.ibm.com>
> + *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>   *
>   * This work is licensed under the GNU GPL license version 2 or later.
>   *
> @@ -126,19 +128,25 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>
> -int qemu_fdt_setprop(void *fdt, const char *node_path,
> -                     const char *property, const void *val, int size)
> +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
> +                      const void *val, int size, Error **errp)
>  {
>      int r;
>
>      r = fdt_setprop(fdt, findnode_nofail(fdt, node_path), property, val, size);
>      if (r < 0) {
> -        fprintf(stderr, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
> -                property, fdt_strerror(r));
> -        exit(1);
> +        error_setg(errp, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
> +                   property, fdt_strerror(r));
>      }
> +}
>
> -    return r;
> +void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
> +                             const char *property, const void *val, int size)
> +{
> +    Error *errp = NULL;
> +
> +    qemu_fdt_setprop(fdt, node_path, property, val, size, &errp);
> +    assert_no_error(errp);
>  }
>
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
> @@ -156,11 +164,20 @@ int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>      return r;
>  }
>
> -int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> -                         const char *property, uint64_t val)
> +void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> +                          const char *property, uint64_t val, Error **errp)
>  {
>      val = cpu_to_be64(val);
> -    return qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val));
> +    qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val), errp);
> +}
> +
> +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
> +                                 const char *property, uint64_t val)
> +{
> +    Error *errp = NULL;
> +
> +    qemu_fdt_setprop_u64(fdt, node_path, property, val, &errp);
> +    assert_no_error(errp);
>  }
>
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 2768b2b..9164bb9 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -233,6 +233,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>      char *filename;
>      int size, rc;
>      uint32_t acells, scells, hival;
> +    Error *errp = NULL;
>
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>      if (!filename) {
> @@ -276,9 +277,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>          goto fail;
>      }
>
> -    rc = qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> -                          mem_reg_propsize * sizeof(uint32_t));
> -    if (rc < 0) {
> +    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> +                     mem_reg_propsize * sizeof(uint32_t), &errp);
> +    if (errp) {
>          fprintf(stderr, "couldn't set /memory/reg\n");
>          goto fail;
>      }
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 0c2b54d..4ce5e78 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -111,10 +111,10 @@ static void dt_serial_create(void *fdt, unsigned long long offset,
>      qemu_fdt_add_subnode(fdt, ser);
>      qemu_fdt_setprop_string(fdt, ser, "device_type", "serial");
>      qemu_fdt_setprop_string(fdt, ser, "compatible", "ns16550");
> -    qemu_fdt_setprop_cells(fdt, ser, "reg", offset, 0x100);
> +    qemu_fdt_setprop_cells_assert(fdt, ser, "reg", offset, 0x100);
>      qemu_fdt_setprop_cell(fdt, ser, "cell-index", idx);
>      qemu_fdt_setprop_cell(fdt, ser, "clock-frequency", 0);
> -    qemu_fdt_setprop_cells(fdt, ser, "interrupts", 42, 2);
> +    qemu_fdt_setprop_cells_assert(fdt, ser, "interrupts", 42, 2);
>      qemu_fdt_setprop_phandle(fdt, ser, "interrupt-parent", mpic);
>      qemu_fdt_setprop_string(fdt, "/aliases", alias, ser);
>
> @@ -192,8 +192,8 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>
>      qemu_fdt_add_subnode(fdt, "/memory");
>      qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
> -    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> -                     sizeof(mem_reg_property));
> +    qemu_fdt_setprop_assert(fdt, "/memory", "reg", mem_reg_property,
> +                            sizeof(mem_reg_property));
>
>      qemu_fdt_add_subnode(fdt, "/chosen");
>      if (initrd_size) {
> @@ -225,11 +225,11 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>          qemu_fdt_setprop_string(fdt, "/hypervisor", "compatible",
>                                  "linux,kvm");
>          kvmppc_get_hypercall(env, hypercall, sizeof(hypercall));
> -        qemu_fdt_setprop(fdt, "/hypervisor", "hcall-instructions",
> -                         hypercall, sizeof(hypercall));
> +        qemu_fdt_setprop_assert(fdt, "/hypervisor", "hcall-instructions",
> +                                hypercall, sizeof(hypercall));
>          /* if KVM supports the idle hcall, set property indicating this */
>          if (kvmppc_get_hasidle(env)) {
> -            qemu_fdt_setprop(fdt, "/hypervisor", "has-idle", NULL, 0);
> +            qemu_fdt_setprop_assert(fdt, "/hypervisor", "has-idle", NULL, 0);
>          }
>      }
>
> @@ -269,8 +269,8 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>              qemu_fdt_setprop_string(fdt, cpu_name, "status", "disabled");
>              qemu_fdt_setprop_string(fdt, cpu_name, "enable-method",
>                                      "spin-table");
> -            qemu_fdt_setprop_u64(fdt, cpu_name, "cpu-release-addr",
> -                                 cpu_release_addr);
> +            qemu_fdt_setprop_u64_assert(fdt, cpu_name, "cpu-release-addr",
> +                                        cpu_release_addr);
>          } else {
>              qemu_fdt_setprop_string(fdt, cpu_name, "status", "okay");
>          }
> @@ -281,13 +281,13 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>      snprintf(soc, sizeof(soc), "/soc@%llx", MPC8544_CCSRBAR_BASE);
>      qemu_fdt_add_subnode(fdt, soc);
>      qemu_fdt_setprop_string(fdt, soc, "device_type", "soc");
> -    qemu_fdt_setprop(fdt, soc, "compatible", compatible_sb,
> -                     sizeof(compatible_sb));
> +    qemu_fdt_setprop_assert(fdt, soc, "compatible", compatible_sb,
> +                            sizeof(compatible_sb));
>      qemu_fdt_setprop_cell(fdt, soc, "#address-cells", 1);
>      qemu_fdt_setprop_cell(fdt, soc, "#size-cells", 1);
> -    qemu_fdt_setprop_cells(fdt, soc, "ranges", 0x0,
> -                           MPC8544_CCSRBAR_BASE >> 32, MPC8544_CCSRBAR_BASE,
> -                           MPC8544_CCSRBAR_SIZE);
> +    qemu_fdt_setprop_cells_assert(fdt, soc, "ranges", 0x0,
> +                                  MPC8544_CCSRBAR_BASE >> 32,
> +                                  MPC8544_CCSRBAR_BASE, MPC8544_CCSRBAR_SIZE);
>      /* XXX should contain a reasonable value */
>      qemu_fdt_setprop_cell(fdt, soc, "bus-frequency", 0);
>
> @@ -295,14 +295,14 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>      qemu_fdt_add_subnode(fdt, mpic);
>      qemu_fdt_setprop_string(fdt, mpic, "device_type", "open-pic");
>      qemu_fdt_setprop_string(fdt, mpic, "compatible", "fsl,mpic");
> -    qemu_fdt_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
> -                           0x40000);
> +    qemu_fdt_setprop_cells_assert(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
> +                                  0x40000);
>      qemu_fdt_setprop_cell(fdt, mpic, "#address-cells", 0);
>      qemu_fdt_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
>      mpic_ph = qemu_fdt_alloc_phandle(fdt);
>      qemu_fdt_setprop_cell(fdt, mpic, "phandle", mpic_ph);
>      qemu_fdt_setprop_cell(fdt, mpic, "linux,phandle", mpic_ph);
> -    qemu_fdt_setprop(fdt, mpic, "interrupt-controller", NULL, 0);
> +    qemu_fdt_setprop_assert(fdt, mpic, "interrupt-controller", NULL, 0);
>
>      /*
>       * We have to generate ser1 first, because Linux takes the first
> @@ -318,17 +318,19 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>               MPC8544_UTIL_OFFSET);
>      qemu_fdt_add_subnode(fdt, gutil);
>      qemu_fdt_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
> -    qemu_fdt_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
> -    qemu_fdt_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
> +    qemu_fdt_setprop_cells_assert(fdt, gutil, "reg", MPC8544_UTIL_OFFSET,
> +                                  0x1000);
> +    qemu_fdt_setprop_assert(fdt, gutil, "fsl,has-rstcr", NULL, 0);
>
>      snprintf(msi, sizeof(msi), "/%s/msi@%llx", soc, MPC8544_MSI_REGS_OFFSET);
>      qemu_fdt_add_subnode(fdt, msi);
>      qemu_fdt_setprop_string(fdt, msi, "compatible", "fsl,mpic-msi");
> -    qemu_fdt_setprop_cells(fdt, msi, "reg", MPC8544_MSI_REGS_OFFSET, 0x200);
> +    qemu_fdt_setprop_cells_assert(fdt, msi, "reg", MPC8544_MSI_REGS_OFFSET,
> +                                  0x200);
>      msi_ph = qemu_fdt_alloc_phandle(fdt);
> -    qemu_fdt_setprop_cells(fdt, msi, "msi-available-ranges", 0x0, 0x100);
> +    qemu_fdt_setprop_cells_assert(fdt, msi, "msi-available-ranges", 0x0, 0x100);
>      qemu_fdt_setprop_phandle(fdt, msi, "interrupt-parent", mpic);
> -    qemu_fdt_setprop_cells(fdt, msi, "interrupts",
> +    qemu_fdt_setprop_cells_assert(fdt, msi, "interrupts",
>          0xe0, 0x0,
>          0xe1, 0x0,
>          0xe2, 0x0,
> @@ -345,22 +347,22 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>      qemu_fdt_setprop_cell(fdt, pci, "cell-index", 0);
>      qemu_fdt_setprop_string(fdt, pci, "compatible", "fsl,mpc8540-pci");
>      qemu_fdt_setprop_string(fdt, pci, "device_type", "pci");
> -    qemu_fdt_setprop_cells(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
> -                           0x0, 0x7);
> +    qemu_fdt_setprop_cells_assert(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
> +                                  0x0, 0x7);
>      pci_map = pci_map_create(fdt, qemu_fdt_get_phandle(fdt, mpic),
>                               params->pci_first_slot, params->pci_nr_slots,
>                               &len);
> -    qemu_fdt_setprop(fdt, pci, "interrupt-map", pci_map, len);
> +    qemu_fdt_setprop_assert(fdt, pci, "interrupt-map", pci_map, len);
>      qemu_fdt_setprop_phandle(fdt, pci, "interrupt-parent", mpic);
> -    qemu_fdt_setprop_cells(fdt, pci, "interrupts", 24, 2);
> -    qemu_fdt_setprop_cells(fdt, pci, "bus-range", 0, 255);
> +    qemu_fdt_setprop_cells_assert(fdt, pci, "interrupts", 24, 2);
> +    qemu_fdt_setprop_cells_assert(fdt, pci, "bus-range", 0, 255);
>      for (i = 0; i < 14; i++) {
>          pci_ranges[i] = cpu_to_be32(pci_ranges[i]);
>      }
>      qemu_fdt_setprop_cell(fdt, pci, "fsl,msi", msi_ph);
> -    qemu_fdt_setprop(fdt, pci, "ranges", pci_ranges, sizeof(pci_ranges));
> -    qemu_fdt_setprop_cells(fdt, pci, "reg", MPC8544_PCI_REGS_BASE >> 32,
> -                           MPC8544_PCI_REGS_BASE, 0, 0x1000);
> +    qemu_fdt_setprop_assert(fdt, pci, "ranges", pci_ranges, sizeof(pci_ranges));
> +    qemu_fdt_setprop_cells_assert(fdt, pci, "reg", MPC8544_PCI_REGS_BASE >> 32,
> +                                  MPC8544_PCI_REGS_BASE, 0, 0x1000);
>      qemu_fdt_setprop_cell(fdt, pci, "clock-frequency", 66666666);
>      qemu_fdt_setprop_cell(fdt, pci, "#interrupt-cells", 1);
>      qemu_fdt_setprop_cell(fdt, pci, "#size-cells", 2);
> @@ -370,8 +372,8 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>      params->fixup_devtree(params, fdt);
>
>      if (toplevel_compat) {
> -        qemu_fdt_setprop(fdt, "/", "compatible", toplevel_compat,
> -                         strlen(toplevel_compat) + 1);
> +        qemu_fdt_setprop_assert(fdt, "/", "compatible", toplevel_compat,
> +                                strlen(toplevel_compat) + 1);
>      }
>
>  done:
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index 6eecdc5..16c0282 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -23,9 +23,9 @@ static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
>      const char model[] = "QEMU ppce500";
>      const char compatible[] = "fsl,qemu-e500";
>
> -    qemu_fdt_setprop(fdt, "/", "model", model, sizeof(model));
> -    qemu_fdt_setprop(fdt, "/", "compatible", compatible,
> -                     sizeof(compatible));
> +    qemu_fdt_setprop_assert(fdt, "/", "model", model, sizeof(model));
> +    qemu_fdt_setprop_assert(fdt, "/", "compatible", compatible,
> +                            sizeof(compatible));
>  }
>
>  static void e500plat_init(QEMUMachineInitArgs *args)
> diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
> index 4ed66f1..113fd13 100644
> --- a/hw/ppc/mpc8544ds.c
> +++ b/hw/ppc/mpc8544ds.c
> @@ -21,9 +21,9 @@ static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
>      const char model[] = "MPC8544DS";
>      const char compatible[] = "MPC8544DS\0MPC85xxDS";
>
> -    qemu_fdt_setprop(fdt, "/", "model", model, sizeof(model));
> -    qemu_fdt_setprop(fdt, "/", "compatible", compatible,
> -                     sizeof(compatible));
> +    qemu_fdt_setprop_assert(fdt, "/", "model", model, sizeof(model));
> +    qemu_fdt_setprop_assert(fdt, "/", "compatible", compatible,
> +                            sizeof(compatible));
>  }
>
>  static void mpc8544ds_init(QEMUMachineInitArgs *args)
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index db9d38b..39badfa 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -64,6 +64,7 @@ static int bamboo_load_device_tree(hwaddr addr,
>      void *fdt;
>      uint32_t tb_freq = 400000000;
>      uint32_t clock_freq = 400000000;
> +    Error *errp = NULL;
>
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
>      if (!filename) {
> @@ -77,10 +78,11 @@ static int bamboo_load_device_tree(hwaddr addr,
>
>      /* Manipulate device tree in memory. */
>
> -    ret = qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> -                           sizeof(mem_reg_property));
> -    if (ret < 0)
> +    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> +                     sizeof(mem_reg_property), &errp);
> +    if (errp) {
>          fprintf(stderr, "couldn't set /memory/reg\n");
> +    }
>
>      ret = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
>                                  initrd_base);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index b4650c5..adaf5c2 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -4,8 +4,10 @@
>   * interface.
>   *
>   * Copyright 2008 IBM Corporation.
> + * Copyright 2013 Xilinx Inc.
>   * Authors: Jerone Young <jyoung5@us.ibm.com>
>   *          Hollis Blanchard <hollisb@us.ibm.com>
> + *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>   *
>   * This work is licensed under the GNU GPL license version 2 or later.
>   *
> @@ -14,15 +16,68 @@
>  #ifndef __DEVICE_TREE_H__
>  #define __DEVICE_TREE_H__
>
> +#include "qapi/qmp/qerror.h"
> +
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>
> -int qemu_fdt_setprop(void *fdt, const char *node_path,
> -                     const char *property, const void *val, int size);
> +/**
> + * qemu_fdt_setprop - create or change a property
> + * @fdt: pointer to the device tree blob
> + * @node_path: node-path of the node whose property to change
> + * @property: name of the property to change
> + * @val: pointer to data to set the property value to
> + * @size: length of the property value
> + * @errp: Error to populate incase of error
> + *
> + * qemu_fdt_setprop() sets the value of the named property in the given
> + * node to the given value and length, creating the property if it
> + * does not already exist.
> + */
> +
> +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
> +                      const void *val, int size, Error **errp);
> +
> +/**
> + * qemu_fdt_setprop_assert - create or change a property and assert success
> + *
> + * Same as qemu_fdt_setprop() except no errp argument required, and
> + * asserts the success of the operation.
> + */
> +
> +void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
> +                             const char *property, const void *val, int size);
> +
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>                            const char *property, uint32_t val);
> -int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> -                         const char *property, uint64_t val);
> +
> +/**
> + * qemu_fdt_setprop_u64 - create or change a 64bit int property
> + * @fdt: pointer to the device tree blob
> + * @node_path: node-path of the node whose property to change
> + * @property: name of the property to change
> + * @val: value to set
> + * @errp: Error to populate incase of error
> + *
> + * qemu_fdt_setprop_u64() sets the value of the named property in the given
> + * node to the given uint64_t value. The value is converted to big endian
> + * format as per device tree formatting.
> + */
> +
> +void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> +                          const char *property, uint64_t val, Error **errp);
> +
> +/**
> + * qemu_fdt_setprop_u64_assert - create or change a 64 bit int property and
> + * assert success
> + *
> + * Same as qemu_fdt_setprop_u64() except no errp argument required, and
> + * asserts the success of the operation.
> + */
> +
> +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
> +                                 const char *property, uint64_t val);
> +
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>                              const char *property, const char *string);
>  int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
> @@ -37,7 +92,21 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt);
>  int qemu_fdt_nop_node(void *fdt, const char *node_path);
>  int qemu_fdt_add_subnode(void *fdt, const char *name);
>
> -#define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
> +/**
> + * qemu_fdt_setprop_cells - create or change a multi-cell property
> + * @fdt: pointer to the device tree blob
> + * @node_path: node-path of the node whose property to change
> + * @property: name of the property to change
> + * @errp: Error to populate incase of error
> + * @...: varargs list of cells to write to property
> + *
> + * qemu_fdt_setprop_cells() sets the value of the named property in the given
> + * node to a list of cells. The varargs are converted to an appropriate length
> + * uint32_t array and converted to big endian. The array is then written as
> + * the property value.
> + */
> +
> +#define qemu_fdt_setprop_cells(fdt, node_path, property, errp, ...)           \
>      do {                                                                      \
>          uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
>          int i;                                                                \
> @@ -46,7 +115,23 @@ int qemu_fdt_add_subnode(void *fdt, const char *name);
>              qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);                             \
>          }                                                                     \
>          qemu_fdt_setprop(fdt, node_path, property, qdt_tmp,                   \
> -                         sizeof(qdt_tmp));                                    \
> +                         sizeof(qdt_tmp), errp);                              \
> +    } while (0)
> +
> +/**
> + * qemu_fdt_setprop_cells_assert - create or change a mult-cell property and
> + * assert success
> + *
> + * Same as qemu_fdt_setprop_cells() except no errp argument required, and
> + * asserts the success of the operation.
> + */
> +
> +#define qemu_fdt_setprop_cells_assert(fdt, node_path, property, ...)          \
> +    do {                                                                      \
> +        Error *errp = NULL;                                                   \
> +                                                                              \
> +        qemu_fdt_setprop_cells(fdt, node_path, property, &errp, __VA_ARGS__); \
> +        assert_no_error(errp);                                                \
>      } while (0)
>
>  void qemu_fdt_dumpdtb(void *fdt, int size);
> --
> 1.8.3.rc1.44.gb387c77.dirty
>
Andreas Färber - July 20, 2013, 12:07 p.m.
Am 12.07.2013 06:29, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> There are a mix of usages of the qemu_fdt_* API calls, some which

"some of which"

> wish to assert and abort QEMU on failure and some of which wish to do
> their own error handling. The latter in more correct, but the former

"is more"

> is in the majority and more pragmatic, so facilitate both schemes by
> creating asserting and non asserting variants. This patch does this
> for qemu fdt_setprop and its wrapping users:
> 
>  * qemu_fdt_setprop
>  * qemu_fdt_setprop_u64
>  * qemu_fdt_setprop_cells
> 
> Error reporting is stylistically udpated to use Error ** instead of

"updated"

> integer return codes and exit(1).
> 
> Users of these APIs that ignore the return code are converted to using
> the _assert variants. Users that check the return code are converted to
> use Error ** for their error detection paths.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> If this is ok, I will apply the change pattern to the entire
> qemu_fdt API
> 
>  device_tree.c                | 35 ++++++++++++----
>  hw/arm/boot.c                |  7 ++--
>  hw/ppc/e500.c                | 66 +++++++++++++++---------------
>  hw/ppc/e500plat.c            |  6 +--
>  hw/ppc/mpc8544ds.c           |  6 +--
>  hw/ppc/ppc440_bamboo.c       |  8 ++--
>  include/sysemu/device_tree.h | 97 +++++++++++++++++++++++++++++++++++++++++---
>  7 files changed, 166 insertions(+), 59 deletions(-)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 048b8e1..ca2a88d 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -4,8 +4,10 @@
>   * interface.
>   *
>   * Copyright 2008 IBM Corporation.
> + * Copyright 2013 Xilinx Inc.
>   * Authors: Jerone Young <jyoung5@us.ibm.com>
>   *          Hollis Blanchard <hollisb@us.ibm.com>
> + *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>   *
>   * This work is licensed under the GNU GPL license version 2 or later.
>   *
> @@ -126,19 +128,25 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>  
> -int qemu_fdt_setprop(void *fdt, const char *node_path,
> -                     const char *property, const void *val, int size)
> +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
> +                      const void *val, int size, Error **errp)
>  {
>      int r;
>  
>      r = fdt_setprop(fdt, findnode_nofail(fdt, node_path), property, val, size);
>      if (r < 0) {
> -        fprintf(stderr, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
> -                property, fdt_strerror(r));
> -        exit(1);
> +        error_setg(errp, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
> +                   property, fdt_strerror(r));

The error_set*() functions shouldn't get \n appended. I think it would
be better to drop __func__, too. Otherwise the idea looks sane. Thanks
for looking into this.

>      }
> +}
>  
> -    return r;
> +void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
> +                             const char *property, const void *val, int size)
> +{
> +    Error *errp = NULL;

Please use err for Error* here, errp is used for Error**.

> +
> +    qemu_fdt_setprop(fdt, node_path, property, val, size, &errp);
> +    assert_no_error(errp);
>  }
>  
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
> @@ -156,11 +164,20 @@ int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>      return r;
>  }
>  
> -int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> -                         const char *property, uint64_t val)
> +void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> +                          const char *property, uint64_t val, Error **errp)
>  {
>      val = cpu_to_be64(val);
> -    return qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val));
> +    qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val), errp);
> +}
> +
> +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
> +                                 const char *property, uint64_t val)
> +{
> +    Error *errp = NULL;
> +
> +    qemu_fdt_setprop_u64(fdt, node_path, property, val, &errp);
> +    assert_no_error(errp);
>  }
>  
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
[...]
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index db9d38b..39badfa 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -64,6 +64,7 @@ static int bamboo_load_device_tree(hwaddr addr,
>      void *fdt;
>      uint32_t tb_freq = 400000000;
>      uint32_t clock_freq = 400000000;
> +    Error *errp = NULL;

err

>  
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
>      if (!filename) {
> @@ -77,10 +78,11 @@ static int bamboo_load_device_tree(hwaddr addr,
>  
>      /* Manipulate device tree in memory. */
>  
> -    ret = qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> -                           sizeof(mem_reg_property));
> -    if (ret < 0)
> +    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
> +                     sizeof(mem_reg_property), &errp);
> +    if (errp) {
>          fprintf(stderr, "couldn't set /memory/reg\n");

error_free(err) and possibly append it to the error message?

> +    }
>  
>      ret = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
>                                  initrd_base);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index b4650c5..adaf5c2 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -4,8 +4,10 @@
>   * interface.
>   *
>   * Copyright 2008 IBM Corporation.
> + * Copyright 2013 Xilinx Inc.
>   * Authors: Jerone Young <jyoung5@us.ibm.com>
>   *          Hollis Blanchard <hollisb@us.ibm.com>
> + *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>   *
>   * This work is licensed under the GNU GPL license version 2 or later.
>   *
> @@ -14,15 +16,68 @@
>  #ifndef __DEVICE_TREE_H__
>  #define __DEVICE_TREE_H__
>  
> +#include "qapi/qmp/qerror.h"

qapi/error.h?

> +
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  
> -int qemu_fdt_setprop(void *fdt, const char *node_path,
> -                     const char *property, const void *val, int size);
> +/**
> + * qemu_fdt_setprop - create or change a property

 * qemu_fdt_setprop:

Description goes below the arguments.

> + * @fdt: pointer to the device tree blob
> + * @node_path: node-path of the node whose property to change
> + * @property: name of the property to change
> + * @val: pointer to data to set the property value to
> + * @size: length of the property value
> + * @errp: Error to populate incase of error

"in case"

> + *
> + * qemu_fdt_setprop() sets the value of the named property in the given
> + * node to the given value and length, creating the property if it
> + * does not already exist.
> + */
> +
> +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
> +                      const void *val, int size, Error **errp);
> +
> +/**
> + * qemu_fdt_setprop_assert - create or change a property and assert success
> + *
> + * Same as qemu_fdt_setprop() except no errp argument required, and
> + * asserts the success of the operation.
> + */
> +
> +void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
> +                             const char *property, const void *val, int size);
> +
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>                            const char *property, uint32_t val);
> -int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> -                         const char *property, uint64_t val);
> +
> +/**
> + * qemu_fdt_setprop_u64 - create or change a 64bit int property
> + * @fdt: pointer to the device tree blob
> + * @node_path: node-path of the node whose property to change
> + * @property: name of the property to change
> + * @val: value to set
> + * @errp: Error to populate incase of error
> + *
> + * qemu_fdt_setprop_u64() sets the value of the named property in the given
> + * node to the given uint64_t value. The value is converted to big endian
> + * format as per device tree formatting.
> + */
> +
> +void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
> +                          const char *property, uint64_t val, Error **errp);
> +
> +/**
> + * qemu_fdt_setprop_u64_assert - create or change a 64 bit int property and
> + * assert success
> + *
> + * Same as qemu_fdt_setprop_u64() except no errp argument required, and
> + * asserts the success of the operation.
> + */
> +
> +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
> +                                 const char *property, uint64_t val);
> +
>  int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>                              const char *property, const char *string);
>  int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
> @@ -37,7 +92,21 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt);
>  int qemu_fdt_nop_node(void *fdt, const char *node_path);
>  int qemu_fdt_add_subnode(void *fdt, const char *name);
>  
> -#define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
> +/**
> + * qemu_fdt_setprop_cells - create or change a multi-cell property
> + * @fdt: pointer to the device tree blob
> + * @node_path: node-path of the node whose property to change
> + * @property: name of the property to change
> + * @errp: Error to populate incase of error
> + * @...: varargs list of cells to write to property
> + *
> + * qemu_fdt_setprop_cells() sets the value of the named property in the given
> + * node to a list of cells. The varargs are converted to an appropriate length
> + * uint32_t array and converted to big endian. The array is then written as
> + * the property value.
> + */
> +
> +#define qemu_fdt_setprop_cells(fdt, node_path, property, errp, ...)           \
>      do {                                                                      \
>          uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
>          int i;                                                                \
> @@ -46,7 +115,23 @@ int qemu_fdt_add_subnode(void *fdt, const char *name);
>              qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);                             \
>          }                                                                     \
>          qemu_fdt_setprop(fdt, node_path, property, qdt_tmp,                   \
> -                         sizeof(qdt_tmp));                                    \
> +                         sizeof(qdt_tmp), errp);                              \
> +    } while (0)
> +
> +/**
> + * qemu_fdt_setprop_cells_assert - create or change a mult-cell property and
> + * assert success
> + *
> + * Same as qemu_fdt_setprop_cells() except no errp argument required, and
> + * asserts the success of the operation.
> + */
> +
> +#define qemu_fdt_setprop_cells_assert(fdt, node_path, property, ...)          \
> +    do {                                                                      \
> +        Error *errp = NULL;                                                   \

err

> +                                                                              \
> +        qemu_fdt_setprop_cells(fdt, node_path, property, &errp, __VA_ARGS__); \
> +        assert_no_error(errp);                                                \
>      } while (0)
>  
>  void qemu_fdt_dumpdtb(void *fdt, int size);

Regards,
Andreas

Patch

diff --git a/device_tree.c b/device_tree.c
index 048b8e1..ca2a88d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -4,8 +4,10 @@ 
  * interface.
  *
  * Copyright 2008 IBM Corporation.
+ * Copyright 2013 Xilinx Inc.
  * Authors: Jerone Young <jyoung5@us.ibm.com>
  *          Hollis Blanchard <hollisb@us.ibm.com>
+ *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
  *
  * This work is licensed under the GNU GPL license version 2 or later.
  *
@@ -126,19 +128,25 @@  static int findnode_nofail(void *fdt, const char *node_path)
     return offset;
 }
 
-int qemu_fdt_setprop(void *fdt, const char *node_path,
-                     const char *property, const void *val, int size)
+void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
+                      const void *val, int size, Error **errp)
 {
     int r;
 
     r = fdt_setprop(fdt, findnode_nofail(fdt, node_path), property, val, size);
     if (r < 0) {
-        fprintf(stderr, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
-                property, fdt_strerror(r));
-        exit(1);
+        error_setg(errp, "%s: Couldn't set %s/%s: %s\n", __func__, node_path,
+                   property, fdt_strerror(r));
     }
+}
 
-    return r;
+void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
+                             const char *property, const void *val, int size)
+{
+    Error *errp = NULL;
+
+    qemu_fdt_setprop(fdt, node_path, property, val, size, &errp);
+    assert_no_error(errp);
 }
 
 int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
@@ -156,11 +164,20 @@  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
     return r;
 }
 
-int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
-                         const char *property, uint64_t val)
+void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
+                          const char *property, uint64_t val, Error **errp)
 {
     val = cpu_to_be64(val);
-    return qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val));
+    qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val), errp);
+}
+
+void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
+                                 const char *property, uint64_t val)
+{
+    Error *errp = NULL;
+
+    qemu_fdt_setprop_u64(fdt, node_path, property, val, &errp);
+    assert_no_error(errp);
 }
 
 int qemu_fdt_setprop_string(void *fdt, const char *node_path,
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 2768b2b..9164bb9 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -233,6 +233,7 @@  static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
     char *filename;
     int size, rc;
     uint32_t acells, scells, hival;
+    Error *errp = NULL;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
     if (!filename) {
@@ -276,9 +277,9 @@  static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
         goto fail;
     }
 
-    rc = qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
-                          mem_reg_propsize * sizeof(uint32_t));
-    if (rc < 0) {
+    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
+                     mem_reg_propsize * sizeof(uint32_t), &errp);
+    if (errp) {
         fprintf(stderr, "couldn't set /memory/reg\n");
         goto fail;
     }
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 0c2b54d..4ce5e78 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -111,10 +111,10 @@  static void dt_serial_create(void *fdt, unsigned long long offset,
     qemu_fdt_add_subnode(fdt, ser);
     qemu_fdt_setprop_string(fdt, ser, "device_type", "serial");
     qemu_fdt_setprop_string(fdt, ser, "compatible", "ns16550");
-    qemu_fdt_setprop_cells(fdt, ser, "reg", offset, 0x100);
+    qemu_fdt_setprop_cells_assert(fdt, ser, "reg", offset, 0x100);
     qemu_fdt_setprop_cell(fdt, ser, "cell-index", idx);
     qemu_fdt_setprop_cell(fdt, ser, "clock-frequency", 0);
-    qemu_fdt_setprop_cells(fdt, ser, "interrupts", 42, 2);
+    qemu_fdt_setprop_cells_assert(fdt, ser, "interrupts", 42, 2);
     qemu_fdt_setprop_phandle(fdt, ser, "interrupt-parent", mpic);
     qemu_fdt_setprop_string(fdt, "/aliases", alias, ser);
 
@@ -192,8 +192,8 @@  static int ppce500_load_device_tree(CPUPPCState *env,
 
     qemu_fdt_add_subnode(fdt, "/memory");
     qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory");
-    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
-                     sizeof(mem_reg_property));
+    qemu_fdt_setprop_assert(fdt, "/memory", "reg", mem_reg_property,
+                            sizeof(mem_reg_property));
 
     qemu_fdt_add_subnode(fdt, "/chosen");
     if (initrd_size) {
@@ -225,11 +225,11 @@  static int ppce500_load_device_tree(CPUPPCState *env,
         qemu_fdt_setprop_string(fdt, "/hypervisor", "compatible",
                                 "linux,kvm");
         kvmppc_get_hypercall(env, hypercall, sizeof(hypercall));
-        qemu_fdt_setprop(fdt, "/hypervisor", "hcall-instructions",
-                         hypercall, sizeof(hypercall));
+        qemu_fdt_setprop_assert(fdt, "/hypervisor", "hcall-instructions",
+                                hypercall, sizeof(hypercall));
         /* if KVM supports the idle hcall, set property indicating this */
         if (kvmppc_get_hasidle(env)) {
-            qemu_fdt_setprop(fdt, "/hypervisor", "has-idle", NULL, 0);
+            qemu_fdt_setprop_assert(fdt, "/hypervisor", "has-idle", NULL, 0);
         }
     }
 
@@ -269,8 +269,8 @@  static int ppce500_load_device_tree(CPUPPCState *env,
             qemu_fdt_setprop_string(fdt, cpu_name, "status", "disabled");
             qemu_fdt_setprop_string(fdt, cpu_name, "enable-method",
                                     "spin-table");
-            qemu_fdt_setprop_u64(fdt, cpu_name, "cpu-release-addr",
-                                 cpu_release_addr);
+            qemu_fdt_setprop_u64_assert(fdt, cpu_name, "cpu-release-addr",
+                                        cpu_release_addr);
         } else {
             qemu_fdt_setprop_string(fdt, cpu_name, "status", "okay");
         }
@@ -281,13 +281,13 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     snprintf(soc, sizeof(soc), "/soc@%llx", MPC8544_CCSRBAR_BASE);
     qemu_fdt_add_subnode(fdt, soc);
     qemu_fdt_setprop_string(fdt, soc, "device_type", "soc");
-    qemu_fdt_setprop(fdt, soc, "compatible", compatible_sb,
-                     sizeof(compatible_sb));
+    qemu_fdt_setprop_assert(fdt, soc, "compatible", compatible_sb,
+                            sizeof(compatible_sb));
     qemu_fdt_setprop_cell(fdt, soc, "#address-cells", 1);
     qemu_fdt_setprop_cell(fdt, soc, "#size-cells", 1);
-    qemu_fdt_setprop_cells(fdt, soc, "ranges", 0x0,
-                           MPC8544_CCSRBAR_BASE >> 32, MPC8544_CCSRBAR_BASE,
-                           MPC8544_CCSRBAR_SIZE);
+    qemu_fdt_setprop_cells_assert(fdt, soc, "ranges", 0x0,
+                                  MPC8544_CCSRBAR_BASE >> 32,
+                                  MPC8544_CCSRBAR_BASE, MPC8544_CCSRBAR_SIZE);
     /* XXX should contain a reasonable value */
     qemu_fdt_setprop_cell(fdt, soc, "bus-frequency", 0);
 
@@ -295,14 +295,14 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     qemu_fdt_add_subnode(fdt, mpic);
     qemu_fdt_setprop_string(fdt, mpic, "device_type", "open-pic");
     qemu_fdt_setprop_string(fdt, mpic, "compatible", "fsl,mpic");
-    qemu_fdt_setprop_cells(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
-                           0x40000);
+    qemu_fdt_setprop_cells_assert(fdt, mpic, "reg", MPC8544_MPIC_REGS_OFFSET,
+                                  0x40000);
     qemu_fdt_setprop_cell(fdt, mpic, "#address-cells", 0);
     qemu_fdt_setprop_cell(fdt, mpic, "#interrupt-cells", 2);
     mpic_ph = qemu_fdt_alloc_phandle(fdt);
     qemu_fdt_setprop_cell(fdt, mpic, "phandle", mpic_ph);
     qemu_fdt_setprop_cell(fdt, mpic, "linux,phandle", mpic_ph);
-    qemu_fdt_setprop(fdt, mpic, "interrupt-controller", NULL, 0);
+    qemu_fdt_setprop_assert(fdt, mpic, "interrupt-controller", NULL, 0);
 
     /*
      * We have to generate ser1 first, because Linux takes the first
@@ -318,17 +318,19 @@  static int ppce500_load_device_tree(CPUPPCState *env,
              MPC8544_UTIL_OFFSET);
     qemu_fdt_add_subnode(fdt, gutil);
     qemu_fdt_setprop_string(fdt, gutil, "compatible", "fsl,mpc8544-guts");
-    qemu_fdt_setprop_cells(fdt, gutil, "reg", MPC8544_UTIL_OFFSET, 0x1000);
-    qemu_fdt_setprop(fdt, gutil, "fsl,has-rstcr", NULL, 0);
+    qemu_fdt_setprop_cells_assert(fdt, gutil, "reg", MPC8544_UTIL_OFFSET,
+                                  0x1000);
+    qemu_fdt_setprop_assert(fdt, gutil, "fsl,has-rstcr", NULL, 0);
 
     snprintf(msi, sizeof(msi), "/%s/msi@%llx", soc, MPC8544_MSI_REGS_OFFSET);
     qemu_fdt_add_subnode(fdt, msi);
     qemu_fdt_setprop_string(fdt, msi, "compatible", "fsl,mpic-msi");
-    qemu_fdt_setprop_cells(fdt, msi, "reg", MPC8544_MSI_REGS_OFFSET, 0x200);
+    qemu_fdt_setprop_cells_assert(fdt, msi, "reg", MPC8544_MSI_REGS_OFFSET,
+                                  0x200);
     msi_ph = qemu_fdt_alloc_phandle(fdt);
-    qemu_fdt_setprop_cells(fdt, msi, "msi-available-ranges", 0x0, 0x100);
+    qemu_fdt_setprop_cells_assert(fdt, msi, "msi-available-ranges", 0x0, 0x100);
     qemu_fdt_setprop_phandle(fdt, msi, "interrupt-parent", mpic);
-    qemu_fdt_setprop_cells(fdt, msi, "interrupts",
+    qemu_fdt_setprop_cells_assert(fdt, msi, "interrupts",
         0xe0, 0x0,
         0xe1, 0x0,
         0xe2, 0x0,
@@ -345,22 +347,22 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     qemu_fdt_setprop_cell(fdt, pci, "cell-index", 0);
     qemu_fdt_setprop_string(fdt, pci, "compatible", "fsl,mpc8540-pci");
     qemu_fdt_setprop_string(fdt, pci, "device_type", "pci");
-    qemu_fdt_setprop_cells(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
-                           0x0, 0x7);
+    qemu_fdt_setprop_cells_assert(fdt, pci, "interrupt-map-mask", 0xf800, 0x0,
+                                  0x0, 0x7);
     pci_map = pci_map_create(fdt, qemu_fdt_get_phandle(fdt, mpic),
                              params->pci_first_slot, params->pci_nr_slots,
                              &len);
-    qemu_fdt_setprop(fdt, pci, "interrupt-map", pci_map, len);
+    qemu_fdt_setprop_assert(fdt, pci, "interrupt-map", pci_map, len);
     qemu_fdt_setprop_phandle(fdt, pci, "interrupt-parent", mpic);
-    qemu_fdt_setprop_cells(fdt, pci, "interrupts", 24, 2);
-    qemu_fdt_setprop_cells(fdt, pci, "bus-range", 0, 255);
+    qemu_fdt_setprop_cells_assert(fdt, pci, "interrupts", 24, 2);
+    qemu_fdt_setprop_cells_assert(fdt, pci, "bus-range", 0, 255);
     for (i = 0; i < 14; i++) {
         pci_ranges[i] = cpu_to_be32(pci_ranges[i]);
     }
     qemu_fdt_setprop_cell(fdt, pci, "fsl,msi", msi_ph);
-    qemu_fdt_setprop(fdt, pci, "ranges", pci_ranges, sizeof(pci_ranges));
-    qemu_fdt_setprop_cells(fdt, pci, "reg", MPC8544_PCI_REGS_BASE >> 32,
-                           MPC8544_PCI_REGS_BASE, 0, 0x1000);
+    qemu_fdt_setprop_assert(fdt, pci, "ranges", pci_ranges, sizeof(pci_ranges));
+    qemu_fdt_setprop_cells_assert(fdt, pci, "reg", MPC8544_PCI_REGS_BASE >> 32,
+                                  MPC8544_PCI_REGS_BASE, 0, 0x1000);
     qemu_fdt_setprop_cell(fdt, pci, "clock-frequency", 66666666);
     qemu_fdt_setprop_cell(fdt, pci, "#interrupt-cells", 1);
     qemu_fdt_setprop_cell(fdt, pci, "#size-cells", 2);
@@ -370,8 +372,8 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     params->fixup_devtree(params, fdt);
 
     if (toplevel_compat) {
-        qemu_fdt_setprop(fdt, "/", "compatible", toplevel_compat,
-                         strlen(toplevel_compat) + 1);
+        qemu_fdt_setprop_assert(fdt, "/", "compatible", toplevel_compat,
+                                strlen(toplevel_compat) + 1);
     }
 
 done:
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 6eecdc5..16c0282 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -23,9 +23,9 @@  static void e500plat_fixup_devtree(PPCE500Params *params, void *fdt)
     const char model[] = "QEMU ppce500";
     const char compatible[] = "fsl,qemu-e500";
 
-    qemu_fdt_setprop(fdt, "/", "model", model, sizeof(model));
-    qemu_fdt_setprop(fdt, "/", "compatible", compatible,
-                     sizeof(compatible));
+    qemu_fdt_setprop_assert(fdt, "/", "model", model, sizeof(model));
+    qemu_fdt_setprop_assert(fdt, "/", "compatible", compatible,
+                            sizeof(compatible));
 }
 
 static void e500plat_init(QEMUMachineInitArgs *args)
diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
index 4ed66f1..113fd13 100644
--- a/hw/ppc/mpc8544ds.c
+++ b/hw/ppc/mpc8544ds.c
@@ -21,9 +21,9 @@  static void mpc8544ds_fixup_devtree(PPCE500Params *params, void *fdt)
     const char model[] = "MPC8544DS";
     const char compatible[] = "MPC8544DS\0MPC85xxDS";
 
-    qemu_fdt_setprop(fdt, "/", "model", model, sizeof(model));
-    qemu_fdt_setprop(fdt, "/", "compatible", compatible,
-                     sizeof(compatible));
+    qemu_fdt_setprop_assert(fdt, "/", "model", model, sizeof(model));
+    qemu_fdt_setprop_assert(fdt, "/", "compatible", compatible,
+                            sizeof(compatible));
 }
 
 static void mpc8544ds_init(QEMUMachineInitArgs *args)
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index db9d38b..39badfa 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -64,6 +64,7 @@  static int bamboo_load_device_tree(hwaddr addr,
     void *fdt;
     uint32_t tb_freq = 400000000;
     uint32_t clock_freq = 400000000;
+    Error *errp = NULL;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
     if (!filename) {
@@ -77,10 +78,11 @@  static int bamboo_load_device_tree(hwaddr addr,
 
     /* Manipulate device tree in memory. */
 
-    ret = qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
-                           sizeof(mem_reg_property));
-    if (ret < 0)
+    qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property,
+                     sizeof(mem_reg_property), &errp);
+    if (errp) {
         fprintf(stderr, "couldn't set /memory/reg\n");
+    }
 
     ret = qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start",
                                 initrd_base);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index b4650c5..adaf5c2 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -4,8 +4,10 @@ 
  * interface.
  *
  * Copyright 2008 IBM Corporation.
+ * Copyright 2013 Xilinx Inc.
  * Authors: Jerone Young <jyoung5@us.ibm.com>
  *          Hollis Blanchard <hollisb@us.ibm.com>
+ *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
  *
  * This work is licensed under the GNU GPL license version 2 or later.
  *
@@ -14,15 +16,68 @@ 
 #ifndef __DEVICE_TREE_H__
 #define __DEVICE_TREE_H__
 
+#include "qapi/qmp/qerror.h"
+
 void *create_device_tree(int *sizep);
 void *load_device_tree(const char *filename_path, int *sizep);
 
-int qemu_fdt_setprop(void *fdt, const char *node_path,
-                     const char *property, const void *val, int size);
+/**
+ * qemu_fdt_setprop - create or change a property
+ * @fdt: pointer to the device tree blob
+ * @node_path: node-path of the node whose property to change
+ * @property: name of the property to change
+ * @val: pointer to data to set the property value to
+ * @size: length of the property value
+ * @errp: Error to populate incase of error
+ *
+ * qemu_fdt_setprop() sets the value of the named property in the given
+ * node to the given value and length, creating the property if it
+ * does not already exist.
+ */
+
+void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
+                      const void *val, int size, Error **errp);
+
+/**
+ * qemu_fdt_setprop_assert - create or change a property and assert success
+ *
+ * Same as qemu_fdt_setprop() except no errp argument required, and
+ * asserts the success of the operation.
+ */
+
+void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
+                             const char *property, const void *val, int size);
+
 int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
                           const char *property, uint32_t val);
-int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
-                         const char *property, uint64_t val);
+
+/**
+ * qemu_fdt_setprop_u64 - create or change a 64bit int property
+ * @fdt: pointer to the device tree blob
+ * @node_path: node-path of the node whose property to change
+ * @property: name of the property to change
+ * @val: value to set
+ * @errp: Error to populate incase of error
+ *
+ * qemu_fdt_setprop_u64() sets the value of the named property in the given
+ * node to the given uint64_t value. The value is converted to big endian
+ * format as per device tree formatting.
+ */
+
+void qemu_fdt_setprop_u64(void *fdt, const char *node_path,
+                          const char *property, uint64_t val, Error **errp);
+
+/**
+ * qemu_fdt_setprop_u64_assert - create or change a 64 bit int property and
+ * assert success
+ *
+ * Same as qemu_fdt_setprop_u64() except no errp argument required, and
+ * asserts the success of the operation.
+ */
+
+void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path,
+                                 const char *property, uint64_t val);
+
 int qemu_fdt_setprop_string(void *fdt, const char *node_path,
                             const char *property, const char *string);
 int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
@@ -37,7 +92,21 @@  uint32_t qemu_fdt_alloc_phandle(void *fdt);
 int qemu_fdt_nop_node(void *fdt, const char *node_path);
 int qemu_fdt_add_subnode(void *fdt, const char *name);
 
-#define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
+/**
+ * qemu_fdt_setprop_cells - create or change a multi-cell property
+ * @fdt: pointer to the device tree blob
+ * @node_path: node-path of the node whose property to change
+ * @property: name of the property to change
+ * @errp: Error to populate incase of error
+ * @...: varargs list of cells to write to property
+ *
+ * qemu_fdt_setprop_cells() sets the value of the named property in the given
+ * node to a list of cells. The varargs are converted to an appropriate length
+ * uint32_t array and converted to big endian. The array is then written as
+ * the property value.
+ */
+
+#define qemu_fdt_setprop_cells(fdt, node_path, property, errp, ...)           \
     do {                                                                      \
         uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
         int i;                                                                \
@@ -46,7 +115,23 @@  int qemu_fdt_add_subnode(void *fdt, const char *name);
             qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);                             \
         }                                                                     \
         qemu_fdt_setprop(fdt, node_path, property, qdt_tmp,                   \
-                         sizeof(qdt_tmp));                                    \
+                         sizeof(qdt_tmp), errp);                              \
+    } while (0)
+
+/**
+ * qemu_fdt_setprop_cells_assert - create or change a mult-cell property and
+ * assert success
+ *
+ * Same as qemu_fdt_setprop_cells() except no errp argument required, and
+ * asserts the success of the operation.
+ */
+
+#define qemu_fdt_setprop_cells_assert(fdt, node_path, property, ...)          \
+    do {                                                                      \
+        Error *errp = NULL;                                                   \
+                                                                              \
+        qemu_fdt_setprop_cells(fdt, node_path, property, &errp, __VA_ARGS__); \
+        assert_no_error(errp);                                                \
     } while (0)
 
 void qemu_fdt_dumpdtb(void *fdt, int size);