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);
if (drc) {
- spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged,
- &local_err);
+ spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
if (local_err) {
g_free(fdt);
error_propagate(errp, local_err);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index aa37a47..f34355d 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -340,7 +340,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
}
void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
- int fdt_start_offset, bool coldplug, Error **errp)
+ int fdt_start_offset, Error **errp)
{
trace_spapr_drc_attach(spapr_drc_index(drc));
@@ -351,12 +351,11 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
}
- g_assert(fdt || coldplug);
+ g_assert(fdt);
drc->dev = d;
drc->fdt = fdt;
drc->fdt_start_offset = fdt_start_offset;
- drc->configured = coldplug;
if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->awaiting_allocation = true;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f09b4e1..49c8db8 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1435,8 +1435,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
goto out;
}
- spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
- !plugged_dev->hotplugged, &local_err);
+ spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset, &local_err);
if (local_err) {
goto out;
}
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 6fd84d1..d15e9eb 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -234,7 +234,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
uint32_t drc_type_mask);
void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
- int fdt_start_offset, bool coldplug, Error **errp);
+ int fdt_start_offset, Error **errp);
void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
#endif /* HW_SPAPR_DRC_H */
--
2.9.4
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.
Regards,
Bharata.
On Wed, 12 Jul 2017 13:56:37 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> 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
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.
>
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
© 2016 - 2025 Red Hat, Inc.