[Qemu-devel] [PATCH 0/4] spapr:DRC cleanups (part I)

[Qemu-devel] [PATCH 0/4] spapr:DRC cleanups (part I)

[David Gibson]

The code managing DRCs[0] has quite a few things that are more
complicated than they need to be.  In particular the object
representing a DRC has a bunch of method pointers, despite the fact
that there are currently no subclasses, and even if there were the
method implementations would be unlikely to differ.

This appears to be a misguided attempt to "abstract" or hide things in
a way which is bureaucraticl, rather than meaningful.  We may have an
object model, but we don't have to adopt Java's kingdom-of-nouns
nonsense[1].

This series makes a start on simplifying things.  There's still plenty
more, but you have to start somewhere.

[0] "Dynamic Reconfiguration Connectors" a firmware abstraction used
    in hotplug operations
[1] https://steve-yegge.blogspot.com.au/2006/03/execution-in-kingdom-of-nouns.html

David Gibson (4):
  spapr: Move DRC RTAS calls into spapr_drc.c
  spapr: Abolish DRC get_fdt method
  spapr: Abolish DRC set_configured method
  spapr: Make DRC get_index and get_type methods into plain functions

 hw/ppc/spapr.c             |  13 +-
 hw/ppc/spapr_drc.c         | 404 ++++++++++++++++++++++++++++++++++++++-------
 hw/ppc/spapr_events.c      |  10 +-
 hw/ppc/spapr_pci.c         |   4 +-
 hw/ppc/spapr_rtas.c        | 304 ----------------------------------
 hw/ppc/trace-events        |   2 -
 include/hw/ppc/spapr_drc.h |   9 +-
 7 files changed, 355 insertions(+), 391 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[David Gibson]

Currently implementations of the RTAS calls related to DRCs are in
spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
to related code, and we'll be able to make some more things local.

spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
that don't belong anywhere else, not every RTAS implementation.

Code motion only.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
 2 files changed, 315 insertions(+), 311 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index cc2400b..ae8800d 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -27,6 +27,34 @@
 #define DRC_INDEX_TYPE_SHIFT 28
 #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
 
+static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
+                                                    uint32_t drc_index)
+{
+    sPAPRConfigureConnectorState *ccs = NULL;
+
+    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
+        if (ccs->drc_index == drc_index) {
+            break;
+        }
+    }
+
+    return ccs;
+}
+
+static void spapr_ccs_add(sPAPRMachineState *spapr,
+                          sPAPRConfigureConnectorState *ccs)
+{
+    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
+    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
+}
+
+static void spapr_ccs_remove(sPAPRMachineState *spapr,
+                             sPAPRConfigureConnectorState *ccs)
+{
+    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
+    g_free(ccs);
+}
+
 static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
 {
     uint32_t shift = 0;
@@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = {
     .class_init    = spapr_dr_connector_class_init,
 };
 
-static void spapr_drc_register_types(void)
-{
-    type_register_static(&spapr_dr_connector_info);
-}
-
-type_init(spapr_drc_register_types)
-
 /* helper functions for external users */
 
 sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
@@ -932,3 +953,290 @@ out:
 
     return ret;
 }
+
+/*
+ * RTAS calls
+ */
+
+static bool sensor_type_is_dr(uint32_t sensor_type)
+{
+    switch (sensor_type) {
+    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
+    case RTAS_SENSOR_TYPE_DR:
+    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
+        return true;
+    }
+
+    return false;
+}
+
+static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                               uint32_t token, uint32_t nargs,
+                               target_ulong args, uint32_t nret,
+                               target_ulong rets)
+{
+    uint32_t sensor_type;
+    uint32_t sensor_index;
+    uint32_t sensor_state;
+    uint32_t ret = RTAS_OUT_SUCCESS;
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+
+    if (nargs != 3 || nret != 1) {
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    sensor_type = rtas_ld(args, 0);
+    sensor_index = rtas_ld(args, 1);
+    sensor_state = rtas_ld(args, 2);
+
+    if (!sensor_type_is_dr(sensor_type)) {
+        goto out_unimplemented;
+    }
+
+    /* if this is a DR sensor we can assume sensor_index == drc_index */
+    drc = spapr_dr_connector_by_index(sensor_index);
+    if (!drc) {
+        trace_spapr_rtas_set_indicator_invalid(sensor_index);
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+    switch (sensor_type) {
+    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
+            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
+                                                               sensor_index);
+            if (ccs) {
+                spapr_ccs_remove(spapr, ccs);
+            }
+        }
+        ret = drck->set_isolation_state(drc, sensor_state);
+        break;
+    case RTAS_SENSOR_TYPE_DR:
+        ret = drck->set_indicator_state(drc, sensor_state);
+        break;
+    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
+        ret = drck->set_allocation_state(drc, sensor_state);
+        break;
+    default:
+        goto out_unimplemented;
+    }
+
+out:
+    rtas_st(rets, 0, ret);
+    return;
+
+out_unimplemented:
+    /* currently only DR-related sensors are implemented */
+    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
+    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+}
+
+static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args, uint32_t nret,
+                                  target_ulong rets)
+{
+    uint32_t sensor_type;
+    uint32_t sensor_index;
+    uint32_t sensor_state = 0;
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    uint32_t ret = RTAS_OUT_SUCCESS;
+
+    if (nargs != 2 || nret != 2) {
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    sensor_type = rtas_ld(args, 0);
+    sensor_index = rtas_ld(args, 1);
+
+    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
+        /* currently only DR-related sensors are implemented */
+        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
+                                                        sensor_type);
+        ret = RTAS_OUT_NOT_SUPPORTED;
+        goto out;
+    }
+
+    drc = spapr_dr_connector_by_index(sensor_index);
+    if (!drc) {
+        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    ret = drck->entity_sense(drc, &sensor_state);
+
+out:
+    rtas_st(rets, 0, ret);
+    rtas_st(rets, 1, sensor_state);
+}
+
+/* configure-connector work area offsets, int32_t units for field
+ * indexes, bytes for field offset/len values.
+ *
+ * as documented by PAPR+ v2.7, 13.5.3.5
+ */
+#define CC_IDX_NODE_NAME_OFFSET 2
+#define CC_IDX_PROP_NAME_OFFSET 2
+#define CC_IDX_PROP_LEN 3
+#define CC_IDX_PROP_DATA_OFFSET 4
+#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
+#define CC_WA_LEN 4096
+
+static void configure_connector_st(target_ulong addr, target_ulong offset,
+                                   const void *buf, size_t len)
+{
+    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
+                              buf, MIN(len, CC_WA_LEN - offset));
+}
+
+void spapr_ccs_reset_hook(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
+
+    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
+        spapr_ccs_remove(spapr, ccs);
+    }
+}
+
+static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
+                                         sPAPRMachineState *spapr,
+                                         uint32_t token, uint32_t nargs,
+                                         target_ulong args, uint32_t nret,
+                                         target_ulong rets)
+{
+    uint64_t wa_addr;
+    uint64_t wa_offset;
+    uint32_t drc_index;
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    sPAPRConfigureConnectorState *ccs;
+    sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
+    int rc;
+    const void *fdt;
+
+    if (nargs != 2 || nret != 1) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
+
+    drc_index = rtas_ld(wa_addr, 0);
+    drc = spapr_dr_connector_by_index(drc_index);
+    if (!drc) {
+        trace_spapr_rtas_ibm_configure_connector_invalid(drc_index);
+        rc = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    fdt = drck->get_fdt(drc, NULL);
+    if (!fdt) {
+        trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
+        rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
+        goto out;
+    }
+
+    ccs = spapr_ccs_find(spapr, drc_index);
+    if (!ccs) {
+        ccs = g_new0(sPAPRConfigureConnectorState, 1);
+        (void)drck->get_fdt(drc, &ccs->fdt_offset);
+        ccs->drc_index = drc_index;
+        spapr_ccs_add(spapr, ccs);
+    }
+
+    do {
+        uint32_t tag;
+        const char *name;
+        const struct fdt_property *prop;
+        int fdt_offset_next, prop_len;
+
+        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
+
+        switch (tag) {
+        case FDT_BEGIN_NODE:
+            ccs->fdt_depth++;
+            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
+
+            /* provide the name of the next OF node */
+            wa_offset = CC_VAL_DATA_OFFSET;
+            rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
+            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
+            resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
+            break;
+        case FDT_END_NODE:
+            ccs->fdt_depth--;
+            if (ccs->fdt_depth == 0) {
+                /* done sending the device tree, don't need to track
+                 * the state anymore
+                 */
+                drck->set_configured(drc);
+                spapr_ccs_remove(spapr, ccs);
+                ccs = NULL;
+                resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
+            } else {
+                resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
+            }
+            break;
+        case FDT_PROP:
+            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
+                                              &prop_len);
+            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+
+            /* provide the name of the next OF property */
+            wa_offset = CC_VAL_DATA_OFFSET;
+            rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
+            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
+
+            /* provide the length and value of the OF property. data gets
+             * placed immediately after NULL terminator of the OF property's
+             * name string
+             */
+            wa_offset += strlen(name) + 1,
+            rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
+            rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
+            configure_connector_st(wa_addr, wa_offset, prop->data, prop_len);
+            resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
+            break;
+        case FDT_END:
+            resp = SPAPR_DR_CC_RESPONSE_ERROR;
+        default:
+            /* keep seeking for an actionable tag */
+            break;
+        }
+        if (ccs) {
+            ccs->fdt_offset = fdt_offset_next;
+        }
+    } while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
+
+    rc = resp;
+out:
+    rtas_st(rets, 0, rc);
+}
+
+static void spapr_drc_register_types(void)
+{
+    type_register_static(&spapr_dr_connector_info);
+
+    spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
+                        rtas_set_indicator);
+    spapr_rtas_register(RTAS_GET_SENSOR_STATE, "get-sensor-state",
+                        rtas_get_sensor_state);
+    spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
+                        rtas_ibm_configure_connector);
+}
+type_init(spapr_drc_register_types)
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 128d993..0bdb5fc 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -48,44 +48,6 @@
 #include "trace.h"
 #include "hw/ppc/fdt.h"
 
-static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
-                                                    uint32_t drc_index)
-{
-    sPAPRConfigureConnectorState *ccs = NULL;
-
-    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
-        if (ccs->drc_index == drc_index) {
-            break;
-        }
-    }
-
-    return ccs;
-}
-
-static void spapr_ccs_add(sPAPRMachineState *spapr,
-                          sPAPRConfigureConnectorState *ccs)
-{
-    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
-    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
-}
-
-static void spapr_ccs_remove(sPAPRMachineState *spapr,
-                             sPAPRConfigureConnectorState *ccs)
-{
-    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
-    g_free(ccs);
-}
-
-void spapr_ccs_reset_hook(void *opaque)
-{
-    sPAPRMachineState *spapr = opaque;
-    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
-
-    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
-        spapr_ccs_remove(spapr, ccs);
-    }
-}
-
 static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                    uint32_t token, uint32_t nargs,
                                    target_ulong args,
@@ -390,266 +352,6 @@ static void rtas_get_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     rtas_st(rets, 1, 100);
 }
 
-static bool sensor_type_is_dr(uint32_t sensor_type)
-{
-    switch (sensor_type) {
-    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
-    case RTAS_SENSOR_TYPE_DR:
-    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
-        return true;
-    }
-
-    return false;
-}
-
-static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
-                               uint32_t token, uint32_t nargs,
-                               target_ulong args, uint32_t nret,
-                               target_ulong rets)
-{
-    uint32_t sensor_type;
-    uint32_t sensor_index;
-    uint32_t sensor_state;
-    uint32_t ret = RTAS_OUT_SUCCESS;
-    sPAPRDRConnector *drc;
-    sPAPRDRConnectorClass *drck;
-
-    if (nargs != 3 || nret != 1) {
-        ret = RTAS_OUT_PARAM_ERROR;
-        goto out;
-    }
-
-    sensor_type = rtas_ld(args, 0);
-    sensor_index = rtas_ld(args, 1);
-    sensor_state = rtas_ld(args, 2);
-
-    if (!sensor_type_is_dr(sensor_type)) {
-        goto out_unimplemented;
-    }
-
-    /* if this is a DR sensor we can assume sensor_index == drc_index */
-    drc = spapr_dr_connector_by_index(sensor_index);
-    if (!drc) {
-        trace_spapr_rtas_set_indicator_invalid(sensor_index);
-        ret = RTAS_OUT_PARAM_ERROR;
-        goto out;
-    }
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-
-    switch (sensor_type) {
-    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
-                                                               sensor_index);
-            if (ccs) {
-                spapr_ccs_remove(spapr, ccs);
-            }
-        }
-        ret = drck->set_isolation_state(drc, sensor_state);
-        break;
-    case RTAS_SENSOR_TYPE_DR:
-        ret = drck->set_indicator_state(drc, sensor_state);
-        break;
-    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
-        ret = drck->set_allocation_state(drc, sensor_state);
-        break;
-    default:
-        goto out_unimplemented;
-    }
-
-out:
-    rtas_st(rets, 0, ret);
-    return;
-
-out_unimplemented:
-    /* currently only DR-related sensors are implemented */
-    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
-    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
-}
-
-static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
-                                  uint32_t token, uint32_t nargs,
-                                  target_ulong args, uint32_t nret,
-                                  target_ulong rets)
-{
-    uint32_t sensor_type;
-    uint32_t sensor_index;
-    uint32_t sensor_state = 0;
-    sPAPRDRConnector *drc;
-    sPAPRDRConnectorClass *drck;
-    uint32_t ret = RTAS_OUT_SUCCESS;
-
-    if (nargs != 2 || nret != 2) {
-        ret = RTAS_OUT_PARAM_ERROR;
-        goto out;
-    }
-
-    sensor_type = rtas_ld(args, 0);
-    sensor_index = rtas_ld(args, 1);
-
-    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
-        /* currently only DR-related sensors are implemented */
-        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
-                                                        sensor_type);
-        ret = RTAS_OUT_NOT_SUPPORTED;
-        goto out;
-    }
-
-    drc = spapr_dr_connector_by_index(sensor_index);
-    if (!drc) {
-        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
-        ret = RTAS_OUT_PARAM_ERROR;
-        goto out;
-    }
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    ret = drck->entity_sense(drc, &sensor_state);
-
-out:
-    rtas_st(rets, 0, ret);
-    rtas_st(rets, 1, sensor_state);
-}
-
-/* configure-connector work area offsets, int32_t units for field
- * indexes, bytes for field offset/len values.
- *
- * as documented by PAPR+ v2.7, 13.5.3.5
- */
-#define CC_IDX_NODE_NAME_OFFSET 2
-#define CC_IDX_PROP_NAME_OFFSET 2
-#define CC_IDX_PROP_LEN 3
-#define CC_IDX_PROP_DATA_OFFSET 4
-#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
-#define CC_WA_LEN 4096
-
-static void configure_connector_st(target_ulong addr, target_ulong offset,
-                                   const void *buf, size_t len)
-{
-    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
-                              buf, MIN(len, CC_WA_LEN - offset));
-}
-
-static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
-                                         sPAPRMachineState *spapr,
-                                         uint32_t token, uint32_t nargs,
-                                         target_ulong args, uint32_t nret,
-                                         target_ulong rets)
-{
-    uint64_t wa_addr;
-    uint64_t wa_offset;
-    uint32_t drc_index;
-    sPAPRDRConnector *drc;
-    sPAPRDRConnectorClass *drck;
-    sPAPRConfigureConnectorState *ccs;
-    sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
-    int rc;
-    const void *fdt;
-
-    if (nargs != 2 || nret != 1) {
-        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
-        return;
-    }
-
-    wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
-
-    drc_index = rtas_ld(wa_addr, 0);
-    drc = spapr_dr_connector_by_index(drc_index);
-    if (!drc) {
-        trace_spapr_rtas_ibm_configure_connector_invalid(drc_index);
-        rc = RTAS_OUT_PARAM_ERROR;
-        goto out;
-    }
-
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    fdt = drck->get_fdt(drc, NULL);
-    if (!fdt) {
-        trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
-        rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
-        goto out;
-    }
-
-    ccs = spapr_ccs_find(spapr, drc_index);
-    if (!ccs) {
-        ccs = g_new0(sPAPRConfigureConnectorState, 1);
-        (void)drck->get_fdt(drc, &ccs->fdt_offset);
-        ccs->drc_index = drc_index;
-        spapr_ccs_add(spapr, ccs);
-    }
-
-    do {
-        uint32_t tag;
-        const char *name;
-        const struct fdt_property *prop;
-        int fdt_offset_next, prop_len;
-
-        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
-
-        switch (tag) {
-        case FDT_BEGIN_NODE:
-            ccs->fdt_depth++;
-            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
-
-            /* provide the name of the next OF node */
-            wa_offset = CC_VAL_DATA_OFFSET;
-            rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
-            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
-            resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
-            break;
-        case FDT_END_NODE:
-            ccs->fdt_depth--;
-            if (ccs->fdt_depth == 0) {
-                /* done sending the device tree, don't need to track
-                 * the state anymore
-                 */
-                drck->set_configured(drc);
-                spapr_ccs_remove(spapr, ccs);
-                ccs = NULL;
-                resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
-            } else {
-                resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
-            }
-            break;
-        case FDT_PROP:
-            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
-                                              &prop_len);
-            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-
-            /* provide the name of the next OF property */
-            wa_offset = CC_VAL_DATA_OFFSET;
-            rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
-            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
-
-            /* provide the length and value of the OF property. data gets
-             * placed immediately after NULL terminator of the OF property's
-             * name string
-             */
-            wa_offset += strlen(name) + 1,
-            rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
-            rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
-            configure_connector_st(wa_addr, wa_offset, prop->data, prop_len);
-            resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
-            break;
-        case FDT_END:
-            resp = SPAPR_DR_CC_RESPONSE_ERROR;
-        default:
-            /* keep seeking for an actionable tag */
-            break;
-        }
-        if (ccs) {
-            ccs->fdt_offset = fdt_offset_next;
-        }
-    } while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
-
-    rc = resp;
-out:
-    rtas_st(rets, 0, rc);
-}
-
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;
@@ -791,12 +493,6 @@ static void core_rtas_register_types(void)
                         rtas_set_power_level);
     spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level",
                         rtas_get_power_level);
-    spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
-                        rtas_set_indicator);
-    spapr_rtas_register(RTAS_GET_SENSOR_STATE, "get-sensor-state",
-                        rtas_get_sensor_state);
-    spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
-                        rtas_ibm_configure_connector);
 }
 
 type_init(core_rtas_register_types)
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[Laurent Vivier]

On 01/06/2017 03:52, David Gibson wrote:
> Currently implementations of the RTAS calls related to DRCs are in
> spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> to related code, and we'll be able to make some more things local.
> 
> spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> that don't belong anywhere else, not every RTAS implementation.
> 
> Code motion only.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
>  2 files changed, 315 insertions(+), 311 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index cc2400b..ae8800d 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,6 +27,34 @@
>  #define DRC_INDEX_TYPE_SHIFT 28
>  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
>  
> +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> +                                                    uint32_t drc_index)
> +{
> +    sPAPRConfigureConnectorState *ccs = NULL;
> +
> +    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> +        if (ccs->drc_index == drc_index) {
> +            break;
> +        }
> +    }
> +
> +    return ccs;
> +}
> +
> +static void spapr_ccs_add(sPAPRMachineState *spapr,
> +                          sPAPRConfigureConnectorState *ccs)
> +{
> +    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> +    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> +}
> +
> +static void spapr_ccs_remove(sPAPRMachineState *spapr,
> +                             sPAPRConfigureConnectorState *ccs)
> +{
> +    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> +    g_free(ccs);
> +}
> +
>  static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
>  {
>      uint32_t shift = 0;
> @@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = {
>      .class_init    = spapr_dr_connector_class_init,
>  };
>  
> -static void spapr_drc_register_types(void)
> -{
> -    type_register_static(&spapr_dr_connector_info);
> -}
> -
> -type_init(spapr_drc_register_types)
> -
>  /* helper functions for external users */
>  
>  sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> @@ -932,3 +953,290 @@ out:
>  
>      return ret;
>  }
> +
> +/*
> + * RTAS calls
> + */
> +
> +static bool sensor_type_is_dr(uint32_t sensor_type)
> +{
> +    switch (sensor_type) {
> +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> +    case RTAS_SENSOR_TYPE_DR:
> +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                               uint32_t token, uint32_t nargs,
> +                               target_ulong args, uint32_t nret,
> +                               target_ulong rets)
> +{
> +    uint32_t sensor_type;
> +    uint32_t sensor_index;
> +    uint32_t sensor_state;
> +    uint32_t ret = RTAS_OUT_SUCCESS;
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +
> +    if (nargs != 3 || nret != 1) {
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    sensor_type = rtas_ld(args, 0);
> +    sensor_index = rtas_ld(args, 1);
> +    sensor_state = rtas_ld(args, 2);
> +
> +    if (!sensor_type_is_dr(sensor_type)) {
> +        goto out_unimplemented;
> +    }
> +
> +    /* if this is a DR sensor we can assume sensor_index == drc_index */
> +    drc = spapr_dr_connector_by_index(sensor_index);
> +    if (!drc) {
> +        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> +    switch (sensor_type) {
> +    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> +                                                               sensor_index);
> +            if (ccs) {
> +                spapr_ccs_remove(spapr, ccs);
> +            }
> +        }
> +        ret = drck->set_isolation_state(drc, sensor_state);
> +        break;
> +    case RTAS_SENSOR_TYPE_DR:
> +        ret = drck->set_indicator_state(drc, sensor_state);
> +        break;
> +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> +        ret = drck->set_allocation_state(drc, sensor_state);
> +        break;
> +    default:
> +        goto out_unimplemented;
> +    }
> +
> +out:
> +    rtas_st(rets, 0, ret);
> +    return;
> +
> +out_unimplemented:
> +    /* currently only DR-related sensors are implemented */
> +    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
> +    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> +}
> +
> +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                                  uint32_t token, uint32_t nargs,
> +                                  target_ulong args, uint32_t nret,
> +                                  target_ulong rets)
> +{
> +    uint32_t sensor_type;
> +    uint32_t sensor_index;
> +    uint32_t sensor_state = 0;
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    uint32_t ret = RTAS_OUT_SUCCESS;
> +
> +    if (nargs != 2 || nret != 2) {
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    sensor_type = rtas_ld(args, 0);
> +    sensor_index = rtas_ld(args, 1);
> +
> +    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
> +        /* currently only DR-related sensors are implemented */
> +        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
> +                                                        sensor_type);
> +        ret = RTAS_OUT_NOT_SUPPORTED;
> +        goto out;
> +    }
> +
> +    drc = spapr_dr_connector_by_index(sensor_index);
> +    if (!drc) {
> +        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    ret = drck->entity_sense(drc, &sensor_state);
> +
> +out:
> +    rtas_st(rets, 0, ret);
> +    rtas_st(rets, 1, sensor_state);
> +}
> +
> +/* configure-connector work area offsets, int32_t units for field
> + * indexes, bytes for field offset/len values.
> + *
> + * as documented by PAPR+ v2.7, 13.5.3.5
> + */
> +#define CC_IDX_NODE_NAME_OFFSET 2
> +#define CC_IDX_PROP_NAME_OFFSET 2
> +#define CC_IDX_PROP_LEN 3
> +#define CC_IDX_PROP_DATA_OFFSET 4
> +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> +#define CC_WA_LEN 4096
> +
> +static void configure_connector_st(target_ulong addr, target_ulong offset,
> +                                   const void *buf, size_t len)
> +{
> +    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> +                              buf, MIN(len, CC_WA_LEN - offset));
> +}
> +
> +void spapr_ccs_reset_hook(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> +
> +    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> +        spapr_ccs_remove(spapr, ccs);
> +    }
> +}


Why do you move this function in the middle of the "RTAS calls" whereas
previously it was with spapr_ccs_find(), spapr_ccs_add() and
spapr_ccs_remove() and it is only used by a reset handler in spapr.c?

Laurent

Re: [Qemu-devel] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[Michael Roth]

Quoting Laurent Vivier (2017-06-01 08:36:36)
> On 01/06/2017 03:52, David Gibson wrote:
> > Currently implementations of the RTAS calls related to DRCs are in
> > spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> > to related code, and we'll be able to make some more things local.
> > 
> > spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> > that don't belong anywhere else, not every RTAS implementation.
> > 
> > Code motion only.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
> >  2 files changed, 315 insertions(+), 311 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index cc2400b..ae8800d 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -27,6 +27,34 @@
> >  #define DRC_INDEX_TYPE_SHIFT 28
> >  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
> >  
> > +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> > +                                                    uint32_t drc_index)
> > +{
> > +    sPAPRConfigureConnectorState *ccs = NULL;
> > +
> > +    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> > +        if (ccs->drc_index == drc_index) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    return ccs;
> > +}
> > +
> > +static void spapr_ccs_add(sPAPRMachineState *spapr,
> > +                          sPAPRConfigureConnectorState *ccs)
> > +{
> > +    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> > +    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> > +}
> > +
> > +static void spapr_ccs_remove(sPAPRMachineState *spapr,
> > +                             sPAPRConfigureConnectorState *ccs)
> > +{
> > +    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> > +    g_free(ccs);
> > +}
> > +
> >  static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> >  {
> >      uint32_t shift = 0;
> > @@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = {
> >      .class_init    = spapr_dr_connector_class_init,
> >  };
> >  
> > -static void spapr_drc_register_types(void)
> > -{
> > -    type_register_static(&spapr_dr_connector_info);
> > -}
> > -
> > -type_init(spapr_drc_register_types)
> > -
> >  /* helper functions for external users */
> >  
> >  sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> > @@ -932,3 +953,290 @@ out:
> >  
> >      return ret;
> >  }
> > +
> > +/*
> > + * RTAS calls
> > + */
> > +
> > +static bool sensor_type_is_dr(uint32_t sensor_type)
> > +{
> > +    switch (sensor_type) {
> > +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> > +    case RTAS_SENSOR_TYPE_DR:
> > +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > +                               uint32_t token, uint32_t nargs,
> > +                               target_ulong args, uint32_t nret,
> > +                               target_ulong rets)
> > +{
> > +    uint32_t sensor_type;
> > +    uint32_t sensor_index;
> > +    uint32_t sensor_state;
> > +    uint32_t ret = RTAS_OUT_SUCCESS;
> > +    sPAPRDRConnector *drc;
> > +    sPAPRDRConnectorClass *drck;
> > +
> > +    if (nargs != 3 || nret != 1) {
> > +        ret = RTAS_OUT_PARAM_ERROR;
> > +        goto out;
> > +    }
> > +
> > +    sensor_type = rtas_ld(args, 0);
> > +    sensor_index = rtas_ld(args, 1);
> > +    sensor_state = rtas_ld(args, 2);
> > +
> > +    if (!sensor_type_is_dr(sensor_type)) {
> > +        goto out_unimplemented;
> > +    }
> > +
> > +    /* if this is a DR sensor we can assume sensor_index == drc_index */
> > +    drc = spapr_dr_connector_by_index(sensor_index);
> > +    if (!drc) {
> > +        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> > +        ret = RTAS_OUT_PARAM_ERROR;
> > +        goto out;
> > +    }
> > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > +    switch (sensor_type) {
> > +    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > +            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> > +                                                               sensor_index);
> > +            if (ccs) {
> > +                spapr_ccs_remove(spapr, ccs);
> > +            }
> > +        }
> > +        ret = drck->set_isolation_state(drc, sensor_state);
> > +        break;
> > +    case RTAS_SENSOR_TYPE_DR:
> > +        ret = drck->set_indicator_state(drc, sensor_state);
> > +        break;
> > +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > +        ret = drck->set_allocation_state(drc, sensor_state);
> > +        break;
> > +    default:
> > +        goto out_unimplemented;
> > +    }
> > +
> > +out:
> > +    rtas_st(rets, 0, ret);
> > +    return;
> > +
> > +out_unimplemented:
> > +    /* currently only DR-related sensors are implemented */
> > +    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
> > +    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> > +}
> > +
> > +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > +                                  uint32_t token, uint32_t nargs,
> > +                                  target_ulong args, uint32_t nret,
> > +                                  target_ulong rets)
> > +{
> > +    uint32_t sensor_type;
> > +    uint32_t sensor_index;
> > +    uint32_t sensor_state = 0;
> > +    sPAPRDRConnector *drc;
> > +    sPAPRDRConnectorClass *drck;
> > +    uint32_t ret = RTAS_OUT_SUCCESS;
> > +
> > +    if (nargs != 2 || nret != 2) {
> > +        ret = RTAS_OUT_PARAM_ERROR;
> > +        goto out;
> > +    }
> > +
> > +    sensor_type = rtas_ld(args, 0);
> > +    sensor_index = rtas_ld(args, 1);
> > +
> > +    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
> > +        /* currently only DR-related sensors are implemented */
> > +        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
> > +                                                        sensor_type);
> > +        ret = RTAS_OUT_NOT_SUPPORTED;
> > +        goto out;
> > +    }
> > +
> > +    drc = spapr_dr_connector_by_index(sensor_index);
> > +    if (!drc) {
> > +        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> > +        ret = RTAS_OUT_PARAM_ERROR;
> > +        goto out;
> > +    }
> > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +    ret = drck->entity_sense(drc, &sensor_state);
> > +
> > +out:
> > +    rtas_st(rets, 0, ret);
> > +    rtas_st(rets, 1, sensor_state);
> > +}
> > +
> > +/* configure-connector work area offsets, int32_t units for field
> > + * indexes, bytes for field offset/len values.
> > + *
> > + * as documented by PAPR+ v2.7, 13.5.3.5
> > + */
> > +#define CC_IDX_NODE_NAME_OFFSET 2
> > +#define CC_IDX_PROP_NAME_OFFSET 2
> > +#define CC_IDX_PROP_LEN 3
> > +#define CC_IDX_PROP_DATA_OFFSET 4
> > +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> > +#define CC_WA_LEN 4096
> > +
> > +static void configure_connector_st(target_ulong addr, target_ulong offset,
> > +                                   const void *buf, size_t len)
> > +{
> > +    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> > +                              buf, MIN(len, CC_WA_LEN - offset));
> > +}
> > +
> > +void spapr_ccs_reset_hook(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> > +
> > +    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> > +        spapr_ccs_remove(spapr, ccs);
> > +    }
> > +}
> 
> 
> Why do you move this function in the middle of the "RTAS calls" whereas
> previously it was with spapr_ccs_find(), spapr_ccs_add() and
> spapr_ccs_remove() and it is only used by a reset handler in spapr.c?

I think it's as good a spot as any after the move, and allows
spapr_ccs_* functions to remain static.

But all the CCS stuff got hung off of sPAPRMachineState in the first
place due to an attempt to separate "RTAS" state from the DRC state. Now
that we're treating both as internal state, I think a logical follow-up
would be to drop the CCS list completely and instead hang each instance
off of the corresponding DRC. At that point we'd probably want to move
the reset functionality into the DRC reset hook anyway.

> 
> Laurent
> 

Re: [Qemu-devel] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[David Gibson]

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

On Thu, Jun 01, 2017 at 11:05:37AM -0500, Michael Roth wrote:
> Quoting Laurent Vivier (2017-06-01 08:36:36)
> > On 01/06/2017 03:52, David Gibson wrote:
> > > Currently implementations of the RTAS calls related to DRCs are in
> > > spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> > > to related code, and we'll be able to make some more things local.
> > > 
> > > spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> > > that don't belong anywhere else, not every RTAS implementation.
> > > 
> > > Code motion only.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
> > >  2 files changed, 315 insertions(+), 311 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index cc2400b..ae8800d 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -27,6 +27,34 @@
> > >  #define DRC_INDEX_TYPE_SHIFT 28
> > >  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
> > >  
> > > +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> > > +                                                    uint32_t drc_index)
> > > +{
> > > +    sPAPRConfigureConnectorState *ccs = NULL;
> > > +
> > > +    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> > > +        if (ccs->drc_index == drc_index) {
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    return ccs;
> > > +}
> > > +
> > > +static void spapr_ccs_add(sPAPRMachineState *spapr,
> > > +                          sPAPRConfigureConnectorState *ccs)
> > > +{
> > > +    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> > > +    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> > > +}
> > > +
> > > +static void spapr_ccs_remove(sPAPRMachineState *spapr,
> > > +                             sPAPRConfigureConnectorState *ccs)
> > > +{
> > > +    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> > > +    g_free(ccs);
> > > +}
> > > +
> > >  static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> > >  {
> > >      uint32_t shift = 0;
> > > @@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = {
> > >      .class_init    = spapr_dr_connector_class_init,
> > >  };
> > >  
> > > -static void spapr_drc_register_types(void)
> > > -{
> > > -    type_register_static(&spapr_dr_connector_info);
> > > -}
> > > -
> > > -type_init(spapr_drc_register_types)
> > > -
> > >  /* helper functions for external users */
> > >  
> > >  sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> > > @@ -932,3 +953,290 @@ out:
> > >  
> > >      return ret;
> > >  }
> > > +
> > > +/*
> > > + * RTAS calls
> > > + */
> > > +
> > > +static bool sensor_type_is_dr(uint32_t sensor_type)
> > > +{
> > > +    switch (sensor_type) {
> > > +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> > > +    case RTAS_SENSOR_TYPE_DR:
> > > +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > > +        return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > +                               uint32_t token, uint32_t nargs,
> > > +                               target_ulong args, uint32_t nret,
> > > +                               target_ulong rets)
> > > +{
> > > +    uint32_t sensor_type;
> > > +    uint32_t sensor_index;
> > > +    uint32_t sensor_state;
> > > +    uint32_t ret = RTAS_OUT_SUCCESS;
> > > +    sPAPRDRConnector *drc;
> > > +    sPAPRDRConnectorClass *drck;
> > > +
> > > +    if (nargs != 3 || nret != 1) {
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +
> > > +    sensor_type = rtas_ld(args, 0);
> > > +    sensor_index = rtas_ld(args, 1);
> > > +    sensor_state = rtas_ld(args, 2);
> > > +
> > > +    if (!sensor_type_is_dr(sensor_type)) {
> > > +        goto out_unimplemented;
> > > +    }
> > > +
> > > +    /* if this is a DR sensor we can assume sensor_index == drc_index */
> > > +    drc = spapr_dr_connector_by_index(sensor_index);
> > > +    if (!drc) {
> > > +        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +
> > > +    switch (sensor_type) {
> > > +    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > > +            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> > > +                                                               sensor_index);
> > > +            if (ccs) {
> > > +                spapr_ccs_remove(spapr, ccs);
> > > +            }
> > > +        }
> > > +        ret = drck->set_isolation_state(drc, sensor_state);
> > > +        break;
> > > +    case RTAS_SENSOR_TYPE_DR:
> > > +        ret = drck->set_indicator_state(drc, sensor_state);
> > > +        break;
> > > +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > > +        ret = drck->set_allocation_state(drc, sensor_state);
> > > +        break;
> > > +    default:
> > > +        goto out_unimplemented;
> > > +    }
> > > +
> > > +out:
> > > +    rtas_st(rets, 0, ret);
> > > +    return;
> > > +
> > > +out_unimplemented:
> > > +    /* currently only DR-related sensors are implemented */
> > > +    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
> > > +    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> > > +}
> > > +
> > > +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > +                                  uint32_t token, uint32_t nargs,
> > > +                                  target_ulong args, uint32_t nret,
> > > +                                  target_ulong rets)
> > > +{
> > > +    uint32_t sensor_type;
> > > +    uint32_t sensor_index;
> > > +    uint32_t sensor_state = 0;
> > > +    sPAPRDRConnector *drc;
> > > +    sPAPRDRConnectorClass *drck;
> > > +    uint32_t ret = RTAS_OUT_SUCCESS;
> > > +
> > > +    if (nargs != 2 || nret != 2) {
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +
> > > +    sensor_type = rtas_ld(args, 0);
> > > +    sensor_index = rtas_ld(args, 1);
> > > +
> > > +    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
> > > +        /* currently only DR-related sensors are implemented */
> > > +        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
> > > +                                                        sensor_type);
> > > +        ret = RTAS_OUT_NOT_SUPPORTED;
> > > +        goto out;
> > > +    }
> > > +
> > > +    drc = spapr_dr_connector_by_index(sensor_index);
> > > +    if (!drc) {
> > > +        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +    ret = drck->entity_sense(drc, &sensor_state);
> > > +
> > > +out:
> > > +    rtas_st(rets, 0, ret);
> > > +    rtas_st(rets, 1, sensor_state);
> > > +}
> > > +
> > > +/* configure-connector work area offsets, int32_t units for field
> > > + * indexes, bytes for field offset/len values.
> > > + *
> > > + * as documented by PAPR+ v2.7, 13.5.3.5
> > > + */
> > > +#define CC_IDX_NODE_NAME_OFFSET 2
> > > +#define CC_IDX_PROP_NAME_OFFSET 2
> > > +#define CC_IDX_PROP_LEN 3
> > > +#define CC_IDX_PROP_DATA_OFFSET 4
> > > +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> > > +#define CC_WA_LEN 4096
> > > +
> > > +static void configure_connector_st(target_ulong addr, target_ulong offset,
> > > +                                   const void *buf, size_t len)
> > > +{
> > > +    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> > > +                              buf, MIN(len, CC_WA_LEN - offset));
> > > +}
> > > +
> > > +void spapr_ccs_reset_hook(void *opaque)
> > > +{
> > > +    sPAPRMachineState *spapr = opaque;
> > > +    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> > > +
> > > +    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> > > +        spapr_ccs_remove(spapr, ccs);
> > > +    }
> > > +}
> > 
> > 
> > Why do you move this function in the middle of the "RTAS calls" whereas
> > previously it was with spapr_ccs_find(), spapr_ccs_add() and
> > spapr_ccs_remove() and it is only used by a reset handler in spapr.c?
> 
> I think it's as good a spot as any after the move, and allows
> spapr_ccs_* functions to remain static.

Yeah, what he said.

> But all the CCS stuff got hung off of sPAPRMachineState in the first
> place due to an attempt to separate "RTAS" state from the DRC state.

Ah, right.  I think that was a mistake - RTAS is just an interface to
various subsystems; it doesn't have state of its own.

> Now
> that we're treating both as internal state, I think a logical follow-up
> would be to drop the CCS list completely and instead hang each instance
> off of the corresponding DRC. At that point we'd probably want to move
> the reset functionality into the DRC reset hook anyway.

I'll look into that.

-- 
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 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[Michael Roth]

Quoting David Gibson (2017-05-31 20:52:15)
> Currently implementations of the RTAS calls related to DRCs are in
> spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> to related code, and we'll be able to make some more things local.
> 
> spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> that don't belong anywhere else, not every RTAS implementation.
> 
> Code motion only.

Technically rtas-get-sensor-state and rtas-set-indicator aren't specific
to DRCs, but looking through the documented indicators/sensors (tables
40 and 42 in LoPAPR v11) it doesn't seem too likely we'll ever implement
any others so the move seems reasonable.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
>  2 files changed, 315 insertions(+), 311 deletions(-)

Re: [Qemu-devel] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[David Gibson]

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

On Thu, Jun 01, 2017 at 10:56:50AM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-05-31 20:52:15)
> > Currently implementations of the RTAS calls related to DRCs are in
> > spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> > to related code, and we'll be able to make some more things local.
> > 
> > spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> > that don't belong anywhere else, not every RTAS implementation.
> > 
> > Code motion only.
> 
> Technically rtas-get-sensor-state and rtas-set-indicator aren't specific
> to DRCs, but looking through the documented indicators/sensors (tables
> 40 and 42 in LoPAPR v11) it doesn't seem too likely we'll ever implement
> any others so the move seems reasonable.

True, I realised that a little after I posted.

Even if we did implement other sensors, though, the natural way to
split this would be to have the generic set-indicator function check
simply that the indicator is a DR related one, and pass the options
straight to a function in the DRC code, rather doing preliminary
looking into DRC internals as it does now.

-- 
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] [Qemu-ppc] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[Greg Kurz]

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

On Thu,  1 Jun 2017 11:52:15 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently implementations of the RTAS calls related to DRCs are in
> spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> to related code, and we'll be able to make some more things local.
> 
> spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> that don't belong anywhere else, not every RTAS implementation.
> 
> Code motion only.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
>  2 files changed, 315 insertions(+), 311 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index cc2400b..ae8800d 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,6 +27,34 @@
>  #define DRC_INDEX_TYPE_SHIFT 28
>  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
>  
> +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> +                                                    uint32_t drc_index)
> +{
> +    sPAPRConfigureConnectorState *ccs = NULL;
> +
> +    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> +        if (ccs->drc_index == drc_index) {
> +            break;
> +        }
> +    }
> +
> +    return ccs;
> +}
> +
> +static void spapr_ccs_add(sPAPRMachineState *spapr,
> +                          sPAPRConfigureConnectorState *ccs)
> +{
> +    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> +    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> +}
> +
> +static void spapr_ccs_remove(sPAPRMachineState *spapr,
> +                             sPAPRConfigureConnectorState *ccs)
> +{
> +    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> +    g_free(ccs);
> +}
> +
>  static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
>  {
>      uint32_t shift = 0;
> @@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = {
>      .class_init    = spapr_dr_connector_class_init,
>  };
>  
> -static void spapr_drc_register_types(void)
> -{
> -    type_register_static(&spapr_dr_connector_info);
> -}
> -
> -type_init(spapr_drc_register_types)
> -
>  /* helper functions for external users */
>  
>  sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> @@ -932,3 +953,290 @@ out:
>  
>      return ret;
>  }
> +
> +/*
> + * RTAS calls
> + */
> +
> +static bool sensor_type_is_dr(uint32_t sensor_type)
> +{
> +    switch (sensor_type) {
> +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> +    case RTAS_SENSOR_TYPE_DR:
> +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                               uint32_t token, uint32_t nargs,
> +                               target_ulong args, uint32_t nret,
> +                               target_ulong rets)
> +{
> +    uint32_t sensor_type;
> +    uint32_t sensor_index;
> +    uint32_t sensor_state;
> +    uint32_t ret = RTAS_OUT_SUCCESS;
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +
> +    if (nargs != 3 || nret != 1) {
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    sensor_type = rtas_ld(args, 0);
> +    sensor_index = rtas_ld(args, 1);
> +    sensor_state = rtas_ld(args, 2);
> +
> +    if (!sensor_type_is_dr(sensor_type)) {
> +        goto out_unimplemented;
> +    }
> +
> +    /* if this is a DR sensor we can assume sensor_index == drc_index */
> +    drc = spapr_dr_connector_by_index(sensor_index);
> +    if (!drc) {
> +        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> +    switch (sensor_type) {
> +    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> +                                                               sensor_index);
> +            if (ccs) {
> +                spapr_ccs_remove(spapr, ccs);
> +            }
> +        }
> +        ret = drck->set_isolation_state(drc, sensor_state);
> +        break;
> +    case RTAS_SENSOR_TYPE_DR:
> +        ret = drck->set_indicator_state(drc, sensor_state);
> +        break;
> +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> +        ret = drck->set_allocation_state(drc, sensor_state);
> +        break;
> +    default:
> +        goto out_unimplemented;
> +    }
> +
> +out:
> +    rtas_st(rets, 0, ret);
> +    return;
> +
> +out_unimplemented:
> +    /* currently only DR-related sensors are implemented */
> +    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
> +    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> +}
> +
> +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                                  uint32_t token, uint32_t nargs,
> +                                  target_ulong args, uint32_t nret,
> +                                  target_ulong rets)
> +{
> +    uint32_t sensor_type;
> +    uint32_t sensor_index;
> +    uint32_t sensor_state = 0;
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    uint32_t ret = RTAS_OUT_SUCCESS;
> +
> +    if (nargs != 2 || nret != 2) {
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    sensor_type = rtas_ld(args, 0);
> +    sensor_index = rtas_ld(args, 1);
> +
> +    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
> +        /* currently only DR-related sensors are implemented */
> +        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
> +                                                        sensor_type);
> +        ret = RTAS_OUT_NOT_SUPPORTED;
> +        goto out;
> +    }
> +
> +    drc = spapr_dr_connector_by_index(sensor_index);
> +    if (!drc) {
> +        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    ret = drck->entity_sense(drc, &sensor_state);
> +
> +out:
> +    rtas_st(rets, 0, ret);
> +    rtas_st(rets, 1, sensor_state);
> +}
> +
> +/* configure-connector work area offsets, int32_t units for field
> + * indexes, bytes for field offset/len values.
> + *
> + * as documented by PAPR+ v2.7, 13.5.3.5
> + */
> +#define CC_IDX_NODE_NAME_OFFSET 2
> +#define CC_IDX_PROP_NAME_OFFSET 2
> +#define CC_IDX_PROP_LEN 3
> +#define CC_IDX_PROP_DATA_OFFSET 4
> +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> +#define CC_WA_LEN 4096
> +
> +static void configure_connector_st(target_ulong addr, target_ulong offset,
> +                                   const void *buf, size_t len)
> +{
> +    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> +                              buf, MIN(len, CC_WA_LEN - offset));
> +}
> +
> +void spapr_ccs_reset_hook(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> +
> +    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> +        spapr_ccs_remove(spapr, ccs);
> +    }
> +}
> +
> +static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> +                                         sPAPRMachineState *spapr,
> +                                         uint32_t token, uint32_t nargs,
> +                                         target_ulong args, uint32_t nret,
> +                                         target_ulong rets)
> +{
> +    uint64_t wa_addr;
> +    uint64_t wa_offset;
> +    uint32_t drc_index;
> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +    sPAPRConfigureConnectorState *ccs;
> +    sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
> +    int rc;
> +    const void *fdt;
> +
> +    if (nargs != 2 || nret != 1) {
> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +        return;
> +    }
> +
> +    wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
> +
> +    drc_index = rtas_ld(wa_addr, 0);
> +    drc = spapr_dr_connector_by_index(drc_index);
> +    if (!drc) {
> +        trace_spapr_rtas_ibm_configure_connector_invalid(drc_index);
> +        rc = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    fdt = drck->get_fdt(drc, NULL);
> +    if (!fdt) {
> +        trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
> +        rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
> +        goto out;
> +    }
> +
> +    ccs = spapr_ccs_find(spapr, drc_index);
> +    if (!ccs) {
> +        ccs = g_new0(sPAPRConfigureConnectorState, 1);
> +        (void)drck->get_fdt(drc, &ccs->fdt_offset);
> +        ccs->drc_index = drc_index;
> +        spapr_ccs_add(spapr, ccs);
> +    }
> +
> +    do {
> +        uint32_t tag;
> +        const char *name;
> +        const struct fdt_property *prop;
> +        int fdt_offset_next, prop_len;
> +
> +        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
> +
> +        switch (tag) {
> +        case FDT_BEGIN_NODE:
> +            ccs->fdt_depth++;
> +            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
> +
> +            /* provide the name of the next OF node */
> +            wa_offset = CC_VAL_DATA_OFFSET;
> +            rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
> +            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
> +            resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
> +            break;
> +        case FDT_END_NODE:
> +            ccs->fdt_depth--;
> +            if (ccs->fdt_depth == 0) {
> +                /* done sending the device tree, don't need to track
> +                 * the state anymore
> +                 */
> +                drck->set_configured(drc);
> +                spapr_ccs_remove(spapr, ccs);
> +                ccs = NULL;
> +                resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> +            } else {
> +                resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
> +            }
> +            break;
> +        case FDT_PROP:
> +            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
> +                                              &prop_len);
> +            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> +
> +            /* provide the name of the next OF property */
> +            wa_offset = CC_VAL_DATA_OFFSET;
> +            rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
> +            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
> +
> +            /* provide the length and value of the OF property. data gets
> +             * placed immediately after NULL terminator of the OF property's
> +             * name string
> +             */
> +            wa_offset += strlen(name) + 1,
> +            rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
> +            rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
> +            configure_connector_st(wa_addr, wa_offset, prop->data, prop_len);
> +            resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
> +            break;
> +        case FDT_END:
> +            resp = SPAPR_DR_CC_RESPONSE_ERROR;
> +        default:
> +            /* keep seeking for an actionable tag */
> +            break;
> +        }
> +        if (ccs) {
> +            ccs->fdt_offset = fdt_offset_next;
> +        }
> +    } while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
> +
> +    rc = resp;
> +out:
> +    rtas_st(rets, 0, rc);
> +}
> +
> +static void spapr_drc_register_types(void)
> +{
> +    type_register_static(&spapr_dr_connector_info);
> +
> +    spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> +                        rtas_set_indicator);
> +    spapr_rtas_register(RTAS_GET_SENSOR_STATE, "get-sensor-state",
> +                        rtas_get_sensor_state);
> +    spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
> +                        rtas_ibm_configure_connector);
> +}
> +type_init(spapr_drc_register_types)
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 128d993..0bdb5fc 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -48,44 +48,6 @@
>  #include "trace.h"
>  #include "hw/ppc/fdt.h"
>  
> -static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> -                                                    uint32_t drc_index)
> -{
> -    sPAPRConfigureConnectorState *ccs = NULL;
> -
> -    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> -        if (ccs->drc_index == drc_index) {
> -            break;
> -        }
> -    }
> -
> -    return ccs;
> -}
> -
> -static void spapr_ccs_add(sPAPRMachineState *spapr,
> -                          sPAPRConfigureConnectorState *ccs)
> -{
> -    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> -    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> -}
> -
> -static void spapr_ccs_remove(sPAPRMachineState *spapr,
> -                             sPAPRConfigureConnectorState *ccs)
> -{
> -    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> -    g_free(ccs);
> -}
> -
> -void spapr_ccs_reset_hook(void *opaque)
> -{
> -    sPAPRMachineState *spapr = opaque;
> -    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> -
> -    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> -        spapr_ccs_remove(spapr, ccs);
> -    }
> -}
> -
>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                                     uint32_t token, uint32_t nargs,
>                                     target_ulong args,
> @@ -390,266 +352,6 @@ static void rtas_get_power_level(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      rtas_st(rets, 1, 100);
>  }
>  
> -static bool sensor_type_is_dr(uint32_t sensor_type)
> -{
> -    switch (sensor_type) {
> -    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> -    case RTAS_SENSOR_TYPE_DR:
> -    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> -        return true;
> -    }
> -
> -    return false;
> -}
> -
> -static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> -                               uint32_t token, uint32_t nargs,
> -                               target_ulong args, uint32_t nret,
> -                               target_ulong rets)
> -{
> -    uint32_t sensor_type;
> -    uint32_t sensor_index;
> -    uint32_t sensor_state;
> -    uint32_t ret = RTAS_OUT_SUCCESS;
> -    sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
> -
> -    if (nargs != 3 || nret != 1) {
> -        ret = RTAS_OUT_PARAM_ERROR;
> -        goto out;
> -    }
> -
> -    sensor_type = rtas_ld(args, 0);
> -    sensor_index = rtas_ld(args, 1);
> -    sensor_state = rtas_ld(args, 2);
> -
> -    if (!sensor_type_is_dr(sensor_type)) {
> -        goto out_unimplemented;
> -    }
> -
> -    /* if this is a DR sensor we can assume sensor_index == drc_index */
> -    drc = spapr_dr_connector_by_index(sensor_index);
> -    if (!drc) {
> -        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> -        ret = RTAS_OUT_PARAM_ERROR;
> -        goto out;
> -    }
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -
> -    switch (sensor_type) {
> -    case RTAS_SENSOR_TYPE_ISOLATION_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 (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> -                                                               sensor_index);
> -            if (ccs) {
> -                spapr_ccs_remove(spapr, ccs);
> -            }
> -        }
> -        ret = drck->set_isolation_state(drc, sensor_state);
> -        break;
> -    case RTAS_SENSOR_TYPE_DR:
> -        ret = drck->set_indicator_state(drc, sensor_state);
> -        break;
> -    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> -        ret = drck->set_allocation_state(drc, sensor_state);
> -        break;
> -    default:
> -        goto out_unimplemented;
> -    }
> -
> -out:
> -    rtas_st(rets, 0, ret);
> -    return;
> -
> -out_unimplemented:
> -    /* currently only DR-related sensors are implemented */
> -    trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
> -    rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> -}
> -
> -static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> -                                  uint32_t token, uint32_t nargs,
> -                                  target_ulong args, uint32_t nret,
> -                                  target_ulong rets)
> -{
> -    uint32_t sensor_type;
> -    uint32_t sensor_index;
> -    uint32_t sensor_state = 0;
> -    sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
> -    uint32_t ret = RTAS_OUT_SUCCESS;
> -
> -    if (nargs != 2 || nret != 2) {
> -        ret = RTAS_OUT_PARAM_ERROR;
> -        goto out;
> -    }
> -
> -    sensor_type = rtas_ld(args, 0);
> -    sensor_index = rtas_ld(args, 1);
> -
> -    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
> -        /* currently only DR-related sensors are implemented */
> -        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
> -                                                        sensor_type);
> -        ret = RTAS_OUT_NOT_SUPPORTED;
> -        goto out;
> -    }
> -
> -    drc = spapr_dr_connector_by_index(sensor_index);
> -    if (!drc) {
> -        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> -        ret = RTAS_OUT_PARAM_ERROR;
> -        goto out;
> -    }
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    ret = drck->entity_sense(drc, &sensor_state);
> -
> -out:
> -    rtas_st(rets, 0, ret);
> -    rtas_st(rets, 1, sensor_state);
> -}
> -
> -/* configure-connector work area offsets, int32_t units for field
> - * indexes, bytes for field offset/len values.
> - *
> - * as documented by PAPR+ v2.7, 13.5.3.5
> - */
> -#define CC_IDX_NODE_NAME_OFFSET 2
> -#define CC_IDX_PROP_NAME_OFFSET 2
> -#define CC_IDX_PROP_LEN 3
> -#define CC_IDX_PROP_DATA_OFFSET 4
> -#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> -#define CC_WA_LEN 4096
> -
> -static void configure_connector_st(target_ulong addr, target_ulong offset,
> -                                   const void *buf, size_t len)
> -{
> -    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> -                              buf, MIN(len, CC_WA_LEN - offset));
> -}
> -
> -static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> -                                         sPAPRMachineState *spapr,
> -                                         uint32_t token, uint32_t nargs,
> -                                         target_ulong args, uint32_t nret,
> -                                         target_ulong rets)
> -{
> -    uint64_t wa_addr;
> -    uint64_t wa_offset;
> -    uint32_t drc_index;
> -    sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
> -    sPAPRConfigureConnectorState *ccs;
> -    sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
> -    int rc;
> -    const void *fdt;
> -
> -    if (nargs != 2 || nret != 1) {
> -        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> -        return;
> -    }
> -
> -    wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
> -
> -    drc_index = rtas_ld(wa_addr, 0);
> -    drc = spapr_dr_connector_by_index(drc_index);
> -    if (!drc) {
> -        trace_spapr_rtas_ibm_configure_connector_invalid(drc_index);
> -        rc = RTAS_OUT_PARAM_ERROR;
> -        goto out;
> -    }
> -
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    fdt = drck->get_fdt(drc, NULL);
> -    if (!fdt) {
> -        trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
> -        rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
> -        goto out;
> -    }
> -
> -    ccs = spapr_ccs_find(spapr, drc_index);
> -    if (!ccs) {
> -        ccs = g_new0(sPAPRConfigureConnectorState, 1);
> -        (void)drck->get_fdt(drc, &ccs->fdt_offset);
> -        ccs->drc_index = drc_index;
> -        spapr_ccs_add(spapr, ccs);
> -    }
> -
> -    do {
> -        uint32_t tag;
> -        const char *name;
> -        const struct fdt_property *prop;
> -        int fdt_offset_next, prop_len;
> -
> -        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
> -
> -        switch (tag) {
> -        case FDT_BEGIN_NODE:
> -            ccs->fdt_depth++;
> -            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
> -
> -            /* provide the name of the next OF node */
> -            wa_offset = CC_VAL_DATA_OFFSET;
> -            rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
> -            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
> -            resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
> -            break;
> -        case FDT_END_NODE:
> -            ccs->fdt_depth--;
> -            if (ccs->fdt_depth == 0) {
> -                /* done sending the device tree, don't need to track
> -                 * the state anymore
> -                 */
> -                drck->set_configured(drc);
> -                spapr_ccs_remove(spapr, ccs);
> -                ccs = NULL;
> -                resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> -            } else {
> -                resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
> -            }
> -            break;
> -        case FDT_PROP:
> -            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
> -                                              &prop_len);
> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> -
> -            /* provide the name of the next OF property */
> -            wa_offset = CC_VAL_DATA_OFFSET;
> -            rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
> -            configure_connector_st(wa_addr, wa_offset, name, strlen(name) + 1);
> -
> -            /* provide the length and value of the OF property. data gets
> -             * placed immediately after NULL terminator of the OF property's
> -             * name string
> -             */
> -            wa_offset += strlen(name) + 1,
> -            rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
> -            rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
> -            configure_connector_st(wa_addr, wa_offset, prop->data, prop_len);
> -            resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
> -            break;
> -        case FDT_END:
> -            resp = SPAPR_DR_CC_RESPONSE_ERROR;
> -        default:
> -            /* keep seeking for an actionable tag */
> -            break;
> -        }
> -        if (ccs) {
> -            ccs->fdt_offset = fdt_offset_next;
> -        }
> -    } while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
> -
> -    rc = resp;
> -out:
> -    rtas_st(rets, 0, rc);
> -}
> -
>  static struct rtas_call {
>      const char *name;
>      spapr_rtas_fn fn;
> @@ -791,12 +493,6 @@ static void core_rtas_register_types(void)
>                          rtas_set_power_level);
>      spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level",
>                          rtas_get_power_level);
> -    spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> -                        rtas_set_indicator);
> -    spapr_rtas_register(RTAS_GET_SENSOR_STATE, "get-sensor-state",
> -                        rtas_get_sensor_state);
> -    spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
> -                        rtas_ibm_configure_connector);
>  }
>  
>  type_init(core_rtas_register_types)


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

[Qemu-devel] [PATCH 2/4] spapr: Abolish DRC get_fdt method

[David Gibson]

The DRConnectorClass includes a get_fdt method.  However
  * There's only one implementation, and there's only likely to ever be one
  * Both callers are local to spapr_drc
  * Each caller only uses one half of the actual implementation

So abolish get_fdt() entirely, and just open-code what we need.

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

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index ae8800d..f5b7b68 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -199,14 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
     return drc->name;
 }
 
-static const void *get_fdt(sPAPRDRConnector *drc, int *fdt_start_offset)
-{
-    if (fdt_start_offset) {
-        *fdt_start_offset = drc->fdt_start_offset;
-    }
-    return drc->fdt;
-}
-
 static void set_configured(sPAPRDRConnector *drc)
 {
     trace_spapr_drc_set_configured(get_index(drc));
@@ -753,7 +745,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     drck->get_index = get_index;
     drck->get_type = get_type;
     drck->get_name = get_name;
-    drck->get_fdt = get_fdt;
     drck->set_configured = set_configured;
     drck->entity_sense = entity_sense;
     drck->attach = attach;
@@ -1126,7 +1117,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
     sPAPRConfigureConnectorState *ccs;
     sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
     int rc;
-    const void *fdt;
 
     if (nargs != 2 || nret != 1) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -1144,8 +1134,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
     }
 
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    fdt = drck->get_fdt(drc, NULL);
-    if (!fdt) {
+    if (!drc->fdt) {
         trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
         rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
         goto out;
@@ -1154,7 +1143,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
     ccs = spapr_ccs_find(spapr, drc_index);
     if (!ccs) {
         ccs = g_new0(sPAPRConfigureConnectorState, 1);
-        (void)drck->get_fdt(drc, &ccs->fdt_offset);
+        ccs->fdt_offset = drc->fdt_start_offset;
         ccs->drc_index = drc_index;
         spapr_ccs_add(spapr, ccs);
     }
@@ -1165,12 +1154,12 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
         const struct fdt_property *prop;
         int fdt_offset_next, prop_len;
 
-        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
+        tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next);
 
         switch (tag) {
         case FDT_BEGIN_NODE:
             ccs->fdt_depth++;
-            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
+            name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL);
 
             /* provide the name of the next OF node */
             wa_offset = CC_VAL_DATA_OFFSET;
@@ -1193,9 +1182,9 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
             }
             break;
         case FDT_PROP:
-            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
+            prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset,
                                               &prop_len);
-            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+            name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff));
 
             /* provide the name of the next OF property */
             wa_offset = CC_VAL_DATA_OFFSET;
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 813b9ff..80db955 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -178,7 +178,6 @@ typedef struct sPAPRDRConnectorClass {
     uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
 
     /* QEMU interfaces for managing FDT/configure-connector */
-    const void *(*get_fdt)(sPAPRDRConnector *drc, int *fdt_start_offset);
     void (*set_configured)(sPAPRDRConnector *drc);
 
     /* QEMU interfaces for managing hotplug operations */
-- 
2.9.4

Re: [Qemu-devel] [PATCH 2/4] spapr: Abolish DRC get_fdt method

[Laurent Vivier]

On 01/06/2017 03:52, David Gibson wrote:
> The DRConnectorClass includes a get_fdt method.  However
>   * There's only one implementation, and there's only likely to ever be one
>   * Both callers are local to spapr_drc
>   * Each caller only uses one half of the actual implementation
> 
> So abolish get_fdt() entirely, and just open-code what we need.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr_drc.c         | 23 ++++++-----------------
>  include/hw/ppc/spapr_drc.h |  1 -
>  2 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index ae8800d..f5b7b68 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -199,14 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
>      return drc->name;
>  }
>  
> -static const void *get_fdt(sPAPRDRConnector *drc, int *fdt_start_offset)
> -{
> -    if (fdt_start_offset) {
> -        *fdt_start_offset = drc->fdt_start_offset;
> -    }
> -    return drc->fdt;
> -}
> -
>  static void set_configured(sPAPRDRConnector *drc)
>  {
>      trace_spapr_drc_set_configured(get_index(drc));
> @@ -753,7 +745,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->get_index = get_index;
>      drck->get_type = get_type;
>      drck->get_name = get_name;
> -    drck->get_fdt = get_fdt;
>      drck->set_configured = set_configured;
>      drck->entity_sense = entity_sense;
>      drck->attach = attach;
> @@ -1126,7 +1117,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      sPAPRConfigureConnectorState *ccs;
>      sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
>      int rc;
> -    const void *fdt;
>  
>      if (nargs != 2 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -1144,8 +1134,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      }
>  
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    fdt = drck->get_fdt(drc, NULL);
> -    if (!fdt) {
> +    if (!drc->fdt) {
>          trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
>          goto out;
> @@ -1154,7 +1143,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      ccs = spapr_ccs_find(spapr, drc_index);
>      if (!ccs) {
>          ccs = g_new0(sPAPRConfigureConnectorState, 1);
> -        (void)drck->get_fdt(drc, &ccs->fdt_offset);
> +        ccs->fdt_offset = drc->fdt_start_offset;
>          ccs->drc_index = drc_index;
>          spapr_ccs_add(spapr, ccs);
>      }
> @@ -1165,12 +1154,12 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          const struct fdt_property *prop;
>          int fdt_offset_next, prop_len;
>  
> -        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
> +        tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next);
>  
>          switch (tag) {
>          case FDT_BEGIN_NODE:
>              ccs->fdt_depth++;
> -            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
> +            name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL);
>  
>              /* provide the name of the next OF node */
>              wa_offset = CC_VAL_DATA_OFFSET;
> @@ -1193,9 +1182,9 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>              }
>              break;
>          case FDT_PROP:
> -            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
> +            prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset,
>                                                &prop_len);
> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> +            name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff));
>  
>              /* provide the name of the next OF property */
>              wa_offset = CC_VAL_DATA_OFFSET;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 813b9ff..80db955 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -178,7 +178,6 @@ typedef struct sPAPRDRConnectorClass {
>      uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
>  
>      /* QEMU interfaces for managing FDT/configure-connector */
> -    const void *(*get_fdt)(sPAPRDRConnector *drc, int *fdt_start_offset);
>      void (*set_configured)(sPAPRDRConnector *drc);
>  
>      /* QEMU interfaces for managing hotplug operations */
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] spapr: Abolish DRC get_fdt method

[Greg Kurz]

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

On Thu,  1 Jun 2017 11:52:16 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> The DRConnectorClass includes a get_fdt method.  However
>   * There's only one implementation, and there's only likely to ever be one
>   * Both callers are local to spapr_drc
>   * Each caller only uses one half of the actual implementation
> 
> So abolish get_fdt() entirely, and just open-code what we need.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_drc.c         | 23 ++++++-----------------
>  include/hw/ppc/spapr_drc.h |  1 -
>  2 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index ae8800d..f5b7b68 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -199,14 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
>      return drc->name;
>  }
>  
> -static const void *get_fdt(sPAPRDRConnector *drc, int *fdt_start_offset)
> -{
> -    if (fdt_start_offset) {
> -        *fdt_start_offset = drc->fdt_start_offset;
> -    }
> -    return drc->fdt;
> -}
> -
>  static void set_configured(sPAPRDRConnector *drc)
>  {
>      trace_spapr_drc_set_configured(get_index(drc));
> @@ -753,7 +745,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->get_index = get_index;
>      drck->get_type = get_type;
>      drck->get_name = get_name;
> -    drck->get_fdt = get_fdt;
>      drck->set_configured = set_configured;
>      drck->entity_sense = entity_sense;
>      drck->attach = attach;
> @@ -1126,7 +1117,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      sPAPRConfigureConnectorState *ccs;
>      sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
>      int rc;
> -    const void *fdt;
>  
>      if (nargs != 2 || nret != 1) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> @@ -1144,8 +1134,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      }
>  
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    fdt = drck->get_fdt(drc, NULL);
> -    if (!fdt) {
> +    if (!drc->fdt) {
>          trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
>          goto out;
> @@ -1154,7 +1143,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      ccs = spapr_ccs_find(spapr, drc_index);
>      if (!ccs) {
>          ccs = g_new0(sPAPRConfigureConnectorState, 1);
> -        (void)drck->get_fdt(drc, &ccs->fdt_offset);
> +        ccs->fdt_offset = drc->fdt_start_offset;
>          ccs->drc_index = drc_index;
>          spapr_ccs_add(spapr, ccs);
>      }
> @@ -1165,12 +1154,12 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          const struct fdt_property *prop;
>          int fdt_offset_next, prop_len;
>  
> -        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
> +        tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next);
>  
>          switch (tag) {
>          case FDT_BEGIN_NODE:
>              ccs->fdt_depth++;
> -            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
> +            name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL);
>  
>              /* provide the name of the next OF node */
>              wa_offset = CC_VAL_DATA_OFFSET;
> @@ -1193,9 +1182,9 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>              }
>              break;
>          case FDT_PROP:
> -            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
> +            prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset,
>                                                &prop_len);
> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> +            name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff));
>  
>              /* provide the name of the next OF property */
>              wa_offset = CC_VAL_DATA_OFFSET;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 813b9ff..80db955 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -178,7 +178,6 @@ typedef struct sPAPRDRConnectorClass {
>      uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
>  
>      /* QEMU interfaces for managing FDT/configure-connector */
> -    const void *(*get_fdt)(sPAPRDRConnector *drc, int *fdt_start_offset);
>      void (*set_configured)(sPAPRDRConnector *drc);
>  
>      /* QEMU interfaces for managing hotplug operations */


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

[Qemu-devel] [PATCH 3/4] spapr: Abolish DRC set_configured method

[David Gibson]

DRConnectorClass has a set_configured method, however:
  * There is only one implementation, and only ever likely to be one
  * There's exactly one caller, and that's (now) local
  * The implementation is very straightforward

So abolish the method entirely, and just open-code what we need.  We also
remove the tracepoints associated with it, since they don't look to be
terribly useful.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 20 ++++----------------
 hw/ppc/trace-events        |  2 --
 include/hw/ppc/spapr_drc.h |  3 ---
 3 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index f5b7b68..025453b 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -199,18 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
     return drc->name;
 }
 
-static void set_configured(sPAPRDRConnector *drc)
-{
-    trace_spapr_drc_set_configured(get_index(drc));
-
-    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
-        /* guest should be not configuring an isolated device */
-        trace_spapr_drc_set_configured_skipping(get_index(drc));
-        return;
-    }
-    drc->configured = true;
-}
-
 /* has the guest been notified of device attachment? */
 static void set_signalled(sPAPRDRConnector *drc)
 {
@@ -745,7 +733,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     drck->get_index = get_index;
     drck->get_type = get_type;
     drck->get_name = get_name;
-    drck->set_configured = set_configured;
     drck->entity_sense = entity_sense;
     drck->attach = attach;
     drck->detach = detach;
@@ -1113,7 +1100,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
     uint64_t wa_offset;
     uint32_t drc_index;
     sPAPRDRConnector *drc;
-    sPAPRDRConnectorClass *drck;
     sPAPRConfigureConnectorState *ccs;
     sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
     int rc;
@@ -1133,7 +1119,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
         goto out;
     }
 
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     if (!drc->fdt) {
         trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
         rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
@@ -1170,10 +1155,13 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
         case FDT_END_NODE:
             ccs->fdt_depth--;
             if (ccs->fdt_depth == 0) {
+                sPAPRDRIsolationState state = drc->isolation_state;
                 /* done sending the device tree, don't need to track
                  * the state anymore
                  */
-                drck->set_configured(drc);
+                if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
+                    drc->configured = true;
+                }
                 spapr_ccs_remove(spapr, ccs);
                 ccs = NULL;
                 resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 43d265f..96ffc02 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -42,8 +42,6 @@ spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
 spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
 spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
 spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
-spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
-spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
 spapr_drc_entity_sense(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
 spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
 spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 80db955..90f4953 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -177,9 +177,6 @@ typedef struct sPAPRDRConnectorClass {
 
     uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
 
-    /* QEMU interfaces for managing FDT/configure-connector */
-    void (*set_configured)(sPAPRDRConnector *drc);
-
     /* QEMU interfaces for managing hotplug operations */
     void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
                    int fdt_start_offset, bool coldplug, Error **errp);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 3/4] spapr: Abolish DRC set_configured method

[Laurent Vivier]

On 01/06/2017 03:52, David Gibson wrote:
> DRConnectorClass has a set_configured method, however:
>   * There is only one implementation, and only ever likely to be one
>   * There's exactly one caller, and that's (now) local
>   * The implementation is very straightforward
> 
> So abolish the method entirely, and just open-code what we need.  We also
> remove the tracepoints associated with it, since they don't look to be
> terribly useful.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr_drc.c         | 20 ++++----------------
>  hw/ppc/trace-events        |  2 --
>  include/hw/ppc/spapr_drc.h |  3 ---
>  3 files changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f5b7b68..025453b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -199,18 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
>      return drc->name;
>  }
>  
> -static void set_configured(sPAPRDRConnector *drc)
> -{
> -    trace_spapr_drc_set_configured(get_index(drc));
> -
> -    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> -        /* guest should be not configuring an isolated device */
> -        trace_spapr_drc_set_configured_skipping(get_index(drc));
> -        return;
> -    }
> -    drc->configured = true;
> -}
> -
>  /* has the guest been notified of device attachment? */
>  static void set_signalled(sPAPRDRConnector *drc)
>  {
> @@ -745,7 +733,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->get_index = get_index;
>      drck->get_type = get_type;
>      drck->get_name = get_name;
> -    drck->set_configured = set_configured;
>      drck->entity_sense = entity_sense;
>      drck->attach = attach;
>      drck->detach = detach;
> @@ -1113,7 +1100,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      uint64_t wa_offset;
>      uint32_t drc_index;
>      sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
>      sPAPRConfigureConnectorState *ccs;
>      sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
>      int rc;
> @@ -1133,7 +1119,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          goto out;
>      }
>  
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      if (!drc->fdt) {
>          trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
> @@ -1170,10 +1155,13 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          case FDT_END_NODE:
>              ccs->fdt_depth--;
>              if (ccs->fdt_depth == 0) {
> +                sPAPRDRIsolationState state = drc->isolation_state;
>                  /* done sending the device tree, don't need to track
>                   * the state anymore
>                   */
> -                drck->set_configured(drc);
> +                if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> +                    drc->configured = true;
> +                }
>                  spapr_ccs_remove(spapr, ccs);
>                  ccs = NULL;
>                  resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 43d265f..96ffc02 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -42,8 +42,6 @@ spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
>  spapr_drc_entity_sense(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 80db955..90f4953 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -177,9 +177,6 @@ typedef struct sPAPRDRConnectorClass {
>  
>      uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
>  
> -    /* QEMU interfaces for managing FDT/configure-connector */
> -    void (*set_configured)(sPAPRDRConnector *drc);
> -
>      /* QEMU interfaces for managing hotplug operations */
>      void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                     int fdt_start_offset, bool coldplug, Error **errp);
> 

Re: [Qemu-devel] [PATCH 3/4] spapr: Abolish DRC set_configured method

[Michael Roth]

Quoting David Gibson (2017-05-31 20:52:17)
> DRConnectorClass has a set_configured method, however:
>   * There is only one implementation, and only ever likely to be one
>   * There's exactly one caller, and that's (now) local
>   * The implementation is very straightforward
> 
> So abolish the method entirely, and just open-code what we need.  We also
> remove the tracepoints associated with it, since they don't look to be
> terribly useful.

Dropping the method makes sense, but the 'configured' state affects a
lot of the state-transitions throughout the code so I think it may
be useful to keep the traces.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c         | 20 ++++----------------
>  hw/ppc/trace-events        |  2 --
>  include/hw/ppc/spapr_drc.h |  3 ---
>  3 files changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f5b7b68..025453b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -199,18 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
>      return drc->name;
>  }
> 
> -static void set_configured(sPAPRDRConnector *drc)
> -{
> -    trace_spapr_drc_set_configured(get_index(drc));
> -
> -    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> -        /* guest should be not configuring an isolated device */
> -        trace_spapr_drc_set_configured_skipping(get_index(drc));
> -        return;
> -    }
> -    drc->configured = true;
> -}
> -
>  /* has the guest been notified of device attachment? */
>  static void set_signalled(sPAPRDRConnector *drc)
>  {
> @@ -745,7 +733,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->get_index = get_index;
>      drck->get_type = get_type;
>      drck->get_name = get_name;
> -    drck->set_configured = set_configured;
>      drck->entity_sense = entity_sense;
>      drck->attach = attach;
>      drck->detach = detach;
> @@ -1113,7 +1100,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      uint64_t wa_offset;
>      uint32_t drc_index;
>      sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
>      sPAPRConfigureConnectorState *ccs;
>      sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
>      int rc;
> @@ -1133,7 +1119,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          goto out;
>      }
> 
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      if (!drc->fdt) {
>          trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
> @@ -1170,10 +1155,13 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          case FDT_END_NODE:
>              ccs->fdt_depth--;
>              if (ccs->fdt_depth == 0) {
> +                sPAPRDRIsolationState state = drc->isolation_state;
>                  /* done sending the device tree, don't need to track
>                   * the state anymore
>                   */
> -                drck->set_configured(drc);
> +                if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> +                    drc->configured = true;
> +                }
>                  spapr_ccs_remove(spapr, ccs);
>                  ccs = NULL;
>                  resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 43d265f..96ffc02 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -42,8 +42,6 @@ spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
>  spapr_drc_entity_sense(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 80db955..90f4953 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -177,9 +177,6 @@ typedef struct sPAPRDRConnectorClass {
> 
>      uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
> 
> -    /* QEMU interfaces for managing FDT/configure-connector */
> -    void (*set_configured)(sPAPRDRConnector *drc);
> -
>      /* QEMU interfaces for managing hotplug operations */
>      void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                     int fdt_start_offset, bool coldplug, Error **errp);
> -- 
> 2.9.4
> 
> 

Re: [Qemu-devel] [PATCH 3/4] spapr: Abolish DRC set_configured method

[David Gibson]

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

On Thu, Jun 01, 2017 at 10:37:39AM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-05-31 20:52:17)
> > DRConnectorClass has a set_configured method, however:
> >   * There is only one implementation, and only ever likely to be one
> >   * There's exactly one caller, and that's (now) local
> >   * The implementation is very straightforward
> > 
> > So abolish the method entirely, and just open-code what we need.  We also
> > remove the tracepoints associated with it, since they don't look to be
> > terribly useful.
> 
> Dropping the method makes sense, but the 'configured' state affects a
> lot of the state-transitions throughout the code so I think it may
> be useful to keep the traces.

Fair point, I'll put the traces back in.

-- 
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] [Qemu-ppc] [PATCH 3/4] spapr: Abolish DRC set_configured method

[Greg Kurz]

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

On Thu,  1 Jun 2017 11:52:17 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> DRConnectorClass has a set_configured method, however:
>   * There is only one implementation, and only ever likely to be one
>   * There's exactly one caller, and that's (now) local
>   * The implementation is very straightforward
> 
> So abolish the method entirely, and just open-code what we need.  We also
> remove the tracepoints associated with it, since they don't look to be
> terribly useful.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_drc.c         | 20 ++++----------------
>  hw/ppc/trace-events        |  2 --
>  include/hw/ppc/spapr_drc.h |  3 ---
>  3 files changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f5b7b68..025453b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -199,18 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc)
>      return drc->name;
>  }
>  
> -static void set_configured(sPAPRDRConnector *drc)
> -{
> -    trace_spapr_drc_set_configured(get_index(drc));
> -
> -    if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> -        /* guest should be not configuring an isolated device */
> -        trace_spapr_drc_set_configured_skipping(get_index(drc));
> -        return;
> -    }
> -    drc->configured = true;
> -}
> -
>  /* has the guest been notified of device attachment? */
>  static void set_signalled(sPAPRDRConnector *drc)
>  {
> @@ -745,7 +733,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->get_index = get_index;
>      drck->get_type = get_type;
>      drck->get_name = get_name;
> -    drck->set_configured = set_configured;
>      drck->entity_sense = entity_sense;
>      drck->attach = attach;
>      drck->detach = detach;
> @@ -1113,7 +1100,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>      uint64_t wa_offset;
>      uint32_t drc_index;
>      sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
>      sPAPRConfigureConnectorState *ccs;
>      sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
>      int rc;
> @@ -1133,7 +1119,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          goto out;
>      }
>  
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      if (!drc->fdt) {
>          trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index);
>          rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE;
> @@ -1170,10 +1155,13 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>          case FDT_END_NODE:
>              ccs->fdt_depth--;
>              if (ccs->fdt_depth == 0) {
> +                sPAPRDRIsolationState state = drc->isolation_state;
>                  /* done sending the device tree, don't need to track
>                   * the state anymore
>                   */
> -                drck->set_configured(drc);
> +                if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> +                    drc->configured = true;
> +                }
>                  spapr_ccs_remove(spapr, ccs);
>                  ccs = NULL;
>                  resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 43d265f..96ffc02 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -42,8 +42,6 @@ spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
>  spapr_drc_entity_sense(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>  spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 80db955..90f4953 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -177,9 +177,6 @@ typedef struct sPAPRDRConnectorClass {
>  
>      uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
>  
> -    /* QEMU interfaces for managing FDT/configure-connector */
> -    void (*set_configured)(sPAPRDRConnector *drc);
> -
>      /* QEMU interfaces for managing hotplug operations */
>      void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                     int fdt_start_offset, bool coldplug, Error **errp);


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

[Qemu-devel] [PATCH 4/4] spapr: Make DRC get_index and get_type methods into plain functions

[David Gibson]

These two methods only have one implementation, and the spec they're
implementing means any other implementation is unlikely, verging on
impossible.

So replace them with simple functions.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c             | 13 +++-------
 hw/ppc/spapr_drc.c         | 61 ++++++++++++++++++++++------------------------
 hw/ppc/spapr_events.c      | 10 +++-----
 hw/ppc/spapr_pci.c         |  4 +--
 include/hw/ppc/spapr_drc.h |  5 ++--
 5 files changed, 41 insertions(+), 52 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ab3aab1..5d10366 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -456,15 +456,13 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
     int compat_smt = MIN(smp_threads, ppc_compat_max_threads(cpu));
     sPAPRDRConnector *drc;
-    sPAPRDRConnectorClass *drck;
     int drc_index;
     uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
     int i;
 
     drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
     if (drc) {
-        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-        drc_index = drck->get_index(drc);
+        drc_index = spapr_drc_index(drc);
         _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
     }
 
@@ -654,15 +652,13 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
 
         if (i >= hotplug_lmb_start) {
             sPAPRDRConnector *drc;
-            sPAPRDRConnectorClass *drck;
 
             drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i);
             g_assert(drc);
-            drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
             dynamic_memory[0] = cpu_to_be32(addr >> 32);
             dynamic_memory[1] = cpu_to_be32(addr & 0xffffffff);
-            dynamic_memory[2] = cpu_to_be32(drck->get_index(drc));
+            dynamic_memory[2] = cpu_to_be32(spapr_drc_index(drc));
             dynamic_memory[3] = cpu_to_be32(0); /* reserved */
             dynamic_memory[4] = cpu_to_be32(numa_get_node(addr, NULL));
             if (memory_region_present(get_system_memory(), addr)) {
@@ -2560,7 +2556,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
             drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
             spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
                                                    nr_lmbs,
-                                                   drck->get_index(drc));
+                                                   spapr_drc_index(drc));
         } else {
             spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB,
                                            nr_lmbs);
@@ -2770,8 +2766,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
                                    addr_start / SPAPR_MEMORY_BLOCK_SIZE);
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
-                                              nr_lmbs,
-                                              drck->get_index(drc));
+                                              nr_lmbs, spapr_drc_index(drc));
 out:
     error_propagate(errp, local_err);
 }
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 025453b..0c9a60d 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -70,7 +70,7 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
     return shift;
 }
 
-static uint32_t get_index(sPAPRDRConnector *drc)
+uint32_t spapr_drc_index(sPAPRDRConnector *drc)
 {
     /* no set format for a drc index: it only needs to be globally
      * unique. this is how we encode the DRC type on bare-metal
@@ -85,7 +85,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
-    trace_spapr_drc_set_isolation_state(get_index(drc), state);
+    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
 
     if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
         /* cannot unisolate a non-existent resource, and, or resources
@@ -126,11 +126,12 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
          * 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(get_index(drc));
+                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
                 drck->detach(drc, DEVICE(drc->dev), NULL);
             } else {
-                trace_spapr_drc_set_isolation_state_deferring(get_index(drc));
+                trace_spapr_drc_set_isolation_state_deferring(drc_index);
             }
         }
         drc->configured = false;
@@ -142,7 +143,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
 static uint32_t set_indicator_state(sPAPRDRConnector *drc,
                                     sPAPRDRIndicatorState state)
 {
-    trace_spapr_drc_set_indicator_state(get_index(drc), state);
+    trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
     drc->indicator_state = state;
     return RTAS_OUT_SUCCESS;
 }
@@ -152,7 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
-    trace_spapr_drc_set_allocation_state(get_index(drc), state);
+    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
@@ -180,7 +181,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
         drc->allocation_state = state;
         if (drc->awaiting_release &&
             drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-            trace_spapr_drc_set_allocation_state_finalizing(get_index(drc));
+            uint32_t drc_index = spapr_drc_index(drc);
+            trace_spapr_drc_set_allocation_state_finalizing(drc_index);
             drck->detach(drc, DEVICE(drc->dev), NULL);
         } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
             drc->awaiting_allocation = false;
@@ -189,7 +191,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
     return RTAS_OUT_SUCCESS;
 }
 
-static uint32_t get_type(sPAPRDRConnector *drc)
+sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
 {
     return drc->type;
 }
@@ -243,7 +245,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
         }
     }
 
-    trace_spapr_drc_entity_sense(get_index(drc), *state);
+    trace_spapr_drc_entity_sense(spapr_drc_index(drc), *state);
     return RTAS_OUT_SUCCESS;
 }
 
@@ -251,8 +253,7 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
                            void *opaque, Error **errp)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    uint32_t value = (uint32_t)drck->get_index(drc);
+    uint32_t value = spapr_drc_index(drc);
     visit_type_uint32(v, name, &value, errp);
 }
 
@@ -260,8 +261,7 @@ static void prop_get_type(Object *obj, Visitor *v, const char *name,
                           void *opaque, Error **errp)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    uint32_t value = (uint32_t)drck->get_type(drc);
+    uint32_t value = (uint32_t)spapr_drc_type(drc);
     visit_type_uint32(v, name, &value, errp);
 }
 
@@ -362,7 +362,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
 static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
                    int fdt_start_offset, bool coldplug, Error **errp)
 {
-    trace_spapr_drc_attach(get_index(drc));
+    trace_spapr_drc_attach(spapr_drc_index(drc));
 
     if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
         error_setg(errp, "an attached device is still awaiting release");
@@ -413,7 +413,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
 
 static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
 {
-    trace_spapr_drc_detach(get_index(drc));
+    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
@@ -436,14 +436,14 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
     }
 
     if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-        trace_spapr_drc_awaiting_isolated(get_index(drc));
+        trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
         drc->awaiting_release = true;
         return;
     }
 
     if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
         drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-        trace_spapr_drc_awaiting_unusable(get_index(drc));
+        trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
         drc->awaiting_release = true;
         return;
     }
@@ -451,7 +451,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
     if (drc->awaiting_allocation) {
         if (!drc->awaiting_allocation_skippable) {
             drc->awaiting_release = true;
-            trace_spapr_drc_awaiting_allocation(get_index(drc));
+            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
             return;
         }
     }
@@ -495,7 +495,7 @@ static void reset(DeviceState *d)
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     sPAPRDREntitySense state;
 
-    trace_spapr_drc_reset(drck->get_index(drc));
+    trace_spapr_drc_reset(spapr_drc_index(drc));
     /* 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
@@ -584,13 +584,12 @@ static const VMStateDescription vmstate_spapr_drc = {
 static void realize(DeviceState *d, Error **errp)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     Object *root_container;
     char link_name[256];
     gchar *child_name;
     Error *err = NULL;
 
-    trace_spapr_drc_realize(drck->get_index(drc));
+    trace_spapr_drc_realize(spapr_drc_index(drc));
     /* NOTE: we do this as part of realize/unrealize due to the fact
      * that the guest will communicate with the DRC via RTAS calls
      * referencing the global DRC index. By unlinking the DRC
@@ -599,9 +598,9 @@ static void realize(DeviceState *d, Error **errp)
      * existing in the composition tree
      */
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
-    snprintf(link_name, sizeof(link_name), "%x", drck->get_index(drc));
+    snprintf(link_name, sizeof(link_name), "%x", spapr_drc_index(drc));
     child_name = object_get_canonical_path_component(OBJECT(drc));
-    trace_spapr_drc_realize_child(drck->get_index(drc), child_name);
+    trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
     object_property_add_alias(root_container, link_name,
                               drc->owner, child_name, &err);
     if (err) {
@@ -609,22 +608,21 @@ static void realize(DeviceState *d, Error **errp)
         object_unref(OBJECT(drc));
     }
     g_free(child_name);
-    vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
+    vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
                      drc);
-    trace_spapr_drc_realize_complete(drck->get_index(drc));
+    trace_spapr_drc_realize_complete(spapr_drc_index(drc));
 }
 
 static void unrealize(DeviceState *d, Error **errp)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     Object *root_container;
     char name[256];
     Error *err = NULL;
 
-    trace_spapr_drc_unrealize(drck->get_index(drc));
+    trace_spapr_drc_unrealize(spapr_drc_index(drc));
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
-    snprintf(name, sizeof(name), "%x", drck->get_index(drc));
+    snprintf(name, sizeof(name), "%x", spapr_drc_index(drc));
     object_property_del(root_container, name, &err);
     if (err) {
         error_report_err(err);
@@ -645,7 +643,8 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
     drc->type = type;
     drc->id = id;
     drc->owner = owner;
-    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
+    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
+                                spapr_drc_index(drc));
     object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
     object_property_set_bool(OBJECT(drc), true, "realized", NULL);
     g_free(prop_name);
@@ -730,8 +729,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     drck->set_isolation_state = set_isolation_state;
     drck->set_indicator_state = set_indicator_state;
     drck->set_allocation_state = set_allocation_state;
-    drck->get_index = get_index;
-    drck->get_type = get_type;
     drck->get_name = get_name;
     drck->entity_sense = entity_sense;
     drck->attach = attach;
@@ -868,7 +865,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
         drc_count++;
 
         /* ibm,drc-indexes */
-        drc_index = cpu_to_be32(drck->get_index(drc));
+        drc_index = cpu_to_be32(spapr_drc_index(drc));
         g_array_append_val(drc_indexes, drc_index);
 
         /* ibm,drc-power-domains */
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 73e2a18..6b01b04 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -571,22 +571,20 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
 
 void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc)
 {
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    sPAPRDRConnectorType drc_type = drck->get_type(drc);
+    sPAPRDRConnectorType drc_type = spapr_drc_type(drc);
     union drc_identifier drc_id;
 
-    drc_id.index = drck->get_index(drc);
+    drc_id.index = spapr_drc_index(drc);
     spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
                             RTAS_LOG_V6_HP_ACTION_ADD, drc_type, &drc_id);
 }
 
 void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc)
 {
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    sPAPRDRConnectorType drc_type = drck->get_type(drc);
+    sPAPRDRConnectorType drc_type = spapr_drc_type(drc);
     union drc_identifier drc_id;
 
-    drc_id.index = drck->get_index(drc);
+    drc_id.index = spapr_drc_index(drc);
     spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
                             RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, &drc_id);
 }
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e4daf8d..7a208a7 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1417,14 +1417,12 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
                                             PCIDevice *pdev)
 {
     sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
-    sPAPRDRConnectorClass *drck;
 
     if (!drc) {
         return 0;
     }
 
-    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    return drck->get_index(drc);
+    return spapr_drc_index(drc);
 }
 
 static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 90f4953..10e7c24 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -171,8 +171,6 @@ typedef struct sPAPRDRConnectorClass {
                                     sPAPRDRIndicatorState state);
     uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
                                      sPAPRDRAllocationState state);
-    uint32_t (*get_index)(sPAPRDRConnector *drc);
-    uint32_t (*get_type)(sPAPRDRConnector *drc);
     const char *(*get_name)(sPAPRDRConnector *drc);
 
     uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
@@ -185,6 +183,9 @@ typedef struct sPAPRDRConnectorClass {
     void (*set_signalled)(sPAPRDRConnector *drc);
 } sPAPRDRConnectorClass;
 
+uint32_t spapr_drc_index(sPAPRDRConnector *drc);
+sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
+
 sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
                                          sPAPRDRConnectorType type,
                                          uint32_t id);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 4/4] spapr: Make DRC get_index and get_type methods into plain functions

[Laurent Vivier]

On 01/06/2017 03:52, David Gibson wrote:
> These two methods only have one implementation, and the spec they're
> implementing means any other implementation is unlikely, verging on
> impossible.
> 
> So replace them with simple functions.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr.c             | 13 +++-------
>  hw/ppc/spapr_drc.c         | 61 ++++++++++++++++++++++------------------------
>  hw/ppc/spapr_events.c      | 10 +++-----
>  hw/ppc/spapr_pci.c         |  4 +--
>  include/hw/ppc/spapr_drc.h |  5 ++--
>  5 files changed, 41 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab3aab1..5d10366 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -456,15 +456,13 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
>      int compat_smt = MIN(smp_threads, ppc_compat_max_threads(cpu));
>      sPAPRDRConnector *drc;
> -    sPAPRDRConnectorClass *drck;
>      int drc_index;
>      uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
>      int i;
>  
>      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
>      if (drc) {
> -        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -        drc_index = drck->get_index(drc);
> +        drc_index = spapr_drc_index(drc);
>          _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
>      }
>  
> @@ -654,15 +652,13 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
>  
>          if (i >= hotplug_lmb_start) {
>              sPAPRDRConnector *drc;
> -            sPAPRDRConnectorClass *drck;
>  
>              drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i);
>              g_assert(drc);
> -            drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
>              dynamic_memory[0] = cpu_to_be32(addr >> 32);
>              dynamic_memory[1] = cpu_to_be32(addr & 0xffffffff);
> -            dynamic_memory[2] = cpu_to_be32(drck->get_index(drc));
> +            dynamic_memory[2] = cpu_to_be32(spapr_drc_index(drc));
>              dynamic_memory[3] = cpu_to_be32(0); /* reserved */
>              dynamic_memory[4] = cpu_to_be32(numa_get_node(addr, NULL));
>              if (memory_region_present(get_system_memory(), addr)) {
> @@ -2560,7 +2556,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>              drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>              spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
>                                                     nr_lmbs,
> -                                                   drck->get_index(drc));
> +                                                   spapr_drc_index(drc));
>          } else {
>              spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB,
>                                             nr_lmbs);
> @@ -2770,8 +2766,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>                                     addr_start / SPAPR_MEMORY_BLOCK_SIZE);
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -                                              nr_lmbs,
> -                                              drck->get_index(drc));
> +                                              nr_lmbs, spapr_drc_index(drc));
>  out:
>      error_propagate(errp, local_err);
>  }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 025453b..0c9a60d 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -70,7 +70,7 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
>      return shift;
>  }
>  
> -static uint32_t get_index(sPAPRDRConnector *drc)
> +uint32_t spapr_drc_index(sPAPRDRConnector *drc)
>  {
>      /* no set format for a drc index: it only needs to be globally
>       * unique. this is how we encode the DRC type on bare-metal
> @@ -85,7 +85,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> -    trace_spapr_drc_set_isolation_state(get_index(drc), state);
> +    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
>  
>      if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
>          /* cannot unisolate a non-existent resource, and, or resources
> @@ -126,11 +126,12 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>           * 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(get_index(drc));
> +                trace_spapr_drc_set_isolation_state_finalizing(drc_index);
>                  drck->detach(drc, DEVICE(drc->dev), NULL);
>              } else {
> -                trace_spapr_drc_set_isolation_state_deferring(get_index(drc));
> +                trace_spapr_drc_set_isolation_state_deferring(drc_index);
>              }
>          }
>          drc->configured = false;
> @@ -142,7 +143,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>  static uint32_t set_indicator_state(sPAPRDRConnector *drc,
>                                      sPAPRDRIndicatorState state)
>  {
> -    trace_spapr_drc_set_indicator_state(get_index(drc), state);
> +    trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
>      drc->indicator_state = state;
>      return RTAS_OUT_SUCCESS;
>  }
> @@ -152,7 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> -    trace_spapr_drc_set_allocation_state(get_index(drc), state);
> +    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
> @@ -180,7 +181,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
>          drc->allocation_state = state;
>          if (drc->awaiting_release &&
>              drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -            trace_spapr_drc_set_allocation_state_finalizing(get_index(drc));
> +            uint32_t drc_index = spapr_drc_index(drc);
> +            trace_spapr_drc_set_allocation_state_finalizing(drc_index);
>              drck->detach(drc, DEVICE(drc->dev), NULL);
>          } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
>              drc->awaiting_allocation = false;
> @@ -189,7 +191,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
>      return RTAS_OUT_SUCCESS;
>  }
>  
> -static uint32_t get_type(sPAPRDRConnector *drc)
> +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
>  {
>      return drc->type;
>  }
> @@ -243,7 +245,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
>          }
>      }
>  
> -    trace_spapr_drc_entity_sense(get_index(drc), *state);
> +    trace_spapr_drc_entity_sense(spapr_drc_index(drc), *state);
>      return RTAS_OUT_SUCCESS;
>  }
>  
> @@ -251,8 +253,7 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
>                             void *opaque, Error **errp)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    uint32_t value = (uint32_t)drck->get_index(drc);
> +    uint32_t value = spapr_drc_index(drc);
>      visit_type_uint32(v, name, &value, errp);
>  }
>  
> @@ -260,8 +261,7 @@ static void prop_get_type(Object *obj, Visitor *v, const char *name,
>                            void *opaque, Error **errp)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    uint32_t value = (uint32_t)drck->get_type(drc);
> +    uint32_t value = (uint32_t)spapr_drc_type(drc);
>      visit_type_uint32(v, name, &value, errp);
>  }
>  
> @@ -362,7 +362,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>  static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                     int fdt_start_offset, bool coldplug, Error **errp)
>  {
> -    trace_spapr_drc_attach(get_index(drc));
> +    trace_spapr_drc_attach(spapr_drc_index(drc));
>  
>      if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
>          error_setg(errp, "an attached device is still awaiting release");
> @@ -413,7 +413,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>  {
> -    trace_spapr_drc_detach(get_index(drc));
> +    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
> @@ -436,14 +436,14 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      }
>  
>      if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -        trace_spapr_drc_awaiting_isolated(get_index(drc));
> +        trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
>          drc->awaiting_release = true;
>          return;
>      }
>  
>      if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
>          drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> -        trace_spapr_drc_awaiting_unusable(get_index(drc));
> +        trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
>          drc->awaiting_release = true;
>          return;
>      }
> @@ -451,7 +451,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      if (drc->awaiting_allocation) {
>          if (!drc->awaiting_allocation_skippable) {
>              drc->awaiting_release = true;
> -            trace_spapr_drc_awaiting_allocation(get_index(drc));
> +            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
>              return;
>          }
>      }
> @@ -495,7 +495,7 @@ static void reset(DeviceState *d)
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      sPAPRDREntitySense state;
>  
> -    trace_spapr_drc_reset(drck->get_index(drc));
> +    trace_spapr_drc_reset(spapr_drc_index(drc));
>      /* 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
> @@ -584,13 +584,12 @@ static const VMStateDescription vmstate_spapr_drc = {
>  static void realize(DeviceState *d, Error **errp)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      Object *root_container;
>      char link_name[256];
>      gchar *child_name;
>      Error *err = NULL;
>  
> -    trace_spapr_drc_realize(drck->get_index(drc));
> +    trace_spapr_drc_realize(spapr_drc_index(drc));
>      /* NOTE: we do this as part of realize/unrealize due to the fact
>       * that the guest will communicate with the DRC via RTAS calls
>       * referencing the global DRC index. By unlinking the DRC
> @@ -599,9 +598,9 @@ static void realize(DeviceState *d, Error **errp)
>       * existing in the composition tree
>       */
>      root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> -    snprintf(link_name, sizeof(link_name), "%x", drck->get_index(drc));
> +    snprintf(link_name, sizeof(link_name), "%x", spapr_drc_index(drc));
>      child_name = object_get_canonical_path_component(OBJECT(drc));
> -    trace_spapr_drc_realize_child(drck->get_index(drc), child_name);
> +    trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
>      object_property_add_alias(root_container, link_name,
>                                drc->owner, child_name, &err);
>      if (err) {
> @@ -609,22 +608,21 @@ static void realize(DeviceState *d, Error **errp)
>          object_unref(OBJECT(drc));
>      }
>      g_free(child_name);
> -    vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
> +    vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>                       drc);
> -    trace_spapr_drc_realize_complete(drck->get_index(drc));
> +    trace_spapr_drc_realize_complete(spapr_drc_index(drc));
>  }
>  
>  static void unrealize(DeviceState *d, Error **errp)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      Object *root_container;
>      char name[256];
>      Error *err = NULL;
>  
> -    trace_spapr_drc_unrealize(drck->get_index(drc));
> +    trace_spapr_drc_unrealize(spapr_drc_index(drc));
>      root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> -    snprintf(name, sizeof(name), "%x", drck->get_index(drc));
> +    snprintf(name, sizeof(name), "%x", spapr_drc_index(drc));
>      object_property_del(root_container, name, &err);
>      if (err) {
>          error_report_err(err);
> @@ -645,7 +643,8 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>      drc->type = type;
>      drc->id = id;
>      drc->owner = owner;
> -    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
> +    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> +                                spapr_drc_index(drc));
>      object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
>      object_property_set_bool(OBJECT(drc), true, "realized", NULL);
>      g_free(prop_name);
> @@ -730,8 +729,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      drck->set_isolation_state = set_isolation_state;
>      drck->set_indicator_state = set_indicator_state;
>      drck->set_allocation_state = set_allocation_state;
> -    drck->get_index = get_index;
> -    drck->get_type = get_type;
>      drck->get_name = get_name;
>      drck->entity_sense = entity_sense;
>      drck->attach = attach;
> @@ -868,7 +865,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>          drc_count++;
>  
>          /* ibm,drc-indexes */
> -        drc_index = cpu_to_be32(drck->get_index(drc));
> +        drc_index = cpu_to_be32(spapr_drc_index(drc));
>          g_array_append_val(drc_indexes, drc_index);
>  
>          /* ibm,drc-power-domains */
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 73e2a18..6b01b04 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -571,22 +571,20 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>  
>  void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc)
>  {
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> +    sPAPRDRConnectorType drc_type = spapr_drc_type(drc);
>      union drc_identifier drc_id;
>  
> -    drc_id.index = drck->get_index(drc);
> +    drc_id.index = spapr_drc_index(drc);
>      spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
>                              RTAS_LOG_V6_HP_ACTION_ADD, drc_type, &drc_id);
>  }
>  
>  void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc)
>  {
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> +    sPAPRDRConnectorType drc_type = spapr_drc_type(drc);
>      union drc_identifier drc_id;
>  
> -    drc_id.index = drck->get_index(drc);
> +    drc_id.index = spapr_drc_index(drc);
>      spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
>                              RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, &drc_id);
>  }
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e4daf8d..7a208a7 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1417,14 +1417,12 @@ static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
>                                              PCIDevice *pdev)
>  {
>      sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
> -    sPAPRDRConnectorClass *drck;
>  
>      if (!drc) {
>          return 0;
>      }
>  
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    return drck->get_index(drc);
> +    return spapr_drc_index(drc);
>  }
>  
>  static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 90f4953..10e7c24 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -171,8 +171,6 @@ typedef struct sPAPRDRConnectorClass {
>                                      sPAPRDRIndicatorState state);
>      uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
>                                       sPAPRDRAllocationState state);
> -    uint32_t (*get_index)(sPAPRDRConnector *drc);
> -    uint32_t (*get_type)(sPAPRDRConnector *drc);
>      const char *(*get_name)(sPAPRDRConnector *drc);
>  
>      uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
> @@ -185,6 +183,9 @@ typedef struct sPAPRDRConnectorClass {
>      void (*set_signalled)(sPAPRDRConnector *drc);
>  } sPAPRDRConnectorClass;
>  
> +uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
> +
>  sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>                                           sPAPRDRConnectorType type,
>                                           uint32_t id);
> 

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

[Bharata B Rao]

On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
> The code managing DRCs[0] has quite a few things that are more
> complicated than they need to be.  In particular the object
> representing a DRC has a bunch of method pointers, despite the fact
> that there are currently no subclasses, and even if there were the
> method implementations would be unlikely to differ.

So you are getting rid of a few methods. How about other methods ?
Specially attach and detach which have incorporated all the logic needed
to handle logical and physical DRs into their implementations ?

Guess these will be follow in subequent parts ? 

Regards,
Bharata.

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

[David Gibson]

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

On Thu, Jun 01, 2017 at 09:36:46AM +0530, Bharata B Rao wrote:
> On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
> > The code managing DRCs[0] has quite a few things that are more
> > complicated than they need to be.  In particular the object
> > representing a DRC has a bunch of method pointers, despite the fact
> > that there are currently no subclasses, and even if there were the
> > method implementations would be unlikely to differ.
> 
> So you are getting rid of a few methods. How about other methods ?
> Specially attach and detach which have incorporated all the logic needed
> to handle logical and physical DRs into their implementations ?
> 
> Guess these will be follow in subequent parts ?

That's the plan.  In fact I already have patches underway, these are
just the ones that are polished enough to post so far.

-- 
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/4] spapr:DRC cleanups (part I)

[Michael Roth]

Quoting Bharata B Rao (2017-05-31 23:06:46)
> On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
> > The code managing DRCs[0] has quite a few things that are more
> > complicated than they need to be.  In particular the object
> > representing a DRC has a bunch of method pointers, despite the fact
> > that there are currently no subclasses, and even if there were the
> > method implementations would be unlikely to differ.
> 
> So you are getting rid of a few methods. How about other methods ?
> Specially attach and detach which have incorporated all the logic needed
> to handle logical and physical DRs into their implementations ?

I would avoid any methods that incorporate special-casing for
physical vs. logical DRCs, since that seems like a good logical
starting point for moving to 'physical'/'logical' DRC
sub-classes to help simplify the increasingly complicated
state-tracking.

I also don't think we should expose DRC internal fields to
outside callers (which attach/detach would involve). This
series does that to some extent with the RTAS calls, but
since those are now moved to spapr_drc.c it makes more sense.

> 
> Guess these will be follow in subequent parts ? 
> 
> Regards,
> Bharata.
> 
> 

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

[David Gibson]

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

On Wed, May 31, 2017 at 11:25:41PM -0500, Michael Roth wrote:
> Quoting Bharata B Rao (2017-05-31 23:06:46)
> > On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
> > > The code managing DRCs[0] has quite a few things that are more
> > > complicated than they need to be.  In particular the object
> > > representing a DRC has a bunch of method pointers, despite the fact
> > > that there are currently no subclasses, and even if there were the
> > > method implementations would be unlikely to differ.
> > 
> > So you are getting rid of a few methods. How about other methods ?
> > Specially attach and detach which have incorporated all the logic needed
> > to handle logical and physical DRs into their implementations ?
> 
> I would avoid any methods that incorporate special-casing for
> physical vs. logical DRCs, since that seems like a good logical
> starting point for moving to 'physical'/'logical' DRC
> sub-classes to help simplify the increasingly complicated
> state-tracking.

Right, I'm looking at making subclasses for each of the DRC types.
Possibly with intermediate subclasses for physical vs. logical, we'll
see how it works out.

> I also don't think we should expose DRC internal fields to
> outside callers (which attach/detach would involve).

Well.. just changing attach/detach to plain functions instead of
methods wouldn't break that.

> This
> series does that to some extent with the RTAS calls, but
> since those are now moved to spapr_drc.c it makes more sense.

Right - the semantics of the RTAS calls are tied closely to the DRC
semantics, so I don't think there's any point considering the RTAS
calls to be "outside" the DRC code itself.

-- 
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] [Qemu-ppc] [PATCH 0/4] spapr:DRC cleanups (part I)

[Daniel Henrique Barboza]

On 06/01/2017 02:30 AM, David Gibson wrote:
> On Wed, May 31, 2017 at 11:25:41PM -0500, Michael Roth wrote:
>> Quoting Bharata B Rao (2017-05-31 23:06:46)
>>> On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
>>>> The code managing DRCs[0] has quite a few things that are more
>>>> complicated than they need to be.  In particular the object
>>>> representing a DRC has a bunch of method pointers, despite the fact
>>>> that there are currently no subclasses, and even if there were the
>>>> method implementations would be unlikely to differ.
>>> So you are getting rid of a few methods. How about other methods ?
>>> Specially attach and detach which have incorporated all the logic needed
>>> to handle logical and physical DRs into their implementations ?
>> I would avoid any methods that incorporate special-casing for
>> physical vs. logical DRCs, since that seems like a good logical
>> starting point for moving to 'physical'/'logical' DRC
>> sub-classes to help simplify the increasingly complicated
>> state-tracking.
> Right, I'm looking at making subclasses for each of the DRC types.
> Possibly with intermediate subclasses for physical vs. logical, we'll
> see how it works out.

Back in the DRC migration patch series I talked with Mike about refactoring
the DRC code in such fashion (physical DRC and logical DRC). But first I 
would
implement some kind of unit testing in this code to avoid breaking too much
stuff during this refactoring.

I am not sure about the effort to implementing unit test in the current 
DRC code.
This series is simplifying the DRC code, making it more minimalist and 
possibly
easier to be tested. In the end it would be a first step towards unit 
testing.

However, there is the issue of backward compatibility. I fear this DRC 
refactoring
of Logical/Physical DRC would be too drastic to maintain such compatibility
(assuming that it is not already broken). If this refactor goes live 
only in 2.11 then
we will have a hard time to migrate from 2.11 to 2.10.

All that said, I believe we can live without unit testing for a little 
longer and if
we're going for this Physical/DRC refactoring, we need to push it for 
2.10. We can
think about unit test later with the refactored code. Feel free to send 
to me any
unfinished/beta DRC refactoring code you might be working on and want
tested. I can help in the refactoring too, just let me know.


Daniel


>
>> I also don't think we should expose DRC internal fields to
>> outside callers (which attach/detach would involve).
> Well.. just changing attach/detach to plain functions instead of
> methods wouldn't break that.
>
>> This
>> series does that to some extent with the RTAS calls, but
>> since those are now moved to spapr_drc.c it makes more sense.
> Right - the semantics of the RTAS calls are tied closely to the DRC
> semantics, so I don't think there's any point considering the RTAS
> calls to be "outside" the DRC code itself.
>

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/4] spapr:DRC cleanups (part I)

[David Gibson]

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

On Thu, Jun 01, 2017 at 12:41:40PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 06/01/2017 02:30 AM, David Gibson wrote:
> > On Wed, May 31, 2017 at 11:25:41PM -0500, Michael Roth wrote:
> > > Quoting Bharata B Rao (2017-05-31 23:06:46)
> > > > On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
> > > > > The code managing DRCs[0] has quite a few things that are more
> > > > > complicated than they need to be.  In particular the object
> > > > > representing a DRC has a bunch of method pointers, despite the fact
> > > > > that there are currently no subclasses, and even if there were the
> > > > > method implementations would be unlikely to differ.
> > > > So you are getting rid of a few methods. How about other methods ?
> > > > Specially attach and detach which have incorporated all the logic needed
> > > > to handle logical and physical DRs into their implementations ?
> > > I would avoid any methods that incorporate special-casing for
> > > physical vs. logical DRCs, since that seems like a good logical
> > > starting point for moving to 'physical'/'logical' DRC
> > > sub-classes to help simplify the increasingly complicated
> > > state-tracking.
> > Right, I'm looking at making subclasses for each of the DRC types.
> > Possibly with intermediate subclasses for physical vs. logical, we'll
> > see how it works out.
> 
> Back in the DRC migration patch series I talked with Mike about refactoring
> the DRC code in such fashion (physical DRC and logical DRC). But first I
> would
> implement some kind of unit testing in this code to avoid breaking too much
> stuff during this refactoring.

So, I'd love to have good unit tests, but everything takes time.

> I am not sure about the effort to implementing unit test in the
> current DRC code.  This series is simplifying the DRC code, making
> it more minimalist and possibly easier to be tested. In the end it
> would be a first step towards unit testing.

..and as you say, extra complexity in the code makes testing and
reliability harder.

> 
> However, there is the issue of backward compatibility. I fear this DRC
> refactoring
> of Logical/Physical DRC would be too drastic to maintain such compatibility
> (assuming that it is not already broken). If this refactor goes live only in
> 2.11 then
> we will have a hard time to migrate from 2.11 to 2.10.

Right such a rework could break migration.

> All that said, I believe we can live without unit testing for a little
> longer and if
> we're going for this Physical/DRC refactoring, we need to push it for 2.10.
> We can
> think about unit test later with the refactored code. Feel free to send to
> me any
> unfinished/beta DRC refactoring code you might be working on and want
> tested. I can help in the refactoring too, just let me know.

So like you I think getting it into 2.10 would be a good idea, before
we have any released version with DRC migration to break.

-- 
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] [Qemu-ppc] [PATCH 0/4] spapr:DRC cleanups (part I)

[Daniel Henrique Barboza]

On 05/31/2017 10:52 PM, David Gibson wrote:
> The code managing DRCs[0] has quite a few things that are more
> complicated than they need to be.  In particular the object
> representing a DRC has a bunch of method pointers, despite the fact
> that there are currently no subclasses, and even if there were the
> method implementations would be unlikely to differ.
>
> This appears to be a misguided attempt to "abstract" or hide things in
> a way which is bureaucraticl, rather than meaningful.  We may have an
> object model, but we don't have to adopt Java's kingdom-of-nouns
> nonsense[1].
>
> This series makes a start on simplifying things.  There's still plenty
> more, but you have to start somewhere.
>
> [0] "Dynamic Reconfiguration Connectors" a firmware abstraction used
>      in hotplug operations
> [1] https://steve-yegge.blogspot.com.au/2006/03/execution-in-kingdom-of-nouns.html
>
> David Gibson (4):
>    spapr: Move DRC RTAS calls into spapr_drc.c
>    spapr: Abolish DRC get_fdt method
>    spapr: Abolish DRC set_configured method
>    spapr: Make DRC get_index and get_type methods into plain functions
>
>   hw/ppc/spapr.c             |  13 +-
>   hw/ppc/spapr_drc.c         | 404 ++++++++++++++++++++++++++++++++++++++-------
>   hw/ppc/spapr_events.c      |  10 +-
>   hw/ppc/spapr_pci.c         |   4 +-
>   hw/ppc/spapr_rtas.c        | 304 ----------------------------------
>   hw/ppc/trace-events        |   2 -
>   include/hw/ppc/spapr_drc.h |   9 +-
>   7 files changed, 355 insertions(+), 391 deletions(-)
>
Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>

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

[David Gibson]

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

On Thu, Jun 01, 2017 at 11:52:14AM +1000, David Gibson wrote:
> The code managing DRCs[0] has quite a few things that are more
> complicated than they need to be.  In particular the object
> representing a DRC has a bunch of method pointers, despite the fact
> that there are currently no subclasses, and even if there were the
> method implementations would be unlikely to differ.
> 
> This appears to be a misguided attempt to "abstract" or hide things in
> a way which is bureaucraticl, rather than meaningful.  We may have an
> object model, but we don't have to adopt Java's kingdom-of-nouns
> nonsense[1].
> 
> This series makes a start on simplifying things.  There's still plenty
> more, but you have to start somewhere.
> 
> [0] "Dynamic Reconfiguration Connectors" a firmware abstraction used
>     in hotplug operations
> [1]
>     https://steve-yegge.blogspot.com.au/2006/03/execution-in-kingdom-of-nouns.html

I've had enough acks that I've merged this series (with minor
corrections) into ppc-for-2.10.

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