From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avIdF-00064c-96 for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:07:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avIdD-0002vd-Ew for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:07:29 -0400 Date: Wed, 27 Apr 2016 16:07:51 +1000 From: David Gibson Message-ID: <20160427060751.GC18476@voom.redhat.com> References: <1461119601-4936-1-git-send-email-david@gibson.dropbear.id.au> <1461119601-4936-6-git-send-email-david@gibson.dropbear.id.au> <571FAFC1.7030603@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7gGkHNMELEOhSGF6" Content-Disposition: inline In-Reply-To: <571FAFC1.7030603@redhat.com> Subject: Re: [Qemu-devel] [RFC for-2.7 05/11] pseries: Build device tree only at reset time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: agraf@suse.de, crosthwaite.peter@gmail.com, aik@ozlabs.ru, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --7gGkHNMELEOhSGF6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Apr 26, 2016 at 08:13:21PM +0200, Thomas Huth wrote: > On 20.04.2016 04:33, David Gibson wrote: > > Currently the pseries code builds a "skeleton" device tree at machine i= nit > > time, then adds a bunch of stuff to it at reset. Over time, more and m= ore > > logic has had to be moved from init to reset time, and there's really no > > advantage to doing any of it at init time. > >=20 > > This patch removes the init time spapr_create_fdt_skel() and moves its > > logic into the reset time spapr_build_fdt(). There's still a fairly > > pointless divide between the "skeleton" logic (using libfdt serial-write > > functions) and the "tweak" logic (using libfdt random access functions) > > but at least it all happens at the same time, making further consolidat= ion > > easier. > >=20 > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 398 ++++++++++++++++++++++++-----------------= -------- > > include/hw/ppc/spapr.h | 1 - > > 2 files changed, 192 insertions(+), 207 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index da10136..6e1192f 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > ... > > @@ -901,7 +702,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spa= pr, > > hwaddr rtas_addr, > > hwaddr rtas_size) > > { > > - MachineState *machine =3D MACHINE(qdev_get_machine()); > > + MachineState *machine =3D MACHINE(spapr); > > sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(machine); > > const char *boot_device =3D machine->boot_order; > > int ret, i; > > @@ -909,11 +710,200 @@ static void *spapr_build_fdt(sPAPRMachineState *= spapr, > > char *bootlist; > > void *fdt; > > sPAPRPHBState *phb; > > + uint32_t start_prop =3D cpu_to_be32(spapr->initrd_base); > > + uint32_t end_prop =3D cpu_to_be32(spapr->initrd_base + spapr->init= rd_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)}; > > + uint32_t interrupt_server_ranges_prop[] =3D {0, cpu_to_be32(max_cp= us)}; > > + unsigned char vec5[] =3D {0x0, 0x0, 0x0, 0x0, 0x0, 0x80}; > > + char *buf; > > + > > + add_str(hypertas, "hcall-pft"); > > + add_str(hypertas, "hcall-term"); > > + add_str(hypertas, "hcall-dabr"); > > + add_str(hypertas, "hcall-interrupt"); > > + add_str(hypertas, "hcall-tce"); > > + add_str(hypertas, "hcall-vio"); > > + add_str(hypertas, "hcall-splpar"); > > + add_str(hypertas, "hcall-bulk"); > > + add_str(hypertas, "hcall-set-mode"); > > + add_str(qemu_hypertas, "hcall-memop1"); > > + > > + fdt =3D g_malloc0(FDT_MAX_SIZE); > > + _FDT((fdt_create(fdt, FDT_MAX_SIZE))); > > + > > + if (spapr->kernel_size) { > > + _FDT((fdt_add_reservemap_entry(fdt, KERNEL_LOAD_ADDR, > > + spapr->kernel_size))); > > + } > > + if (spapr->initrd_size) { > > + _FDT((fdt_add_reservemap_entry(fdt, spapr->initrd_base, > > + spapr->initrd_size))); > > + } > > + _FDT((fdt_finish_reservemap(fdt))); > > + > > + /* Root node */ > > + _FDT((fdt_begin_node(fdt, ""))); > > + _FDT((fdt_property_string(fdt, "device_type", "chrp"))); > > + _FDT((fdt_property_string(fdt, "model", "IBM pSeries (emulated by = qemu)"))); > > + _FDT((fdt_property_string(fdt, "compatible", "qemu,pseries"))); > > + > > + /* > > + * Add info to guest to indentify which host is it being run on > > + * and what is the uuid of the guest > > + */ > > + if (kvmppc_get_host_model(&buf)) { > > + _FDT((fdt_property_string(fdt, "host-model", buf))); > > + g_free(buf); > > + } > > + if (kvmppc_get_host_serial(&buf)) { > > + _FDT((fdt_property_string(fdt, "host-serial", buf))); > > + g_free(buf); > > + } > > =20 > > - fdt =3D g_malloc(FDT_MAX_SIZE); > > + buf =3D g_strdup_printf(UUID_FMT, qemu_uuid[0], qemu_uuid[1], > > + qemu_uuid[2], qemu_uuid[3], qemu_uuid[4], > > + qemu_uuid[5], qemu_uuid[6], qemu_uuid[7], > > + qemu_uuid[8], qemu_uuid[9], qemu_uuid[10], > > + qemu_uuid[11], qemu_uuid[12], qemu_uuid[13], > > + qemu_uuid[14], qemu_uuid[15]); > > + > > + _FDT((fdt_property_string(fdt, "vm,uuid", buf))); > > + if (qemu_uuid_set) { > > + _FDT((fdt_property_string(fdt, "system-id", buf))); > > + } > > + g_free(buf); > > + > > + if (qemu_get_vm_name()) { > > + _FDT((fdt_property_string(fdt, "ibm,partition-name", > > + qemu_get_vm_name()))); > > + } > > + > > + _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(vec= 5)))); > > + > > + _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(kpr= op)))); > > + 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"))); > > + > > + if (!kvm_enabled() || kvmppc_spapr_use_multitce()) { > > + add_str(hypertas, "hcall-multi-tce"); > > + } > > + _FDT((fdt_property(fdt, "ibm,hypertas-functions", hypertas->str, > > + hypertas->len))); > > + g_string_free(hypertas, TRUE); > > + _FDT((fdt_property(fdt, "qemu,hypertas-functions", qemu_hypertas->= str, > > + qemu_hypertas->len))); > > + g_string_free(qemu_hypertas, TRUE); > > + > > + _FDT((fdt_property(fdt, "ibm,associativity-reference-points", > > + refpoints, sizeof(refpoints)))); > > + > > + _FDT((fdt_property_cell(fdt, "rtas-error-log-max", RTAS_ERROR_LOG_= MAX))); > > + _FDT((fdt_property_cell(fdt, "rtas-event-scan-rate", > > + RTAS_EVENT_SCAN_RATE))); > > + > > + if (msi_nonbroken) { > > + _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0))); > > + } > > + > > + /* > > + * According to PAPR, rtas ibm,os-term does not guarantee a return > > + * back to the guest cpu. > > + * > > + * While an additional ibm,extended-os-term property indicates that > > + * rtas call return will always occur. Set this property. > > + */ > > + _FDT((fdt_property(fdt, "ibm,extended-os-term", NULL, 0))); > > + > > + _FDT((fdt_end_node(fdt))); > > + > > + /* interrupt controller */ > > + _FDT((fdt_begin_node(fdt, "interrupt-controller"))); > > + > > + _FDT((fdt_property_string(fdt, "device_type", > > + "PowerPC-External-Interrupt-Presentation= "))); > > + _FDT((fdt_property_string(fdt, "compatible", "IBM,ppc-xicp"))); > > + _FDT((fdt_property(fdt, "interrupt-controller", NULL, 0))); > > + _FDT((fdt_property(fdt, "ibm,interrupt-server-ranges", > > + interrupt_server_ranges_prop, > > + sizeof(interrupt_server_ranges_prop)))); > > + _FDT((fdt_property_cell(fdt, "#interrupt-cells", 2))); > > + _FDT((fdt_property_cell(fdt, "linux,phandle", PHANDLE_XICP))); > > + _FDT((fdt_property_cell(fdt, "phandle", PHANDLE_XICP))); > > + > > + _FDT((fdt_end_node(fdt))); > > + > > + /* vdevice */ > > + _FDT((fdt_begin_node(fdt, "vdevice"))); > > + > > + _FDT((fdt_property_string(fdt, "device_type", "vdevice"))); > > + _FDT((fdt_property_string(fdt, "compatible", "IBM,vdevice"))); > > + _FDT((fdt_property_cell(fdt, "#address-cells", 0x1))); > > + _FDT((fdt_property_cell(fdt, "#size-cells", 0x0))); > > + _FDT((fdt_property_cell(fdt, "#interrupt-cells", 0x2))); > > + _FDT((fdt_property(fdt, "interrupt-controller", NULL, 0))); > > + > > + _FDT((fdt_end_node(fdt))); > > + > > + /* event-sources */ > > + spapr_events_fdt_skel(fdt, spapr->check_exception_irq); > > + > > + /* /hypervisor node */ > > + if (kvm_enabled()) { > > + uint8_t hypercall[16]; > > + > > + /* indicate KVM hypercall interface */ > > + _FDT((fdt_begin_node(fdt, "hypervisor"))); > > + _FDT((fdt_property_string(fdt, "compatible", "linux,kvm"))); > > + if (kvmppc_has_cap_fixup_hcalls()) { > > + /* > > + * Older KVM versions with older guest kernels were broken= with the > > + * magic page, don't allow the guest to map it. > > + */ > > + if (!kvmppc_get_hypercall(first_cpu->env_ptr, hypercall, > > + sizeof(hypercall))) { > > + _FDT((fdt_property(fdt, "hcall-instructions", hypercal= l, > > + sizeof(hypercall)))); > > + } > > + } > > + _FDT((fdt_end_node(fdt))); > > + } > > + > > + _FDT((fdt_end_node(fdt))); /* close root node */ > > + _FDT((fdt_finish(fdt))); >=20 > The old spapr_finalize_fdt() was already quite big - if you now move all > this code hiere, this function becomes a really bloated. So while you're > reworking all this stuff ... maybe it would be nicer to split the big > chunk into separate functions, e.g. one function for each device tree nod= e? Perhaps - it's all in sequence code, though, so I'm not particularly fussed by it being long. It will also shrink a bit as we fold together the fdt and qdt code. > > /* open out the base tree into a temp buffer for the final tweaks = */ > > - _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE))); > > + _FDT((fdt_open_into(fdt, fdt, FDT_MAX_SIZE))); >=20 > Can fdt_open_into deal with input =3D output pointer? I haven't checked > it, but that looks somewhat strange. Is this line still required at all > after you moved to code here? Yes, it absolutely can. Looks like I never got around to writing a doc block for fdt_open_into() but it was always designed to handle both the expanding in place and expanding to elsewhere cases. > > ret =3D spapr_populate_memory(spapr, fdt); > > if (ret < 0) { > > @@ -1980,10 +1970,6 @@ static void ppc_spapr_init(MachineState *machine) > > register_savevm_live(NULL, "spapr/htab", -1, 1, > > &savevm_htab_handlers, spapr); > > =20 > > - /* Prepare the device tree */ > > - spapr->fdt_skel =3D spapr_create_fdt_skel(spapr); > > - assert(spapr->fdt_skel !=3D NULL); > > - > > /* used by RTAS */ > > QTAILQ_INIT(&spapr->ccs_list); > > qemu_register_reset(spapr_ccs_reset_hook, spapr); > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 88f29a8..cd72586 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -62,7 +62,6 @@ struct sPAPRMachineState { > > bool kernel_le; > > uint32_t initrd_base; > > long initrd_size; > > - void *fdt_skel; > > uint64_t rtc_offset; /* Now used only during incoming migration */ > > struct PPCTimebase tb; > > bool has_graphics; >=20 > Thomas >=20 --=20 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 --7gGkHNMELEOhSGF6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXIFc3AAoJEGw4ysog2bOSDfUP/iu64NresWk4AZvgE8PJgrss ySv9oRJ8VLi2ZBnTS0Rlw1dobZeqhotYwhZJEdz2TJmqFI26Ckf+RJZg/fROuC9T SLeO7Yd+/o/sWBRR2TVMisogfvNDZAwbbFyp8u3IsNAAMgnn2sTa1gLKKJGdTvAv NPWU66ow0fsOyDBZ87HhqJhFTUEHETQzGpJCEK9bfD++Q5RElcXctlIy+WMJ9oCS TfjbAOclmrJZTsCXHsV2kUMN92DNq0nvjTXe/8pQu8DFQbdhazshSS4ZUiROkuzD Uduo8hab7ONYQ/X9WePVnSMhTGLLtmuEI3KyN+HT7seYUUthChOBrwkr29UirTbS tJ32saQCBwLvD2Afakvr67J92DJ5wref777Em3sVpaeH5syk4nyebbQBhLJPFulK /mTgTSf4EHSifb6Bd9s1XZxgzuSOngaZ/Ow7ln/36HGpLq0dCq5ulImyVNxrP698 QzR/5U/H8W0mRI4p4n6xa2xr+ZqSp/7guL75/gVAvjsYPjqKpszM4ibErbUofqho oSb158YzCDnHXbd2QU4o7MhA8T5oqIUTaN2hSwhtgoq4bJz47jNQyqyebPAJUdvq Jz1bpDvPWk6IaLFhdnYAZrEYZYgpeq126DnpKKYPjYVdtwg3046HSkK1UpDSpFoN qZ+2ZCYw+Exzd/oFG7nb =o/e4 -----END PGP SIGNATURE----- --7gGkHNMELEOhSGF6--