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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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...

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"? :)


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);
>


-- 
Alexey