Re: [Qemu-devel] [RFC for-2.7 08/11] pseries: Start using qdt library for building device tree

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 18735 bytes --]

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 <david@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]