Re: [Qemu-devel] [PULL 09/17] spapr: Remove unnecessary differences between hotplug and coldplug paths

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

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

On Wed, Jul 12, 2017 at 01:56:37PM +0530, Bharata B Rao wrote:
> On Tue, Jul 11, 2017 at 02:39:09PM +1000, David Gibson wrote:
> > spapr_drc_attach() has a 'coldplug' parameter which sets the DRC into
> > configured state initially, instead of the usual ISOLATED/UNUSABLE state.
> > It turns out this is unnecessary: although coldplugged devices do need to
> > be in CONFIGURED state once the guest starts, that will already be
> > accomplished by the reset code which will move DRCs for already plugged
> > devices into a coldplug equivalent state.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c             | 13 +++----------
> >  hw/ppc/spapr_drc.c         |  5 ++---
> >  hw/ppc/spapr_pci.c         |  3 +--
> >  include/hw/ppc/spapr_drc.h |  2 +-
> >  4 files changed, 7 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 4fa982d..70b3fd3 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2611,7 +2611,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> >          fdt_offset = spapr_populate_memory_node(fdt, node, addr,
> >                                                  SPAPR_MEMORY_BLOCK_SIZE);
> > 
> > -        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> > +        spapr_drc_attach(drc, dev, fdt, fdt_offset, errp);
> >          addr += SPAPR_MEMORY_BLOCK_SIZE;
> >      }
> >      /* send hotplug notification to the
> > @@ -2956,17 +2956,10 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > 
> >      g_assert(drc || !mc->has_hotpluggable_cpus);
> > 
> > -    /*
> > -     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> > -     * coldplugged CPUs DT entries are setup in spapr_build_fdt().
> > -     */
> > -    if (dev->hotplugged) {
> > -        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> > -    }
> > +    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> 
> A harmless (but still good to get rid of) side effect of this change is
> that we are now building the CPU device tree for boot time and cold-plugged
> CPUs twice:
> 
> first from spapr_core_plug() -> spapr_populate_hotplug_cpu_dt()
> second from spapr_build_fdt() -> spapr_populate_cpus_dt_node()
> 
> The first one is not used while the 2nd one acutally ends up building
> the CPUs DT entries for the boot time and cold-plugged CPUs.

Yeah, I know.  I think simpler code is more important than the handful
of wasted cycles.  We might be able to avoid this with some of the DT
generation cleanups I have on the long term radar.

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