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

[David Gibson]

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 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 <address@hidden>
> > ---
> >  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?

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

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

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