Re: [Qemu-devel] [RFC PATCH v4 3/5] spapr: Support ibm, dynamic-reconfiguration-memory

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

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

On Wed, Jun 24, 2015 at 07:55:44AM +0530, Bharata B Rao wrote:
> On Tue, Jun 23, 2015 at 11:54:29AM +1000, David Gibson wrote:
> > On Fri, Jun 19, 2015 at 03:47:55PM +0530, Bharata B Rao wrote:
> > > Parse ibm,architecture.vec table obtained from the guest and enable
> > > memory node configuration via ibm,dynamic-reconfiguration-memory if guest
> > > supports it. This is in preparation to support memory hotplug for
> > > sPAPR guests.
> > > 
> > > This changes the way memory node configuration is done. Currently all
> > > memory nodes are built upfront. But after this patch, only memory@0 node
> > > for RMA is built upfront. Guest kernel boots with just that and rest of
> > > the memory nodes (via memory@XXX or ibm,dynamic-reconfiguration-memory)
> > > are built when guest does ibm,client-architecture-support call.
> > > 
> > > Note: This patch needs a SLOF enhancement which is already part of
> > > SLOF binary in QEMU.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > [snip]
> > > +int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
> > > +                                 target_ulong addr, target_ulong size,
> > > +                                 bool cpu_update, bool memory_update)
> > > +{
> > > +    void *fdt, *fdt_skel;
> > > +    sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> > > +
> > > +    size -= sizeof(hdr);
> > > +
> > > +    /* Create sceleton */
> > > +    fdt_skel = g_malloc0(size);
> > > +    _FDT((fdt_create(fdt_skel, size)));
> > > +    _FDT((fdt_begin_node(fdt_skel, "")));
> > > +    _FDT((fdt_end_node(fdt_skel)));
> > > +    _FDT((fdt_finish(fdt_skel)));
> > > +    fdt = g_malloc0(size);
> > > +    _FDT((fdt_open_into(fdt_skel, fdt, size)));
> > > +    g_free(fdt_skel);
> > > +
> > > +    /* Fixup cpu nodes */
> > > +    if (cpu_update) {
> > > +        _FDT((spapr_fixup_cpu_dt(fdt, spapr)));
> > > +    }
> > 
> > The cpu_update parameter seems like its not related to memory hotplug
> > at all.  I'm guessing it relates to CPU hotplug, in which case please
> > defer it until those patches are ready to go.
> 
> This change isn't related to cpu hotplug. Earlier this compose response
> routine only did CPU device tree fixup based on some conditions. I have
> enabled it to check for availability DRCONF_MEMORY feature and accordingly
> fixup memory DT. So this change just checks if cpu fixup is necessary
> or not. Essentially we aren't changing any behaviour wrt cpu dt
> fixup here.

Hm, ok.  Would there be any problem with just unconditionally doing
both fixups?  This is about as far from a hot path as its possible to
get.


> 
> > 
> > > +
> > > +    /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
> > > +    if (memory_update && spapr->dr_lmb_enabled) {
> > > +        _FDT((spapr_populate_drconf_memory(spapr, fdt)));
> > > +    } else {
> > > +        _FDT((spapr_populate_memory(spapr, fdt)));
> > > +    }
> > > +
> > > +    /* Pack resulting tree */
> > > +    _FDT((fdt_pack(fdt)));
> > > +
> > > +    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> > > +        trace_spapr_cas_failed(size);
> > > +        return -1;
> > > +    }
> > > +
> > > +    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> > > +    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> > > +    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> > > +    g_free(fdt);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> > >                                 hwaddr fdt_addr,
> > >                                 hwaddr rtas_addr,
> > > @@ -756,10 +866,16 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
> > >      /* open out the base tree into a temp buffer for the final tweaks */
> > >      _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
> > >  
> > > -    ret = spapr_populate_memory(spapr, fdt);
> > > -    if (ret < 0) {
> > > -        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
> > > -        exit(1);
> > > +    /*
> > > +     * Add memory@0 node to represent RMA. Rest of the memory is either
> > > +     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
> > > +     * node later during ibm,client-architecture-support call.
> > > +     */
> > > +    for (i = 0; i < nb_numa_nodes; ++i) {
> > > +        if (numa_info[i].node_mem) {
> > > +            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> > > +            break;
> > > +        }
> > 
> > ?? The code doesn't seem to match the comment - you appear to be
> > creating a memory@0 node for every NUMA node, not just for the RMA,
> > which doesn't make much sense.
> 
> I have a break there to ensure memory@0 is created only once from the 1st
> memory-less node. I am slightly changing this in next version to ensure
> that this works correctly even when -numa isn't specified.

Ah, sorry, I missed the break;.  That should be ok then.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]