Re: [Qemu-ppc] [PATCHv2 07/12] pseries: Consolidate construction of /chosen device tree node

[Michael Roth]

Quoting David Gibson (2016-10-23 20:07:29)
> On Sun, Oct 23, 2016 at 07:33:59PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2016-10-20 21:56:35)
> > > 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 <address@hidden>
> > > Reviewed-by: Thomas Huth <address@hidden>
> 
> [snip]
> > > +    _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_path) {
> > > +        _FDT(fdt_setprop_string(fdt, chosen, "linux,stdout-path", 
> > > stdout_path));
> > > +    }
> > > +
> > > +    g_free(stdout);
> > 
> > Looks like this got left-over from the stdout->stdout_path change.
> 
> Yes indeed.  And the fact that it didn't give me a compile error
> demonstrates exactly why naming it stdout was a terrible idea in the
> first place.
> 
> Please review the revised version below:
> 
> From 899ffee24234c0818d9801b95e26dd47a1675a09 Mon Sep 17 00:00:00 2001
> From: David Gibson <address@hidden>
> Date: Mon, 24 Oct 2016 12:05:57 +1100
> Subject: [PATCH] pseries: Consolidate construction of /chosen device tree node
> 
> 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 <address@hidden>
> Reviewed-by: Thomas Huth <address@hidden>

Reviewed-by: Michael Roth <address@hidden>

> ---
>  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 2989c9d..8e71f1e 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_path = 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_path) {
> +        _FDT(fdt_setprop_string(fdt, chosen, "linux,stdout-path", 
> stdout_path));
> +    }
> +
> +    g_free(stdout_path);
> +    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)
>  {
> -- 
> 2.7.4
> 
> 
> 
> -- 
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson