[4/6] spapr: Make DRC reset force DRC into known state

Commit Message

Bharata B Rao June 20, 2017, 8:23 a.m. UTC

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 <[e-mail]>
> ---
>  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:


Regards,
Bharata.

Comments

David Gibson June 21, 2017, 7:24 a.m. UTC | #1

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 <[e-mail]>
> > ---
> >  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.

Patch

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);
         }
     }
 }