[Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[David Gibson]

This fourth isntallment of cleanups to the DRC code introduces the
first changes to the fundamental state handling.  We change the
initial states in the reset code and attach code for PCI devices, and
are able to remove the 'signalled' state variable with those fixes.

There are also some more mechanical cleanups in preparation for
further cleanups and fixes to the state management.

David Gibson (6):
  spapr: Start hotplugged PCI devices in ISOLATED state
  spapr: Eliminate DRC 'signalled' state variable
  spapr: Split DRC release from DRC detach
  spapr: Make DRC reset force DRC into known state
  spapr: Clean up DRC set_allocation_state path
  spapr: Clean up DRC set_isolation_state() path

 hw/ppc/spapr.c             |  15 --
 hw/ppc/spapr_drc.c         | 363 +++++++++++++++++++++++----------------------
 hw/ppc/spapr_events.c      |  10 --
 include/hw/ppc/spapr_drc.h |  10 +-
 4 files changed, 188 insertions(+), 210 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/6] spapr: Start hotplugged PCI devices in ISOLATED state

[David Gibson]

PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation
state once the device is attached.  This has been there from the initial
implementation, and it's not clear why.

The state diagram in PAPR 13.4 suggests PCI devices should start in
ISOLATED state until the guest moves them into UNISOLATED, and the code in
the guest-side drmgr tool seems to work that way too.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_drc.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 15ef67d..6186f79 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -315,16 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     }
     g_assert(fdt || coldplug);
 
-    /* NOTE: setting initial isolation state to UNISOLATED means we can't
-     * detach unless guest has a userspace/kernel that moves this state
-     * back to ISOLATED in response to an unplug event, or this is done
-     * manually by the admin prior. if we force things while the guest
-     * may be accessing the device, we can easily crash the guest, so we
-     * we defer completion of removal in such cases to the reset() hook.
-     */
-    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
-        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
-    }
     drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
 
     drc->dev = d;
-- 
2.9.4

[Qemu-devel] [PATCH 2/6] spapr: Eliminate DRC 'signalled' state variable

[David Gibson]

The 'signalled' field in the DRC appears to be entirely a torturous
workaround for the fact that PCI devices were started in UNISOLATED state
for unclear reasons.

1) 'signalled' is already meaningless for logical (so far, all non PCI)
DRCs.  It's always set to true (at least at any point it might be tested),
and can't be assigned any real meaning due to the way signalling works for
logical DRCs.

2) For PCI DRCs, the only time signalled would be false is when non-zero
functions of a multifunction device are hotplugged, followed by function
zero (the other way around is explicitly not permitted). In that case the
secondary function DRCs are attached, but the notification isn't sent to
the guest until function 0 is plugged.

3) signalled being false is used to allow a DRC detach to switch mode
back to ISOLATED state, which allows a secondary function to be hotplugged
then unplugged with function 0 never inserted.  Without this a secondary
function starting in UNISOLATED state couldn't be detached again without
function 0 being inserted, all the functions configured by the guest, then
sent back to ISOLATED state.

4) But now that PCI DRCs start in ISOLATED state, there's nothing to be
done.  If the guest doesn't get the notification, it won't switch the
device to UNISOLATED state, so nothing prevents it from being unplugged.
If the guest does move it to UNISOLATED state without the signal (due to
a manual drmgr call, for instance) then it really isn't safe to unplug it.


So, this patch removes the signalled variable and all code related to it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 45 +--------------------------------------------
 hw/ppc/spapr_events.c      | 10 ----------
 include/hw/ppc/spapr_drc.h |  2 --
 3 files changed, 1 insertion(+), 56 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 6186f79..afd68f4 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -183,12 +183,6 @@ static const char *spapr_drc_name(sPAPRDRConnector *drc)
     return g_strdup_printf("%s%d", drck->drc_name_prefix, drc->id);
 }
 
-/* has the guest been notified of device attachment? */
-static void set_signalled(sPAPRDRConnector *drc)
-{
-    drc->signalled = true;
-}
-
 /*
  * dr-entity-sense sensor value
  * returned via get-sensor-state RTAS calls
@@ -321,17 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     drc->fdt = fdt;
     drc->fdt_start_offset = fdt_start_offset;
     drc->configured = coldplug;
-    /* 'logical' DR resources such as memory/cpus are in some cases treated
-     * as a pool of resources from which the guest is free to choose from
-     * based on only a count. for resources that can be assigned in this
-     * fashion, we must assume the resource is signalled immediately
-     * since a single hotplug request might make an arbitrary number of
-     * such attached resources available to the guest, as opposed to
-     * 'physical' DR resources such as PCI where each device/resource is
-     * signalled individually.
-     */
-    drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
-                     ? true : coldplug;
 
     if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
         drc->awaiting_allocation = true;
@@ -347,26 +330,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
 {
     trace_spapr_drc_detach(spapr_drc_index(drc));
 
-    /* if we've signalled device presence to the guest, or if the guest
-     * has gone ahead and configured the device (via manually-executed
-     * device add via drmgr in guest, namely), we need to wait
-     * for the guest to quiesce the device before completing detach.
-     * Otherwise, we can assume the guest hasn't seen it and complete the
-     * detach immediately. Note that there is a small race window
-     * just before, or during, configuration, which is this context
-     * refers mainly to fetching the device tree via RTAS.
-     * During this window the device access will be arbitrated by
-     * associated DRC, which will simply fail the RTAS calls as invalid.
-     * This is recoverable within guest and current implementations of
-     * drmgr should be able to cope.
-     */
-    if (!drc->signalled && !drc->configured) {
-        /* if the guest hasn't seen the device we can't rely on it to
-         * set it back to an isolated state via RTAS, so do it here manually
-         */
-        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
-    }
-
     if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
         trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
         drc->awaiting_release = true;
@@ -455,10 +418,6 @@ static void reset(DeviceState *d)
             drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
         }
     }
-
-    if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
-        drck->set_signalled(drc);
-    }
 }
 
 static bool spapr_drc_needed(void *opaque)
@@ -483,7 +442,7 @@ static bool spapr_drc_needed(void *opaque)
     case SPAPR_DR_CONNECTOR_TYPE_LMB:
         rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
                (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
-               drc->configured && drc->signalled && !drc->awaiting_release);
+               drc->configured && !drc->awaiting_release);
         break;
     case SPAPR_DR_CONNECTOR_TYPE_PHB:
     case SPAPR_DR_CONNECTOR_TYPE_VIO:
@@ -505,7 +464,6 @@ static const VMStateDescription vmstate_spapr_drc = {
         VMSTATE_BOOL(configured, sPAPRDRConnector),
         VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
         VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
-        VMSTATE_BOOL(signalled, sPAPRDRConnector),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -603,7 +561,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     drck->set_isolation_state = set_isolation_state;
     drck->set_allocation_state = set_allocation_state;
     drck->release_pending = release_pending;
-    drck->set_signalled = set_signalled;
     /*
      * Reason: it crashes FIXME find and document the real reason
      */
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 171aedc..587a3da 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -475,13 +475,6 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
                                                        RTAS_LOG_TYPE_EPOW)));
 }
 
-static void spapr_hotplug_set_signalled(uint32_t drc_index)
-{
-    sPAPRDRConnector *drc = spapr_drc_by_index(drc_index);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    drck->set_signalled(drc);
-}
-
 static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
                                     sPAPRDRConnectorType drc_type,
                                     union drc_identifier *drc_id)
@@ -528,9 +521,6 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
     switch (drc_type) {
     case SPAPR_DR_CONNECTOR_TYPE_PCI:
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
-        if (hp->hotplug_action == RTAS_LOG_V6_HP_ACTION_ADD) {
-            spapr_hotplug_set_signalled(drc_id->index);
-        }
         break;
     case SPAPR_DR_CONNECTOR_TYPE_LMB:
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index c487123..f24188d 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -199,7 +199,6 @@ typedef struct sPAPRDRConnector {
     sPAPRConfigureConnectorState *ccs;
 
     bool awaiting_release;
-    bool signalled;
     bool awaiting_allocation;
     bool awaiting_allocation_skippable;
 
@@ -226,7 +225,6 @@ typedef struct sPAPRDRConnectorClass {
 
     /* QEMU interfaces for managing hotplug operations */
     bool (*release_pending)(sPAPRDRConnector *drc);
-    void (*set_signalled)(sPAPRDRConnector *drc);
 } sPAPRDRConnectorClass;
 
 uint32_t spapr_drc_index(sPAPRDRConnector *drc);
-- 
2.9.4

[Qemu-devel] [PATCH 3/6] spapr: Split DRC release from DRC detach

[David Gibson]

spapr_drc_detach() is called when qemu generic code requests a device be
unplugged.  It makes a number of tests, which could well delay further
action until later, before actually detach the device from the DRC.

This splits out the part which actually removes the device from the DRC
into spapr_drc_release().  This will be useful for further cleanups.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c | 54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index afd68f4..dc4ac77 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -326,31 +326,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
                              NULL, 0, NULL);
 }
 
-void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
-{
-    trace_spapr_drc_detach(spapr_drc_index(drc));
-
-    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-        trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
-        drc->awaiting_release = true;
-        return;
-    }
-
-    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
-        drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-        trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
-        drc->awaiting_release = true;
-        return;
-    }
-
-    if (drc->awaiting_allocation) {
-        if (!drc->awaiting_allocation_skippable) {
-            drc->awaiting_release = true;
-            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
-            return;
-        }
-    }
 
+static void spapr_drc_release(sPAPRDRConnector *drc)
+{
     drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
 
     /* Calling release callbacks based on spapr_drc_type(drc). */
@@ -379,6 +357,34 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
     drc->dev = NULL;
 }
 
+void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
+{
+    trace_spapr_drc_detach(spapr_drc_index(drc));
+
+    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
+        trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
+        drc->awaiting_release = true;
+        return;
+    }
+
+    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+        drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
+        trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
+        drc->awaiting_release = true;
+        return;
+    }
+
+    if (drc->awaiting_allocation) {
+        if (!drc->awaiting_allocation_skippable) {
+            drc->awaiting_release = true;
+            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
+            return;
+        }
+    }
+
+    spapr_drc_release(drc);
+}
+
 static bool release_pending(sPAPRDRConnector *drc)
 {
     return drc->awaiting_release;
-- 
2.9.4

[Qemu-devel] [PATCH 4/6] spapr: Make DRC reset force DRC into known state

[David Gibson]

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

[Qemu-devel] [PATCH 5/6] spapr: Clean up DRC set_allocation_state path

[David Gibson]

The allocation-state indicator should only actually be implemented for
"logical" DRCs, not physical ones.  Factor a check for this, and also for
valid indicator state values into rtas_set_allocation_state().  Because
they don't exist for physical DRCs, there's no reason that we'd ever want
more than one method implementation, so it can just be a plain function.

In addition, the setting to USABLE and setting to UNUSABLE paths in
set_allocation_state() don't actually have much in common.  So, split the
method separate functions for each parameter value (drc_set_usable()
and drc_set_unusable()).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 85 +++++++++++++++++++++++++---------------------
 include/hw/ppc/spapr_drc.h |  2 --
 2 files changed, 46 insertions(+), 41 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 7e17f34..9e01d7b 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -114,44 +114,43 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
     return RTAS_OUT_SUCCESS;
 }
 
-static uint32_t set_allocation_state(sPAPRDRConnector *drc,
-                                     sPAPRDRAllocationState state)
+static uint32_t drc_set_usable(sPAPRDRConnector *drc)
 {
-    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
-
-    if (state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
-        /* if there's no resource/device associated with the DRC, there's
-         * no way for us to put it in an allocation state consistent with
-         * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
-         * result in an RTAS return code of -3 / "no such indicator"
+    /* if there's no resource/device associated with the DRC, there's
+     * no way for us to put it in an allocation state consistent with
+     * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
+     * result in an RTAS return code of -3 / "no such indicator"
+     */
+    if (!drc->dev) {
+        return RTAS_OUT_NO_SUCH_INDICATOR;
+    }
+    if (drc->awaiting_release && drc->awaiting_allocation) {
+        /* kernel is acknowledging a previous hotplug event
+         * while we are already removing it.
+         * it's safe to ignore awaiting_allocation here since we know the
+         * situation is predicated on the guest either already having done
+         * so (boot-time hotplug), or never being able to acquire in the
+         * first place (hotplug followed by immediate unplug).
          */
-        if (!drc->dev) {
-            return RTAS_OUT_NO_SUCH_INDICATOR;
-        }
-        if (drc->awaiting_release && drc->awaiting_allocation) {
-            /* kernel is acknowledging a previous hotplug event
-             * while we are already removing it.
-             * it's safe to ignore awaiting_allocation here since we know the
-             * situation is predicated on the guest either already having done
-             * so (boot-time hotplug), or never being able to acquire in the
-             * first place (hotplug followed by immediate unplug).
-             */
-            drc->awaiting_allocation_skippable = true;
-            return RTAS_OUT_NO_SUCH_INDICATOR;
-        }
+        drc->awaiting_allocation_skippable = true;
+        return RTAS_OUT_NO_SUCH_INDICATOR;
     }
 
-    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
-        drc->allocation_state = state;
-        if (drc->awaiting_release &&
-            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-            uint32_t drc_index = spapr_drc_index(drc);
-            trace_spapr_drc_set_allocation_state_finalizing(drc_index);
-            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
-        } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
-            drc->awaiting_allocation = false;
-        }
+    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
+    drc->awaiting_allocation = false;
+
+    return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
+{
+    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
+    if (drc->awaiting_release) {
+        uint32_t drc_index = spapr_drc_index(drc);
+        trace_spapr_drc_set_allocation_state_finalizing(drc_index);
+        spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
     }
+
     return RTAS_OUT_SUCCESS;
 }
 
@@ -553,7 +552,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     dk->realize = realize;
     dk->unrealize = unrealize;
     drck->set_isolation_state = set_isolation_state;
-    drck->set_allocation_state = set_allocation_state;
     drck->release_pending = release_pending;
     /*
      * Reason: it crashes FIXME find and document the real reason
@@ -823,14 +821,23 @@ static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
 static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
 {
     sPAPRDRConnector *drc = spapr_drc_by_index(idx);
-    sPAPRDRConnectorClass *drck;
 
-    if (!drc) {
-        return RTAS_OUT_PARAM_ERROR;
+    if (!drc || !object_dynamic_cast(OBJECT(drc), TYPE_SPAPR_DRC_LOGICAL)) {
+        return RTAS_OUT_NO_SUCH_INDICATOR;
     }
 
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    return drck->set_allocation_state(drc, state);
+    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
+
+    switch (state) {
+    case SPAPR_DR_ALLOCATION_STATE_USABLE:
+        return drc_set_usable(drc);
+
+    case SPAPR_DR_ALLOCATION_STATE_UNUSABLE:
+        return drc_set_unusable(drc);
+
+    default:
+        return RTAS_OUT_PARAM_ERROR;
+    }
 }
 
 static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index f24188d..0e09afb 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -220,8 +220,6 @@ typedef struct sPAPRDRConnectorClass {
     /* accessors for guest-visible (generally via RTAS) DR state */
     uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
                                     sPAPRDRIsolationState state);
-    uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
-                                     sPAPRDRAllocationState state);
 
     /* QEMU interfaces for managing hotplug operations */
     bool (*release_pending)(sPAPRDRConnector *drc);
-- 
2.9.4

[Qemu-devel] [PATCH 6/6] spapr: Clean up DRC set_isolation_state() path

[David Gibson]

There are substantial differences in the various paths through
set_isolation_state(), both for setting to ISOLATED versus UNISOLATED
state and for logical versus physical DRCs.

So, split the set_isolation_state() method into isolate() and unisolate()
methods, and give it different implementations for the two DRC types.

Factor some minimal common checks, including for valid indicator values
(which we weren't previously checking) into rtas_set_isolation_state().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 145 ++++++++++++++++++++++++++++++++-------------
 include/hw/ppc/spapr_drc.h |   6 +-
 2 files changed, 105 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9e01d7b..90c0fde 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -46,30 +46,64 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
         | (drc->id & DRC_INDEX_ID_MASK);
 }
 
-static uint32_t set_isolation_state(sPAPRDRConnector *drc,
-                                    sPAPRDRIsolationState state)
+static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
 {
-    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
-
     /* if the guest is configuring a device attached to this DRC, we
      * should reset the configuration state at this point since it may
      * no longer be reliable (guest released device and needs to start
      * over, or unplug occurred so the FDT is no longer valid)
      */
-    if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-        g_free(drc->ccs);
-        drc->ccs = NULL;
-    }
+    g_free(drc->ccs);
+    drc->ccs = NULL;
 
-    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
-        /* cannot unisolate a non-existent resource, and, or resources
-         * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
-         */
-        if (!drc->dev ||
-            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-            return RTAS_OUT_NO_SUCH_INDICATOR;
+    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
+
+    /* if we're awaiting release, but still in an unconfigured state,
+     * it's likely the guest is still in the process of configuring
+     * the device and is transitioning the devices to an ISOLATED
+     * state as a part of that process. so we only complete the
+     * removal when this transition happens for a device in a
+     * configured state, as suggested by the state diagram from PAPR+
+     * 2.7, 13.4
+     */
+    if (drc->awaiting_release) {
+        uint32_t drc_index = spapr_drc_index(drc);
+        if (drc->configured) {
+            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
+            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
+        } else {
+            trace_spapr_drc_set_isolation_state_deferring(drc_index);
         }
     }
+    drc->configured = false;
+
+    return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
+{
+    /* cannot unisolate a non-existent resource, and, or resources
+     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
+     * 13.5.3.5)
+     */
+    if (!drc->dev) {
+        return RTAS_OUT_NO_SUCH_INDICATOR;
+    }
+
+    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
+
+    return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
+{
+    /* if the guest is configuring a device attached to this DRC, we
+     * should reset the configuration state at this point since it may
+     * no longer be reliable (guest released device and needs to start
+     * over, or unplug occurred so the FDT is no longer valid)
+     */
+    g_free(drc->ccs);
+    drc->ccs = NULL;
 
     /*
      * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
@@ -81,35 +115,47 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
      * If the LMB being removed doesn't belong to a DIMM device that is
      * actually being unplugged, fail the isolation request here.
      */
-    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
-        if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
-             !drc->awaiting_release) {
-            return RTAS_OUT_HW_ERROR;
-        }
+    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
+        && !drc->awaiting_release) {
+        return RTAS_OUT_HW_ERROR;
     }
 
-    drc->isolation_state = state;
+    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
 
-    if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-        /* if we're awaiting release, but still in an unconfigured state,
-         * it's likely the guest is still in the process of configuring
-         * the device and is transitioning the devices to an ISOLATED
-         * state as a part of that process. so we only complete the
-         * removal when this transition happens for a device in a
-         * configured state, as suggested by the state diagram from
-         * PAPR+ 2.7, 13.4
-         */
-        if (drc->awaiting_release) {
-            uint32_t drc_index = spapr_drc_index(drc);
-            if (drc->configured) {
-                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
-                spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
-            } else {
-                trace_spapr_drc_set_isolation_state_deferring(drc_index);
-            }
+    /* if we're awaiting release, but still in an unconfigured state,
+     * it's likely the guest is still in the process of configuring
+     * the device and is transitioning the devices to an ISOLATED
+     * state as a part of that process. so we only complete the
+     * removal when this transition happens for a device in a
+     * configured state, as suggested by the state diagram from PAPR+
+     * 2.7, 13.4
+     */
+    if (drc->awaiting_release) {
+        uint32_t drc_index = spapr_drc_index(drc);
+        if (drc->configured) {
+            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
+            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
+        } else {
+            trace_spapr_drc_set_isolation_state_deferring(drc_index);
         }
-        drc->configured = false;
     }
+    drc->configured = false;
+
+    return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_unisolate_logical(sPAPRDRConnector *drc)
+{
+    /* cannot unisolate a non-existent resource, and, or resources
+     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
+     * 13.5.3.5)
+     */
+    if (!drc->dev ||
+        drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
+        return RTAS_OUT_NO_SUCH_INDICATOR;
+    }
+
+    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
 
     return RTAS_OUT_SUCCESS;
 }
@@ -551,7 +597,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     dk->reset = reset;
     dk->realize = realize;
     dk->unrealize = unrealize;
-    drck->set_isolation_state = set_isolation_state;
     drck->release_pending = release_pending;
     /*
      * Reason: it crashes FIXME find and document the real reason
@@ -564,6 +609,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
 
     drck->dr_entity_sense = physical_entity_sense;
+    drck->isolate = drc_isolate_physical;
+    drck->unisolate = drc_unisolate_physical;
 }
 
 static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
@@ -571,6 +618,8 @@ static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
 
     drck->dr_entity_sense = logical_entity_sense;
+    drck->isolate = drc_isolate_logical;
+    drck->unisolate = drc_unisolate_logical;
 }
 
 static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
@@ -811,11 +860,23 @@ static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
     sPAPRDRConnectorClass *drck;
 
     if (!drc) {
-        return RTAS_OUT_PARAM_ERROR;
+        return RTAS_OUT_NO_SUCH_INDICATOR;
     }
 
+    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
+
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    return drck->set_isolation_state(drc, state);
+
+    switch (state) {
+    case SPAPR_DR_ISOLATION_STATE_ISOLATED:
+        return drck->isolate(drc);
+
+    case SPAPR_DR_ISOLATION_STATE_UNISOLATED:
+        return drck->unisolate(drc);
+
+    default:
+        return RTAS_OUT_PARAM_ERROR;
+    }
 }
 
 static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 0e09afb..3e93bdd 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -216,10 +216,8 @@ typedef struct sPAPRDRConnectorClass {
     const char *drc_name_prefix; /* used other places in device tree */
 
     sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
-
-    /* accessors for guest-visible (generally via RTAS) DR state */
-    uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
-                                    sPAPRDRIsolationState state);
+    uint32_t (*isolate)(sPAPRDRConnector *drc);
+    uint32_t (*unisolate)(sPAPRDRConnector *drc);
 
     /* QEMU interfaces for managing hotplug operations */
     bool (*release_pending)(sPAPRDRConnector *drc);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[Laurent Vivier]

On 08/06/2017 07:09, David Gibson wrote:
> This fourth isntallment of cleanups to the DRC code introduces the
> first changes to the fundamental state handling.  We change the
> initial states in the reset code and attach code for PCI devices, and
> are able to remove the 'signalled' state variable with those fixes.
> 
> There are also some more mechanical cleanups in preparation for
> further cleanups and fixes to the state management.
> 
> David Gibson (6):
>   spapr: Start hotplugged PCI devices in ISOLATED state
>   spapr: Eliminate DRC 'signalled' state variable
>   spapr: Split DRC release from DRC detach
>   spapr: Make DRC reset force DRC into known state
>   spapr: Clean up DRC set_allocation_state path
>   spapr: Clean up DRC set_isolation_state() path
> 
>  hw/ppc/spapr.c             |  15 --
>  hw/ppc/spapr_drc.c         | 363 +++++++++++++++++++++++----------------------
>  hw/ppc/spapr_events.c      |  10 --
>  include/hw/ppc/spapr_drc.h |  10 +-
>  4 files changed, 188 insertions(+), 210 deletions(-)
> 

I've tested your series rebased on master.

- plugging a CPU while the OS is not started (stopped in SLOF/GRUB):

    The cpu can be hotplugged, but once the OS is started, the OS
    doesn't detect it and it can't be unplugged.

- plugging a memory DIMM while the OS is not started:

    The first device_del does nothing, the second one crashes qemu

- migration with hotplugged cpu (with OS started):

     CPU cannot be unplugged on destination side

- migration with hotplugged memory DIMM (with OS started):

     The first device_del does nothing, the second one crashes qemu

As it's cleanup, I guess this is what is expected
(the results are the same as before cleanup series).

Laurent

Re: [Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[David Gibson]

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

On Thu, Jun 15, 2017 at 07:10:55PM +0200, Laurent Vivier wrote:
> On 08/06/2017 07:09, David Gibson wrote:
> > This fourth isntallment of cleanups to the DRC code introduces the
> > first changes to the fundamental state handling.  We change the
> > initial states in the reset code and attach code for PCI devices, and
> > are able to remove the 'signalled' state variable with those fixes.
> > 
> > There are also some more mechanical cleanups in preparation for
> > further cleanups and fixes to the state management.
> > 
> > David Gibson (6):
> >   spapr: Start hotplugged PCI devices in ISOLATED state
> >   spapr: Eliminate DRC 'signalled' state variable
> >   spapr: Split DRC release from DRC detach
> >   spapr: Make DRC reset force DRC into known state
> >   spapr: Clean up DRC set_allocation_state path
> >   spapr: Clean up DRC set_isolation_state() path
> > 
> >  hw/ppc/spapr.c             |  15 --
> >  hw/ppc/spapr_drc.c         | 363 +++++++++++++++++++++++----------------------
> >  hw/ppc/spapr_events.c      |  10 --
> >  include/hw/ppc/spapr_drc.h |  10 +-
> >  4 files changed, 188 insertions(+), 210 deletions(-)
> > 
> 
> I've tested your series rebased on master.
> 
> - plugging a CPU while the OS is not started (stopped in SLOF/GRUB):
> 
>     The cpu can be hotplugged, but once the OS is started, the OS
>     doesn't detect it and it can't be unplugged.
> 
> - plugging a memory DIMM while the OS is not started:
> 
>     The first device_del does nothing, the second one crashes qemu
> 
> - migration with hotplugged cpu (with OS started):
> 
>      CPU cannot be unplugged on destination side
> 
> - migration with hotplugged memory DIMM (with OS started):
> 
>      The first device_del does nothing, the second one crashes qemu
> 
> As it's cleanup, I guess this is what is expected
> (the results are the same as before cleanup series).

Right, I wouldn't expect this series to fix problems yet - it just
makes the code clearer so it will be easier to do so in future.

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

Re: [Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[Alexey Kardashevskiy]

On 16/06/17 03:10, Laurent Vivier wrote:
> On 08/06/2017 07:09, David Gibson wrote:
>> This fourth isntallment of cleanups to the DRC code introduces the
>> first changes to the fundamental state handling.  We change the
>> initial states in the reset code and attach code for PCI devices, and
>> are able to remove the 'signalled' state variable with those fixes.
>>
>> There are also some more mechanical cleanups in preparation for
>> further cleanups and fixes to the state management.
>>
>> David Gibson (6):
>>   spapr: Start hotplugged PCI devices in ISOLATED state
>>   spapr: Eliminate DRC 'signalled' state variable
>>   spapr: Split DRC release from DRC detach
>>   spapr: Make DRC reset force DRC into known state
>>   spapr: Clean up DRC set_allocation_state path
>>   spapr: Clean up DRC set_isolation_state() path
>>
>>  hw/ppc/spapr.c             |  15 --
>>  hw/ppc/spapr_drc.c         | 363 +++++++++++++++++++++++----------------------
>>  hw/ppc/spapr_events.c      |  10 --
>>  include/hw/ppc/spapr_drc.h |  10 +-
>>  4 files changed, 188 insertions(+), 210 deletions(-)
>>
> 
> I've tested your series rebased on master.


I have tested git://github.com/dgibson/qemu.git drcIV branch with 2xXHCI
multifunction PCI hotplug, works fine.


> 
> - plugging a CPU while the OS is not started (stopped in SLOF/GRUB):
> 
>     The cpu can be hotplugged, but once the OS is started, the OS
>     doesn't detect it and it can't be unplugged.
> 
> - plugging a memory DIMM while the OS is not started:
> 
>     The first device_del does nothing, the second one crashes qemu
> 
> - migration with hotplugged cpu (with OS started):
> 
>      CPU cannot be unplugged on destination side
> 
> - migration with hotplugged memory DIMM (with OS started):
> 
>      The first device_del does nothing, the second one crashes qemu
> 
> As it's cleanup, I guess this is what is expected
> (the results are the same as before cleanup series).
> 
> Laurent
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/6] spapr: Start hotplugged PCI devices in ISOLATED state

[Greg Kurz]

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

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

> PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation
> state once the device is attached.  This has been there from the initial
> implementation, and it's not clear why.
> 
> The state diagram in PAPR 13.4 suggests PCI devices should start in
> ISOLATED state until the guest moves them into UNISOLATED, and the code in
> the guest-side drmgr tool seems to work that way too.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---

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

>  hw/ppc/spapr_drc.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 15ef67d..6186f79 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -315,16 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      }
>      g_assert(fdt || coldplug);
>  
> -    /* NOTE: setting initial isolation state to UNISOLATED means we can't
> -     * detach unless guest has a userspace/kernel that moves this state
> -     * back to ISOLATED in response to an unplug event, or this is done
> -     * manually by the admin prior. if we force things while the guest
> -     * may be accessing the device, we can easily crash the guest, so we
> -     * we defer completion of removal in such cases to the reset() hook.
> -     */
> -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> -    }
>      drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>  
>      drc->dev = d;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 2/6] spapr: Eliminate DRC 'signalled' state variable

[Greg Kurz]

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

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

> The 'signalled' field in the DRC appears to be entirely a torturous
> workaround for the fact that PCI devices were started in UNISOLATED state
> for unclear reasons.
> 
> 1) 'signalled' is already meaningless for logical (so far, all non PCI)
> DRCs.  It's always set to true (at least at any point it might be tested),
> and can't be assigned any real meaning due to the way signalling works for
> logical DRCs.
> 
> 2) For PCI DRCs, the only time signalled would be false is when non-zero
> functions of a multifunction device are hotplugged, followed by function
> zero (the other way around is explicitly not permitted). In that case the
> secondary function DRCs are attached, but the notification isn't sent to
> the guest until function 0 is plugged.
> 
> 3) signalled being false is used to allow a DRC detach to switch mode
> back to ISOLATED state, which allows a secondary function to be hotplugged
> then unplugged with function 0 never inserted.  Without this a secondary
> function starting in UNISOLATED state couldn't be detached again without
> function 0 being inserted, all the functions configured by the guest, then
> sent back to ISOLATED state.
> 
> 4) But now that PCI DRCs start in ISOLATED state, there's nothing to be
> done.  If the guest doesn't get the notification, it won't switch the
> device to UNISOLATED state, so nothing prevents it from being unplugged.
> If the guest does move it to UNISOLATED state without the signal (due to
> a manual drmgr call, for instance) then it really isn't safe to unplug it.
> 
> 
> So, this patch removes the signalled variable and all code related to it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_drc.c         | 45 +--------------------------------------------
>  hw/ppc/spapr_events.c      | 10 ----------
>  include/hw/ppc/spapr_drc.h |  2 --
>  3 files changed, 1 insertion(+), 56 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 6186f79..afd68f4 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -183,12 +183,6 @@ static const char *spapr_drc_name(sPAPRDRConnector *drc)
>      return g_strdup_printf("%s%d", drck->drc_name_prefix, drc->id);
>  }
>  
> -/* has the guest been notified of device attachment? */
> -static void set_signalled(sPAPRDRConnector *drc)
> -{
> -    drc->signalled = true;
> -}
> -
>  /*
>   * dr-entity-sense sensor value
>   * returned via get-sensor-state RTAS calls
> @@ -321,17 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
>      drc->configured = coldplug;
> -    /* 'logical' DR resources such as memory/cpus are in some cases treated
> -     * as a pool of resources from which the guest is free to choose from
> -     * based on only a count. for resources that can be assigned in this
> -     * fashion, we must assume the resource is signalled immediately
> -     * since a single hotplug request might make an arbitrary number of
> -     * such attached resources available to the guest, as opposed to
> -     * 'physical' DR resources such as PCI where each device/resource is
> -     * signalled individually.
> -     */
> -    drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> -                     ? true : coldplug;
>  
>      if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>          drc->awaiting_allocation = true;
> @@ -347,26 +330,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>  {
>      trace_spapr_drc_detach(spapr_drc_index(drc));
>  
> -    /* if we've signalled device presence to the guest, or if the guest
> -     * has gone ahead and configured the device (via manually-executed
> -     * device add via drmgr in guest, namely), we need to wait
> -     * for the guest to quiesce the device before completing detach.
> -     * Otherwise, we can assume the guest hasn't seen it and complete the
> -     * detach immediately. Note that there is a small race window
> -     * just before, or during, configuration, which is this context
> -     * refers mainly to fetching the device tree via RTAS.
> -     * During this window the device access will be arbitrated by
> -     * associated DRC, which will simply fail the RTAS calls as invalid.
> -     * This is recoverable within guest and current implementations of
> -     * drmgr should be able to cope.
> -     */
> -    if (!drc->signalled && !drc->configured) {
> -        /* if the guest hasn't seen the device we can't rely on it to
> -         * set it back to an isolated state via RTAS, so do it here manually
> -         */
> -        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> -    }
> -
>      if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
>          trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
>          drc->awaiting_release = true;
> @@ -455,10 +418,6 @@ static void reset(DeviceState *d)
>              drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
>          }
>      }
> -
> -    if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> -        drck->set_signalled(drc);
> -    }
>  }
>  
>  static bool spapr_drc_needed(void *opaque)
> @@ -483,7 +442,7 @@ static bool spapr_drc_needed(void *opaque)
>      case SPAPR_DR_CONNECTOR_TYPE_LMB:
>          rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
>                 (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> -               drc->configured && drc->signalled && !drc->awaiting_release);
> +               drc->configured && !drc->awaiting_release);
>          break;
>      case SPAPR_DR_CONNECTOR_TYPE_PHB:
>      case SPAPR_DR_CONNECTOR_TYPE_VIO:
> @@ -505,7 +464,6 @@ static const VMStateDescription vmstate_spapr_drc = {
>          VMSTATE_BOOL(configured, sPAPRDRConnector),
>          VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
>          VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> -        VMSTATE_BOOL(signalled, sPAPRDRConnector),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -603,7 +561,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->set_isolation_state = set_isolation_state;
>      drck->set_allocation_state = set_allocation_state;
>      drck->release_pending = release_pending;
> -    drck->set_signalled = set_signalled;
>      /*
>       * Reason: it crashes FIXME find and document the real reason
>       */
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 171aedc..587a3da 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -475,13 +475,6 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>                                                         RTAS_LOG_TYPE_EPOW)));
>  }
>  
> -static void spapr_hotplug_set_signalled(uint32_t drc_index)
> -{
> -    sPAPRDRConnector *drc = spapr_drc_by_index(drc_index);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    drck->set_signalled(drc);
> -}
> -
>  static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>                                      sPAPRDRConnectorType drc_type,
>                                      union drc_identifier *drc_id)
> @@ -528,9 +521,6 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>      switch (drc_type) {
>      case SPAPR_DR_CONNECTOR_TYPE_PCI:
>          hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
> -        if (hp->hotplug_action == RTAS_LOG_V6_HP_ACTION_ADD) {
> -            spapr_hotplug_set_signalled(drc_id->index);
> -        }
>          break;
>      case SPAPR_DR_CONNECTOR_TYPE_LMB:
>          hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index c487123..f24188d 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -199,7 +199,6 @@ typedef struct sPAPRDRConnector {
>      sPAPRConfigureConnectorState *ccs;
>  
>      bool awaiting_release;
> -    bool signalled;
>      bool awaiting_allocation;
>      bool awaiting_allocation_skippable;
>  
> @@ -226,7 +225,6 @@ typedef struct sPAPRDRConnectorClass {
>  
>      /* QEMU interfaces for managing hotplug operations */
>      bool (*release_pending)(sPAPRDRConnector *drc);
> -    void (*set_signalled)(sPAPRDRConnector *drc);
>  } sPAPRDRConnectorClass;
>  
>  uint32_t spapr_drc_index(sPAPRDRConnector *drc);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 3/6] spapr: Split DRC release from DRC detach

[Greg Kurz]

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

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

> spapr_drc_detach() is called when qemu generic code requests a device be
> unplugged.  It makes a number of tests, which could well delay further
> action until later, before actually detach the device from the DRC.
> 
> This splits out the part which actually removes the device from the DRC
> into spapr_drc_release().  This will be useful for further cleanups.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_drc.c | 54 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index afd68f4..dc4ac77 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -326,31 +326,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                               NULL, 0, NULL);
>  }
>  
> -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> -{
> -    trace_spapr_drc_detach(spapr_drc_index(drc));
> -
> -    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -        trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> -        drc->awaiting_release = true;
> -        return;
> -    }
> -
> -    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> -        drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -        trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> -        drc->awaiting_release = true;
> -        return;
> -    }
> -
> -    if (drc->awaiting_allocation) {
> -        if (!drc->awaiting_allocation_skippable) {
> -            drc->awaiting_release = true;
> -            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> -            return;
> -        }
> -    }
>  
> +static void spapr_drc_release(sPAPRDRConnector *drc)
> +{
>      drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
>  
>      /* Calling release callbacks based on spapr_drc_type(drc). */
> @@ -379,6 +357,34 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      drc->dev = NULL;
>  }
>  
> +void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> +{
> +    trace_spapr_drc_detach(spapr_drc_index(drc));
> +
> +    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +        trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> +        drc->awaiting_release = true;
> +        return;
> +    }
> +
> +    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> +        drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> +        trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> +        drc->awaiting_release = true;
> +        return;
> +    }
> +
> +    if (drc->awaiting_allocation) {
> +        if (!drc->awaiting_allocation_skippable) {
> +            drc->awaiting_release = true;
> +            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> +            return;
> +        }
> +    }
> +
> +    spapr_drc_release(drc);
> +}
> +
>  static bool release_pending(sPAPRDRConnector *drc)
>  {
>      return drc->awaiting_release;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 4/6] spapr: Make DRC reset force DRC into known state

[Greg Kurz]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 5/6] spapr: Clean up DRC set_allocation_state path

[Greg Kurz]

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

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

> The allocation-state indicator should only actually be implemented for
> "logical" DRCs, not physical ones.  Factor a check for this, and also for
> valid indicator state values into rtas_set_allocation_state().  Because
> they don't exist for physical DRCs, there's no reason that we'd ever want
> more than one method implementation, so it can just be a plain function.
> 
> In addition, the setting to USABLE and setting to UNUSABLE paths in
> set_allocation_state() don't actually have much in common.  So, split the
> method separate functions for each parameter value (drc_set_usable()
> and drc_set_unusable()).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_drc.c         | 85 +++++++++++++++++++++++++---------------------
>  include/hw/ppc/spapr_drc.h |  2 --
>  2 files changed, 46 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 7e17f34..9e01d7b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -114,44 +114,43 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>      return RTAS_OUT_SUCCESS;
>  }
>  
> -static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> -                                     sPAPRDRAllocationState state)
> +static uint32_t drc_set_usable(sPAPRDRConnector *drc)
>  {
> -    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
> -
> -    if (state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
> -        /* if there's no resource/device associated with the DRC, there's
> -         * no way for us to put it in an allocation state consistent with
> -         * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
> -         * result in an RTAS return code of -3 / "no such indicator"
> +    /* if there's no resource/device associated with the DRC, there's
> +     * no way for us to put it in an allocation state consistent with
> +     * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
> +     * result in an RTAS return code of -3 / "no such indicator"
> +     */
> +    if (!drc->dev) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
> +    }
> +    if (drc->awaiting_release && drc->awaiting_allocation) {
> +        /* kernel is acknowledging a previous hotplug event
> +         * while we are already removing it.
> +         * it's safe to ignore awaiting_allocation here since we know the
> +         * situation is predicated on the guest either already having done
> +         * so (boot-time hotplug), or never being able to acquire in the
> +         * first place (hotplug followed by immediate unplug).
>           */
> -        if (!drc->dev) {
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> -        }
> -        if (drc->awaiting_release && drc->awaiting_allocation) {
> -            /* kernel is acknowledging a previous hotplug event
> -             * while we are already removing it.
> -             * it's safe to ignore awaiting_allocation here since we know the
> -             * situation is predicated on the guest either already having done
> -             * so (boot-time hotplug), or never being able to acquire in the
> -             * first place (hotplug followed by immediate unplug).
> -             */
> -            drc->awaiting_allocation_skippable = true;
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> -        }
> +        drc->awaiting_allocation_skippable = true;
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
>      }
>  
> -    if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -        drc->allocation_state = state;
> -        if (drc->awaiting_release &&
> -            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -            uint32_t drc_index = spapr_drc_index(drc);
> -            trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> -            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> -        } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
> -            drc->awaiting_allocation = false;
> -        }
> +    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> +    drc->awaiting_allocation = false;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
> +{
> +    drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
> +    if (drc->awaiting_release) {
> +        uint32_t drc_index = spapr_drc_index(drc);
> +        trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> +        spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
>      }
> +
>      return RTAS_OUT_SUCCESS;
>  }
>  
> @@ -553,7 +552,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      dk->realize = realize;
>      dk->unrealize = unrealize;
>      drck->set_isolation_state = set_isolation_state;
> -    drck->set_allocation_state = set_allocation_state;
>      drck->release_pending = release_pending;
>      /*
>       * Reason: it crashes FIXME find and document the real reason
> @@ -823,14 +821,23 @@ static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
>  static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
>  {
>      sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> -    sPAPRDRConnectorClass *drck;
>  
> -    if (!drc) {
> -        return RTAS_OUT_PARAM_ERROR;
> +    if (!drc || !object_dynamic_cast(OBJECT(drc), TYPE_SPAPR_DRC_LOGICAL)) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
>      }
>  
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    return drck->set_allocation_state(drc, state);
> +    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
> +
> +    switch (state) {
> +    case SPAPR_DR_ALLOCATION_STATE_USABLE:
> +        return drc_set_usable(drc);
> +
> +    case SPAPR_DR_ALLOCATION_STATE_UNUSABLE:
> +        return drc_set_unusable(drc);
> +
> +    default:
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
>  }
>  
>  static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index f24188d..0e09afb 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -220,8 +220,6 @@ typedef struct sPAPRDRConnectorClass {
>      /* accessors for guest-visible (generally via RTAS) DR state */
>      uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
>                                      sPAPRDRIsolationState state);
> -    uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
> -                                     sPAPRDRAllocationState state);
>  
>      /* QEMU interfaces for managing hotplug operations */
>      bool (*release_pending)(sPAPRDRConnector *drc);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 6/6] spapr: Clean up DRC set_isolation_state() path

[Greg Kurz]

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

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

> There are substantial differences in the various paths through
> set_isolation_state(), both for setting to ISOLATED versus UNISOLATED
> state and for logical versus physical DRCs.
> 
> So, split the set_isolation_state() method into isolate() and unisolate()
> methods, and give it different implementations for the two DRC types.
> 
> Factor some minimal common checks, including for valid indicator values
> (which we weren't previously checking) into rtas_set_isolation_state().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c         | 145 ++++++++++++++++++++++++++++++++-------------
>  include/hw/ppc/spapr_drc.h |   6 +-
>  2 files changed, 105 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9e01d7b..90c0fde 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -46,30 +46,64 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
>          | (drc->id & DRC_INDEX_ID_MASK);
>  }
>  
> -static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> -                                    sPAPRDRIsolationState state)
> +static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
>  {
> -    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> -
>      /* if the guest is configuring a device attached to this DRC, we
>       * should reset the configuration state at this point since it may
>       * no longer be reliable (guest released device and needs to start
>       * over, or unplug occurred so the FDT is no longer valid)
>       */
> -    if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -        g_free(drc->ccs);
> -        drc->ccs = NULL;
> -    }
> +    g_free(drc->ccs);
> +    drc->ccs = NULL;
>  
> -    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> -        /* cannot unisolate a non-existent resource, and, or resources
> -         * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> -         */
> -        if (!drc->dev ||
> -            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> +
> +    /* if we're awaiting release, but still in an unconfigured state,
> +     * it's likely the guest is still in the process of configuring
> +     * the device and is transitioning the devices to an ISOLATED
> +     * state as a part of that process. so we only complete the
> +     * removal when this transition happens for a device in a
> +     * configured state, as suggested by the state diagram from PAPR+
> +     * 2.7, 13.4
> +     */
> +    if (drc->awaiting_release) {
> +        uint32_t drc_index = spapr_drc_index(drc);
> +        if (drc->configured) {
> +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +        } else {
> +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
>          }
>      }
> +    drc->configured = false;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> +{
> +    /* cannot unisolate a non-existent resource, and, or resources
> +     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> +     * 13.5.3.5)

Maybe you can drop everything except 'cannot unisolate a non-existent
resource' since the allocation-state indicator is for logical DRCs only.

Otherwise,

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

> +     */
> +    if (!drc->dev) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
> +    }
> +
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> +{
> +    /* if the guest is configuring a device attached to this DRC, we
> +     * should reset the configuration state at this point since it may
> +     * no longer be reliable (guest released device and needs to start
> +     * over, or unplug occurred so the FDT is no longer valid)
> +     */
> +    g_free(drc->ccs);
> +    drc->ccs = NULL;
>  
>      /*
>       * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> @@ -81,35 +115,47 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>       * If the LMB being removed doesn't belong to a DIMM device that is
>       * actually being unplugged, fail the isolation request here.
>       */
> -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> -        if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> -             !drc->awaiting_release) {
> -            return RTAS_OUT_HW_ERROR;
> -        }
> +    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
> +        && !drc->awaiting_release) {
> +        return RTAS_OUT_HW_ERROR;
>      }
>  
> -    drc->isolation_state = state;
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
>  
> -    if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -        /* if we're awaiting release, but still in an unconfigured state,
> -         * it's likely the guest is still in the process of configuring
> -         * the device and is transitioning the devices to an ISOLATED
> -         * state as a part of that process. so we only complete the
> -         * removal when this transition happens for a device in a
> -         * configured state, as suggested by the state diagram from
> -         * PAPR+ 2.7, 13.4
> -         */
> -        if (drc->awaiting_release) {
> -            uint32_t drc_index = spapr_drc_index(drc);
> -            if (drc->configured) {
> -                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> -                spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> -            } else {
> -                trace_spapr_drc_set_isolation_state_deferring(drc_index);
> -            }
> +    /* if we're awaiting release, but still in an unconfigured state,
> +     * it's likely the guest is still in the process of configuring
> +     * the device and is transitioning the devices to an ISOLATED
> +     * state as a part of that process. so we only complete the
> +     * removal when this transition happens for a device in a
> +     * configured state, as suggested by the state diagram from PAPR+
> +     * 2.7, 13.4
> +     */
> +    if (drc->awaiting_release) {
> +        uint32_t drc_index = spapr_drc_index(drc);
> +        if (drc->configured) {
> +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +        } else {
> +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
>          }
> -        drc->configured = false;
>      }
> +    drc->configured = false;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_unisolate_logical(sPAPRDRConnector *drc)
> +{
> +    /* cannot unisolate a non-existent resource, and, or resources
> +     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> +     * 13.5.3.5)
> +     */
> +    if (!drc->dev ||
> +        drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
> +    }
> +
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
>  
>      return RTAS_OUT_SUCCESS;
>  }
> @@ -551,7 +597,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      dk->reset = reset;
>      dk->realize = realize;
>      dk->unrealize = unrealize;
> -    drck->set_isolation_state = set_isolation_state;
>      drck->release_pending = release_pending;
>      /*
>       * Reason: it crashes FIXME find and document the real reason
> @@ -564,6 +609,8 @@ static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>  
>      drck->dr_entity_sense = physical_entity_sense;
> +    drck->isolate = drc_isolate_physical;
> +    drck->unisolate = drc_unisolate_physical;
>  }
>  
>  static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
> @@ -571,6 +618,8 @@ static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>  
>      drck->dr_entity_sense = logical_entity_sense;
> +    drck->isolate = drc_isolate_logical;
> +    drck->unisolate = drc_unisolate_logical;
>  }
>  
>  static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> @@ -811,11 +860,23 @@ static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
>      sPAPRDRConnectorClass *drck;
>  
>      if (!drc) {
> -        return RTAS_OUT_PARAM_ERROR;
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
>      }
>  
> +    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> +
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    return drck->set_isolation_state(drc, state);
> +
> +    switch (state) {
> +    case SPAPR_DR_ISOLATION_STATE_ISOLATED:
> +        return drck->isolate(drc);
> +
> +    case SPAPR_DR_ISOLATION_STATE_UNISOLATED:
> +        return drck->unisolate(drc);
> +
> +    default:
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
>  }
>  
>  static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 0e09afb..3e93bdd 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -216,10 +216,8 @@ typedef struct sPAPRDRConnectorClass {
>      const char *drc_name_prefix; /* used other places in device tree */
>  
>      sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
> -
> -    /* accessors for guest-visible (generally via RTAS) DR state */
> -    uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> -                                    sPAPRDRIsolationState state);
> +    uint32_t (*isolate)(sPAPRDRConnector *drc);
> +    uint32_t (*unisolate)(sPAPRDRConnector *drc);
>  
>      /* QEMU interfaces for managing hotplug operations */
>      bool (*release_pending)(sPAPRDRConnector *drc);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[David Gibson]

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

On Thu, Jun 08, 2017 at 03:09:24PM +1000, David Gibson wrote:
> This fourth isntallment of cleanups to the DRC code introduces the
> first changes to the fundamental state handling.  We change the
> initial states in the reset code and attach code for PCI devices, and
> are able to remove the 'signalled' state variable with those fixes.
> 
> There are also some more mechanical cleanups in preparation for
> further cleanups and fixes to the state management.

Merged to ppc-for-2.10.

> 
> David Gibson (6):
>   spapr: Start hotplugged PCI devices in ISOLATED state
>   spapr: Eliminate DRC 'signalled' state variable
>   spapr: Split DRC release from DRC detach
>   spapr: Make DRC reset force DRC into known state
>   spapr: Clean up DRC set_allocation_state path
>   spapr: Clean up DRC set_isolation_state() path
> 
>  hw/ppc/spapr.c             |  15 --
>  hw/ppc/spapr_drc.c         | 363 +++++++++++++++++++++++----------------------
>  hw/ppc/spapr_events.c      |  10 --
>  include/hw/ppc/spapr_drc.h |  10 +-
>  4 files changed, 188 insertions(+), 210 deletions(-)
> 

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

Re: [Qemu-devel] [PATCH 6/6] spapr: Clean up DRC set_isolation_state() path

[Michael Roth]

Quoting David Gibson (2017-06-08 00:09:30)
> There are substantial differences in the various paths through
> set_isolation_state(), both for setting to ISOLATED versus UNISOLATED
> state and for logical versus physical DRCs.
> 
> So, split the set_isolation_state() method into isolate() and unisolate()
> methods, and give it different implementations for the two DRC types.
> 
> Factor some minimal common checks, including for valid indicator values
> (which we weren't previously checking) into rtas_set_isolation_state().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c         | 145 ++++++++++++++++++++++++++++++++-------------
>  include/hw/ppc/spapr_drc.h |   6 +-
>  2 files changed, 105 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9e01d7b..90c0fde 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -46,30 +46,64 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
>          | (drc->id & DRC_INDEX_ID_MASK);
>  }
> 
> -static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> -                                    sPAPRDRIsolationState state)
> +static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
>  {
> -    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> -
>      /* if the guest is configuring a device attached to this DRC, we
>       * should reset the configuration state at this point since it may
>       * no longer be reliable (guest released device and needs to start
>       * over, or unplug occurred so the FDT is no longer valid)
>       */
> -    if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -        g_free(drc->ccs);
> -        drc->ccs = NULL;
> -    }
> +    g_free(drc->ccs);
> +    drc->ccs = NULL;
> 
> -    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> -        /* cannot unisolate a non-existent resource, and, or resources
> -         * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> -         */
> -        if (!drc->dev ||
> -            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> +
> +    /* if we're awaiting release, but still in an unconfigured state,
> +     * it's likely the guest is still in the process of configuring
> +     * the device and is transitioning the devices to an ISOLATED
> +     * state as a part of that process. so we only complete the
> +     * removal when this transition happens for a device in a
> +     * configured state, as suggested by the state diagram from PAPR+
> +     * 2.7, 13.4
> +     */
> +    if (drc->awaiting_release) {
> +        uint32_t drc_index = spapr_drc_index(drc);
> +        if (drc->configured) {
> +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +        } else {
> +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
>          }
>      }
> +    drc->configured = false;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> +{
> +    /* cannot unisolate a non-existent resource, and, or resources
> +     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> +     * 13.5.3.5)
> +     */
> +    if (!drc->dev) {
> +        return RTAS_OUT_NO_SUCH_INDICATOR;
> +    }
> +
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
> +static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> +{
> +    /* if the guest is configuring a device attached to this DRC, we
> +     * should reset the configuration state at this point since it may
> +     * no longer be reliable (guest released device and needs to start
> +     * over, or unplug occurred so the FDT is no longer valid)
> +     */
> +    g_free(drc->ccs);
> +    drc->ccs = NULL;
> 
>      /*
>       * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> @@ -81,35 +115,47 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>       * If the LMB being removed doesn't belong to a DIMM device that is
>       * actually being unplugged, fail the isolation request here.
>       */
> -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> -        if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> -             !drc->awaiting_release) {
> -            return RTAS_OUT_HW_ERROR;
> -        }
> +    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
> +        && !drc->awaiting_release) {
> +        return RTAS_OUT_HW_ERROR;
>      }
> 
> -    drc->isolation_state = state;
> +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> 
> -    if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -        /* if we're awaiting release, but still in an unconfigured state,
> -         * it's likely the guest is still in the process of configuring
> -         * the device and is transitioning the devices to an ISOLATED
> -         * state as a part of that process. so we only complete the
> -         * removal when this transition happens for a device in a
> -         * configured state, as suggested by the state diagram from
> -         * PAPR+ 2.7, 13.4
> -         */
> -        if (drc->awaiting_release) {
> -            uint32_t drc_index = spapr_drc_index(drc);
> -            if (drc->configured) {
> -                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> -                spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> -            } else {
> -                trace_spapr_drc_set_isolation_state_deferring(drc_index);
> -            }
> +    /* if we're awaiting release, but still in an unconfigured state,
> +     * it's likely the guest is still in the process of configuring
> +     * the device and is transitioning the devices to an ISOLATED
> +     * state as a part of that process. so we only complete the
> +     * removal when this transition happens for a device in a
> +     * configured state, as suggested by the state diagram from PAPR+
> +     * 2.7, 13.4
> +     */
> +    if (drc->awaiting_release) {
> +        uint32_t drc_index = spapr_drc_index(drc);
> +        if (drc->configured) {
> +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +        } else {
> +            trace_spapr_drc_set_isolation_state_deferring(drc_index);

Not sure this is the right patch to do it, but this refactoring does make
it more apparent that the if (drc->configured) checks should get pushed
down into spapr_drc_detach() like the other deferral checks at some point.

(There are 2 locations that do the detach without checking configured,
but you switched out the one in reset() to use spapr_drc_release()
already, and I don't think drc_set_unusable() without first going
through drc_isolate_*() is a transition that PAPR would allow anyway)

Re: [Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)

[Michael Roth]

Quoting David Gibson (2017-06-08 00:09:24)
> This fourth isntallment of cleanups to the DRC code introduces the
> first changes to the fundamental state handling.  We change the
> initial states in the reset code and attach code for PCI devices, and
> are able to remove the 'signalled' state variable with those fixes.
> 
> There are also some more mechanical cleanups in preparation for
> further cleanups and fixes to the state management.
> 
> David Gibson (6):
>   spapr: Start hotplugged PCI devices in ISOLATED state
>   spapr: Eliminate DRC 'signalled' state variable
>   spapr: Split DRC release from DRC detach
>   spapr: Make DRC reset force DRC into known state
>   spapr: Clean up DRC set_allocation_state path
>   spapr: Clean up DRC set_isolation_state() path

Series:

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
>  hw/ppc/spapr.c             |  15 --
>  hw/ppc/spapr_drc.c         | 363 +++++++++++++++++++++++----------------------
>  hw/ppc/spapr_events.c      |  10 --
>  include/hw/ppc/spapr_drc.h |  10 +-
>  4 files changed, 188 insertions(+), 210 deletions(-)
> 
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [PATCH 6/6] spapr: Clean up DRC set_isolation_state() path

[David Gibson]

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

On Mon, Jun 19, 2017 at 05:52:27PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-08 00:09:30)
> > There are substantial differences in the various paths through
> > set_isolation_state(), both for setting to ISOLATED versus UNISOLATED
> > state and for logical versus physical DRCs.
> > 
> > So, split the set_isolation_state() method into isolate() and unisolate()
> > methods, and give it different implementations for the two DRC types.
> > 
> > Factor some minimal common checks, including for valid indicator values
> > (which we weren't previously checking) into rtas_set_isolation_state().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_drc.c         | 145 ++++++++++++++++++++++++++++++++-------------
> >  include/hw/ppc/spapr_drc.h |   6 +-
> >  2 files changed, 105 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 9e01d7b..90c0fde 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -46,30 +46,64 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> >          | (drc->id & DRC_INDEX_ID_MASK);
> >  }
> > 
> > -static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > -                                    sPAPRDRIsolationState state)
> > +static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
> >  {
> > -    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> > -
> >      /* if the guest is configuring a device attached to this DRC, we
> >       * should reset the configuration state at this point since it may
> >       * no longer be reliable (guest released device and needs to start
> >       * over, or unplug occurred so the FDT is no longer valid)
> >       */
> > -    if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > -        g_free(drc->ccs);
> > -        drc->ccs = NULL;
> > -    }
> > +    g_free(drc->ccs);
> > +    drc->ccs = NULL;
> > 
> > -    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> > -        /* cannot unisolate a non-existent resource, and, or resources
> > -         * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> > -         */
> > -        if (!drc->dev ||
> > -            drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > -            return RTAS_OUT_NO_SUCH_INDICATOR;
> > +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> > +
> > +    /* if we're awaiting release, but still in an unconfigured state,
> > +     * it's likely the guest is still in the process of configuring
> > +     * the device and is transitioning the devices to an ISOLATED
> > +     * state as a part of that process. so we only complete the
> > +     * removal when this transition happens for a device in a
> > +     * configured state, as suggested by the state diagram from PAPR+
> > +     * 2.7, 13.4
> > +     */
> > +    if (drc->awaiting_release) {
> > +        uint32_t drc_index = spapr_drc_index(drc);
> > +        if (drc->configured) {
> > +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > +        } else {
> > +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
> >          }
> >      }
> > +    drc->configured = false;
> > +
> > +    return RTAS_OUT_SUCCESS;
> > +}
> > +
> > +static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
> > +{
> > +    /* cannot unisolate a non-existent resource, and, or resources
> > +     * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
> > +     * 13.5.3.5)
> > +     */
> > +    if (!drc->dev) {
> > +        return RTAS_OUT_NO_SUCH_INDICATOR;
> > +    }
> > +
> > +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > +
> > +    return RTAS_OUT_SUCCESS;
> > +}
> > +
> > +static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
> > +{
> > +    /* if the guest is configuring a device attached to this DRC, we
> > +     * should reset the configuration state at this point since it may
> > +     * no longer be reliable (guest released device and needs to start
> > +     * over, or unplug occurred so the FDT is no longer valid)
> > +     */
> > +    g_free(drc->ccs);
> > +    drc->ccs = NULL;
> > 
> >      /*
> >       * Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
> > @@ -81,35 +115,47 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> >       * If the LMB being removed doesn't belong to a DIMM device that is
> >       * actually being unplugged, fail the isolation request here.
> >       */
> > -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > -        if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > -             !drc->awaiting_release) {
> > -            return RTAS_OUT_HW_ERROR;
> > -        }
> > +    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
> > +        && !drc->awaiting_release) {
> > +        return RTAS_OUT_HW_ERROR;
> >      }
> > 
> > -    drc->isolation_state = state;
> > +    drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
> > 
> > -    if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > -        /* if we're awaiting release, but still in an unconfigured state,
> > -         * it's likely the guest is still in the process of configuring
> > -         * the device and is transitioning the devices to an ISOLATED
> > -         * state as a part of that process. so we only complete the
> > -         * removal when this transition happens for a device in a
> > -         * configured state, as suggested by the state diagram from
> > -         * PAPR+ 2.7, 13.4
> > -         */
> > -        if (drc->awaiting_release) {
> > -            uint32_t drc_index = spapr_drc_index(drc);
> > -            if (drc->configured) {
> > -                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > -                spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > -            } else {
> > -                trace_spapr_drc_set_isolation_state_deferring(drc_index);
> > -            }
> > +    /* if we're awaiting release, but still in an unconfigured state,
> > +     * it's likely the guest is still in the process of configuring
> > +     * the device and is transitioning the devices to an ISOLATED
> > +     * state as a part of that process. so we only complete the
> > +     * removal when this transition happens for a device in a
> > +     * configured state, as suggested by the state diagram from PAPR+
> > +     * 2.7, 13.4
> > +     */
> > +    if (drc->awaiting_release) {
> > +        uint32_t drc_index = spapr_drc_index(drc);
> > +        if (drc->configured) {
> > +            trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> > +            spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> > +        } else {
> > +            trace_spapr_drc_set_isolation_state_deferring(drc_index);
> 
> Not sure this is the right patch to do it, but this refactoring does make
> it more apparent that the if (drc->configured) checks should get pushed
> down into spapr_drc_detach() like the other deferral checks at some point.
> 
> (There are 2 locations that do the detach without checking configured,
> but you switched out the one in reset() to use spapr_drc_release()
> already, and I don't think drc_set_unusable() without first going
> through drc_isolate_*() is a transition that PAPR would allow anyway)

Right, but no, I don't think this patch is the place to do it.

I'll see what this looks like once I've made other cleanups on my
queue.

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

Re: [Qemu-devel] [PATCH 4/6] spapr: Make DRC reset force DRC into known state

[Bharata B Rao]

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.

Re: [Qemu-devel] [PATCH 4/6] spapr: Make DRC reset force DRC into known state

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]