[PATCH for-7.1 0/9] spapr: add drc->index, remove spapr_drc_index()

[PATCH for-7.1 0/9] spapr: add drc->index, remove spapr_drc_index()

[Daniel Henrique Barboza]

Hi,

I decided to make this change after realizing that (1) spapr_drc_index()
always return the same index value for the DRC regardless of machine or
device state and (2) we call spapr_drc_index() a lot throughout the
spapr code.

This means that a new attribute to store the generated index in the DRC
object time will spare us from calling a function that always returns
the same value.

No functional changes were made.

 
Daniel Henrique Barboza (9):
  hw/ppc/spapr_drc.c: add drc->index
  hw/ppc/spapr_drc.c: redefine 'index' SpaprDRC property
  hw/ppc/spapr_drc.c: use drc->index in trace functions
  hw/ppc/spapr_drc.c: use drc->index
  hw/ppc/spapr.c: use drc->index
  hw/ppc/spapr_events.c: use drc->index
  hw/ppc/spapr_nvdimm.c: use drc->index
  hw/ppc/spapr_pci.c: use drc->index
  hw/ppc/spapr_drc.c: remove spapr_drc_index()

 hw/ppc/spapr.c             | 18 ++++-----
 hw/ppc/spapr_drc.c         | 79 +++++++++++++++-----------------------
 hw/ppc/spapr_events.c      |  4 +-
 hw/ppc/spapr_nvdimm.c      | 10 ++---
 hw/ppc/spapr_pci.c         |  5 +--
 include/hw/ppc/spapr_drc.h |  2 +-
 6 files changed, 48 insertions(+), 70 deletions(-)

-- 
2.35.1

[PATCH for-7.1 1/9] hw/ppc/spapr_drc.c: add drc->index

[Daniel Henrique Barboza]

The DRC index is an unique identifier that is used across all spapr
code. Its value is given by spapr_drc_index() as follows:

    return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
        | (drc->id & DRC_INDEX_ID_MASK);

We see that there is nothing that varies with the machine/device state
on spapr_drc_index(). drc->id is defined in spapr_dr_connector_new() and
it's read only, drck->typeshift relies on the DRC class type and it
doesn't change as well and the two macros. Nevertheless,
spapr_drc_index() is called multiple times across spapr files, meaning
that we're always recalculating this value.

This patch adds a new SpaprDrc attribute called 'index'. drc->index will
be initialized with spapr_drc_index() and it's going to be a replacement
for the repetitive spapr_drc_index() usage we have today.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c         | 1 +
 include/hw/ppc/spapr_drc.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 76bc5d42a0..1b8c797192 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -561,6 +561,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
 
     drc->id = id;
     drc->owner = owner;
+    drc->index = spapr_drc_index(drc);
     prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
                                 spapr_drc_index(drc));
     object_property_add_child(owner, prop_name, OBJECT(drc));
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 02a63b3666..93825e47a6 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -173,6 +173,7 @@ typedef struct SpaprDrc {
     DeviceState parent;
 
     uint32_t id;
+    uint32_t index;
     Object *owner;
 
     uint32_t state;
-- 
2.35.1

[PATCH for-7.1 2/9] hw/ppc/spapr_drc.c: redefine 'index' SpaprDRC property

[Daniel Henrique Barboza]

'index' is defined as an uint32 retrieved by prop_get_index(). Change it
to instead return the value of drc->index the same way it's done with
the 'id' property that returns drc->id.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1b8c797192..1a5e9003b2 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -316,14 +316,6 @@ static SpaprDREntitySense logical_entity_sense(SpaprDrc *drc)
     }
 }
 
-static void prop_get_index(Object *obj, Visitor *v, const char *name,
-                           void *opaque, Error **errp)
-{
-    SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
-    uint32_t value = spapr_drc_index(drc);
-    visit_type_uint32(v, name, &value, errp);
-}
-
 static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
                          void *opaque, Error **errp)
 {
@@ -577,8 +569,8 @@ static void spapr_dr_connector_instance_init(Object *obj)
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
     object_property_add_uint32_ptr(obj, "id", &drc->id, OBJ_PROP_FLAG_READ);
-    object_property_add(obj, "index", "uint32", prop_get_index,
-                        NULL, NULL, NULL);
+    object_property_add_uint32_ptr(obj, "index", &drc->index,
+                                   OBJ_PROP_FLAG_READ);
     object_property_add(obj, "fdt", "struct", prop_get_fdt,
                         NULL, NULL, NULL);
     drc->state = drck->empty_state;
-- 
2.35.1

[PATCH for-7.1 3/9] hw/ppc/spapr_drc.c: use drc->index in trace functions

[Daniel Henrique Barboza]

All the trace calls in the file are using spapr_drc_index(). Let's
convert them to use drc->index.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1a5e9003b2..1d751fe9cc 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -81,8 +81,7 @@ static uint32_t drc_isolate_physical(SpaprDrc *drc)
     drc->state = SPAPR_DRC_STATE_PHYSICAL_POWERON;
 
     if (drc->unplug_requested) {
-        uint32_t drc_index = spapr_drc_index(drc);
-        trace_spapr_drc_set_isolation_state_finalizing(drc_index);
+        trace_spapr_drc_set_isolation_state_finalizing(drc->index);
         spapr_drc_release(drc);
     }
 
@@ -247,8 +246,7 @@ static uint32_t drc_set_unusable(SpaprDrc *drc)
 
     drc->state = SPAPR_DRC_STATE_LOGICAL_UNUSABLE;
     if (drc->unplug_requested) {
-        uint32_t drc_index = spapr_drc_index(drc);
-        trace_spapr_drc_set_allocation_state_finalizing(drc_index);
+        trace_spapr_drc_set_allocation_state_finalizing(drc->index);
         spapr_drc_release(drc);
     }
 
@@ -390,7 +388,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
 
 void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
 {
-    trace_spapr_drc_attach(spapr_drc_index(drc));
+    trace_spapr_drc_attach(drc->index);
 
     g_assert(!drc->dev);
     g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
@@ -408,14 +406,14 @@ void spapr_drc_unplug_request(SpaprDrc *drc)
 {
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
-    trace_spapr_drc_unplug_request(spapr_drc_index(drc));
+    trace_spapr_drc_unplug_request(drc->index);
 
     g_assert(drc->dev);
 
     drc->unplug_requested = true;
 
     if (drc->state != drck->empty_state) {
-        trace_spapr_drc_awaiting_quiesce(spapr_drc_index(drc));
+        trace_spapr_drc_awaiting_quiesce(drc->index);
         return;
     }
 
@@ -427,7 +425,7 @@ bool spapr_drc_reset(SpaprDrc *drc)
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     bool unplug_completed = false;
 
-    trace_spapr_drc_reset(spapr_drc_index(drc));
+    trace_spapr_drc_reset(drc->index);
 
     /* immediately upon reset we can safely assume DRCs whose devices
      * are pending removal can be safely removed.
@@ -515,7 +513,7 @@ static void drc_realize(DeviceState *d, Error **errp)
     Object *root_container;
     const char *child_name;
 
-    trace_spapr_drc_realize(spapr_drc_index(drc));
+    trace_spapr_drc_realize(drc->index);
     /* 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
@@ -525,12 +523,12 @@ static void drc_realize(DeviceState *d, Error **errp)
      */
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
     child_name = object_get_canonical_path_component(OBJECT(drc));
-    trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
+    trace_spapr_drc_realize_child(drc->index, child_name);
     object_property_add_alias(root_container, link_name,
                               drc->owner, child_name);
     vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
                      drc);
-    trace_spapr_drc_realize_complete(spapr_drc_index(drc));
+    trace_spapr_drc_realize_complete(drc->index);
 }
 
 static void drc_unrealize(DeviceState *d)
@@ -539,7 +537,7 @@ static void drc_unrealize(DeviceState *d)
     g_autofree gchar *name = g_strdup_printf("%x", spapr_drc_index(drc));
     Object *root_container;
 
-    trace_spapr_drc_unrealize(spapr_drc_index(drc));
+    trace_spapr_drc_unrealize(drc->index);
     vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
     root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
     object_property_del(root_container, name);
@@ -986,7 +984,7 @@ static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
         return RTAS_OUT_NO_SUCH_INDICATOR;
     }
 
-    trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
+    trace_spapr_drc_set_isolation_state(drc->index, state);
 
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
@@ -1010,7 +1008,7 @@ static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
         return RTAS_OUT_NO_SUCH_INDICATOR;
     }
 
-    trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
+    trace_spapr_drc_set_allocation_state(drc->index, state);
 
     switch (state) {
     case SPAPR_DR_ALLOCATION_STATE_USABLE:
@@ -1232,10 +1230,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
         case FDT_END_NODE:
             drc->ccs_depth--;
             if (drc->ccs_depth == 0) {
-                uint32_t drc_index = spapr_drc_index(drc);
-
                 /* done sending the device tree, move to configured state */
-                trace_spapr_drc_set_configured(drc_index);
+                trace_spapr_drc_set_configured(drc->index);
                 drc->state = drck->ready_state;
                 /*
                  * Ensure that we are able to send the FDT fragment
-- 
2.35.1

[PATCH for-7.1 4/9] hw/ppc/spapr_drc.c: use drc->index

[Daniel Henrique Barboza]

After this patch, the only place where spapr_drc_index() is still being
used in this file is in the drc->index initialization.

We can't get rid of spapr_drc_index() yet because of external callers.
We'll handle them next.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1d751fe9cc..11a49620c8 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -509,7 +509,7 @@ static const VMStateDescription vmstate_spapr_drc = {
 static void drc_realize(DeviceState *d, Error **errp)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
-    g_autofree gchar *link_name = g_strdup_printf("%x", spapr_drc_index(drc));
+    g_autofree gchar *link_name = g_strdup_printf("%x", drc->index);
     Object *root_container;
     const char *child_name;
 
@@ -526,15 +526,14 @@ static void drc_realize(DeviceState *d, Error **errp)
     trace_spapr_drc_realize_child(drc->index, child_name);
     object_property_add_alias(root_container, link_name,
                               drc->owner, child_name);
-    vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
-                     drc);
+    vmstate_register(VMSTATE_IF(drc), drc->index, &vmstate_spapr_drc, drc);
     trace_spapr_drc_realize_complete(drc->index);
 }
 
 static void drc_unrealize(DeviceState *d)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
-    g_autofree gchar *name = g_strdup_printf("%x", spapr_drc_index(drc));
+    g_autofree gchar *name = g_strdup_printf("%x", drc->index);
     Object *root_container;
 
     trace_spapr_drc_unrealize(drc->index);
@@ -552,8 +551,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
     drc->id = id;
     drc->owner = owner;
     drc->index = spapr_drc_index(drc);
-    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
-                                spapr_drc_index(drc));
+    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", drc->index);
     object_property_add_child(owner, prop_name, OBJECT(drc));
     object_unref(OBJECT(drc));
     qdev_realize(DEVICE(drc), NULL, NULL);
@@ -633,8 +631,7 @@ static void realize_physical(DeviceState *d, Error **errp)
         return;
     }
 
-    vmstate_register(VMSTATE_IF(drcp),
-                     spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
+    vmstate_register(VMSTATE_IF(drcp), SPAPR_DR_CONNECTOR(drcp)->index,
                      &vmstate_spapr_drc_physical, drcp);
     qemu_register_reset(drc_physical_reset, drcp);
 }
@@ -883,7 +880,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask)
         drc_count++;
 
         /* ibm,drc-indexes */
-        drc_index = cpu_to_be32(spapr_drc_index(drc));
+        drc_index = cpu_to_be32(drc->index);
         g_array_append_val(drc_indexes, drc_index);
 
         /* ibm,drc-power-domains */
-- 
2.35.1

[PATCH for-7.1 5/9] hw/ppc/spapr.c: use drc->index

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 953fc65fa8..6aab04787d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -419,7 +419,7 @@ static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
             drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, cur_addr / lmb_size);
             g_assert(drc);
             elem = spapr_get_drconf_cell((addr - cur_addr) / lmb_size,
-                                         cur_addr, spapr_drc_index(drc), -1, 0);
+                                         cur_addr, drc->index, -1, 0);
             QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry);
             nr_entries++;
         }
@@ -428,7 +428,7 @@ static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
         drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, addr / lmb_size);
         g_assert(drc);
         elem = spapr_get_drconf_cell(size / lmb_size, addr,
-                                     spapr_drc_index(drc), node,
+                                     drc->index, node,
                                      (SPAPR_LMB_FLAGS_ASSIGNED |
                                       SPAPR_LMB_FLAGS_HOTREMOVABLE));
         QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry);
@@ -441,7 +441,7 @@ static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
         drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, cur_addr / lmb_size);
         g_assert(drc);
         elem = spapr_get_drconf_cell((mem_end - cur_addr) / lmb_size,
-                                     cur_addr, spapr_drc_index(drc), -1, 0);
+                                     cur_addr, drc->index, -1, 0);
         QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry);
         nr_entries++;
     }
@@ -497,7 +497,7 @@ static int spapr_dt_dynamic_memory(SpaprMachineState *spapr, void *fdt,
 
             dynamic_memory[0] = cpu_to_be32(addr >> 32);
             dynamic_memory[1] = cpu_to_be32(addr & 0xffffffff);
-            dynamic_memory[2] = cpu_to_be32(spapr_drc_index(drc));
+            dynamic_memory[2] = cpu_to_be32(drc->index);
             dynamic_memory[3] = cpu_to_be32(0); /* reserved */
             dynamic_memory[4] = cpu_to_be32(spapr_pc_dimm_node(dimms, addr));
             if (memory_region_present(get_system_memory(), addr)) {
@@ -663,14 +663,12 @@ static void spapr_dt_cpu(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_vthreads(cpu));
     SpaprDrc *drc;
-    int drc_index;
     uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
     int i;
 
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index);
     if (drc) {
-        drc_index = spapr_drc_index(drc);
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc->index)));
     }
 
     _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
@@ -3448,7 +3446,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     uint64_t addr;
     uint32_t node;
 
-    addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;
+    addr = drc->index * SPAPR_MEMORY_BLOCK_SIZE;
     node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
                                     &error_abort);
     *fdt_start_offset = spapr_dt_memory_node(spapr, fdt, node, addr,
@@ -3491,7 +3489,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
             g_assert(drc);
             spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
                                                    nr_lmbs,
-                                                   spapr_drc_index(drc));
+                                                   drc->index);
         } else {
             spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB,
                                            nr_lmbs);
@@ -3791,7 +3789,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
                           addr_start / SPAPR_MEMORY_BLOCK_SIZE);
     spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
-                                              nr_lmbs, spapr_drc_index(drc));
+                                              nr_lmbs, drc->index);
 }
 
 /* Callback to be called during DRC release. */
-- 
2.35.1

[PATCH for-7.1 6/9] hw/ppc/spapr_events.c: use drc->index

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_events.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 630e86282c..d41f4e47c0 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -675,7 +675,7 @@ void spapr_hotplug_req_add_by_index(SpaprDrc *drc)
     SpaprDrcType drc_type = spapr_drc_type(drc);
     union drc_identifier drc_id;
 
-    drc_id.index = spapr_drc_index(drc);
+    drc_id.index = drc->index;
     spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
                             RTAS_LOG_V6_HP_ACTION_ADD, drc_type, &drc_id);
 }
@@ -685,7 +685,7 @@ void spapr_hotplug_req_remove_by_index(SpaprDrc *drc)
     SpaprDrcType drc_type = spapr_drc_type(drc);
     union drc_identifier drc_id;
 
-    drc_id.index = spapr_drc_index(drc);
+    drc_id.index = drc->index;
     spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
                             RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, &drc_id);
 }
-- 
2.35.1

[PATCH for-7.1 7/9] hw/ppc/spapr_nvdimm.c: use drc->index

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_nvdimm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index c4c97da5de..5acb761220 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -145,7 +145,6 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
     int child_offset;
     char *buf;
     SpaprDrc *drc;
-    uint32_t drc_idx;
     uint32_t node = object_property_get_uint(OBJECT(nvdimm), PC_DIMM_NODE_PROP,
                                              &error_abort);
     uint64_t slot = object_property_get_uint(OBJECT(nvdimm), PC_DIMM_SLOT_PROP,
@@ -157,15 +156,13 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
     g_assert(drc);
 
-    drc_idx = spapr_drc_index(drc);
-
-    buf = g_strdup_printf("ibm,pmemory@%x", drc_idx);
+    buf = g_strdup_printf("ibm,pmemory@%x", drc->index);
     child_offset = fdt_add_subnode(fdt, parent_offset, buf);
     g_free(buf);
 
     _FDT(child_offset);
 
-    _FDT((fdt_setprop_cell(fdt, child_offset, "reg", drc_idx)));
+    _FDT((fdt_setprop_cell(fdt, child_offset, "reg", drc->index)));
     _FDT((fdt_setprop_string(fdt, child_offset, "compatible", "ibm,pmemory")));
     _FDT((fdt_setprop_string(fdt, child_offset, "device_type", "ibm,pmemory")));
 
@@ -175,7 +172,8 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
     _FDT((fdt_setprop_string(fdt, child_offset, "ibm,unit-guid", buf)));
     g_free(buf);
 
-    _FDT((fdt_setprop_cell(fdt, child_offset, "ibm,my-drc-index", drc_idx)));
+    _FDT((fdt_setprop_cell(fdt, child_offset, "ibm,my-drc-index",
+                           drc->index)));
 
     _FDT((fdt_setprop_u64(fdt, child_offset, "ibm,block-size",
                           SPAPR_MINIMUM_SCM_BLOCK_SIZE)));
-- 
2.35.1

[PATCH for-7.1 8/9] hw/ppc/spapr_pci.c: use drc->index

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_pci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5bfd4aa9e5..f9338af071 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1419,8 +1419,7 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
     g_free(loc_code);
 
     if (drc) {
-        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index",
-                              spapr_drc_index(drc)));
+        _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc->index));
     }
 
     if (msi_present(dev)) {
@@ -2429,7 +2428,7 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState *phb,
 
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, phb->index);
     if (drc) {
-        uint32_t drc_index = cpu_to_be32(spapr_drc_index(drc));
+        uint32_t drc_index = cpu_to_be32(drc->index);
 
         _FDT(fdt_setprop(fdt, bus_off, "ibm,my-drc-index", &drc_index,
                          sizeof(drc_index)));
-- 
2.35.1

[PATCH for-7.1 9/9] hw/ppc/spapr_drc.c: remove spapr_drc_index()

[Daniel Henrique Barboza]

The only remaining caller of this function is the initialization of
drc->index in spapr_dr_connector_new().

Open code the body of the function inside spapr_dr_connector_new() and
remove spapr_drc_index().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c         | 23 ++++++++++-------------
 include/hw/ppc/spapr_drc.h |  1 -
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 11a49620c8..8c8654121c 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -39,18 +39,6 @@ SpaprDrcType spapr_drc_type(SpaprDrc *drc)
     return 1 << drck->typeshift;
 }
 
-uint32_t spapr_drc_index(SpaprDrc *drc)
-{
-    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(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
-     * however, so might as well do that here
-     */
-    return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
-        | (drc->id & DRC_INDEX_ID_MASK);
-}
-
 static void spapr_drc_release(SpaprDrc *drc)
 {
     SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -546,11 +534,20 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
                                          uint32_t id)
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(object_new(type));
+    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     g_autofree char *prop_name = NULL;
 
     drc->id = id;
     drc->owner = owner;
-    drc->index = spapr_drc_index(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
+     * however, so might as well do that here.
+     */
+    drc->index = (drck->typeshift << DRC_INDEX_TYPE_SHIFT) |
+                 (drc->id & DRC_INDEX_ID_MASK);
+
     prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", drc->index);
     object_property_add_child(owner, prop_name, OBJECT(drc));
     object_unref(OBJECT(drc));
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 93825e47a6..33cdb3cc20 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -228,7 +228,6 @@ static inline bool spapr_drc_hotplugged(DeviceState *dev)
 /* Returns true if an unplug request completed */
 bool spapr_drc_reset(SpaprDrc *drc);
 
-uint32_t spapr_drc_index(SpaprDrc *drc);
 SpaprDrcType spapr_drc_type(SpaprDrc *drc);
 
 SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
-- 
2.35.1

Re: [PATCH for-7.1 0/9] spapr: add drc->index, remove spapr_drc_index()

[David Gibson]

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

On Fri, Mar 18, 2022 at 02:33:11PM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> I decided to make this change after realizing that (1) spapr_drc_index()
> always return the same index value for the DRC regardless of machine or
> device state and (2) we call spapr_drc_index() a lot throughout the
> spapr code.

Hmm.. so, spapr_drc_index() wasn't ever intended as an abstraction
point.  Rather, it's just there as a matter of data redundancy.  The
index can be derived from the drc->id and the type.  Unless there's a
compelling reason otherwise, it's usually a good idea to store data in
just one form (if there's more it's an opportunity for bugs to let it
get out of sync).

> 
> This means that a new attribute to store the generated index in the DRC
> object time will spare us from calling a function that always returns
> the same value.
> 
> No functional changes were made.
> 
>  
> Daniel Henrique Barboza (9):
>   hw/ppc/spapr_drc.c: add drc->index
>   hw/ppc/spapr_drc.c: redefine 'index' SpaprDRC property
>   hw/ppc/spapr_drc.c: use drc->index in trace functions
>   hw/ppc/spapr_drc.c: use drc->index
>   hw/ppc/spapr.c: use drc->index
>   hw/ppc/spapr_events.c: use drc->index
>   hw/ppc/spapr_nvdimm.c: use drc->index
>   hw/ppc/spapr_pci.c: use drc->index
>   hw/ppc/spapr_drc.c: remove spapr_drc_index()
> 
>  hw/ppc/spapr.c             | 18 ++++-----
>  hw/ppc/spapr_drc.c         | 79 +++++++++++++++-----------------------
>  hw/ppc/spapr_events.c      |  4 +-
>  hw/ppc/spapr_nvdimm.c      | 10 ++---
>  hw/ppc/spapr_pci.c         |  5 +--
>  include/hw/ppc/spapr_drc.h |  2 +-
>  6 files changed, 48 insertions(+), 70 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH for-7.1 0/9] spapr: add drc->index, remove spapr_drc_index()

[Daniel Henrique Barboza]

On 3/21/22 00:55, David Gibson wrote:
> On Fri, Mar 18, 2022 at 02:33:11PM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> I decided to make this change after realizing that (1) spapr_drc_index()
>> always return the same index value for the DRC regardless of machine or
>> device state and (2) we call spapr_drc_index() a lot throughout the
>> spapr code.
> 
> Hmm.. so, spapr_drc_index() wasn't ever intended as an abstraction
> point.  Rather, it's just there as a matter of data redundancy.  The
> index can be derived from the drc->id and the type.  Unless there's a
> compelling reason otherwise, it's usually a good idea to store data in
> just one form (if there's more it's an opportunity for bugs to let it
> get out of sync).


Hmm what if we store drc->index instead and derive drc->id from it? drc->index
is read from several places, while drc->id is used just in spapr_drc_name() to
write the DT (via spapr_dt_drc()).

I'll think more about it.


Thanks,


Daniel

> 
>>
>> This means that a new attribute to store the generated index in the DRC
>> object time will spare us from calling a function that always returns
>> the same value.
>>
>> No functional changes were made.
>>
>>   
>> Daniel Henrique Barboza (9):
>>    hw/ppc/spapr_drc.c: add drc->index
>>    hw/ppc/spapr_drc.c: redefine 'index' SpaprDRC property
>>    hw/ppc/spapr_drc.c: use drc->index in trace functions
>>    hw/ppc/spapr_drc.c: use drc->index
>>    hw/ppc/spapr.c: use drc->index
>>    hw/ppc/spapr_events.c: use drc->index
>>    hw/ppc/spapr_nvdimm.c: use drc->index
>>    hw/ppc/spapr_pci.c: use drc->index
>>    hw/ppc/spapr_drc.c: remove spapr_drc_index()
>>
>>   hw/ppc/spapr.c             | 18 ++++-----
>>   hw/ppc/spapr_drc.c         | 79 +++++++++++++++-----------------------
>>   hw/ppc/spapr_events.c      |  4 +-
>>   hw/ppc/spapr_nvdimm.c      | 10 ++---
>>   hw/ppc/spapr_pci.c         |  5 +--
>>   include/hw/ppc/spapr_drc.h |  2 +-
>>   6 files changed, 48 insertions(+), 70 deletions(-)
>>
> 

Re: [PATCH for-7.1 0/9] spapr: add drc->index, remove spapr_drc_index()

[David Gibson]

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

On Mon, Mar 21, 2022 at 04:58:47AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 3/21/22 00:55, David Gibson wrote:
> > On Fri, Mar 18, 2022 at 02:33:11PM -0300, Daniel Henrique Barboza wrote:
> > > Hi,
> > > 
> > > I decided to make this change after realizing that (1) spapr_drc_index()
> > > always return the same index value for the DRC regardless of machine or
> > > device state and (2) we call spapr_drc_index() a lot throughout the
> > > spapr code.
> > 
> > Hmm.. so, spapr_drc_index() wasn't ever intended as an abstraction
> > point.  Rather, it's just there as a matter of data redundancy.  The
> > index can be derived from the drc->id and the type.  Unless there's a
> > compelling reason otherwise, it's usually a good idea to store data in
> > just one form (if there's more it's an opportunity for bugs to let it
> > get out of sync).
> 
> 
> Hmm what if we store drc->index instead and derive drc->id from it? drc->index
> is read from several places, while drc->id is used just in spapr_drc_name() to
> write the DT (via spapr_dt_drc()).

That could work.  It's still slightly redundant since the type part of
the index can be derived from the class, but I think it's not unreasonable.

-- 
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: 833 bytes --]