* [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses
2017-06-02 7:29 [Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part II) David Gibson
@ 2017-06-02 7:29 ` David Gibson
2017-06-03 22:05 ` Michael Roth
2017-06-05 3:31 ` [Qemu-devel] [PATCHv2 " David Gibson
2017-06-02 7:29 ` [Qemu-devel] [PATCH 2/5] spapr: Clean up spapr_dr_connector_by_*() David Gibson
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: David Gibson @ 2017-06-02 7:29 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
Currently we only have a single QOM type for all DRCs, but lots of
places where we switch behaviour based on the DRC's PAPR defined type.
This is a poor use of our existing type system.
So, instead create QOM subclasses for each PAPR defined DRC type. We
also introduce intermediate subclasses for physical and logical DRCs,
a division which will be useful later on.
Instead of being stored in the DRC object itself, the PAPR type is now
stored in the class structure. There are still many places where we
switch directly on the PAPR type value, but this at least provides the
basis to start to remove those.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 5 +-
hw/ppc/spapr_drc.c | 120 +++++++++++++++++++++++++++++++++------------
hw/ppc/spapr_pci.c | 3 +-
include/hw/ppc/spapr_drc.h | 47 ++++++++++++++++--
4 files changed, 136 insertions(+), 39 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5d10366..456f9e7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1911,7 +1911,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
uint64_t addr;
addr = i * lmb_size + spapr->hotplug_memory.base;
- drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
+ drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
addr/lmb_size);
qemu_register_reset(spapr_drc_reset, drc);
}
@@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
if (mc->has_hotpluggable_cpus) {
sPAPRDRConnector *drc =
- spapr_dr_connector_new(OBJECT(spapr),
- SPAPR_DR_CONNECTOR_TYPE_CPU,
+ spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
(core_id / smp_threads) * smt);
qemu_register_reset(spapr_drc_reset, drc);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a35314e..690b41f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
return shift;
}
+sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+ return 1 << drck->typeshift;
+}
+
uint32_t spapr_drc_index(sPAPRDRConnector *drc)
{
+ sPAPRDRConnectorClass *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 (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
- (drc->id & DRC_INDEX_ID_MASK);
+ return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
+ | (drc->id & DRC_INDEX_ID_MASK);
}
static uint32_t set_isolation_state(sPAPRDRConnector *drc,
@@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
* If the LMB being removed doesn't belong to a DIMM device that is
* actually being unplugged, fail the isolation request here.
*/
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
!drc->awaiting_release) {
return RTAS_OUT_HW_ERROR;
@@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
}
}
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->allocation_state = state;
if (drc->awaiting_release &&
drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
@@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
return RTAS_OUT_SUCCESS;
}
-sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
-{
- return drc->type;
-}
-
static const char *get_name(sPAPRDRConnector *drc)
{
return drc->name;
@@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
{
if (drc->dev) {
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
/* for logical DR, we return a state of UNUSABLE
* iff the allocation state UNUSABLE.
@@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
*state = SPAPR_DR_ENTITY_SENSE_PRESENT;
}
} else {
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
/* PCI devices, and only PCI devices, use EMPTY
* in cases where we'd otherwise use UNUSABLE
*/
@@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
error_setg(errp, "an attached device is still awaiting release");
return;
}
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
}
g_assert(fdt || coldplug);
@@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
* may be accessing the device, we can easily crash the guest, so we
* we defer completion of removal in such cases to the reset() hook.
*/
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
}
drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
@@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
* 'physical' DR resources such as PCI where each device/resource is
* signalled individually.
*/
- drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
+ drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
? true : coldplug;
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->awaiting_allocation = true;
}
@@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
return;
}
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
drc->awaiting_release = true;
@@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
- /* Calling release callbacks based on drc->type. */
- switch (drc->type) {
+ /* Calling release callbacks based on spapr_drc_type(drc). */
+ switch (spapr_drc_type(drc)) {
case SPAPR_DR_CONNECTOR_TYPE_CPU:
spapr_core_release(drc->dev);
break;
@@ -515,7 +519,7 @@ static void reset(DeviceState *d)
}
/* non-PCI devices may be awaiting a transition to UNUSABLE */
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->awaiting_release) {
drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
}
@@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
* If there is dev plugged in, we need to migrate the DRC state when
* it is different from cold-plugged state
*/
- switch (drc->type) {
+ switch (spapr_drc_type(drc)) {
case SPAPR_DR_CONNECTOR_TYPE_PCI:
rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
@@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
}
}
-sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
- sPAPRDRConnectorType type,
+sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
uint32_t id)
{
- sPAPRDRConnector *drc =
- SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
+ sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
char *prop_name;
- g_assert(type);
-
- drc->type = type;
drc->id = id;
drc->owner = owner;
prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
@@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
* DRC names as documented by PAPR+ v2.7, 13.5.2.4
* location codes as documented by PAPR+ v2.7, 12.3.1.5
*/
- switch (drc->type) {
+ switch (spapr_drc_type(drc)) {
case SPAPR_DR_CONNECTOR_TYPE_CPU:
drc->name = g_strdup_printf("CPU %d", id);
break;
@@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
}
/* PCI slot always start in a USABLE state, and stay there */
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
}
@@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->user_creatable = false;
}
+static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
+}
+
+static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
+}
+
+static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
+}
+
static const TypeInfo spapr_dr_connector_info = {
.name = TYPE_SPAPR_DR_CONNECTOR,
.parent = TYPE_DEVICE,
@@ -750,6 +770,39 @@ static const TypeInfo spapr_dr_connector_info = {
.class_init = spapr_dr_connector_class_init,
};
+static const TypeInfo spapr_drc_physical_info = {
+ .name = TYPE_SPAPR_DRC_PHYSICAL,
+ .parent = TYPE_SPAPR_DR_CONNECTOR,
+ .instance_size = sizeof(sPAPRDRConnector),
+};
+
+static const TypeInfo spapr_drc_logical_info = {
+ .name = TYPE_SPAPR_DRC_LOGICAL,
+ .parent = TYPE_SPAPR_DR_CONNECTOR,
+ .instance_size = sizeof(sPAPRDRConnector),
+};
+
+static const TypeInfo spapr_drc_cpu_info = {
+ .name = TYPE_SPAPR_DRC_CPU,
+ .parent = TYPE_SPAPR_DRC_LOGICAL,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_cpu_class_init,
+};
+
+static const TypeInfo spapr_drc_pci_info = {
+ .name = TYPE_SPAPR_DRC_PCI,
+ .parent = TYPE_SPAPR_DRC_PHYSICAL,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_pci_class_init,
+};
+
+static const TypeInfo spapr_drc_lmb_info = {
+ .name = TYPE_SPAPR_DRC_LMB,
+ .parent = TYPE_SPAPR_DRC_LOGICAL,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_lmb_class_init,
+};
+
/* helper functions for external users */
sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
@@ -858,7 +911,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
continue;
}
- if ((drc->type & drc_type_mask) == 0) {
+ if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
continue;
}
@@ -878,7 +931,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
/* ibm,drc-types */
drc_types = g_string_append(drc_types,
- spapr_drc_get_type_str(drc->type));
+ spapr_drc_get_type_str(spapr_drc_type(drc)));
drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
}
@@ -1210,6 +1263,11 @@ out:
static void spapr_drc_register_types(void)
{
type_register_static(&spapr_dr_connector_info);
+ type_register_static(&spapr_drc_physical_info);
+ type_register_static(&spapr_drc_logical_info);
+ type_register_static(&spapr_drc_cpu_info);
+ type_register_static(&spapr_drc_pci_info);
+ type_register_static(&spapr_drc_lmb_info);
spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
rtas_set_indicator);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7a208a7..50d673b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
/* allocate connectors for child PCI devices */
if (sphb->dr_enabled) {
for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
- spapr_dr_connector_new(OBJECT(phb),
- SPAPR_DR_CONNECTOR_TYPE_PCI,
+ spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
(sphb->index << 16) | i);
}
}
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 10e7c24..f77be38 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -26,6 +26,48 @@
#define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
TYPE_SPAPR_DR_CONNECTOR)
+#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
+#define SPAPR_DRC_PHSYICALGET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHYSICAL)
+#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
+ TYPE_SPAPR_DRC_PHSYICAL)
+#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_PHYSICAL)
+
+#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
+#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LOGICAL)
+#define SPAPR_DRC_LOGICAL_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
+ TYPE_SPAPR_DRC_PHSYICAL)
+#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_LOGICAL)
+
+#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
+#define SPAPR_DRC_CPU_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
+#define SPAPR_DRC_CPU_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_CPU)
+#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_CPU)
+
+#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
+#define SPAPR_DRC_PCI_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
+#define SPAPR_DRC_PCI_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PCI)
+#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_PCI)
+
+#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
+#define SPAPR_DRC_LMB_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
+#define SPAPR_DRC_LMB_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_LMB)
+#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_LMB)
+
/*
* Various hotplug types managed by sPAPRDRConnector
*
@@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
/*< private >*/
DeviceState parent;
- sPAPRDRConnectorType type;
uint32_t id;
Object *owner;
const char *name;
@@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
DeviceClass parent;
/*< public >*/
+ sPAPRDRConnectorTypeShift typeshift;
/* accessors for guest-visible (generally via RTAS) DR state */
uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
@@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
uint32_t spapr_drc_index(sPAPRDRConnector *drc);
sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
-sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
- sPAPRDRConnectorType type,
+sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
uint32_t id);
sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
--
2.9.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses
2017-06-02 7:29 ` [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses David Gibson
@ 2017-06-03 22:05 ` Michael Roth
2017-06-04 10:25 ` David Gibson
2017-06-05 3:31 ` [Qemu-devel] [PATCHv2 " David Gibson
1 sibling, 1 reply; 16+ messages in thread
From: Michael Roth @ 2017-06-03 22:05 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-02 02:29:48)
> Currently we only have a single QOM type for all DRCs, but lots of
> places where we switch behaviour based on the DRC's PAPR defined type.
> This is a poor use of our existing type system.
>
> So, instead create QOM subclasses for each PAPR defined DRC type. We
> also introduce intermediate subclasses for physical and logical DRCs,
> a division which will be useful later on.
>
> Instead of being stored in the DRC object itself, the PAPR type is now
> stored in the class structure. There are still many places where we
> switch directly on the PAPR type value, but this at least provides the
> basis to start to remove those.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 5 +-
> hw/ppc/spapr_drc.c | 120 +++++++++++++++++++++++++++++++++------------
> hw/ppc/spapr_pci.c | 3 +-
> include/hw/ppc/spapr_drc.h | 47 ++++++++++++++++--
> 4 files changed, 136 insertions(+), 39 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5d10366..456f9e7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1911,7 +1911,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> uint64_t addr;
>
> addr = i * lmb_size + spapr->hotplug_memory.base;
> - drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
> + drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> addr/lmb_size);
> qemu_register_reset(spapr_drc_reset, drc);
> }
> @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
>
> if (mc->has_hotpluggable_cpus) {
> sPAPRDRConnector *drc =
> - spapr_dr_connector_new(OBJECT(spapr),
> - SPAPR_DR_CONNECTOR_TYPE_CPU,
> + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> (core_id / smp_threads) * smt);
>
> qemu_register_reset(spapr_drc_reset, drc);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a35314e..690b41f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> return shift;
> }
>
> +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> + return 1 << drck->typeshift;
> +}
> +
> uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> {
> + sPAPRDRConnectorClass *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 (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
> - (drc->id & DRC_INDEX_ID_MASK);
> + return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
> + | (drc->id & DRC_INDEX_ID_MASK);
> }
>
> static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> * If the LMB being removed doesn't belong to a DIMM device that is
> * actually being unplugged, fail the isolation request here.
> */
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> !drc->awaiting_release) {
> return RTAS_OUT_HW_ERROR;
> @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> }
> }
>
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->allocation_state = state;
> if (drc->awaiting_release &&
> drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> return RTAS_OUT_SUCCESS;
> }
>
> -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> -{
> - return drc->type;
> -}
> -
> static const char *get_name(sPAPRDRConnector *drc)
> {
> return drc->name;
> @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
> static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> {
> if (drc->dev) {
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> /* for logical DR, we return a state of UNUSABLE
> * iff the allocation state UNUSABLE.
> @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> }
> } else {
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> /* PCI devices, and only PCI devices, use EMPTY
> * in cases where we'd otherwise use UNUSABLE
> */
> @@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> error_setg(errp, "an attached device is still awaiting release");
> return;
> }
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
> }
> g_assert(fdt || coldplug);
> @@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> * may be accessing the device, we can easily crash the guest, so we
> * we defer completion of removal in such cases to the reset() hook.
> */
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> }
> drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> @@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> * 'physical' DR resources such as PCI where each device/resource is
> * signalled individually.
> */
> - drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
> + drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> ? true : coldplug;
>
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->awaiting_allocation = true;
> }
>
> @@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> return;
> }
>
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> drc->awaiting_release = true;
> @@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>
> drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
>
> - /* Calling release callbacks based on drc->type. */
> - switch (drc->type) {
> + /* Calling release callbacks based on spapr_drc_type(drc). */
> + switch (spapr_drc_type(drc)) {
> case SPAPR_DR_CONNECTOR_TYPE_CPU:
> spapr_core_release(drc->dev);
> break;
> @@ -515,7 +519,7 @@ static void reset(DeviceState *d)
> }
>
> /* non-PCI devices may be awaiting a transition to UNUSABLE */
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->awaiting_release) {
> drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> }
> @@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
> * If there is dev plugged in, we need to migrate the DRC state when
> * it is different from cold-plugged state
> */
> - switch (drc->type) {
> + switch (spapr_drc_type(drc)) {
> case SPAPR_DR_CONNECTOR_TYPE_PCI:
> rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> @@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
> }
> }
>
> -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> - sPAPRDRConnectorType type,
> +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> uint32_t id)
> {
> - sPAPRDRConnector *drc =
> - SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
> char *prop_name;
>
> - g_assert(type);
> -
> - drc->type = type;
> drc->id = id;
> drc->owner = owner;
> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> @@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> * DRC names as documented by PAPR+ v2.7, 13.5.2.4
> * location codes as documented by PAPR+ v2.7, 12.3.1.5
> */
> - switch (drc->type) {
> + switch (spapr_drc_type(drc)) {
> case SPAPR_DR_CONNECTOR_TYPE_CPU:
> drc->name = g_strdup_printf("CPU %d", id);
> break;
> @@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> }
>
> /* PCI slot always start in a USABLE state, and stay there */
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> }
>
> @@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->user_creatable = false;
> }
>
> +static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> +}
> +
> +static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> +}
> +
> +static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> +}
> +
> static const TypeInfo spapr_dr_connector_info = {
> .name = TYPE_SPAPR_DR_CONNECTOR,
> .parent = TYPE_DEVICE,
> @@ -750,6 +770,39 @@ static const TypeInfo spapr_dr_connector_info = {
> .class_init = spapr_dr_connector_class_init,
> };
>
> +static const TypeInfo spapr_drc_physical_info = {
> + .name = TYPE_SPAPR_DRC_PHYSICAL,
> + .parent = TYPE_SPAPR_DR_CONNECTOR,
> + .instance_size = sizeof(sPAPRDRConnector),
> +};
> +
> +static const TypeInfo spapr_drc_logical_info = {
> + .name = TYPE_SPAPR_DRC_LOGICAL,
> + .parent = TYPE_SPAPR_DR_CONNECTOR,
> + .instance_size = sizeof(sPAPRDRConnector),
> +};
It doesn't look like there's any intent of being able to instantiate
a "generic" logical/physical DRC for future types, so I think these
should be marked .abstract = true.
> +
> +static const TypeInfo spapr_drc_cpu_info = {
> + .name = TYPE_SPAPR_DRC_CPU,
> + .parent = TYPE_SPAPR_DRC_LOGICAL,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_cpu_class_init,
> +};
> +
> +static const TypeInfo spapr_drc_pci_info = {
> + .name = TYPE_SPAPR_DRC_PCI,
> + .parent = TYPE_SPAPR_DRC_PHYSICAL,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_pci_class_init,
> +};
> +
> +static const TypeInfo spapr_drc_lmb_info = {
> + .name = TYPE_SPAPR_DRC_LMB,
> + .parent = TYPE_SPAPR_DRC_LOGICAL,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_lmb_class_init,
> +};
> +
> /* helper functions for external users */
>
> sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> @@ -858,7 +911,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> continue;
> }
>
> - if ((drc->type & drc_type_mask) == 0) {
> + if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
> continue;
> }
>
> @@ -878,7 +931,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>
> /* ibm,drc-types */
> drc_types = g_string_append(drc_types,
> - spapr_drc_get_type_str(drc->type));
> + spapr_drc_get_type_str(spapr_drc_type(drc)));
> drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
> }
>
> @@ -1210,6 +1263,11 @@ out:
> static void spapr_drc_register_types(void)
> {
> type_register_static(&spapr_dr_connector_info);
> + type_register_static(&spapr_drc_physical_info);
> + type_register_static(&spapr_drc_logical_info);
> + type_register_static(&spapr_drc_cpu_info);
> + type_register_static(&spapr_drc_pci_info);
> + type_register_static(&spapr_drc_lmb_info);
>
> spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> rtas_set_indicator);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7a208a7..50d673b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> /* allocate connectors for child PCI devices */
> if (sphb->dr_enabled) {
> for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> - spapr_dr_connector_new(OBJECT(phb),
> - SPAPR_DR_CONNECTOR_TYPE_PCI,
> + spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> (sphb->index << 16) | i);
> }
> }
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 10e7c24..f77be38 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -26,6 +26,48 @@
> #define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> TYPE_SPAPR_DR_CONNECTOR)
>
> +#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
> +#define SPAPR_DRC_PHSYICALGET_CLASS(obj) \
typo ^
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHYSICAL)
> +#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> + TYPE_SPAPR_DRC_PHSYICAL)
typo ^
> +#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_PHYSICAL)
> +
> +#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
> +#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LOGICAL)
> +#define SPAPR_DRC_LOGICAL_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> + TYPE_SPAPR_DRC_PHSYICAL)
*LOGICAL
> +#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_LOGICAL)
> +
> +#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
> +#define SPAPR_DRC_CPU_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
> +#define SPAPR_DRC_CPU_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_CPU)
> +#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_CPU)
> +
> +#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
> +#define SPAPR_DRC_PCI_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
> +#define SPAPR_DRC_PCI_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PCI)
> +#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_PCI)
> +
> +#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
> +#define SPAPR_DRC_LMB_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
> +#define SPAPR_DRC_LMB_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_LMB)
> +#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_LMB)
> +
> /*
> * Various hotplug types managed by sPAPRDRConnector
> *
> @@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
> /*< private >*/
> DeviceState parent;
>
> - sPAPRDRConnectorType type;
> uint32_t id;
> Object *owner;
> const char *name;
> @@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
> DeviceClass parent;
>
> /*< public >*/
> + sPAPRDRConnectorTypeShift typeshift;
>
> /* accessors for guest-visible (generally via RTAS) DR state */
> uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> @@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
> uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
>
> -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> - sPAPRDRConnectorType type,
> +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> uint32_t id);
> sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
> sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses
2017-06-03 22:05 ` Michael Roth
@ 2017-06-04 10:25 ` David Gibson
0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2017-06-04 10:25 UTC (permalink / raw)
To: Michael Roth; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
[-- Attachment #1: Type: text/plain, Size: 19908 bytes --]
On Sat, Jun 03, 2017 at 05:05:26PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-02 02:29:48)
> > Currently we only have a single QOM type for all DRCs, but lots of
> > places where we switch behaviour based on the DRC's PAPR defined type.
> > This is a poor use of our existing type system.
> >
> > So, instead create QOM subclasses for each PAPR defined DRC type. We
> > also introduce intermediate subclasses for physical and logical DRCs,
> > a division which will be useful later on.
> >
> > Instead of being stored in the DRC object itself, the PAPR type is now
> > stored in the class structure. There are still many places where we
> > switch directly on the PAPR type value, but this at least provides the
> > basis to start to remove those.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c | 5 +-
> > hw/ppc/spapr_drc.c | 120 +++++++++++++++++++++++++++++++++------------
> > hw/ppc/spapr_pci.c | 3 +-
> > include/hw/ppc/spapr_drc.h | 47 ++++++++++++++++--
> > 4 files changed, 136 insertions(+), 39 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5d10366..456f9e7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1911,7 +1911,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> > uint64_t addr;
> >
> > addr = i * lmb_size + spapr->hotplug_memory.base;
> > - drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
> > + drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> > addr/lmb_size);
> > qemu_register_reset(spapr_drc_reset, drc);
> > }
> > @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> >
> > if (mc->has_hotpluggable_cpus) {
> > sPAPRDRConnector *drc =
> > - spapr_dr_connector_new(OBJECT(spapr),
> > - SPAPR_DR_CONNECTOR_TYPE_CPU,
> > + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> > (core_id / smp_threads) * smt);
> >
> > qemu_register_reset(spapr_drc_reset, drc);
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index a35314e..690b41f 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> > return shift;
> > }
> >
> > +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > + return 1 << drck->typeshift;
> > +}
> > +
> > uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> > {
> > + sPAPRDRConnectorClass *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 (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
> > - (drc->id & DRC_INDEX_ID_MASK);
> > + return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
> > + | (drc->id & DRC_INDEX_ID_MASK);
> > }
> >
> > static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > * If the LMB being removed doesn't belong to a DIMM device that is
> > * actually being unplugged, fail the isolation request here.
> > */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > !drc->awaiting_release) {
> > return RTAS_OUT_HW_ERROR;
> > @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > }
> > }
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->allocation_state = state;
> > if (drc->awaiting_release &&
> > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > return RTAS_OUT_SUCCESS;
> > }
> >
> > -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > -{
> > - return drc->type;
> > -}
> > -
> > static const char *get_name(sPAPRDRConnector *drc)
> > {
> > return drc->name;
> > @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
> > static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> > {
> > if (drc->dev) {
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > /* for logical DR, we return a state of UNUSABLE
> > * iff the allocation state UNUSABLE.
> > @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> > *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> > }
> > } else {
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > /* PCI devices, and only PCI devices, use EMPTY
> > * in cases where we'd otherwise use UNUSABLE
> > */
> > @@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > error_setg(errp, "an attached device is still awaiting release");
> > return;
> > }
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
> > }
> > g_assert(fdt || coldplug);
> > @@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > * may be accessing the device, we can easily crash the guest, so we
> > * we defer completion of removal in such cases to the reset() hook.
> > */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > }
> > drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> > @@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > * 'physical' DR resources such as PCI where each device/resource is
> > * signalled individually.
> > */
> > - drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
> > + drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> > ? true : coldplug;
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->awaiting_allocation = true;
> > }
> >
> > @@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > return;
> > }
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> > drc->awaiting_release = true;
> > @@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> >
> > drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> >
> > - /* Calling release callbacks based on drc->type. */
> > - switch (drc->type) {
> > + /* Calling release callbacks based on spapr_drc_type(drc). */
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > spapr_core_release(drc->dev);
> > break;
> > @@ -515,7 +519,7 @@ static void reset(DeviceState *d)
> > }
> >
> > /* non-PCI devices may be awaiting a transition to UNUSABLE */
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->awaiting_release) {
> > drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> > }
> > @@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
> > * If there is dev plugged in, we need to migrate the DRC state when
> > * it is different from cold-plugged state
> > */
> > - switch (drc->type) {
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> > (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> > @@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
> > }
> > }
> >
> > -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > - sPAPRDRConnectorType type,
> > +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > uint32_t id)
> > {
> > - sPAPRDRConnector *drc =
> > - SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> > + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
> > char *prop_name;
> >
> > - g_assert(type);
> > -
> > - drc->type = type;
> > drc->id = id;
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > @@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > * DRC names as documented by PAPR+ v2.7, 13.5.2.4
> > * location codes as documented by PAPR+ v2.7, 12.3.1.5
> > */
> > - switch (drc->type) {
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > drc->name = g_strdup_printf("CPU %d", id);
> > break;
> > @@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > }
> >
> > /* PCI slot always start in a USABLE state, and stay there */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> > }
> >
> > @@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > dk->user_creatable = false;
> > }
> >
> > +static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> > +}
> > +
> > +static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> > +}
> > +
> > +static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> > +}
> > +
> > static const TypeInfo spapr_dr_connector_info = {
> > .name = TYPE_SPAPR_DR_CONNECTOR,
> > .parent = TYPE_DEVICE,
> > @@ -750,6 +770,39 @@ static const TypeInfo spapr_dr_connector_info = {
> > .class_init = spapr_dr_connector_class_init,
> > };
> >
> > +static const TypeInfo spapr_drc_physical_info = {
> > + .name = TYPE_SPAPR_DRC_PHYSICAL,
> > + .parent = TYPE_SPAPR_DR_CONNECTOR,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > +};
> > +
> > +static const TypeInfo spapr_drc_logical_info = {
> > + .name = TYPE_SPAPR_DRC_LOGICAL,
> > + .parent = TYPE_SPAPR_DR_CONNECTOR,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > +};
>
> It doesn't look like there's any intent of being able to instantiate
> a "generic" logical/physical DRC for future types, so I think these
> should be marked .abstract = true.
Ah, good idea.
> > +
> > +static const TypeInfo spapr_drc_cpu_info = {
> > + .name = TYPE_SPAPR_DRC_CPU,
> > + .parent = TYPE_SPAPR_DRC_LOGICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_cpu_class_init,
> > +};
> > +
> > +static const TypeInfo spapr_drc_pci_info = {
> > + .name = TYPE_SPAPR_DRC_PCI,
> > + .parent = TYPE_SPAPR_DRC_PHYSICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_pci_class_init,
> > +};
> > +
> > +static const TypeInfo spapr_drc_lmb_info = {
> > + .name = TYPE_SPAPR_DRC_LMB,
> > + .parent = TYPE_SPAPR_DRC_LOGICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_lmb_class_init,
> > +};
> > +
> > /* helper functions for external users */
> >
> > sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> > @@ -858,7 +911,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> > continue;
> > }
> >
> > - if ((drc->type & drc_type_mask) == 0) {
> > + if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
> > continue;
> > }
> >
> > @@ -878,7 +931,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> >
> > /* ibm,drc-types */
> > drc_types = g_string_append(drc_types,
> > - spapr_drc_get_type_str(drc->type));
> > + spapr_drc_get_type_str(spapr_drc_type(drc)));
> > drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
> > }
> >
> > @@ -1210,6 +1263,11 @@ out:
> > static void spapr_drc_register_types(void)
> > {
> > type_register_static(&spapr_dr_connector_info);
> > + type_register_static(&spapr_drc_physical_info);
> > + type_register_static(&spapr_drc_logical_info);
> > + type_register_static(&spapr_drc_cpu_info);
> > + type_register_static(&spapr_drc_pci_info);
> > + type_register_static(&spapr_drc_lmb_info);
> >
> > spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > rtas_set_indicator);
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 7a208a7..50d673b 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > /* allocate connectors for child PCI devices */
> > if (sphb->dr_enabled) {
> > for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> > - spapr_dr_connector_new(OBJECT(phb),
> > - SPAPR_DR_CONNECTOR_TYPE_PCI,
> > + spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> > (sphb->index << 16) | i);
> > }
> > }
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 10e7c24..f77be38 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -26,6 +26,48 @@
> > #define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > TYPE_SPAPR_DR_CONNECTOR)
> >
> > +#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
> > +#define SPAPR_DRC_PHSYICALGET_CLASS(obj) \
>
> typo ^
>
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHYSICAL)
> > +#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> > + TYPE_SPAPR_DRC_PHSYICAL)
>
> typo ^
>
> > +#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_PHYSICAL)
> > +
> > +#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
> > +#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LOGICAL)
> > +#define SPAPR_DRC_LOGICAL_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> > + TYPE_SPAPR_DRC_PHSYICAL)
>
> *LOGICAL
Thank you, corrected.
> > +#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_LOGICAL)
> > +
> > +#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
> > +#define SPAPR_DRC_CPU_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
> > +#define SPAPR_DRC_CPU_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_CPU)
> > +#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_CPU)
> > +
> > +#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
> > +#define SPAPR_DRC_PCI_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
> > +#define SPAPR_DRC_PCI_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PCI)
> > +#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_PCI)
> > +
> > +#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
> > +#define SPAPR_DRC_LMB_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
> > +#define SPAPR_DRC_LMB_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_LMB)
> > +#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_LMB)
> > +
> > /*
> > * Various hotplug types managed by sPAPRDRConnector
> > *
> > @@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
> > /*< private >*/
> > DeviceState parent;
> >
> > - sPAPRDRConnectorType type;
> > uint32_t id;
> > Object *owner;
> > const char *name;
> > @@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
> > DeviceClass parent;
> >
> > /*< public >*/
> > + sPAPRDRConnectorTypeShift typeshift;
> >
> > /* accessors for guest-visible (generally via RTAS) DR state */
> > uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> > @@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
> > uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> > sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
> >
> > -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > - sPAPRDRConnectorType type,
> > +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > uint32_t id);
> > sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
> > sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
>
--
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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCHv2 1/5] spapr: Introduce DRC subclasses
2017-06-02 7:29 ` [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses David Gibson
2017-06-03 22:05 ` Michael Roth
@ 2017-06-05 3:31 ` David Gibson
2017-06-05 16:32 ` Michael Roth
1 sibling, 1 reply; 16+ messages in thread
From: David Gibson @ 2017-06-05 3:31 UTC (permalink / raw)
To: mdroth; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
[-- Attachment #1: Type: text/plain, Size: 18109 bytes --]
Currently we only have a single QOM type for all DRCs, but lots of
places where we switch behaviour based on the DRC's PAPR defined type.
This is a poor use of our existing type system.
So, instead create QOM subclasses for each PAPR defined DRC type. We
also introduce intermediate subclasses for physical and logical DRCs,
a division which will be useful later on.
Instead of being stored in the DRC object itself, the PAPR type is now
stored in the class structure. There are still many places where we
switch directly on the PAPR type value, but this at least provides the
basis to start to remove those.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 5 +-
hw/ppc/spapr_drc.c | 123 +++++++++++++++++++++++++++++++++------------
hw/ppc/spapr_pci.c | 3 +-
include/hw/ppc/spapr_drc.h | 47 +++++++++++++++--
4 files changed, 139 insertions(+), 39 deletions(-)
Mike, here's an updated version of the patch addressing the problems
you pointed out. If I can get an ack, I can merge the series, which
would be nice.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5d10366..456f9e7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1911,7 +1911,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
uint64_t addr;
addr = i * lmb_size + spapr->hotplug_memory.base;
- drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
+ drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
addr/lmb_size);
qemu_register_reset(spapr_drc_reset, drc);
}
@@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
if (mc->has_hotpluggable_cpus) {
sPAPRDRConnector *drc =
- spapr_dr_connector_new(OBJECT(spapr),
- SPAPR_DR_CONNECTOR_TYPE_CPU,
+ spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
(core_id / smp_threads) * smt);
qemu_register_reset(spapr_drc_reset, drc);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a35314e..8575022 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
return shift;
}
+sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+ return 1 << drck->typeshift;
+}
+
uint32_t spapr_drc_index(sPAPRDRConnector *drc)
{
+ sPAPRDRConnectorClass *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 (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
- (drc->id & DRC_INDEX_ID_MASK);
+ return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
+ | (drc->id & DRC_INDEX_ID_MASK);
}
static uint32_t set_isolation_state(sPAPRDRConnector *drc,
@@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
* If the LMB being removed doesn't belong to a DIMM device that is
* actually being unplugged, fail the isolation request here.
*/
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
!drc->awaiting_release) {
return RTAS_OUT_HW_ERROR;
@@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
}
}
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->allocation_state = state;
if (drc->awaiting_release &&
drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
@@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
return RTAS_OUT_SUCCESS;
}
-sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
-{
- return drc->type;
-}
-
static const char *get_name(sPAPRDRConnector *drc)
{
return drc->name;
@@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
{
if (drc->dev) {
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
/* for logical DR, we return a state of UNUSABLE
* iff the allocation state UNUSABLE.
@@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
*state = SPAPR_DR_ENTITY_SENSE_PRESENT;
}
} else {
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
/* PCI devices, and only PCI devices, use EMPTY
* in cases where we'd otherwise use UNUSABLE
*/
@@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
error_setg(errp, "an attached device is still awaiting release");
return;
}
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
}
g_assert(fdt || coldplug);
@@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
* may be accessing the device, we can easily crash the guest, so we
* we defer completion of removal in such cases to the reset() hook.
*/
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
}
drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
@@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
* 'physical' DR resources such as PCI where each device/resource is
* signalled individually.
*/
- drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
+ drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
? true : coldplug;
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->awaiting_allocation = true;
}
@@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
return;
}
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
drc->awaiting_release = true;
@@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
- /* Calling release callbacks based on drc->type. */
- switch (drc->type) {
+ /* Calling release callbacks based on spapr_drc_type(drc). */
+ switch (spapr_drc_type(drc)) {
case SPAPR_DR_CONNECTOR_TYPE_CPU:
spapr_core_release(drc->dev);
break;
@@ -515,7 +519,7 @@ static void reset(DeviceState *d)
}
/* non-PCI devices may be awaiting a transition to UNUSABLE */
- if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+ if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
drc->awaiting_release) {
drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
}
@@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
* If there is dev plugged in, we need to migrate the DRC state when
* it is different from cold-plugged state
*/
- switch (drc->type) {
+ switch (spapr_drc_type(drc)) {
case SPAPR_DR_CONNECTOR_TYPE_PCI:
rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
@@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
}
}
-sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
- sPAPRDRConnectorType type,
+sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
uint32_t id)
{
- sPAPRDRConnector *drc =
- SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
+ sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
char *prop_name;
- g_assert(type);
-
- drc->type = type;
drc->id = id;
drc->owner = owner;
prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
@@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
* DRC names as documented by PAPR+ v2.7, 13.5.2.4
* location codes as documented by PAPR+ v2.7, 12.3.1.5
*/
- switch (drc->type) {
+ switch (spapr_drc_type(drc)) {
case SPAPR_DR_CONNECTOR_TYPE_CPU:
drc->name = g_strdup_printf("CPU %d", id);
break;
@@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
}
/* PCI slot always start in a USABLE state, and stay there */
- if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
+ if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
}
@@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->user_creatable = false;
}
+static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
+}
+
+static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
+}
+
+static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
+}
+
static const TypeInfo spapr_dr_connector_info = {
.name = TYPE_SPAPR_DR_CONNECTOR,
.parent = TYPE_DEVICE,
@@ -748,6 +768,42 @@ static const TypeInfo spapr_dr_connector_info = {
.instance_init = spapr_dr_connector_instance_init,
.class_size = sizeof(sPAPRDRConnectorClass),
.class_init = spapr_dr_connector_class_init,
+ .abstract = true,
+};
+
+static const TypeInfo spapr_drc_physical_info = {
+ .name = TYPE_SPAPR_DRC_PHYSICAL,
+ .parent = TYPE_SPAPR_DR_CONNECTOR,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .abstract = true,
+};
+
+static const TypeInfo spapr_drc_logical_info = {
+ .name = TYPE_SPAPR_DRC_LOGICAL,
+ .parent = TYPE_SPAPR_DR_CONNECTOR,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .abstract = true,
+};
+
+static const TypeInfo spapr_drc_cpu_info = {
+ .name = TYPE_SPAPR_DRC_CPU,
+ .parent = TYPE_SPAPR_DRC_LOGICAL,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_cpu_class_init,
+};
+
+static const TypeInfo spapr_drc_pci_info = {
+ .name = TYPE_SPAPR_DRC_PCI,
+ .parent = TYPE_SPAPR_DRC_PHYSICAL,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_pci_class_init,
+};
+
+static const TypeInfo spapr_drc_lmb_info = {
+ .name = TYPE_SPAPR_DRC_LMB,
+ .parent = TYPE_SPAPR_DRC_LOGICAL,
+ .instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_lmb_class_init,
};
/* helper functions for external users */
@@ -858,7 +914,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
continue;
}
- if ((drc->type & drc_type_mask) == 0) {
+ if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
continue;
}
@@ -878,7 +934,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
/* ibm,drc-types */
drc_types = g_string_append(drc_types,
- spapr_drc_get_type_str(drc->type));
+ spapr_drc_get_type_str(spapr_drc_type(drc)));
drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
}
@@ -1210,6 +1266,11 @@ out:
static void spapr_drc_register_types(void)
{
type_register_static(&spapr_dr_connector_info);
+ type_register_static(&spapr_drc_physical_info);
+ type_register_static(&spapr_drc_logical_info);
+ type_register_static(&spapr_drc_cpu_info);
+ type_register_static(&spapr_drc_pci_info);
+ type_register_static(&spapr_drc_lmb_info);
spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
rtas_set_indicator);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7a208a7..50d673b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
/* allocate connectors for child PCI devices */
if (sphb->dr_enabled) {
for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
- spapr_dr_connector_new(OBJECT(phb),
- SPAPR_DR_CONNECTOR_TYPE_PCI,
+ spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
(sphb->index << 16) | i);
}
}
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 10e7c24..969c16d 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -26,6 +26,48 @@
#define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
TYPE_SPAPR_DR_CONNECTOR)
+#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
+#define SPAPR_DRC_PHYSICAL_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHYSICAL)
+#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
+ TYPE_SPAPR_DRC_PHYSICAL)
+#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_PHYSICAL)
+
+#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
+#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LOGICAL)
+#define SPAPR_DRC_LOGICAL_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
+ TYPE_SPAPR_DRC_LOGICAL)
+#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_LOGICAL)
+
+#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
+#define SPAPR_DRC_CPU_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
+#define SPAPR_DRC_CPU_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_CPU)
+#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_CPU)
+
+#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
+#define SPAPR_DRC_PCI_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
+#define SPAPR_DRC_PCI_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PCI)
+#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_PCI)
+
+#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
+#define SPAPR_DRC_LMB_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
+#define SPAPR_DRC_LMB_CLASS(klass) \
+ OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_LMB)
+#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
+ TYPE_SPAPR_DRC_LMB)
+
/*
* Various hotplug types managed by sPAPRDRConnector
*
@@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
/*< private >*/
DeviceState parent;
- sPAPRDRConnectorType type;
uint32_t id;
Object *owner;
const char *name;
@@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
DeviceClass parent;
/*< public >*/
+ sPAPRDRConnectorTypeShift typeshift;
/* accessors for guest-visible (generally via RTAS) DR state */
uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
@@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
uint32_t spapr_drc_index(sPAPRDRConnector *drc);
sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
-sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
- sPAPRDRConnectorType type,
+sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
uint32_t id);
sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
--
2.9.4
--
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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCHv2 1/5] spapr: Introduce DRC subclasses
2017-06-05 3:31 ` [Qemu-devel] [PATCHv2 " David Gibson
@ 2017-06-05 16:32 ` Michael Roth
2017-06-05 23:37 ` David Gibson
0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2017-06-05 16:32 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-04 22:31:12)
> Currently we only have a single QOM type for all DRCs, but lots of
> places where we switch behaviour based on the DRC's PAPR defined type.
> This is a poor use of our existing type system.
>
> So, instead create QOM subclasses for each PAPR defined DRC type. We
> also introduce intermediate subclasses for physical and logical DRCs,
> a division which will be useful later on.
>
> Instead of being stored in the DRC object itself, the PAPR type is now
> stored in the class structure. There are still many places where we
> switch directly on the PAPR type value, but this at least provides the
> basis to start to remove those.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 5 +-
> hw/ppc/spapr_drc.c | 123 +++++++++++++++++++++++++++++++++------------
> hw/ppc/spapr_pci.c | 3 +-
> include/hw/ppc/spapr_drc.h | 47 +++++++++++++++--
> 4 files changed, 139 insertions(+), 39 deletions(-)
>
> Mike, here's an updated version of the patch addressing the problems
> you pointed out. If I can get an ack, I can merge the series, which
> would be nice.
Series:
Acked-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5d10366..456f9e7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1911,7 +1911,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> uint64_t addr;
>
> addr = i * lmb_size + spapr->hotplug_memory.base;
> - drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
> + drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> addr/lmb_size);
> qemu_register_reset(spapr_drc_reset, drc);
> }
> @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
>
> if (mc->has_hotpluggable_cpus) {
> sPAPRDRConnector *drc =
> - spapr_dr_connector_new(OBJECT(spapr),
> - SPAPR_DR_CONNECTOR_TYPE_CPU,
> + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> (core_id / smp_threads) * smt);
>
> qemu_register_reset(spapr_drc_reset, drc);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a35314e..8575022 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> return shift;
> }
>
> +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> + return 1 << drck->typeshift;
> +}
> +
> uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> {
> + sPAPRDRConnectorClass *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 (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
> - (drc->id & DRC_INDEX_ID_MASK);
> + return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
> + | (drc->id & DRC_INDEX_ID_MASK);
> }
>
> static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> * If the LMB being removed doesn't belong to a DIMM device that is
> * actually being unplugged, fail the isolation request here.
> */
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> !drc->awaiting_release) {
> return RTAS_OUT_HW_ERROR;
> @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> }
> }
>
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->allocation_state = state;
> if (drc->awaiting_release &&
> drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> return RTAS_OUT_SUCCESS;
> }
>
> -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> -{
> - return drc->type;
> -}
> -
> static const char *get_name(sPAPRDRConnector *drc)
> {
> return drc->name;
> @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
> static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> {
> if (drc->dev) {
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> /* for logical DR, we return a state of UNUSABLE
> * iff the allocation state UNUSABLE.
> @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> }
> } else {
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> /* PCI devices, and only PCI devices, use EMPTY
> * in cases where we'd otherwise use UNUSABLE
> */
> @@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> error_setg(errp, "an attached device is still awaiting release");
> return;
> }
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
> }
> g_assert(fdt || coldplug);
> @@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> * may be accessing the device, we can easily crash the guest, so we
> * we defer completion of removal in such cases to the reset() hook.
> */
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> }
> drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> @@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> * 'physical' DR resources such as PCI where each device/resource is
> * signalled individually.
> */
> - drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
> + drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> ? true : coldplug;
>
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->awaiting_allocation = true;
> }
>
> @@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> return;
> }
>
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> drc->awaiting_release = true;
> @@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>
> drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
>
> - /* Calling release callbacks based on drc->type. */
> - switch (drc->type) {
> + /* Calling release callbacks based on spapr_drc_type(drc). */
> + switch (spapr_drc_type(drc)) {
> case SPAPR_DR_CONNECTOR_TYPE_CPU:
> spapr_core_release(drc->dev);
> break;
> @@ -515,7 +519,7 @@ static void reset(DeviceState *d)
> }
>
> /* non-PCI devices may be awaiting a transition to UNUSABLE */
> - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> drc->awaiting_release) {
> drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> }
> @@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
> * If there is dev plugged in, we need to migrate the DRC state when
> * it is different from cold-plugged state
> */
> - switch (drc->type) {
> + switch (spapr_drc_type(drc)) {
> case SPAPR_DR_CONNECTOR_TYPE_PCI:
> rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> @@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
> }
> }
>
> -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> - sPAPRDRConnectorType type,
> +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> uint32_t id)
> {
> - sPAPRDRConnector *drc =
> - SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
> char *prop_name;
>
> - g_assert(type);
> -
> - drc->type = type;
> drc->id = id;
> drc->owner = owner;
> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> @@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> * DRC names as documented by PAPR+ v2.7, 13.5.2.4
> * location codes as documented by PAPR+ v2.7, 12.3.1.5
> */
> - switch (drc->type) {
> + switch (spapr_drc_type(drc)) {
> case SPAPR_DR_CONNECTOR_TYPE_CPU:
> drc->name = g_strdup_printf("CPU %d", id);
> break;
> @@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> }
>
> /* PCI slot always start in a USABLE state, and stay there */
> - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> }
>
> @@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->user_creatable = false;
> }
>
> +static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> +}
> +
> +static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> +}
> +
> +static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> +}
> +
> static const TypeInfo spapr_dr_connector_info = {
> .name = TYPE_SPAPR_DR_CONNECTOR,
> .parent = TYPE_DEVICE,
> @@ -748,6 +768,42 @@ static const TypeInfo spapr_dr_connector_info = {
> .instance_init = spapr_dr_connector_instance_init,
> .class_size = sizeof(sPAPRDRConnectorClass),
> .class_init = spapr_dr_connector_class_init,
> + .abstract = true,
> +};
> +
> +static const TypeInfo spapr_drc_physical_info = {
> + .name = TYPE_SPAPR_DRC_PHYSICAL,
> + .parent = TYPE_SPAPR_DR_CONNECTOR,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .abstract = true,
> +};
> +
> +static const TypeInfo spapr_drc_logical_info = {
> + .name = TYPE_SPAPR_DRC_LOGICAL,
> + .parent = TYPE_SPAPR_DR_CONNECTOR,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .abstract = true,
> +};
> +
> +static const TypeInfo spapr_drc_cpu_info = {
> + .name = TYPE_SPAPR_DRC_CPU,
> + .parent = TYPE_SPAPR_DRC_LOGICAL,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_cpu_class_init,
> +};
> +
> +static const TypeInfo spapr_drc_pci_info = {
> + .name = TYPE_SPAPR_DRC_PCI,
> + .parent = TYPE_SPAPR_DRC_PHYSICAL,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_pci_class_init,
> +};
> +
> +static const TypeInfo spapr_drc_lmb_info = {
> + .name = TYPE_SPAPR_DRC_LMB,
> + .parent = TYPE_SPAPR_DRC_LOGICAL,
> + .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_lmb_class_init,
> };
>
> /* helper functions for external users */
> @@ -858,7 +914,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> continue;
> }
>
> - if ((drc->type & drc_type_mask) == 0) {
> + if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
> continue;
> }
>
> @@ -878,7 +934,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>
> /* ibm,drc-types */
> drc_types = g_string_append(drc_types,
> - spapr_drc_get_type_str(drc->type));
> + spapr_drc_get_type_str(spapr_drc_type(drc)));
> drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
> }
>
> @@ -1210,6 +1266,11 @@ out:
> static void spapr_drc_register_types(void)
> {
> type_register_static(&spapr_dr_connector_info);
> + type_register_static(&spapr_drc_physical_info);
> + type_register_static(&spapr_drc_logical_info);
> + type_register_static(&spapr_drc_cpu_info);
> + type_register_static(&spapr_drc_pci_info);
> + type_register_static(&spapr_drc_lmb_info);
>
> spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> rtas_set_indicator);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7a208a7..50d673b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> /* allocate connectors for child PCI devices */
> if (sphb->dr_enabled) {
> for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> - spapr_dr_connector_new(OBJECT(phb),
> - SPAPR_DR_CONNECTOR_TYPE_PCI,
> + spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> (sphb->index << 16) | i);
> }
> }
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 10e7c24..969c16d 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -26,6 +26,48 @@
> #define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> TYPE_SPAPR_DR_CONNECTOR)
>
> +#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
> +#define SPAPR_DRC_PHYSICAL_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHYSICAL)
> +#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> + TYPE_SPAPR_DRC_PHYSICAL)
> +#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_PHYSICAL)
> +
> +#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
> +#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LOGICAL)
> +#define SPAPR_DRC_LOGICAL_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> + TYPE_SPAPR_DRC_LOGICAL)
> +#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_LOGICAL)
> +
> +#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
> +#define SPAPR_DRC_CPU_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
> +#define SPAPR_DRC_CPU_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_CPU)
> +#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_CPU)
> +
> +#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
> +#define SPAPR_DRC_PCI_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
> +#define SPAPR_DRC_PCI_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PCI)
> +#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_PCI)
> +
> +#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
> +#define SPAPR_DRC_LMB_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
> +#define SPAPR_DRC_LMB_CLASS(klass) \
> + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_LMB)
> +#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> + TYPE_SPAPR_DRC_LMB)
> +
> /*
> * Various hotplug types managed by sPAPRDRConnector
> *
> @@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
> /*< private >*/
> DeviceState parent;
>
> - sPAPRDRConnectorType type;
> uint32_t id;
> Object *owner;
> const char *name;
> @@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
> DeviceClass parent;
>
> /*< public >*/
> + sPAPRDRConnectorTypeShift typeshift;
>
> /* accessors for guest-visible (generally via RTAS) DR state */
> uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> @@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
> uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
>
> -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> - sPAPRDRConnectorType type,
> +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> uint32_t id);
> sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
> sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
> --
> 2.9.4
>
>
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCHv2 1/5] spapr: Introduce DRC subclasses
2017-06-05 16:32 ` Michael Roth
@ 2017-06-05 23:37 ` David Gibson
0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2017-06-05 23:37 UTC (permalink / raw)
To: Michael Roth; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
[-- Attachment #1: Type: text/plain, Size: 20105 bytes --]
On Mon, Jun 05, 2017 at 11:32:07AM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-04 22:31:12)
> > Currently we only have a single QOM type for all DRCs, but lots of
> > places where we switch behaviour based on the DRC's PAPR defined type.
> > This is a poor use of our existing type system.
> >
> > So, instead create QOM subclasses for each PAPR defined DRC type. We
> > also introduce intermediate subclasses for physical and logical DRCs,
> > a division which will be useful later on.
> >
> > Instead of being stored in the DRC object itself, the PAPR type is now
> > stored in the class structure. There are still many places where we
> > switch directly on the PAPR type value, but this at least provides the
> > basis to start to remove those.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> > ---
> > hw/ppc/spapr.c | 5 +-
> > hw/ppc/spapr_drc.c | 123 +++++++++++++++++++++++++++++++++------------
> > hw/ppc/spapr_pci.c | 3 +-
> > include/hw/ppc/spapr_drc.h | 47 +++++++++++++++--
> > 4 files changed, 139 insertions(+), 39 deletions(-)
> >
> > Mike, here's an updated version of the patch addressing the problems
> > you pointed out. If I can get an ack, I can merge the series, which
> > would be nice.
>
> Series:
>
> Acked-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Thanks, I've merged this into ppc-for-2.10.
>
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5d10366..456f9e7 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1911,7 +1911,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
> > uint64_t addr;
> >
> > addr = i * lmb_size + spapr->hotplug_memory.base;
> > - drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
> > + drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> > addr/lmb_size);
> > qemu_register_reset(spapr_drc_reset, drc);
> > }
> > @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> >
> > if (mc->has_hotpluggable_cpus) {
> > sPAPRDRConnector *drc =
> > - spapr_dr_connector_new(OBJECT(spapr),
> > - SPAPR_DR_CONNECTOR_TYPE_CPU,
> > + spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> > (core_id / smp_threads) * smt);
> >
> > qemu_register_reset(spapr_drc_reset, drc);
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index a35314e..8575022 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> > return shift;
> > }
> >
> > +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > + return 1 << drck->typeshift;
> > +}
> > +
> > uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> > {
> > + sPAPRDRConnectorClass *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 (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
> > - (drc->id & DRC_INDEX_ID_MASK);
> > + return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
> > + | (drc->id & DRC_INDEX_ID_MASK);
> > }
> >
> > static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > * If the LMB being removed doesn't belong to a DIMM device that is
> > * actually being unplugged, fail the isolation request here.
> > */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> > if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > !drc->awaiting_release) {
> > return RTAS_OUT_HW_ERROR;
> > @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > }
> > }
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->allocation_state = state;
> > if (drc->awaiting_release &&
> > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > return RTAS_OUT_SUCCESS;
> > }
> >
> > -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > -{
> > - return drc->type;
> > -}
> > -
> > static const char *get_name(sPAPRDRConnector *drc)
> > {
> > return drc->name;
> > @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
> > static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> > {
> > if (drc->dev) {
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > /* for logical DR, we return a state of UNUSABLE
> > * iff the allocation state UNUSABLE.
> > @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> > *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> > }
> > } else {
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > /* PCI devices, and only PCI devices, use EMPTY
> > * in cases where we'd otherwise use UNUSABLE
> > */
> > @@ -368,7 +372,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > error_setg(errp, "an attached device is still awaiting release");
> > return;
> > }
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
> > }
> > g_assert(fdt || coldplug);
> > @@ -380,7 +384,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > * may be accessing the device, we can easily crash the guest, so we
> > * we defer completion of removal in such cases to the reset() hook.
> > */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > }
> > drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> > @@ -398,10 +402,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > * 'physical' DR resources such as PCI where each device/resource is
> > * signalled individually.
> > */
> > - drc->signalled = (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI)
> > + drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
> > ? true : coldplug;
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->awaiting_allocation = true;
> > }
> >
> > @@ -441,7 +445,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > return;
> > }
> >
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> > trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> > drc->awaiting_release = true;
> > @@ -458,8 +462,8 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> >
> > drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> >
> > - /* Calling release callbacks based on drc->type. */
> > - switch (drc->type) {
> > + /* Calling release callbacks based on spapr_drc_type(drc). */
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > spapr_core_release(drc->dev);
> > break;
> > @@ -515,7 +519,7 @@ static void reset(DeviceState *d)
> > }
> >
> > /* non-PCI devices may be awaiting a transition to UNUSABLE */
> > - if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > + if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> > drc->awaiting_release) {
> > drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
> > }
> > @@ -544,7 +548,7 @@ static bool spapr_drc_needed(void *opaque)
> > * If there is dev plugged in, we need to migrate the DRC state when
> > * it is different from cold-plugged state
> > */
> > - switch (drc->type) {
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> > (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> > @@ -630,17 +634,12 @@ static void unrealize(DeviceState *d, Error **errp)
> > }
> > }
> >
> > -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > - sPAPRDRConnectorType type,
> > +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > uint32_t id)
> > {
> > - sPAPRDRConnector *drc =
> > - SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> > + sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
> > char *prop_name;
> >
> > - g_assert(type);
> > -
> > - drc->type = type;
> > drc->id = id;
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > @@ -670,7 +669,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > * DRC names as documented by PAPR+ v2.7, 13.5.2.4
> > * location codes as documented by PAPR+ v2.7, 12.3.1.5
> > */
> > - switch (drc->type) {
> > + switch (spapr_drc_type(drc)) {
> > case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > drc->name = g_strdup_printf("CPU %d", id);
> > break;
> > @@ -689,7 +688,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > }
> >
> > /* PCI slot always start in a USABLE state, and stay there */
> > - if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > + if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
> > }
> >
> > @@ -741,6 +740,27 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > dk->user_creatable = false;
> > }
> >
> > +static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> > +}
> > +
> > +static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> > +}
> > +
> > +static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > +{
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> > +
> > + drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> > +}
> > +
> > static const TypeInfo spapr_dr_connector_info = {
> > .name = TYPE_SPAPR_DR_CONNECTOR,
> > .parent = TYPE_DEVICE,
> > @@ -748,6 +768,42 @@ static const TypeInfo spapr_dr_connector_info = {
> > .instance_init = spapr_dr_connector_instance_init,
> > .class_size = sizeof(sPAPRDRConnectorClass),
> > .class_init = spapr_dr_connector_class_init,
> > + .abstract = true,
> > +};
> > +
> > +static const TypeInfo spapr_drc_physical_info = {
> > + .name = TYPE_SPAPR_DRC_PHYSICAL,
> > + .parent = TYPE_SPAPR_DR_CONNECTOR,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .abstract = true,
> > +};
> > +
> > +static const TypeInfo spapr_drc_logical_info = {
> > + .name = TYPE_SPAPR_DRC_LOGICAL,
> > + .parent = TYPE_SPAPR_DR_CONNECTOR,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .abstract = true,
> > +};
> > +
> > +static const TypeInfo spapr_drc_cpu_info = {
> > + .name = TYPE_SPAPR_DRC_CPU,
> > + .parent = TYPE_SPAPR_DRC_LOGICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_cpu_class_init,
> > +};
> > +
> > +static const TypeInfo spapr_drc_pci_info = {
> > + .name = TYPE_SPAPR_DRC_PCI,
> > + .parent = TYPE_SPAPR_DRC_PHYSICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_pci_class_init,
> > +};
> > +
> > +static const TypeInfo spapr_drc_lmb_info = {
> > + .name = TYPE_SPAPR_DRC_LMB,
> > + .parent = TYPE_SPAPR_DRC_LOGICAL,
> > + .instance_size = sizeof(sPAPRDRConnector),
> > + .class_init = spapr_drc_lmb_class_init,
> > };
> >
> > /* helper functions for external users */
> > @@ -858,7 +914,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> > continue;
> > }
> >
> > - if ((drc->type & drc_type_mask) == 0) {
> > + if ((spapr_drc_type(drc) & drc_type_mask) == 0) {
> > continue;
> > }
> >
> > @@ -878,7 +934,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> >
> > /* ibm,drc-types */
> > drc_types = g_string_append(drc_types,
> > - spapr_drc_get_type_str(drc->type));
> > + spapr_drc_get_type_str(spapr_drc_type(drc)));
> > drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
> > }
> >
> > @@ -1210,6 +1266,11 @@ out:
> > static void spapr_drc_register_types(void)
> > {
> > type_register_static(&spapr_dr_connector_info);
> > + type_register_static(&spapr_drc_physical_info);
> > + type_register_static(&spapr_drc_logical_info);
> > + type_register_static(&spapr_drc_cpu_info);
> > + type_register_static(&spapr_drc_pci_info);
> > + type_register_static(&spapr_drc_lmb_info);
> >
> > spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> > rtas_set_indicator);
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 7a208a7..50d673b 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1761,8 +1761,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > /* allocate connectors for child PCI devices */
> > if (sphb->dr_enabled) {
> > for (i = 0; i < PCI_SLOT_MAX * 8; i++) {
> > - spapr_dr_connector_new(OBJECT(phb),
> > - SPAPR_DR_CONNECTOR_TYPE_PCI,
> > + spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
> > (sphb->index << 16) | i);
> > }
> > }
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 10e7c24..969c16d 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -26,6 +26,48 @@
> > #define SPAPR_DR_CONNECTOR(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > TYPE_SPAPR_DR_CONNECTOR)
> >
> > +#define TYPE_SPAPR_DRC_PHYSICAL "spapr-drc-physical"
> > +#define SPAPR_DRC_PHYSICAL_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PHYSICAL)
> > +#define SPAPR_DRC_PHYSICAL_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> > + TYPE_SPAPR_DRC_PHYSICAL)
> > +#define SPAPR_DRC_PHYSICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_PHYSICAL)
> > +
> > +#define TYPE_SPAPR_DRC_LOGICAL "spapr-drc-logical"
> > +#define SPAPR_DRC_LOGICAL_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LOGICAL)
> > +#define SPAPR_DRC_LOGICAL_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, \
> > + TYPE_SPAPR_DRC_LOGICAL)
> > +#define SPAPR_DRC_LOGICAL(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_LOGICAL)
> > +
> > +#define TYPE_SPAPR_DRC_CPU "spapr-drc-cpu"
> > +#define SPAPR_DRC_CPU_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_CPU)
> > +#define SPAPR_DRC_CPU_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_CPU)
> > +#define SPAPR_DRC_CPU(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_CPU)
> > +
> > +#define TYPE_SPAPR_DRC_PCI "spapr-drc-pci"
> > +#define SPAPR_DRC_PCI_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_PCI)
> > +#define SPAPR_DRC_PCI_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_PCI)
> > +#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_PCI)
> > +
> > +#define TYPE_SPAPR_DRC_LMB "spapr-drc-lmb"
> > +#define SPAPR_DRC_LMB_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(sPAPRDRConnectorClass, obj, TYPE_SPAPR_DRC_LMB)
> > +#define SPAPR_DRC_LMB_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(sPAPRDRConnectorClass, klass, TYPE_SPAPR_DRC_LMB)
> > +#define SPAPR_DRC_LMB(obj) OBJECT_CHECK(sPAPRDRConnector, (obj), \
> > + TYPE_SPAPR_DRC_LMB)
> > +
> > /*
> > * Various hotplug types managed by sPAPRDRConnector
> > *
> > @@ -134,7 +176,6 @@ typedef struct sPAPRDRConnector {
> > /*< private >*/
> > DeviceState parent;
> >
> > - sPAPRDRConnectorType type;
> > uint32_t id;
> > Object *owner;
> > const char *name;
> > @@ -163,6 +204,7 @@ typedef struct sPAPRDRConnectorClass {
> > DeviceClass parent;
> >
> > /*< public >*/
> > + sPAPRDRConnectorTypeShift typeshift;
> >
> > /* accessors for guest-visible (generally via RTAS) DR state */
> > uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> > @@ -186,8 +228,7 @@ typedef struct sPAPRDRConnectorClass {
> > uint32_t spapr_drc_index(sPAPRDRConnector *drc);
> > sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
> >
> > -sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> > - sPAPRDRConnectorType type,
> > +sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> > uint32_t id);
> > sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
> > sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
>
--
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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 2/5] spapr: Clean up spapr_dr_connector_by_*()
2017-06-02 7:29 [Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part II) David Gibson
2017-06-02 7:29 ` [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses David Gibson
@ 2017-06-02 7:29 ` David Gibson
2017-06-03 22:16 ` Michael Roth
2017-06-02 7:29 ` [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC David Gibson
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2017-06-02 7:29 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
* Change names to something less ludicrously verbose
* Now that we have QOM subclasses for the different DRC types, use a QOM
typename instead of a PAPR type value parameter
The latter allows removal of the get_type_shift() helper.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 28 ++++++++++++++--------------
hw/ppc/spapr_drc.c | 34 ++++++++++------------------------
hw/ppc/spapr_events.c | 2 +-
hw/ppc/spapr_pci.c | 6 ++----
include/hw/ppc/spapr_drc.h | 5 ++---
5 files changed, 29 insertions(+), 46 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 456f9e7..7aac3b9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -460,7 +460,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
int i;
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
+ 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)));
@@ -653,7 +653,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
if (i >= hotplug_lmb_start) {
sPAPRDRConnector *drc;
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, i);
g_assert(drc);
dynamic_memory[0] = cpu_to_be32(addr >> 32);
@@ -2528,8 +2528,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
uint64_t addr = addr_start;
for (i = 0; i < nr_lmbs; i++) {
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
- addr/SPAPR_MEMORY_BLOCK_SIZE);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+ addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
fdt = create_device_tree(&fdt_size);
@@ -2550,8 +2550,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
*/
if (dev->hotplugged) {
if (dedicated_hp_event_source) {
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
- addr_start / SPAPR_MEMORY_BLOCK_SIZE);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+ addr_start / SPAPR_MEMORY_BLOCK_SIZE);
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
nr_lmbs,
@@ -2668,8 +2668,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
addr = addr_start;
for (i = 0; i < nr_lmbs; i++) {
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
- addr / SPAPR_MEMORY_BLOCK_SIZE);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+ addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
if (drc->indicator_state != SPAPR_DR_INDICATOR_STATE_INACTIVE) {
avail_lmbs++;
@@ -2752,8 +2752,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
addr = addr_start;
for (i = 0; i < nr_lmbs; i++) {
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
- addr / SPAPR_MEMORY_BLOCK_SIZE);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+ addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -2761,8 +2761,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
addr += SPAPR_MEMORY_BLOCK_SIZE;
}
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
- addr_start / SPAPR_MEMORY_BLOCK_SIZE);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+ 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, spapr_drc_index(drc));
@@ -2833,7 +2833,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
return;
}
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
g_assert(drc);
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -2868,7 +2868,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
cc->core_id);
return;
}
- drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
g_assert(drc || !mc->has_hotpluggable_cpus);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 690b41f..2514f87 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -55,21 +55,6 @@ static void spapr_ccs_remove(sPAPRMachineState *spapr,
g_free(ccs);
}
-static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
-{
- uint32_t shift = 0;
-
- /* make sure this isn't SPAPR_DR_CONNECTOR_TYPE_ANY, or some
- * other wonky value.
- */
- g_assert(is_power_of_2(type));
-
- while (type != (1 << shift)) {
- shift++;
- }
- return shift;
-}
-
sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
{
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -805,7 +790,7 @@ static const TypeInfo spapr_drc_lmb_info = {
/* helper functions for external users */
-sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
+sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
{
Object *obj;
char name[256];
@@ -816,12 +801,13 @@ sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
return !obj ? NULL : SPAPR_DR_CONNECTOR(obj);
}
-sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
- uint32_t id)
+sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id)
{
- return spapr_dr_connector_by_index(
- (get_type_shift(type) << DRC_INDEX_TYPE_SHIFT) |
- (id & DRC_INDEX_ID_MASK));
+ sPAPRDRConnectorClass *drck
+ = SPAPR_DR_CONNECTOR_CLASS(object_class_by_name(type));
+
+ return spapr_drc_by_index(drck->typeshift << DRC_INDEX_TYPE_SHIFT
+ | (id & DRC_INDEX_ID_MASK));
}
/* generate a string the describes the DRC to encode into the
@@ -1024,7 +1010,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
}
/* if this is a DR sensor we can assume sensor_index == drc_index */
- drc = spapr_dr_connector_by_index(sensor_index);
+ drc = spapr_drc_by_index(sensor_index);
if (!drc) {
trace_spapr_rtas_set_indicator_invalid(sensor_index);
ret = RTAS_OUT_PARAM_ERROR;
@@ -1097,7 +1083,7 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
goto out;
}
- drc = spapr_dr_connector_by_index(sensor_index);
+ drc = spapr_drc_by_index(sensor_index);
if (!drc) {
trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
ret = RTAS_OUT_PARAM_ERROR;
@@ -1162,7 +1148,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
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);
+ drc = spapr_drc_by_index(drc_index);
if (!drc) {
trace_spapr_rtas_ibm_configure_connector_invalid(drc_index);
rc = RTAS_OUT_PARAM_ERROR;
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 6b01b04..4ab2312 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -478,7 +478,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
static void spapr_hotplug_set_signalled(uint32_t drc_index)
{
- sPAPRDRConnector *drc = spapr_dr_connector_by_index(drc_index);
+ sPAPRDRConnector *drc = spapr_drc_by_index(drc_index);
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
drck->set_signalled(drc);
}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 50d673b..0c181bb 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1400,10 +1400,8 @@ static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
uint32_t busnr,
int32_t devfn)
{
- return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
- (phb->index << 16) |
- (busnr << 8) |
- devfn);
+ return spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
+ (phb->index << 16) | (busnr << 8) | devfn);
}
static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index f77be38..d8cb84b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -230,9 +230,8 @@ sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
uint32_t id);
-sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
-sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
- uint32_t id);
+sPAPRDRConnector *spapr_drc_by_index(uint32_t index);
+sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id);
int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
uint32_t drc_type_mask);
--
2.9.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] spapr: Clean up spapr_dr_connector_by_*()
2017-06-02 7:29 ` [Qemu-devel] [PATCH 2/5] spapr: Clean up spapr_dr_connector_by_*() David Gibson
@ 2017-06-03 22:16 ` Michael Roth
0 siblings, 0 replies; 16+ messages in thread
From: Michael Roth @ 2017-06-03 22:16 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-02 02:29:49)
> * Change names to something less ludicrously verbose
> * Now that we have QOM subclasses for the different DRC types, use a QOM
> typename instead of a PAPR type value parameter
>
> The latter allows removal of the get_type_shift() helper.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 28 ++++++++++++++--------------
> hw/ppc/spapr_drc.c | 34 ++++++++++------------------------
> hw/ppc/spapr_events.c | 2 +-
> hw/ppc/spapr_pci.c | 6 ++----
> include/hw/ppc/spapr_drc.h | 5 ++---
> 5 files changed, 29 insertions(+), 46 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 456f9e7..7aac3b9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -460,7 +460,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
> int i;
>
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> + 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)));
> @@ -653,7 +653,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
> if (i >= hotplug_lmb_start) {
> sPAPRDRConnector *drc;
>
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, i);
> g_assert(drc);
>
> dynamic_memory[0] = cpu_to_be32(addr >> 32);
> @@ -2528,8 +2528,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> uint64_t addr = addr_start;
>
> for (i = 0; i < nr_lmbs; i++) {
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> - addr/SPAPR_MEMORY_BLOCK_SIZE);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> + addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
>
> fdt = create_device_tree(&fdt_size);
> @@ -2550,8 +2550,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> */
> if (dev->hotplugged) {
> if (dedicated_hp_event_source) {
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> - addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> + addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
> nr_lmbs,
> @@ -2668,8 +2668,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>
> addr = addr_start;
> for (i = 0; i < nr_lmbs; i++) {
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> - addr / SPAPR_MEMORY_BLOCK_SIZE);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> + addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
> if (drc->indicator_state != SPAPR_DR_INDICATOR_STATE_INACTIVE) {
> avail_lmbs++;
> @@ -2752,8 +2752,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>
> addr = addr_start;
> for (i = 0; i < nr_lmbs; i++) {
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> - addr / SPAPR_MEMORY_BLOCK_SIZE);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> + addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
>
> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -2761,8 +2761,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> }
>
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> - addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> + 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, spapr_drc_index(drc));
> @@ -2833,7 +2833,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> return;
> }
>
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
> g_assert(drc);
>
> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -2868,7 +2868,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> cc->core_id);
> return;
> }
> - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
>
> g_assert(drc || !mc->has_hotpluggable_cpus);
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 690b41f..2514f87 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -55,21 +55,6 @@ static void spapr_ccs_remove(sPAPRMachineState *spapr,
> g_free(ccs);
> }
>
> -static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> -{
> - uint32_t shift = 0;
> -
> - /* make sure this isn't SPAPR_DR_CONNECTOR_TYPE_ANY, or some
> - * other wonky value.
> - */
> - g_assert(is_power_of_2(type));
> -
> - while (type != (1 << shift)) {
> - shift++;
> - }
> - return shift;
> -}
> -
> sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> {
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -805,7 +790,7 @@ static const TypeInfo spapr_drc_lmb_info = {
>
> /* helper functions for external users */
>
> -sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> +sPAPRDRConnector *spapr_drc_by_index(uint32_t index)
> {
> Object *obj;
> char name[256];
> @@ -816,12 +801,13 @@ sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> return !obj ? NULL : SPAPR_DR_CONNECTOR(obj);
> }
>
> -sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
> - uint32_t id)
> +sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id)
> {
> - return spapr_dr_connector_by_index(
> - (get_type_shift(type) << DRC_INDEX_TYPE_SHIFT) |
> - (id & DRC_INDEX_ID_MASK));
> + sPAPRDRConnectorClass *drck
> + = SPAPR_DR_CONNECTOR_CLASS(object_class_by_name(type));
> +
> + return spapr_drc_by_index(drck->typeshift << DRC_INDEX_TYPE_SHIFT
> + | (id & DRC_INDEX_ID_MASK));
> }
>
> /* generate a string the describes the DRC to encode into the
> @@ -1024,7 +1010,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> }
>
> /* if this is a DR sensor we can assume sensor_index == drc_index */
> - drc = spapr_dr_connector_by_index(sensor_index);
> + drc = spapr_drc_by_index(sensor_index);
> if (!drc) {
> trace_spapr_rtas_set_indicator_invalid(sensor_index);
> ret = RTAS_OUT_PARAM_ERROR;
> @@ -1097,7 +1083,7 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> goto out;
> }
>
> - drc = spapr_dr_connector_by_index(sensor_index);
> + drc = spapr_drc_by_index(sensor_index);
> if (!drc) {
> trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> ret = RTAS_OUT_PARAM_ERROR;
> @@ -1162,7 +1148,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> 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);
> + drc = spapr_drc_by_index(drc_index);
> if (!drc) {
> trace_spapr_rtas_ibm_configure_connector_invalid(drc_index);
> rc = RTAS_OUT_PARAM_ERROR;
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 6b01b04..4ab2312 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -478,7 +478,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>
> static void spapr_hotplug_set_signalled(uint32_t drc_index)
> {
> - sPAPRDRConnector *drc = spapr_dr_connector_by_index(drc_index);
> + sPAPRDRConnector *drc = spapr_drc_by_index(drc_index);
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> drck->set_signalled(drc);
> }
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 50d673b..0c181bb 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1400,10 +1400,8 @@ static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
> uint32_t busnr,
> int32_t devfn)
> {
> - return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
> - (phb->index << 16) |
> - (busnr << 8) |
> - devfn);
> + return spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
> + (phb->index << 16) | (busnr << 8) | devfn);
> }
>
> static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb,
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index f77be38..d8cb84b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -230,9 +230,8 @@ sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
>
> sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> uint32_t id);
> -sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
> -sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
> - uint32_t id);
> +sPAPRDRConnector *spapr_drc_by_index(uint32_t index);
> +sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id);
> int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> uint32_t drc_type_mask);
>
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC
2017-06-02 7:29 [Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part II) David Gibson
2017-06-02 7:29 ` [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses David Gibson
2017-06-02 7:29 ` [Qemu-devel] [PATCH 2/5] spapr: Clean up spapr_dr_connector_by_*() David Gibson
@ 2017-06-02 7:29 ` David Gibson
2017-06-03 22:24 ` Michael Roth
2017-06-02 7:29 ` [Qemu-devel] [PATCH 4/5] spapr: Eliminate spapr_drc_get_type_str() David Gibson
2017-06-02 7:29 ` [Qemu-devel] [PATCH 5/5] spapr: Remove some non-useful properties on DRC objects David Gibson
4 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2017-06-02 7:29 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
Currently the sPAPRMachineState contains a of sPAPRConfigureConnector
structures which store intermediate state for the ibm,configure-connector
RTAS call.
This was an attempt to separate this state from the core of the DRC state.
However the configure connector process is intimately tied to the DRC
model, so there's really no point trying to have two levels of interface
here.
Moving the configure-connector state into its corresponding DRC allows
removal of a number of helpers for maintaining the anciliary list.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 4 ---
hw/ppc/spapr_drc.c | 73 ++++++++++++----------------------------------
include/hw/ppc/spapr.h | 14 ---------
include/hw/ppc/spapr_drc.h | 7 +++++
4 files changed, 25 insertions(+), 73 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7aac3b9..6234dbd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2339,10 +2339,6 @@ static void ppc_spapr_init(MachineState *machine)
register_savevm_live(NULL, "spapr/htab", -1, 1,
&savevm_htab_handlers, spapr);
- /* used by RTAS */
- QTAILQ_INIT(&spapr->ccs_list);
- qemu_register_reset(spapr_ccs_reset_hook, spapr);
-
qemu_register_boot_set(spapr_boot_set, spapr);
if (kvm_enabled()) {
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2514f87..27d4bd3 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -27,34 +27,6 @@
#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);
-}
-
sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
{
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -81,6 +53,16 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
+ /* if the guest is configuring a device attached to this DRC, we
+ * should reset the configuration state at this point since it may
+ * no longer be reliable (guest released device and needs to start
+ * over, or unplug occurred so the FDT is no longer valid)
+ */
+ if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
+ g_free(drc->ccs);
+ drc->ccs = NULL;
+ }
+
if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
/* cannot unisolate a non-existent resource, and, or resources
* which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
@@ -485,6 +467,10 @@ static void reset(DeviceState *d)
sPAPRDREntitySense state;
trace_spapr_drc_reset(spapr_drc_index(drc));
+
+ g_free(drc->ccs);
+ drc->ccs = NULL;
+
/* immediately upon reset we can safely assume DRCs whose devices
* are pending removal can be safely removed, and that they will
* subsequently be left in an ISOLATED state. move the DRC to this
@@ -1020,19 +1006,6 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
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:
@@ -1116,16 +1089,6 @@ static void configure_connector_st(target_ulong addr, target_ulong 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,
@@ -1161,12 +1124,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
goto out;
}
- ccs = spapr_ccs_find(spapr, drc_index);
+ ccs = drc->ccs;
if (!ccs) {
ccs = g_new0(sPAPRConfigureConnectorState, 1);
ccs->fdt_offset = drc->fdt_start_offset;
- ccs->drc_index = drc_index;
- spapr_ccs_add(spapr, ccs);
+ drc->ccs = ccs;
}
do {
@@ -1203,7 +1165,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
/* guest should be not configuring an isolated device */
trace_spapr_drc_set_configured_skipping(drc_index);
}
- spapr_ccs_remove(spapr, ccs);
+ g_free(ccs);
+ drc->ccs = NULL;
ccs = NULL;
resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
} else {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 98fb78b..f973b02 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -11,7 +11,6 @@
struct VIOsPAPRBus;
struct sPAPRPHBState;
struct sPAPRNVRAM;
-typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
typedef struct sPAPREventLogEntry sPAPREventLogEntry;
typedef struct sPAPREventSource sPAPREventSource;
@@ -102,9 +101,6 @@ struct sPAPRMachineState {
bool htab_first_pass;
int htab_fd;
- /* RTAS state */
- QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
-
/* Pending DIMM unplug cache. It is populated when a LMB
* unplug starts. It can be regenerated if a migration
* occurs during the unplug process. */
@@ -646,16 +642,6 @@ void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
void spapr_core_release(DeviceState *dev);
void spapr_lmb_release(DeviceState *dev);
-/* rtas-configure-connector state */
-struct sPAPRConfigureConnectorState {
- uint32_t drc_index;
- int fdt_offset;
- int fdt_depth;
- QTAILQ_ENTRY(sPAPRConfigureConnectorState) next;
-};
-
-void spapr_ccs_reset_hook(void *opaque);
-
void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index d8cb84b..53b0f8b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -172,6 +172,12 @@ typedef enum {
SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
} sPAPRDRCCResponse;
+/* rtas-configure-connector state */
+typedef struct sPAPRConfigureConnectorState {
+ int fdt_offset;
+ int fdt_depth;
+} sPAPRConfigureConnectorState;
+
typedef struct sPAPRDRConnector {
/*< private >*/
DeviceState parent;
@@ -189,6 +195,7 @@ typedef struct sPAPRDRConnector {
void *fdt;
int fdt_start_offset;
bool configured;
+ sPAPRConfigureConnectorState *ccs;
bool awaiting_release;
bool signalled;
--
2.9.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC
2017-06-02 7:29 ` [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC David Gibson
@ 2017-06-03 22:24 ` Michael Roth
2017-06-04 10:26 ` David Gibson
0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2017-06-03 22:24 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-02 02:29:50)
> Currently the sPAPRMachineState contains a of sPAPRConfigureConnector
"contains a list"?
> structures which store intermediate state for the ibm,configure-connector
> RTAS call.
>
> This was an attempt to separate this state from the core of the DRC state.
> However the configure connector process is intimately tied to the DRC
> model, so there's really no point trying to have two levels of interface
> here.
>
> Moving the configure-connector state into its corresponding DRC allows
> removal of a number of helpers for maintaining the anciliary list.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 4 ---
> hw/ppc/spapr_drc.c | 73 ++++++++++++----------------------------------
> include/hw/ppc/spapr.h | 14 ---------
> include/hw/ppc/spapr_drc.h | 7 +++++
> 4 files changed, 25 insertions(+), 73 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7aac3b9..6234dbd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2339,10 +2339,6 @@ static void ppc_spapr_init(MachineState *machine)
> register_savevm_live(NULL, "spapr/htab", -1, 1,
> &savevm_htab_handlers, spapr);
>
> - /* used by RTAS */
> - QTAILQ_INIT(&spapr->ccs_list);
> - qemu_register_reset(spapr_ccs_reset_hook, spapr);
> -
> qemu_register_boot_set(spapr_boot_set, spapr);
>
> if (kvm_enabled()) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 2514f87..27d4bd3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,34 +27,6 @@
> #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);
> -}
> -
> sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> {
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -81,6 +53,16 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>
> trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
>
> + /* if the guest is configuring a device attached to this DRC, we
> + * should reset the configuration state at this point since it may
> + * no longer be reliable (guest released device and needs to start
> + * over, or unplug occurred so the FDT is no longer valid)
> + */
> + if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> + g_free(drc->ccs);
> + drc->ccs = NULL;
> + }
> +
> if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> /* cannot unisolate a non-existent resource, and, or resources
> * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> @@ -485,6 +467,10 @@ static void reset(DeviceState *d)
> sPAPRDREntitySense state;
>
> trace_spapr_drc_reset(spapr_drc_index(drc));
> +
> + g_free(drc->ccs);
> + drc->ccs = NULL;
> +
> /* immediately upon reset we can safely assume DRCs whose devices
> * are pending removal can be safely removed, and that they will
> * subsequently be left in an ISOLATED state. move the DRC to this
> @@ -1020,19 +1006,6 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>
> 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:
> @@ -1116,16 +1089,6 @@ static void configure_connector_st(target_ulong addr, target_ulong 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,
> @@ -1161,12 +1124,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> goto out;
> }
>
> - ccs = spapr_ccs_find(spapr, drc_index);
> + ccs = drc->ccs;
> if (!ccs) {
> ccs = g_new0(sPAPRConfigureConnectorState, 1);
> ccs->fdt_offset = drc->fdt_start_offset;
> - ccs->drc_index = drc_index;
> - spapr_ccs_add(spapr, ccs);
> + drc->ccs = ccs;
> }
>
> do {
> @@ -1203,7 +1165,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> /* guest should be not configuring an isolated device */
> trace_spapr_drc_set_configured_skipping(drc_index);
> }
> - spapr_ccs_remove(spapr, ccs);
> + g_free(ccs);
> + drc->ccs = NULL;
> ccs = NULL;
> resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> } else {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 98fb78b..f973b02 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -11,7 +11,6 @@
> struct VIOsPAPRBus;
> struct sPAPRPHBState;
> struct sPAPRNVRAM;
> -typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
> typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> typedef struct sPAPREventSource sPAPREventSource;
>
> @@ -102,9 +101,6 @@ struct sPAPRMachineState {
> bool htab_first_pass;
> int htab_fd;
>
> - /* RTAS state */
> - QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
> -
> /* Pending DIMM unplug cache. It is populated when a LMB
> * unplug starts. It can be regenerated if a migration
> * occurs during the unplug process. */
> @@ -646,16 +642,6 @@ void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> void spapr_core_release(DeviceState *dev);
> void spapr_lmb_release(DeviceState *dev);
>
> -/* rtas-configure-connector state */
> -struct sPAPRConfigureConnectorState {
> - uint32_t drc_index;
> - int fdt_offset;
> - int fdt_depth;
> - QTAILQ_ENTRY(sPAPRConfigureConnectorState) next;
> -};
> -
> -void spapr_ccs_reset_hook(void *opaque);
> -
> void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index d8cb84b..53b0f8b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -172,6 +172,12 @@ typedef enum {
> SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
> } sPAPRDRCCResponse;
>
> +/* rtas-configure-connector state */
> +typedef struct sPAPRConfigureConnectorState {
> + int fdt_offset;
> + int fdt_depth;
> +} sPAPRConfigureConnectorState;
> +
> typedef struct sPAPRDRConnector {
> /*< private >*/
> DeviceState parent;
> @@ -189,6 +195,7 @@ typedef struct sPAPRDRConnector {
> void *fdt;
> int fdt_start_offset;
> bool configured;
> + sPAPRConfigureConnectorState *ccs;
>
> bool awaiting_release;
> bool signalled;
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC
2017-06-03 22:24 ` Michael Roth
@ 2017-06-04 10:26 ` David Gibson
0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2017-06-04 10:26 UTC (permalink / raw)
To: Michael Roth; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
[-- Attachment #1: Type: text/plain, Size: 9687 bytes --]
On Sat, Jun 03, 2017 at 05:24:23PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-02 02:29:50)
> > Currently the sPAPRMachineState contains a of sPAPRConfigureConnector
>
> "contains a list"?
Ta, corrected.
> > structures which store intermediate state for the ibm,configure-connector
> > RTAS call.
> >
> > This was an attempt to separate this state from the core of the DRC state.
> > However the configure connector process is intimately tied to the DRC
> > model, so there's really no point trying to have two levels of interface
> > here.
> >
> > Moving the configure-connector state into its corresponding DRC allows
> > removal of a number of helpers for maintaining the anciliary list.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> > ---
> > hw/ppc/spapr.c | 4 ---
> > hw/ppc/spapr_drc.c | 73 ++++++++++++----------------------------------
> > include/hw/ppc/spapr.h | 14 ---------
> > include/hw/ppc/spapr_drc.h | 7 +++++
> > 4 files changed, 25 insertions(+), 73 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7aac3b9..6234dbd 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2339,10 +2339,6 @@ static void ppc_spapr_init(MachineState *machine)
> > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > &savevm_htab_handlers, spapr);
> >
> > - /* used by RTAS */
> > - QTAILQ_INIT(&spapr->ccs_list);
> > - qemu_register_reset(spapr_ccs_reset_hook, spapr);
> > -
> > qemu_register_boot_set(spapr_boot_set, spapr);
> >
> > if (kvm_enabled()) {
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 2514f87..27d4bd3 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -27,34 +27,6 @@
> > #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);
> > -}
> > -
> > sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> > {
> > sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -81,6 +53,16 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> >
> > trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> >
> > + /* if the guest is configuring a device attached to this DRC, we
> > + * should reset the configuration state at this point since it may
> > + * no longer be reliable (guest released device and needs to start
> > + * over, or unplug occurred so the FDT is no longer valid)
> > + */
> > + if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > + g_free(drc->ccs);
> > + drc->ccs = NULL;
> > + }
> > +
> > if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> > /* cannot unisolate a non-existent resource, and, or resources
> > * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> > @@ -485,6 +467,10 @@ static void reset(DeviceState *d)
> > sPAPRDREntitySense state;
> >
> > trace_spapr_drc_reset(spapr_drc_index(drc));
> > +
> > + g_free(drc->ccs);
> > + drc->ccs = NULL;
> > +
> > /* immediately upon reset we can safely assume DRCs whose devices
> > * are pending removal can be safely removed, and that they will
> > * subsequently be left in an ISOLATED state. move the DRC to this
> > @@ -1020,19 +1006,6 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >
> > 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:
> > @@ -1116,16 +1089,6 @@ static void configure_connector_st(target_ulong addr, target_ulong 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,
> > @@ -1161,12 +1124,11 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> > goto out;
> > }
> >
> > - ccs = spapr_ccs_find(spapr, drc_index);
> > + ccs = drc->ccs;
> > if (!ccs) {
> > ccs = g_new0(sPAPRConfigureConnectorState, 1);
> > ccs->fdt_offset = drc->fdt_start_offset;
> > - ccs->drc_index = drc_index;
> > - spapr_ccs_add(spapr, ccs);
> > + drc->ccs = ccs;
> > }
> >
> > do {
> > @@ -1203,7 +1165,8 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> > /* guest should be not configuring an isolated device */
> > trace_spapr_drc_set_configured_skipping(drc_index);
> > }
> > - spapr_ccs_remove(spapr, ccs);
> > + g_free(ccs);
> > + drc->ccs = NULL;
> > ccs = NULL;
> > resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> > } else {
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 98fb78b..f973b02 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -11,7 +11,6 @@
> > struct VIOsPAPRBus;
> > struct sPAPRPHBState;
> > struct sPAPRNVRAM;
> > -typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
> > typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> > typedef struct sPAPREventSource sPAPREventSource;
> >
> > @@ -102,9 +101,6 @@ struct sPAPRMachineState {
> > bool htab_first_pass;
> > int htab_fd;
> >
> > - /* RTAS state */
> > - QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
> > -
> > /* Pending DIMM unplug cache. It is populated when a LMB
> > * unplug starts. It can be regenerated if a migration
> > * occurs during the unplug process. */
> > @@ -646,16 +642,6 @@ void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> > void spapr_core_release(DeviceState *dev);
> > void spapr_lmb_release(DeviceState *dev);
> >
> > -/* rtas-configure-connector state */
> > -struct sPAPRConfigureConnectorState {
> > - uint32_t drc_index;
> > - int fdt_offset;
> > - int fdt_depth;
> > - QTAILQ_ENTRY(sPAPRConfigureConnectorState) next;
> > -};
> > -
> > -void spapr_ccs_reset_hook(void *opaque);
> > -
> > void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> > int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
> >
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index d8cb84b..53b0f8b 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -172,6 +172,12 @@ typedef enum {
> > SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
> > } sPAPRDRCCResponse;
> >
> > +/* rtas-configure-connector state */
> > +typedef struct sPAPRConfigureConnectorState {
> > + int fdt_offset;
> > + int fdt_depth;
> > +} sPAPRConfigureConnectorState;
> > +
> > typedef struct sPAPRDRConnector {
> > /*< private >*/
> > DeviceState parent;
> > @@ -189,6 +195,7 @@ typedef struct sPAPRDRConnector {
> > void *fdt;
> > int fdt_start_offset;
> > bool configured;
> > + sPAPRConfigureConnectorState *ccs;
> >
> > bool awaiting_release;
> > bool signalled;
>
--
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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 4/5] spapr: Eliminate spapr_drc_get_type_str()
2017-06-02 7:29 [Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part II) David Gibson
` (2 preceding siblings ...)
2017-06-02 7:29 ` [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC David Gibson
@ 2017-06-02 7:29 ` David Gibson
2017-06-03 22:27 ` Michael Roth
2017-06-02 7:29 ` [Qemu-devel] [PATCH 5/5] spapr: Remove some non-useful properties on DRC objects David Gibson
4 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2017-06-02 7:29 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
This function was used in generating the device tree. However, now that
we have different QOM types for different DRC types we can easily store
the information we need in the class structure and avoid this specialized
lookup function.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 31 ++++---------------------------
include/hw/ppc/spapr_drc.h | 1 +
2 files changed, 5 insertions(+), 27 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 27d4bd3..d43c9cd 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -716,6 +716,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
+ drck->typename = "CPU";
}
static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
@@ -723,6 +724,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
+ drck->typename = "28";
}
static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
@@ -730,6 +732,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
+ drck->typename = "MEM";
}
static const TypeInfo spapr_dr_connector_info = {
@@ -796,31 +799,6 @@ sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id)
| (id & DRC_INDEX_ID_MASK));
}
-/* generate a string the describes the DRC to encode into the
- * device tree.
- *
- * as documented by PAPR+ v2.7, 13.5.2.6 and C.6.1
- */
-static const char *spapr_drc_get_type_str(sPAPRDRConnectorType type)
-{
- switch (type) {
- case SPAPR_DR_CONNECTOR_TYPE_CPU:
- return "CPU";
- case SPAPR_DR_CONNECTOR_TYPE_PHB:
- return "PHB";
- case SPAPR_DR_CONNECTOR_TYPE_VIO:
- return "SLOT";
- case SPAPR_DR_CONNECTOR_TYPE_PCI:
- return "28";
- case SPAPR_DR_CONNECTOR_TYPE_LMB:
- return "MEM";
- default:
- g_assert(false);
- }
-
- return NULL;
-}
-
/**
* spapr_drc_populate_dt
*
@@ -902,8 +880,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
/* ibm,drc-types */
- drc_types = g_string_append(drc_types,
- spapr_drc_get_type_str(spapr_drc_type(drc)));
+ drc_types = g_string_append(drc_types, drck->typename);
drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
}
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 53b0f8b..8a4889a 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -212,6 +212,7 @@ typedef struct sPAPRDRConnectorClass {
/*< public >*/
sPAPRDRConnectorTypeShift typeshift;
+ const char *typename; /* used in device tree, PAPR 13.5.2.6 & C.6.1 */
/* accessors for guest-visible (generally via RTAS) DR state */
uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
--
2.9.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] spapr: Eliminate spapr_drc_get_type_str()
2017-06-02 7:29 ` [Qemu-devel] [PATCH 4/5] spapr: Eliminate spapr_drc_get_type_str() David Gibson
@ 2017-06-03 22:27 ` Michael Roth
0 siblings, 0 replies; 16+ messages in thread
From: Michael Roth @ 2017-06-03 22:27 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-02 02:29:51)
> This function was used in generating the device tree. However, now that
> we have different QOM types for different DRC types we can easily store
> the information we need in the class structure and avoid this specialized
> lookup function.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 31 ++++---------------------------
> include/hw/ppc/spapr_drc.h | 1 +
> 2 files changed, 5 insertions(+), 27 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 27d4bd3..d43c9cd 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -716,6 +716,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>
> drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> + drck->typename = "CPU";
> }
>
> static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> @@ -723,6 +724,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>
> drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> + drck->typename = "28";
> }
>
> static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> @@ -730,6 +732,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>
> drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> + drck->typename = "MEM";
> }
>
> static const TypeInfo spapr_dr_connector_info = {
> @@ -796,31 +799,6 @@ sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id)
> | (id & DRC_INDEX_ID_MASK));
> }
>
> -/* generate a string the describes the DRC to encode into the
> - * device tree.
> - *
> - * as documented by PAPR+ v2.7, 13.5.2.6 and C.6.1
> - */
> -static const char *spapr_drc_get_type_str(sPAPRDRConnectorType type)
> -{
> - switch (type) {
> - case SPAPR_DR_CONNECTOR_TYPE_CPU:
> - return "CPU";
> - case SPAPR_DR_CONNECTOR_TYPE_PHB:
> - return "PHB";
> - case SPAPR_DR_CONNECTOR_TYPE_VIO:
> - return "SLOT";
> - case SPAPR_DR_CONNECTOR_TYPE_PCI:
> - return "28";
> - case SPAPR_DR_CONNECTOR_TYPE_LMB:
> - return "MEM";
> - default:
> - g_assert(false);
> - }
> -
> - return NULL;
> -}
> -
> /**
> * spapr_drc_populate_dt
> *
> @@ -902,8 +880,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
>
> /* ibm,drc-types */
> - drc_types = g_string_append(drc_types,
> - spapr_drc_get_type_str(spapr_drc_type(drc)));
> + drc_types = g_string_append(drc_types, drck->typename);
> drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
> }
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 53b0f8b..8a4889a 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -212,6 +212,7 @@ typedef struct sPAPRDRConnectorClass {
>
> /*< public >*/
> sPAPRDRConnectorTypeShift typeshift;
> + const char *typename; /* used in device tree, PAPR 13.5.2.6 & C.6.1 */
>
> /* accessors for guest-visible (generally via RTAS) DR state */
> uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 5/5] spapr: Remove some non-useful properties on DRC objects
2017-06-02 7:29 [Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part II) David Gibson
` (3 preceding siblings ...)
2017-06-02 7:29 ` [Qemu-devel] [PATCH 4/5] spapr: Eliminate spapr_drc_get_type_str() David Gibson
@ 2017-06-02 7:29 ` David Gibson
2017-06-03 22:34 ` Michael Roth
4 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2017-06-02 7:29 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
* 'connector_type' is easily derived from the 'index' property, so there's
no point to it (it's also implicit in the QOM type of the DRC)
* 'isolation-state', 'indicator-state' and 'allocation-state' are
part of the transaction between qemu and guest during PAPR hotplug
operations, and outside tools really have no business looking at it
(especially not changing, and these were RW properties)
* 'entity-sense' is basically just a weird PAPR encoding of whether there
is a device connected to this DRC
Strictly speaking removing these properties is breaking the qemu interface.
However, I'm pretty sure no management tools have ever used these. For
debugging there are better alternatives. Therefore, I think removing these
broken interfaces is the better option.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 29 -----------------------------
1 file changed, 29 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index d43c9cd..4dd26a8 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -228,14 +228,6 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
visit_type_uint32(v, name, &value, errp);
}
-static void prop_get_type(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
- sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
- uint32_t value = (uint32_t)spapr_drc_type(drc);
- visit_type_uint32(v, name, &value, errp);
-}
-
static char *prop_get_name(Object *obj, Error **errp)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
@@ -243,17 +235,6 @@ static char *prop_get_name(Object *obj, Error **errp)
return g_strdup(drck->get_name(drc));
}
-static void prop_get_entity_sense(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;
-
- drck->entity_sense(drc, &value);
- visit_type_uint32(v, name, &value, errp);
-}
-
static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -670,20 +651,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
- object_property_add_uint32_ptr(obj, "isolation-state",
- &drc->isolation_state, NULL);
- object_property_add_uint32_ptr(obj, "indicator-state",
- &drc->indicator_state, NULL);
- object_property_add_uint32_ptr(obj, "allocation-state",
- &drc->allocation_state, NULL);
object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
object_property_add(obj, "index", "uint32", prop_get_index,
NULL, NULL, NULL, NULL);
- object_property_add(obj, "connector_type", "uint32", prop_get_type,
- NULL, NULL, NULL, NULL);
object_property_add_str(obj, "name", prop_get_name, NULL, NULL);
- object_property_add(obj, "entity-sense", "uint32", prop_get_entity_sense,
- NULL, NULL, NULL, NULL);
object_property_add(obj, "fdt", "struct", prop_get_fdt,
NULL, NULL, NULL, NULL);
}
--
2.9.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH 5/5] spapr: Remove some non-useful properties on DRC objects
2017-06-02 7:29 ` [Qemu-devel] [PATCH 5/5] spapr: Remove some non-useful properties on DRC objects David Gibson
@ 2017-06-03 22:34 ` Michael Roth
0 siblings, 0 replies; 16+ messages in thread
From: Michael Roth @ 2017-06-03 22:34 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-02 02:29:52)
> * 'connector_type' is easily derived from the 'index' property, so there's
> no point to it (it's also implicit in the QOM type of the DRC)
> * 'isolation-state', 'indicator-state' and 'allocation-state' are
> part of the transaction between qemu and guest during PAPR hotplug
> operations, and outside tools really have no business looking at it
> (especially not changing, and these were RW properties)
> * 'entity-sense' is basically just a weird PAPR encoding of whether there
> is a device connected to this DRC
>
> Strictly speaking removing these properties is breaking the qemu interface.
> However, I'm pretty sure no management tools have ever used these. For
> debugging there are better alternatives. Therefore, I think removing these
> broken interfaces is the better option.
Debugging was indeed the primary motivation behind these. I agree these
don't really serve any useful purpose now and probably should've just
been traces from the start.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 29 -----------------------------
> 1 file changed, 29 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index d43c9cd..4dd26a8 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -228,14 +228,6 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
> visit_type_uint32(v, name, &value, errp);
> }
>
> -static void prop_get_type(Object *obj, Visitor *v, const char *name,
> - void *opaque, Error **errp)
> -{
> - sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> - uint32_t value = (uint32_t)spapr_drc_type(drc);
> - visit_type_uint32(v, name, &value, errp);
> -}
> -
> static char *prop_get_name(Object *obj, Error **errp)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> @@ -243,17 +235,6 @@ static char *prop_get_name(Object *obj, Error **errp)
> return g_strdup(drck->get_name(drc));
> }
>
> -static void prop_get_entity_sense(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;
> -
> - drck->entity_sense(drc, &value);
> - visit_type_uint32(v, name, &value, errp);
> -}
> -
> static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -670,20 +651,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
>
> - object_property_add_uint32_ptr(obj, "isolation-state",
> - &drc->isolation_state, NULL);
> - object_property_add_uint32_ptr(obj, "indicator-state",
> - &drc->indicator_state, NULL);
> - object_property_add_uint32_ptr(obj, "allocation-state",
> - &drc->allocation_state, NULL);
> object_property_add_uint32_ptr(obj, "id", &drc->id, NULL);
> object_property_add(obj, "index", "uint32", prop_get_index,
> NULL, NULL, NULL, NULL);
> - object_property_add(obj, "connector_type", "uint32", prop_get_type,
> - NULL, NULL, NULL, NULL);
> object_property_add_str(obj, "name", prop_get_name, NULL, NULL);
> - object_property_add(obj, "entity-sense", "uint32", prop_get_entity_sense,
> - NULL, NULL, NULL, NULL);
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> NULL, NULL, NULL, NULL);
> }
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread