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

[Greg Kurz]

On Wed, 12 Jul 2017 13:56:37 +0530
Bharata B Rao <address@hidden> 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 <address@hidden>
> > Reviewed-by: Laurent Vivier <address@hidden>
> > Reviewed-by: Greg Kurz <address@hidden>
> > ---
> >  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

Well, not that harmless... this is leaked for machine types that don't
support CPU hotplug. I'll send a patch.

> 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.
> 
> Regards,
> Bharata.
>

pgp1SEnNhdvDa.pgp
Description: OpenPGP digital signature