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

[Bharata B Rao]

On Wed, Jun 24, 2015 at 03:55:08PM +1000, David Gibson wrote:
> 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 
> > > > address@hidden node
> > > > for RMA is built upfront. Guest kernel boots with just that and rest of
> > > > the memory nodes (via address@hidden 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 <address@hidden>
> > > 
> > > [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.

Right now I don't fully understand under what circumstances cpu fixup
will be done. What I can deduce is that it is conditionally done from
ibm,client-architecture-support call and I have just ensured that I
retain the same behaviour with this change.

Regards,
Bharata.