On Thu, Apr 21, 2016 at 02:04:16PM +1000, Alexey Kardashevskiy wrote: > On 04/20/2016 12:33 PM, David Gibson wrote: > >This starts the process of converting the pseries machine type to use > >the qdt library code to build the guest's device tree instead of > >working directly and awkwardly with the flattened device tree format. > > > >For now we just convert the first section of spapr_build_fdt() which > >creates a tree sequentially, so that it builds the qdt then flattens > >it. This leaves a lot of code which still manipulates the fdt after > >that point, but the intention is to convert those to work with the qdt > >format in future. > > > >Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > 9 patches or 11 look good as they do one right thing at the time. > > But this one does: > 1. replace _FDT() with qdt_setprop_xxx > 2. "consolidates" spapr_events_fdt_skel() (other code movements got own > patches but not this one) > 3. moves bits around like the hyperrtas property composer. > > This all makes it harder to verify that you have not lost anything in > transition...
Yeah, there is a fair bit here; the individual components didn't see
very sensible on their own. I'll take another look on the respin.
> I'd also expect this patch to be the second (right after "qdt:
> IEEE1275-style device tree utility code") or after all "consolidation"
> patches (this would make even more sense), and this patch would convert all
> _FDTs, and then remove _FDT() macro. Or "in future" from the commit log
> means "nearest future"? :)
I wanted to put the intermediate patches first so I didn't need to
port some uglies fromt the old approach to the new before removing
them.
And yes, I want to convert all (or at least, nearly all) the fdt code
in spapr, but I wanted to get some review of the overall concept
before I converted everything. VIO and PCI will both be a bit fiddly.
> btw after all these device tree rendering code gathered in one place - how
> many of the original justification points are still valid? Persistent
> handles or being slow should not be problems anymore (as it is sequential
> device tree rendering, without reordering).
>
>
> >---
> > hw/ppc/spapr.c | 236
> > ++++++++++++++++++++++++-------------------------
> > hw/ppc/spapr_events.c | 19 ----
> > include/hw/ppc/spapr.h | 1 -
> > 3 files changed, 117 insertions(+), 139 deletions(-)
> >
> >diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >index aef44a2..d04d403 100644
> >--- a/hw/ppc/spapr.c
> >+++ b/hw/ppc/spapr.c
> >@@ -66,6 +66,8 @@
> > #include "hw/compat.h"
> > #include "qemu/cutils.h"
> >
> >+#include "qemu/qdt.h"
> >+
> > #include <libfdt.h>
> >
> > /* SLOF memory layout:
> >@@ -710,47 +712,27 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> > char *bootlist;
> > void *fdt;
> > sPAPRPHBState *phb;
> >- 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)};
> >- uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(max_cpus)};
> >- unsigned char vec5[] = {0x0, 0x0, 0x0, 0x0, 0x0, 0x80};
> > char *buf;
> >+ QDTNode *root;
> >
> >- 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 = g_malloc0(FDT_MAX_SIZE);
> >- _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> >+ root = qdt_new_tree();
> >
> >- _FDT((fdt_finish_reservemap(fdt)));
> >+ /* / (root node) */
> >
> >- /* 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")));
> >+ qdt_setprop_string(root, "device_type", "chrp");
> >+ qdt_setprop_string(root, "model", "IBM pSeries (emulated by qemu)");
> >+ qdt_setprop_string(root, "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)));
> >+ qdt_setprop_string(root, "host-model", buf);
> > g_free(buf);
> > }
> > if (kvmppc_get_host_serial(&buf)) {
> >- _FDT((fdt_property_string(fdt, "host-serial", buf)));
> >+ qdt_setprop_string(root, "host-serial", buf);
> > g_free(buf);
> > }
> >
> >@@ -761,138 +743,154 @@ static void *spapr_build_fdt(sPAPRMachineState
> >*spapr,
> > qemu_uuid[11], qemu_uuid[12], qemu_uuid[13],
> > qemu_uuid[14], qemu_uuid[15]);
> >
> >- _FDT((fdt_property_string(fdt, "vm,uuid", buf)));
> >+ qdt_setprop_string(root, "vm,uuid", buf);
> > if (qemu_uuid_set) {
> >- _FDT((fdt_property_string(fdt, "system-id", buf)));
> >+ qdt_setprop_string(root, "system-id", buf);
> > }
> > g_free(buf);
> >
> > if (qemu_get_vm_name()) {
> >- _FDT((fdt_property_string(fdt, "ibm,partition-name",
> >- qemu_get_vm_name())));
> >+ qdt_setprop_string(root, "ibm,partition-name", qemu_get_vm_name());
> > }
> >
> >- _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
> >- _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
> >+ qdt_setprop_cells(root, "#address-cells", 0x2);
> >+ qdt_setprop_cells(root, "#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)));
> >+ {
> >+ QDTNode *chosen = qdt_add_subnode(root, "chosen");
> >+
> >+ /* Set Form1_affinity */
> >+ qdt_setprop_bytes(chosen, "ibm,architecture-vec-5",
> >+ 0x0, 0x0, 0x0, 0x0, 0x0, 0x80);
> >+
> >+ qdt_setprop_string(chosen, "bootargs", machine->kernel_cmdline);
> >+ qdt_setprop_cells(chosen, "linux,initrd-start",
> >+ spapr->initrd_base);
> >+ qdt_setprop_cells(chosen, "linux,initrd-end",
> >+ spapr->initrd_base + spapr->initrd_size);
> >+ if (spapr->kernel_size) {
> >+ qdt_setprop_u64s(chosen, "qemu,boot-kernel",
> >+ KERNEL_LOAD_ADDR, spapr->kernel_size);
> >+ if (spapr->kernel_le) {
> >+ qdt_setprop_empty(chosen, "qemu,boot-kernel-le");
> >+ }
> > }
> >+ if (boot_menu) {
> >+ qdt_setprop_cells(chosen, "qemu,boot-menu", boot_menu);
> >+ }
> >+ qdt_setprop_cells(chosen, "qemu,graphic-width", graphic_width);
> >+ qdt_setprop_cells(chosen, "qemu,graphic-height", graphic_height);
> >+ qdt_setprop_cells(chosen, "qemu,graphic-depth", graphic_depth);
> > }
> >- 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")));
> >+ {
> >+ QDTNode *rtas = qdt_add_subnode(root, "rtas");
> >+ GString *hypertas = g_string_sized_new(256);
> >+ GString *qemu_hypertas = g_string_sized_new(256);
> >
> >- 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);
> >+ 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((fdt_property(fdt, "ibm,associativity-reference-points",
> >- refpoints, sizeof(refpoints))));
> >+ if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> >+ add_str(hypertas, "hcall-multi-tce");
> >+ }
> >+ qdt_setprop(rtas, "ibm,hypertas-functions",
> >+ hypertas->str, hypertas->len);
> >+ g_string_free(hypertas, TRUE);
> >+ qdt_setprop(rtas, "qemu,hypertas-functions",
> >+ qemu_hypertas->str, qemu_hypertas->len);
> >+ g_string_free(qemu_hypertas, TRUE);
> >
> >- _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)));
> >+ qdt_setprop_cells(rtas, "ibm,associativity-reference-points", 0x4,
> >0x4);
> >
> >- if (msi_nonbroken) {
> >- _FDT((fdt_property(fdt, "ibm,change-msix-capable", NULL, 0)));
> >- }
> >+ qdt_setprop_cells(rtas, "rtas-error-log-max", RTAS_ERROR_LOG_MAX);
> >+ qdt_setprop_cells(rtas, "rtas-event-scan-rate",
> >RTAS_EVENT_SCAN_RATE);
> >
> >- /*
> >- * 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)));
> >+ if (msi_nonbroken) {
> >+ qdt_setprop_empty(rtas, "ibm,change-msix-capable");
> >+ }
> >+
> >+ /*
> >+ * 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.
> >+ */
> >+ qdt_setprop_empty(rtas, "ibm,extended-os-term");
> >+ }
> >
> >- _FDT((fdt_end_node(fdt)));
> >+ /* /interrupt controller */
> >+ {
> >+ QDTNode *xics = qdt_add_subnode(root, "interrupt-controller");
> >
> >- /* interrupt controller */
> >- _FDT((fdt_begin_node(fdt, "interrupt-controller")));
> >+ qdt_setprop_string(xics, "device_type",
> >+ "PowerPC-External-Interrupt-Presentation");
> >+ qdt_setprop_string(xics, "compatible", "IBM,ppc-xicp");
> >+ qdt_setprop_empty(xics, "interrupt-controller");
> >+ qdt_setprop_cells(xics, "ibm,interrupt-server-ranges", 0, max_cpus);
> >+ qdt_setprop_cells(xics, "#interrupt-cells", 2);
> >+ qdt_set_phandle(xics, PHANDLE_XICP);
> >+ }
> >
> >- _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)));
> >+ /* /vdevice */
> >+ {
> >+ QDTNode *vdevice = qdt_add_subnode(root, "vdevice");
> >
> >- _FDT((fdt_end_node(fdt)));
> >+ qdt_setprop_string(vdevice, "device_type", "vdevice");
> >+ qdt_setprop_string(vdevice, "compatible", "IBM,vdevice");
> >+ qdt_setprop_cells(vdevice, "#address-cells", 0x1);
> >+ qdt_setprop_cells(vdevice, "#size-cells", 0x0);
> >+ qdt_setprop_cells(vdevice, "#interrupt-cells", 0x2);
> >+ qdt_setprop_empty(vdevice, "interrupt-controller");
> >+ }
> >
> >- /* 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)));
> >+ /* /event-sources */
> >+ {
> >+ QDTNode *event_sources = qdt_add_subnode(root, "event-sources");
> >+ QDTNode *epow;
> >
> >- _FDT((fdt_end_node(fdt)));
> >+ qdt_setprop_empty(event_sources, "interrupt-controller");
> >+ qdt_setprop_cells(event_sources, "#interrupt-cells", 2);
> >+ qdt_setprop_cells(event_sources, "interrupt-ranges",
> >+ spapr->check_exception_irq, 1);
> >
> >- /* event-sources */
> >- spapr_events_fdt_skel(fdt, spapr->check_exception_irq);
> >+ epow = qdt_add_subnode(event_sources, "epow-events");
> >+ qdt_setprop_cells(epow, "interrupts", spapr->check_exception_irq,
> >0);
> >+ }
> >
> >- /* /hypervisor node */
> >+ /* /hypervisor */
> > if (kvm_enabled()) {
> >- uint8_t hypercall[16];
> >+ QDTNode *hypervisor = qdt_add_subnode(root, "hypervisor");
> >
> > /* indicate KVM hypercall interface */
> >- _FDT((fdt_begin_node(fdt, "hypervisor")));
> >- _FDT((fdt_property_string(fdt, "compatible", "linux,kvm")));
> >+ qdt_setprop_string(hypervisor, "compatible", "linux,kvm");
> > if (kvmppc_has_cap_fixup_hcalls()) {
> >+ uint8_t hypercall[16];
> > /*
> > * 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", hypercall,
> >- sizeof(hypercall))));
> >+ qdt_setprop(hypervisor, "hcall-instructions",
> >+ hypercall, sizeof(hypercall));
> > }
> > }
> >- _FDT((fdt_end_node(fdt)));
> > }
> >
> >- _FDT((fdt_end_node(fdt))); /* close root node */
> >- _FDT((fdt_finish(fdt)));
> >+ fdt = qdt_flatten(root, FDT_MAX_SIZE, &error_fatal);
> >
> > /* open out the base tree into a temp buffer for the final tweaks */
> > _FDT((fdt_open_into(fdt, fdt, FDT_MAX_SIZE)));
> >diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> >index 269ab7e..cce219f 100644
> >--- a/hw/ppc/spapr_events.c
> >+++ b/hw/ppc/spapr_events.c
> >@@ -220,25 +220,6 @@ struct hp_log_full {
> > } \
> > } while (0)
> >
> >-void spapr_events_fdt_skel(void *fdt, uint32_t check_exception_irq)
> >-{
> >- uint32_t irq_ranges[] = {cpu_to_be32(check_exception_irq),
> >cpu_to_be32(1)};
> >- uint32_t interrupts[] = {cpu_to_be32(check_exception_irq), 0};
> >-
> >- _FDT((fdt_begin_node(fdt, "event-sources")));
> >-
> >- _FDT((fdt_property(fdt, "interrupt-controller", NULL, 0)));
> >- _FDT((fdt_property_cell(fdt, "#interrupt-cells", 2)));
> >- _FDT((fdt_property(fdt, "interrupt-ranges",
> >- irq_ranges, sizeof(irq_ranges))));
> >-
> >- _FDT((fdt_begin_node(fdt, "epow-events")));
> >- _FDT((fdt_property(fdt, "interrupts", interrupts, sizeof(interrupts))));
> >- _FDT((fdt_end_node(fdt)));
> >-
> >- _FDT((fdt_end_node(fdt)));
> >-}
> >-
> > static void rtas_event_log_queue(int log_type, void *data, bool exception)
> > {
> > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >index ebad34f..40d3724 100644
> >--- a/include/hw/ppc/spapr.h
> >+++ b/include/hw/ppc/spapr.h
> >@@ -560,7 +560,6 @@ struct sPAPREventLogEntry {
> > };
> >
> > void spapr_events_init(sPAPRMachineState *sm);
> >-void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
> > int spapr_h_cas_compose_response(sPAPRMachineState *sm,
> > target_ulong addr, target_ulong size,
> > bool cpu_update, bool memory_update);
> >
>
>
--
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
signature.asc
Description: PGP signature
