Re: [Qemu-devel] [RFC PATCH v3 05/24] spapr: Reorganize CPU dt generation code

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Sun, Apr 26, 2015 at 05:17:48PM +0530, Bharata B Rao wrote:
> On Fri, Apr 24, 2015 at 12:17:27PM +0530, Bharata B Rao wrote:
> > Reorganize CPU device tree generation code so that it be reused from
> > hotplug path. CPU dt entries are now generated from spapr_finalize_fdt()
> > instead of spapr_create_fdt_skel().
> 
> Creating CPU DT entries from spapr_finalize_fdt() instead of
> spapr_create_fdt_skel() has an interesting side effect.
> 
> <snip> 
> 
> In both the cases, I am adding CPU DT nodes from QEMU in the same order,
> but not sure why the guest kernel discovers them in different orders in
> each case.

Nikunj and I tracked this down to the difference in device tree APIs that
we are using in two cases.

When CPU DT nodes are created from spapr_create_fdt_skel(), we are using
fdt_begin_node() API which does sequential write and hence CPU DT nodes
end up in the same order in which they are created.

However in my patch when I create CPU DT entries in spapr_finalize_fdt(),
I am using fdt_add_subnode() which ends up writing the CPU DT node at the
same parent offset for all the CPUs. This results in CPU DT nodes being
generated in reverse order in FDT.

> 
> > +static void spapr_populate_cpus_dt_node(void *fdt, sPAPREnvironment *spapr)
> > +{
> > +    CPUState *cs;
> > +    int cpus_offset;
> > +    char *nodename;
> > +    int smt = kvmppc_smt_threads();
> > +
> > +    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> > +    _FDT(cpus_offset);
> > +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
> > +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
> > +
> > +    CPU_FOREACH(cs) {
> > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +        int index = ppc_get_vcpu_dt_id(cpu);
> > +        DeviceClass *dc = DEVICE_GET_CLASS(cs);
> > +        int offset;
> > +
> > +        if ((index % smt) != 0) {
> > +            continue;
> > +        }
> > +
> > +        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
> > +        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> > +        g_free(nodename);
> > +        _FDT(offset);
> > +        spapr_populate_cpu_dt(cs, fdt, offset);
> > +    }
> 
> I can simply fix this by walking the CPUs in reverse order in the above
> code which makes the guest kernel to discover the CPU DT nodes in the
> right order.
> 
> s/CPU_FOREACH(cs)/CPU_FOREACH_REVERSE(cs) will solve this problem. Would this
> be the right approach or should we just leave it to the guest kernel to
> discover and enumerate CPUs in whatever order it finds the DT nodes in FDT ?

So using CPU_FOREACH_REVERSE(cs) appears to be right way to handle this.

Regards,
Bharata.