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.crosthwa...@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwa...@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.crosthwa...@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 <jyou...@us.ibm.com> > * Hollis Blanchard <holl...@us.ibm.com> > + * Peter Crosthwaite <peter.crosthwa...@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 <jyou...@us.ibm.com> > * Hollis Blanchard <holl...@us.ibm.com> > + * Peter Crosthwaite <peter.crosthwa...@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 >