Re: [Qemu-devel] [RFC for-2.7 05/11] pseries: Build device tree only at reset time

[Thomas Huth <thuth@redhat.com>]

On 20.04.2016 04:33, David Gibson wrote:
> Currently the pseries code builds a "skeleton" device tree at machine init
> time, then adds a bunch of stuff to it at reset.  Over time, more and more
> 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.
> 
> 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 consolidation
> easier.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c         | 398 ++++++++++++++++++++++++-------------------------
>  include/hw/ppc/spapr.h |   1 -
>  2 files changed, 192 insertions(+), 207 deletions(-)
> 
> 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 *spapr,
>                               hwaddr rtas_addr,
>                               hwaddr rtas_size)
>  {
> -    MachineState *machine = MACHINE(qdev_get_machine());
> +    MachineState *machine = MACHINE(spapr);
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>      const char *boot_device = 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 = 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;
> +
> +    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)));
> +
> +    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);
> +    }
>  
> -    fdt = g_malloc(FDT_MAX_SIZE);
> +    buf = 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(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)));
> +        }
> +    }
> +    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", hypercall,
> +                                   sizeof(hypercall))));
> +            }
> +        }
> +        _FDT((fdt_end_node(fdt)));
> +    }
> +
> +    _FDT((fdt_end_node(fdt))); /* close root node */
> +    _FDT((fdt_finish(fdt)));

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 node?

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

Can fdt_open_into deal with input = 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?

>      ret = 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);
>  
> -    /* Prepare the device tree */
> -    spapr->fdt_skel = spapr_create_fdt_skel(spapr);
> -    assert(spapr->fdt_skel != 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;

 Thomas