From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55226) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byhpc-0005iu-I2 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 12:10:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byhpZ-0002A9-As for qemu-devel@nongnu.org; Mon, 24 Oct 2016 12:10:36 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:45786) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1byhpZ-00029y-05 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 12:10:33 -0400 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u9OGATcv119241 for ; Mon, 24 Oct 2016 12:10:31 -0400 Received: from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151]) by mx0a-001b2d01.pphosted.com with ESMTP id 269jxpwe2v-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 24 Oct 2016 12:10:29 -0400 Received: from localhost by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 24 Oct 2016 10:10:14 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20161024010729.GD19629@umbus.fritz.box> References: <1477018600-6881-1-git-send-email-david@gibson.dropbear.id.au> <1477018600-6881-8-git-send-email-david@gibson.dropbear.id.au> <20161024003359.17113.85655@loki> <20161024010729.GD19629@umbus.fritz.box> Date: Sun, 23 Oct 2016 21:20:19 -0500 Message-Id: <20161024022019.17113.28959@loki> Subject: Re: [Qemu-devel] [PATCHv2 07/12] pseries: Consolidate construction of /chosen device tree node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aik@ozlabs.ru, groug@kaod.org, agraf@suse.de, lvivier@redhat.com, thuth@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 th= e 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 > > > Reviewed-by: Thomas Huth > = > [snip] > > > + _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-width", graphic= _width)); > > > + _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-height", graphi= c_height)); > > > + _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-depth", graphic= _depth)); > > > + > > > + if (cb && bootlist) { > > > + int i; > > > + > > > + for (i =3D 0; i < cb; i++) { > > > + if (bootlist[i] =3D=3D '\n') { > > > + bootlist[i] =3D ' '; > > > + } > > > + } > > > + _FDT(fdt_setprop_string(fdt, chosen, "qemu,boot-list", bootl= ist)); > > > + } > > > + > > > + if (boot_device && strlen(boot_device)) { > > > + _FDT(fdt_setprop_string(fdt, chosen, "qemu,boot-device", boo= t_device)); > > > + } > > > + > > > + if (!spapr->has_graphics && stdout_path) { > > > + _FDT(fdt_setprop_string(fdt, chosen, "linux,stdout-path", st= dout_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 > 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 > Reviewed-by: Thomas Huth Reviewed-by: Michael Roth > --- > 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 =3D MACHINE(spapr); > void *fdt; > - uint32_t start_prop =3D cpu_to_be32(spapr->initrd_base); > - uint32_t end_prop =3D cpu_to_be32(spapr->initrd_base + spapr->initrd= _size); > GString *hypertas =3D g_string_sized_new(256); > GString *qemu_hypertas =3D g_string_sized_new(256); > uint32_t refpoints[] =3D {cpu_to_be32(0x4), cpu_to_be32(0x4)}; > - unsigned char vec5[] =3D {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] =3D { 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 =3D MACHINE(spapr); > + int chosen; > + const char *boot_device =3D machine->boot_order; > + char *stdout_path =3D spapr_vio_stdout_path(spapr->vio_bus); > + size_t cb =3D 0; > + char *bootlist =3D get_boot_devices_list(&cb, true); > + unsigned char vec5[] =3D {0x0, 0x0, 0x0, 0x0, 0x0, 0x80}; > + > + _FDT(chosen =3D 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_cmd= line)); > + _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] =3D { 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_wid= th)); > + _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-height", graphic_he= ight)); > + _FDT(fdt_setprop_cell(fdt, chosen, "qemu,graphic-depth", graphic_dep= th)); > + > + if (cb && bootlist) { > + int i; > + > + for (i =3D 0; i < cb; i++) { > + if (bootlist[i] =3D=3D '\n') { > + bootlist[i] =3D ' '; > + } > + } > + _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_de= vice)); > + } > + > + 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 *spap= r, > MachineState *machine =3D MACHINE(qdev_get_machine()); > MachineClass *mc =3D MACHINE_GET_CLASS(machine); > sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(machine); > - const char *boot_device =3D machine->boot_order; > - int ret, i; > - size_t cb =3D 0; > - char *bootlist; > + int ret; > void *fdt; > sPAPRPHBState *phb; > = > @@ -932,34 +958,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spap= r, > /* cpus */ > spapr_populate_cpus_dt_node(fdt, spapr); > = > - bootlist =3D get_boot_devices_list(&cb, true); > - if (cb && bootlist) { > - int offset =3D fdt_path_offset(fdt, "/chosen"); > - if (offset < 0) { > - exit(1); > - } > - for (i =3D 0; i < cb; i++) { > - if (bootlist[i] =3D=3D '\n') { > - bootlist[i] =3D ' '; > - } > - > - } > - ret =3D fdt_setprop_string(fdt, offset, "qemu,boot-list", bootli= st); > - } > - > - if (boot_device && strlen(boot_device)) { > - int offset =3D 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 =3D spapr_vty_get_default(bus); > - if (!dev) > - return 0; > - > - offset =3D fdt_path_offset(fdt, "/chosen"); > - if (offset < 0) { > - return offset; > + if (!dev) { > + return NULL; > } > = > name =3D spapr_vio_get_dev_name(DEVICE(dev)); > path =3D g_strdup_printf("/vdevice/%s", name); > = > - ret =3D 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_ _othe= r_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson