The reset handler for DRCs attempts several state transitions which are
subject to various checks and restrictions.  But at reset time we know
there is no guest, so we can ignore most of the usual sequencing rules and
just set the DRC back to a known state.  In fact, it's safer to do so.

The existing code also has several redundant checks for
drc->awaiting_release inside a block which has already tested that.  This
patch removes those and sets the DRC to a fixed initial state based only
on whether a device is currently plugged or not.

With DRCs correctly reset to a state based on device presence, we don't
need to force state transitions as cold plugged devices are processed.
This allows us to remove all the callers of the set_*_state() methods from
outside spapr_drc.c.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c     | 15 ---------------
 hw/ppc/spapr_drc.c | 28 ++++++++--------------------
 2 files changed, 8 insertions(+), 35 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 01dda9e..c988e38 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 
         spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
-        if (!dev->hotplugged) {
-            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-            /* guests expect coldplugged LMBs to be pre-allocated */
-            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
-            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
-        }
     }
     /* send hotplug notification to the
      * guest only in case of hotplugged memory
@@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
          * of hotplugged CPUs.
          */
         spapr_hotplug_req_add_by_index(drc);
-    } else {
-        /*
-         * Set the right DRC states for cold plugged CPU.
-         */
-        if (drc) {
-            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
-            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
-        }
     }
     core_slot->cpu = OBJECT(dev);
 }
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index dc4ac77..7e17f34 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc)
 static void reset(DeviceState *d)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
     trace_spapr_drc_reset(spapr_drc_index(drc));
 
@@ -401,28 +400,17 @@ static void reset(DeviceState *d)
     drc->ccs = NULL;
 
     /* immediately upon reset we can safely assume DRCs whose devices
-     * are pending removal can be safely removed, and that they will
-     * subsequently be left in an ISOLATED state. move the DRC to this
-     * state in these cases (which will in turn complete any pending
-     * device removals)
+     * are pending removal can be safely removed.
      */
     if (drc->awaiting_release) {
-        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED);
-        /* generally this should also finalize the removal, but if the device
-         * hasn't yet been configured we normally defer removal under the
-         * assumption that this transition is taking place as part of device
-         * configuration. so check if we're still waiting after this, and
-         * force removal if we are
-         */
-        if (drc->awaiting_release) {
-            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
-        }
+        spapr_drc_release(drc);
+    }
 
-        /* non-PCI devices may be awaiting a transition to UNUSABLE */
-        if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
-            drc->awaiting_release) {
-            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
-        }
+    drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED
+        : SPAPR_DR_ISOLATION_STATE_ISOLATED;
+    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+        drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE
+            : SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
     }
 }
 
-- 
2.9.4

On Thu,  8 Jun 2017 15:09:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> The reset handler for DRCs attempts several state transitions which are
> subject to various checks and restrictions.  But at reset time we know
> there is no guest, so we can ignore most of the usual sequencing rules and
> just set the DRC back to a known state.  In fact, it's safer to do so.
> 
> The existing code also has several redundant checks for
> drc->awaiting_release inside a block which has already tested that.  This
> patch removes those and sets the DRC to a fixed initial state based only
> on whether a device is currently plugged or not.
> 
> With DRCs correctly reset to a state based on device presence, we don't
> need to force state transitions as cold plugged devices are processed.
> This allows us to remove all the callers of the set_*_state() methods from
> outside spapr_drc.c.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c     | 15 ---------------
>  hw/ppc/spapr_drc.c | 28 ++++++++--------------------
>  2 files changed, 8 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 01dda9e..c988e38 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>  
>          spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
> -        if (!dev->hotplugged) {
> -            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -            /* guests expect coldplugged LMBs to be pre-allocated */
> -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> -            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> -        }
>      }
>      /* send hotplug notification to the
>       * guest only in case of hotplugged memory
> @@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>           * of hotplugged CPUs.
>           */
>          spapr_hotplug_req_add_by_index(drc);
> -    } else {
> -        /*
> -         * Set the right DRC states for cold plugged CPU.
> -         */
> -        if (drc) {
> -            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> -            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> -        }
>      }
>      core_slot->cpu = OBJECT(dev);
>  }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dc4ac77..7e17f34 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc)
>  static void reset(DeviceState *d)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
>      trace_spapr_drc_reset(spapr_drc_index(drc));
>  
> @@ -401,28 +400,17 @@ static void reset(DeviceState *d)
>      drc->ccs = NULL;
>  
>      /* immediately upon reset we can safely assume DRCs whose devices
> -     * are pending removal can be safely removed, and that they will
> -     * subsequently be left in an ISOLATED state. move the DRC to this
> -     * state in these cases (which will in turn complete any pending
> -     * device removals)
> +     * are pending removal can be safely removed.
>       */
>      if (drc->awaiting_release) {
> -        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED);
> -        /* generally this should also finalize the removal, but if the device
> -         * hasn't yet been configured we normally defer removal under the
> -         * assumption that this transition is taking place as part of device
> -         * configuration. so check if we're still waiting after this, and
> -         * force removal if we are
> -         */
> -        if (drc->awaiting_release) {
> -            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> -        }
> +        spapr_drc_release(drc);
> +    }
>  
> -        /* non-PCI devices may be awaiting a transition to UNUSABLE */
> -        if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> -            drc->awaiting_release) {
> -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> -        }
> +    drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED
> +        : SPAPR_DR_ISOLATION_STATE_ISOLATED;
> +    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> +        drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE
> +            : SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
>      }
>  }
>  

On Thu, Jun 08, 2017 at 03:09:28PM +1000, David Gibson wrote:
> The reset handler for DRCs attempts several state transitions which are
> subject to various checks and restrictions.  But at reset time we know
> there is no guest, so we can ignore most of the usual sequencing rules and
> just set the DRC back to a known state.  In fact, it's safer to do so.
> 
> The existing code also has several redundant checks for
> drc->awaiting_release inside a block which has already tested that.  This
> patch removes those and sets the DRC to a fixed initial state based only
> on whether a device is currently plugged or not.
> 
> With DRCs correctly reset to a state based on device presence, we don't
> need to force state transitions as cold plugged devices are processed.
> This allows us to remove all the callers of the set_*_state() methods from
> outside spapr_drc.c.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c     | 15 ---------------
>  hw/ppc/spapr_drc.c | 28 ++++++++--------------------
>  2 files changed, 8 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 01dda9e..c988e38 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> 
>          spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
> -        if (!dev->hotplugged) {
> -            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -            /* guests expect coldplugged LMBs to be pre-allocated */
> -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> -            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> -        }
>      }
>      /* send hotplug notification to the
>       * guest only in case of hotplugged memory
> @@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>           * of hotplugged CPUs.
>           */
>          spapr_hotplug_req_add_by_index(drc);
> -    } else {
> -        /*
> -         * Set the right DRC states for cold plugged CPU.
> -         */
> -        if (drc) {
> -            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> -            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);

So here you are removing the initial state setting for cold plugged CPUs and ...

> -        }
>      }
>      core_slot->cpu = OBJECT(dev);
>  }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index dc4ac77..7e17f34 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc)
>  static void reset(DeviceState *d)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> 
>      trace_spapr_drc_reset(spapr_drc_index(drc));
> 
> @@ -401,28 +400,17 @@ static void reset(DeviceState *d)
>      drc->ccs = NULL;
> 
>      /* immediately upon reset we can safely assume DRCs whose devices
> -     * are pending removal can be safely removed, and that they will
> -     * subsequently be left in an ISOLATED state. move the DRC to this
> -     * state in these cases (which will in turn complete any pending
> -     * device removals)
> +     * are pending removal can be safely removed.
>       */
>      if (drc->awaiting_release) {
> -        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED);
> -        /* generally this should also finalize the removal, but if the device
> -         * hasn't yet been configured we normally defer removal under the
> -         * assumption that this transition is taking place as part of device
> -         * configuration. so check if we're still waiting after this, and
> -         * force removal if we are
> -         */
> -        if (drc->awaiting_release) {
> -            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> -        }
> +        spapr_drc_release(drc);
> +    }
> 
> -        /* non-PCI devices may be awaiting a transition to UNUSABLE */
> -        if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> -            drc->awaiting_release) {
> -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> -        }
> +    drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED
> +        : SPAPR_DR_ISOLATION_STATE_ISOLATED;
> +    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> +        drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE
> +            : SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
>      }

... adding it via reset. However you are setting drc->isolation_state and
drc->allocation_state directly rather than going via
drck->set_isolation_state() and drck->set_allocation_state() routines.
This results in awaiting_allocation not geting cleared for cold plugged
CPUs. So the effect after this commit is that we can't remove the
CPUs that are specified on cmdline using -device.

The following fixes the issue for me:

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd9e07d..da6979a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -450,13 +450,13 @@ static void reset(DeviceState *d)
         /* A device present at reset is coldplugged */
         drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
-            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
+            drc_set_usable(drc);
         }
     } else {
         /* Otherwise device is absent, but might be hotplugged */
         drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
-            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
+            drc_set_unusable(drc);
         }
     }
 }

However I thought this will restore the previous behaviour completely:

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd9e07d..b7726ef 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -433,6 +433,7 @@ static bool release_pending(sPAPRDRConnector *drc)
 static void reset(DeviceState *d)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
+    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
     trace_spapr_drc_reset(spapr_drc_index(drc));
 
@@ -448,15 +449,15 @@ static void reset(DeviceState *d)
 
     if (drc->dev) {
         /* A device present at reset is coldplugged */
-        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
+        drck->unisolate(drc);
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
-            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
+            drc_set_usable(drc);
         }
     } else {
         /* Otherwise device is absent, but might be hotplugged */
-        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
+        drck->isolate(drc);
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
-            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
+            drc_set_unusable(drc);
         }
     }
 }

Regards,
Bharata.

On Tue, Jun 20, 2017 at 01:53:28PM +0530, Bharata B Rao wrote:
> On Thu, Jun 08, 2017 at 03:09:28PM +1000, David Gibson wrote:
> > The reset handler for DRCs attempts several state transitions which are
> > subject to various checks and restrictions.  But at reset time we know
> > there is no guest, so we can ignore most of the usual sequencing rules and
> > just set the DRC back to a known state.  In fact, it's safer to do so.
> > 
> > The existing code also has several redundant checks for
> > drc->awaiting_release inside a block which has already tested that.  This
> > patch removes those and sets the DRC to a fixed initial state based only
> > on whether a device is currently plugged or not.
> > 
> > With DRCs correctly reset to a state based on device presence, we don't
> > need to force state transitions as cold plugged devices are processed.
> > This allows us to remove all the callers of the set_*_state() methods from
> > outside spapr_drc.c.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c     | 15 ---------------
> >  hw/ppc/spapr_drc.c | 28 ++++++++--------------------
> >  2 files changed, 8 insertions(+), 35 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 01dda9e..c988e38 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> > 
> >          spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> >          addr += SPAPR_MEMORY_BLOCK_SIZE;
> > -        if (!dev->hotplugged) {
> > -            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > -            /* guests expect coldplugged LMBs to be pre-allocated */
> > -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > -            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> > -        }
> >      }
> >      /* send hotplug notification to the
> >       * guest only in case of hotplugged memory
> > @@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >           * of hotplugged CPUs.
> >           */
> >          spapr_hotplug_req_add_by_index(drc);
> > -    } else {
> > -        /*
> > -         * Set the right DRC states for cold plugged CPU.
> > -         */
> > -        if (drc) {
> > -            sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> > -            drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> 
> So here you are removing the initial state setting for cold plugged CPUs and ...
> 
> > -        }
> >      }
> >      core_slot->cpu = OBJECT(dev);
> >  }
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index dc4ac77..7e17f34 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc)
> >  static void reset(DeviceState *d)
> >  {
> >      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> > -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > 
> >      trace_spapr_drc_reset(spapr_drc_index(drc));
> > 
> > @@ -401,28 +400,17 @@ static void reset(DeviceState *d)
> >      drc->ccs = NULL;
> > 
> >      /* immediately upon reset we can safely assume DRCs whose devices
> > -     * are pending removal can be safely removed, and that they will
> > -     * subsequently be left in an ISOLATED state. move the DRC to this
> > -     * state in these cases (which will in turn complete any pending
> > -     * device removals)
> > +     * are pending removal can be safely removed.
> >       */
> >      if (drc->awaiting_release) {
> > -        drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED);
> > -        /* generally this should also finalize the removal, but if the device
> > -         * hasn't yet been configured we normally defer removal under the
> > -         * assumption that this transition is taking place as part of device
> > -         * configuration. so check if we're still waiting after this, and
> > -         * force removal if we are
> > -         */
> > -        if (drc->awaiting_release) {
> > -            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > -        }
> > +        spapr_drc_release(drc);
> > +    }
> > 
> > -        /* non-PCI devices may be awaiting a transition to UNUSABLE */
> > -        if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > -            drc->awaiting_release) {
> > -            drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> > -        }
> > +    drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED
> > +        : SPAPR_DR_ISOLATION_STATE_ISOLATED;
> > +    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > +        drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE
> > +            : SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> >      }
> 
> ... adding it via reset. However you are setting drc->isolation_state and
> drc->allocation_state directly rather than going via
> drck->set_isolation_state() and drck->set_allocation_state() routines.

Yes, this is deliberate.  At reset we *should not* be performing the
same checks on transitions that we do at other times.  It is not only
easier, but also safer to set the new state directly.

> This results in awaiting_allocation not geting cleared for cold plugged
> CPUs. So the effect after this commit is that we can't remove the
> CPUs that are specified on cmdline using -device.

Ok, so I should be clearing awaiting_allocation as well (I have
another patch to post soon which gets rid of awaiting_allocation, but
we do want to avoid breaking bisect where possible).

I will fold a change to do that into ppc-for-2.10.

> The following fixes the issue for me:
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fd9e07d..da6979a 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -450,13 +450,13 @@ static void reset(DeviceState *d)
>          /* A device present at reset is coldplugged */
>          drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> +            drc_set_usable(drc);
>          }
>      } else {
>          /* Otherwise device is absent, but might be hotplugged */
>          drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> +            drc_set_unusable(drc);
>          }
>      }
>  }
> 
> However I thought this will restore the previous behaviour completely:
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fd9e07d..b7726ef 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -433,6 +433,7 @@ static bool release_pending(sPAPRDRConnector *drc)
>  static void reset(DeviceState *d)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
>      trace_spapr_drc_reset(spapr_drc_index(drc));
>  
> @@ -448,15 +449,15 @@ static void reset(DeviceState *d)
>  
>      if (drc->dev) {
>          /* A device present at reset is coldplugged */
> -        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> +        drck->unisolate(drc);
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> +            drc_set_usable(drc);
>          }
>      } else {
>          /* Otherwise device is absent, but might be hotplugged */
> -        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> +        drck->isolate(drc);
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -            drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> +            drc_set_unusable(drc);
>          }
>      }
>  }

As noted above using the normal state transition functions misses the
point of this patch.

-- 
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