diff mbox

[08/13] spapr: Consolidate construction of /chosen device tree node

Message ID 1476940330-27705-9-git-send-email-david@gibson.dropbear.id.au (mailing list archive)
State New, archived
Headers show

Commit Message

David Gibson Oct. 20, 2016, 5:12 a.m. UTC 
For historical reasons, building the /chosen node in the guest device tree
is split across several places and includes both parts which write the DT
sequentially and others which use random access functions.

This patch consolidates construction of the node into one place, using
random access functions throughout.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c             | 131 ++++++++++++++++++++++-----------------------
 hw/ppc/spapr_vio.c         |  17 ++----
 include/hw/ppc/spapr_vio.h |   2 +-
 3 files changed, 70 insertions(+), 80 deletions(-)

Comments

Thomas Huth Oct. 20, 2016, 7:48 a.m. UTC | #1 
On 20.10.2016 07:12, David Gibson wrote:
> For historical reasons, building the /chosen node in the guest device tree
> is split across several places and includes both parts which write the DT
> sequentially and others which use random access functions.
> 
> This patch consolidates construction of the node into one place, using
> random access functions throughout.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c             | 131 ++++++++++++++++++++++-----------------------
>  hw/ppc/spapr_vio.c         |  17 ++----
>  include/hw/ppc/spapr_vio.h |   2 +-
>  3 files changed, 70 insertions(+), 80 deletions(-)
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>
Thomas Huth Oct. 20, 2016, 7:49 a.m. UTC | #2 
On 20.10.2016 07:12, David Gibson wrote:
> For historical reasons, building the /chosen node in the guest device tree
> is split across several places and includes both parts which write the DT
> sequentially and others which use random access functions.
> 
> This patch consolidates construction of the node into one place, using
> random access functions throughout.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c             | 131 ++++++++++++++++++++++-----------------------
>  hw/ppc/spapr_vio.c         |  17 ++----
>  include/hw/ppc/spapr_vio.h |   2 +-
>  3 files changed, 70 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2faae32..c608ec8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -273,14 +273,10 @@ static void add_str(GString *s, const gchar *s1)
>  
>  static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
>  {
> -    MachineState *machine = MACHINE(spapr);
>      void *fdt;
> -    uint32_t start_prop = cpu_to_be32(spapr->initrd_base);
> -    uint32_t end_prop = cpu_to_be32(spapr->initrd_base + spapr->initrd_size);
>      GString *hypertas = g_string_sized_new(256);
>      GString *qemu_hypertas = g_string_sized_new(256);
>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> -    unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
>      char *buf;
>  
>      add_str(hypertas, "hcall-pft");
> @@ -337,35 +333,6 @@ static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
>      _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
>      _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
>  
> -    /* /chosen */
> -    _FDT((fdt_begin_node(fdt, "chosen")));
> -
> -    /* Set Form1_affinity */
> -    _FDT((fdt_property(fdt, "ibm,architecture-vec-5", vec5, sizeof(vec5))));
> -
> -    _FDT((fdt_property_string(fdt, "bootargs", machine->kernel_cmdline)));
> -    _FDT((fdt_property(fdt, "linux,initrd-start",
> -                       &start_prop, sizeof(start_prop))));
> -    _FDT((fdt_property(fdt, "linux,initrd-end",
> -                       &end_prop, sizeof(end_prop))));
> -    if (spapr->kernel_size) {
> -        uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
> -                              cpu_to_be64(spapr->kernel_size) };
> -
> -        _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
> -        if (spapr->kernel_le) {
> -            _FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0)));
> -        }
> -    }
> -    if (boot_menu) {
> -        _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
> -    }
> -    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
> -    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
> -    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
> -
> -    _FDT((fdt_end_node(fdt)));
> -
>      /* RTAS */
>      _FDT((fdt_begin_node(fdt, "rtas")));
>  
> @@ -873,6 +840,68 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
>      return 0;
>  }
>  
> +static void spapr_dt_chosen(sPAPRMachineState *spapr, void *fdt)
> +{
> +    MachineState *machine = MACHINE(spapr);
> +    int chosen;
> +    const char *boot_device = machine->boot_order;
> +    char *stdout = spapr_vio_stdout_path(spapr->vio_bus);

Ah, by the way, calling a variable "stdout" like the FILE* from stdio.h
feels somewhat wrong ... could you maybe use another name here instead?

 Thomas
David Gibson Oct. 21, 2016, 1:41 a.m. UTC | #3 
On Thu, Oct 20, 2016 at 09:49:42AM +0200, Thomas Huth wrote:
> On 20.10.2016 07:12, David Gibson wrote:
> > For historical reasons, building the /chosen node in the guest device tree
> > is split across several places and includes both parts which write the DT
> > sequentially and others which use random access functions.
> > 
> > This patch consolidates construction of the node into one place, using
> > random access functions throughout.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c             | 131 ++++++++++++++++++++++-----------------------
> >  hw/ppc/spapr_vio.c         |  17 ++----
> >  include/hw/ppc/spapr_vio.h |   2 +-
> >  3 files changed, 70 insertions(+), 80 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2faae32..c608ec8 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -273,14 +273,10 @@ static void add_str(GString *s, const gchar *s1)
> >  
> >  static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
> >  {
> > -    MachineState *machine = MACHINE(spapr);
> >      void *fdt;
> > -    uint32_t start_prop = cpu_to_be32(spapr->initrd_base);
> > -    uint32_t end_prop = cpu_to_be32(spapr->initrd_base + spapr->initrd_size);
> >      GString *hypertas = g_string_sized_new(256);
> >      GString *qemu_hypertas = g_string_sized_new(256);
> >      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> > -    unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
> >      char *buf;
> >  
> >      add_str(hypertas, "hcall-pft");
> > @@ -337,35 +333,6 @@ static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
> >      _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
> >      _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
> >  
> > -    /* /chosen */
> > -    _FDT((fdt_begin_node(fdt, "chosen")));
> > -
> > -    /* Set Form1_affinity */
> > -    _FDT((fdt_property(fdt, "ibm,architecture-vec-5", vec5, sizeof(vec5))));
> > -
> > -    _FDT((fdt_property_string(fdt, "bootargs", machine->kernel_cmdline)));
> > -    _FDT((fdt_property(fdt, "linux,initrd-start",
> > -                       &start_prop, sizeof(start_prop))));
> > -    _FDT((fdt_property(fdt, "linux,initrd-end",
> > -                       &end_prop, sizeof(end_prop))));
> > -    if (spapr->kernel_size) {
> > -        uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
> > -                              cpu_to_be64(spapr->kernel_size) };
> > -
> > -        _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
> > -        if (spapr->kernel_le) {
> > -            _FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0)));
> > -        }
> > -    }
> > -    if (boot_menu) {
> > -        _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
> > -    }
> > -    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
> > -    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
> > -    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
> > -
> > -    _FDT((fdt_end_node(fdt)));
> > -
> >      /* RTAS */
> >      _FDT((fdt_begin_node(fdt, "rtas")));
> >  
> > @@ -873,6 +840,68 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
> >      return 0;
> >  }
> >  
> > +static void spapr_dt_chosen(sPAPRMachineState *spapr, void *fdt)
> > +{
> > +    MachineState *machine = MACHINE(spapr);
> > +    int chosen;
> > +    const char *boot_device = machine->boot_order;
> > +    char *stdout = spapr_vio_stdout_path(spapr->vio_bus);
> 
> Ah, by the way, calling a variable "stdout" like the FILE* from stdio.h
> feels somewhat wrong ... could you maybe use another name here instead?

Ah.. a very good idea.  Given the troubles I've seen with variables
called 'index' shadowing the standard library function, calling it
stdout is a terrible idea.
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2faae32..c608ec8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -273,14 +273,10 @@  static void add_str(GString *s, const gchar *s1)
 
 static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
 {
-    MachineState *machine = MACHINE(spapr);
     void *fdt;
-    uint32_t start_prop = cpu_to_be32(spapr->initrd_base);
-    uint32_t end_prop = cpu_to_be32(spapr->initrd_base + spapr->initrd_size);
     GString *hypertas = g_string_sized_new(256);
     GString *qemu_hypertas = g_string_sized_new(256);
     uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
-    unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
     char *buf;
 
     add_str(hypertas, "hcall-pft");
@@ -337,35 +333,6 @@  static void *spapr_create_fdt_skel(sPAPRMachineState *spapr)
     _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
     _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
 
-    /* /chosen */
-    _FDT((fdt_begin_node(fdt, "chosen")));
-
-    /* Set Form1_affinity */
-    _FDT((fdt_property(fdt, "ibm,architecture-vec-5", vec5, sizeof(vec5))));
-
-    _FDT((fdt_property_string(fdt, "bootargs", machine->kernel_cmdline)));
-    _FDT((fdt_property(fdt, "linux,initrd-start",
-                       &start_prop, sizeof(start_prop))));
-    _FDT((fdt_property(fdt, "linux,initrd-end",
-                       &end_prop, sizeof(end_prop))));
-    if (spapr->kernel_size) {
-        uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
-                              cpu_to_be64(spapr->kernel_size) };
-
-        _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
-        if (spapr->kernel_le) {
-            _FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0)));
-        }
-    }
-    if (boot_menu) {
-        _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
-    }
-    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
-    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
-    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
-
-    _FDT((fdt_end_node(fdt)));
-
     /* RTAS */
     _FDT((fdt_begin_node(fdt, "rtas")));
 
@@ -873,6 +840,68 @@  int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
     return 0;
 }
 
+static void spapr_dt_chosen(sPAPRMachineState *spapr, void *fdt)
+{
+    MachineState *machine = MACHINE(spapr);
+    int chosen;
+    const char *boot_device = machine->boot_order;
+    char *stdout = spapr_vio_stdout_path(spapr->vio_bus);
+    size_t cb = 0;
+    char *bootlist = get_boot_devices_list(&cb, true);
+    unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
+
+    _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
+
+    /* Set Form1_affinity */
+    _FDT(fdt_setprop(fdt, chosen, "ibm,architecture-vec-5",
+                     vec5, sizeof(vec5)));
+
+    _FDT(fdt_setprop_string(fdt, chosen, "bootargs", machine->kernel_cmdline));
+    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
+                          spapr->initrd_base));
+    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
+                          spapr->initrd_base + spapr->initrd_size));
+
+    if (spapr->kernel_size) {
+        uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
+                              cpu_to_be64(spapr->kernel_size) };
+
+        _FDT(fdt_setprop(fdt, chosen, "qemu,boot-kernel",
+                         &kprop, sizeof(kprop)));
+        if (spapr->kernel_le) {
+            _FDT(fdt_setprop(fdt, chosen, "qemu,boot-kernel-le", NULL, 0));
+        }
+    }
+    if (boot_menu) {
+        _FDT((fdt_setprop_cell(fdt, chosen, "qemu,boot-menu", boot_menu)));
+    }
+    _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-width", graphic_width));
+    _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-height", graphic_height));
+    _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-depth", graphic_depth));
+
+    if (cb && bootlist) {
+        int i;
+
+        for (i = 0; i < cb; i++) {
+            if (bootlist[i] == '\n') {
+                bootlist[i] = ' ';
+            }
+        }
+        _FDT(fdt_setprop_string(fdt, chosen, "qemu,boot-list", bootlist));
+    }
+
+    if (boot_device && strlen(boot_device)) {
+        _FDT(fdt_setprop_string(fdt, chosen, "qemu,boot-device", boot_device));
+    }
+
+    if (!spapr->has_graphics && stdout) {
+        _FDT(fdt_setprop_string(fdt, chosen, "linux,stdout-path", stdout));
+    }
+
+    g_free(stdout);
+    g_free(bootlist);
+}
+
 static void *spapr_build_fdt(sPAPRMachineState *spapr,
                              hwaddr rtas_addr,
                              hwaddr rtas_size)
@@ -880,10 +909,7 @@  static void *spapr_build_fdt(sPAPRMachineState *spapr,
     MachineState *machine = MACHINE(qdev_get_machine());
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
-    const char *boot_device = machine->boot_order;
-    int ret, i;
-    size_t cb = 0;
-    char *bootlist;
+    int ret;
     void *fdt;
     sPAPRPHBState *phb;
 
@@ -932,34 +958,6 @@  static void *spapr_build_fdt(sPAPRMachineState *spapr,
     /* cpus */
     spapr_populate_cpus_dt_node(fdt, spapr);
 
-    bootlist = get_boot_devices_list(&cb, true);
-    if (cb && bootlist) {
-        int offset = fdt_path_offset(fdt, "/chosen");
-        if (offset < 0) {
-            exit(1);
-        }
-        for (i = 0; i < cb; i++) {
-            if (bootlist[i] == '\n') {
-                bootlist[i] = ' ';
-            }
-
-        }
-        ret = fdt_setprop_string(fdt, offset, "qemu,boot-list", bootlist);
-    }
-
-    if (boot_device && strlen(boot_device)) {
-        int offset = fdt_path_offset(fdt, "/chosen");
-
-        if (offset < 0) {
-            exit(1);
-        }
-        fdt_setprop_string(fdt, offset, "qemu,boot-device", boot_device);
-    }
-
-    if (!spapr->has_graphics) {
-        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
-    }
-
     if (smc->dr_lmb_enabled) {
         _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
     }
@@ -974,7 +972,8 @@  static void *spapr_build_fdt(sPAPRMachineState *spapr,
         }
     }
 
-    g_free(bootlist);
+    /* /chosen */
+    spapr_dt_chosen(spapr, fdt);
 
     /* Build memory reserve map */
     if (spapr->kernel_size) {
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 3648aa5..2b67df0 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -665,28 +665,19 @@  out:
     return ret;
 }
 
-int spapr_populate_chosen_stdout(void *fdt, VIOsPAPRBus *bus)
+gchar *spapr_vio_stdout_path(VIOsPAPRBus *bus)
 {
     VIOsPAPRDevice *dev;
     char *name, *path;
-    int ret, offset;
 
     dev = spapr_vty_get_default(bus);
-    if (!dev)
-        return 0;
-
-    offset = fdt_path_offset(fdt, "/chosen");
-    if (offset < 0) {
-        return offset;
+    if (!dev) {
+        return NULL;
     }
 
     name = spapr_vio_get_dev_name(DEVICE(dev));
     path = g_strdup_printf("/vdevice/%s", name);
 
-    ret = fdt_setprop_string(fdt, offset, "linux,stdout-path", path);
-
     g_free(name);
-    g_free(path);
-
-    return ret;
+    return path;
 }
diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index 0b025fd..a0e7542 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -81,7 +81,7 @@  struct VIOsPAPRBus {
 extern VIOsPAPRBus *spapr_vio_bus_init(void);
 extern VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg);
 extern int spapr_populate_vdevice(VIOsPAPRBus *bus, void *fdt);
-extern int spapr_populate_chosen_stdout(void *fdt, VIOsPAPRBus *bus);
+extern gchar *spapr_vio_stdout_path(VIOsPAPRBus *bus);
 
 static inline qemu_irq spapr_vio_qirq(VIOsPAPRDevice *dev)
 {