* [Qemu-devel] [PATCH 1/7] spapr: Clean up DR entity sense handling
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 16:19 ` Michael Roth
2017-06-06 8:32 ` [Qemu-devel] [PATCH 2/7] spapr: Abolish DRC get_name method David Gibson
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
DRC classes have an entity_sense method to determine (in a specific PAPR
sense) the presence or absence of a device plugged into a DRC. However,
we only have one implementation of the method, which explicitly tests for
different DRC types. This changes it to instead have different method
implementations for the two cases: "logical" and "physical" DRCs.
While we're at it, the entity sense method always returns RTAS_OUT_SUCCESS,
and the interesting value is returned via pass-by-reference. Simplify this
to directly return the value we care about
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 76 ++++++++++++++++++++++++----------------------
hw/ppc/spapr_pci.c | 6 ++--
hw/ppc/trace-events | 1 -
include/hw/ppc/spapr_drc.h | 4 +--
4 files changed, 44 insertions(+), 43 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 39e7f30..da3779e 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -185,39 +185,29 @@ static void set_signalled(sPAPRDRConnector *drc)
* based on the current allocation/indicator/power states
* for the DR connector.
*/
-static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
+static sPAPRDREntitySense physical_entity_sense(sPAPRDRConnector *drc)
{
- if (drc->dev) {
- 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.
- * Otherwise, report the state as USABLE/PRESENT,
- * as we would for PCI.
- */
- *state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
- } else {
- /* this assumes all PCI devices are assigned to
- * a 'live insertion' power domain, where QEMU
- * manages power state automatically as opposed
- * to the guest. present, non-PCI resources are
- * unaffected by power state.
- */
- *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
- }
+ /* this assumes all PCI devices are assigned to a 'live insertion'
+ * power domain, where QEMU manages power state automatically as
+ * opposed to the guest. present, non-PCI resources are unaffected
+ * by power state.
+ */
+ return drc->dev ? SPAPR_DR_ENTITY_SENSE_PRESENT
+ : SPAPR_DR_ENTITY_SENSE_EMPTY;
+}
+
+static sPAPRDREntitySense logical_entity_sense(sPAPRDRConnector *drc)
+{
+ /* for logical DR, we return a state of UNUSABLE iff the
+ * allocation state UNUSABLE. Otherwise, report the state as
+ * USABLE/PRESENT, as we would for PCI.
+ */
+ if (drc->dev
+ && (drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE)) {
+ return SPAPR_DR_ENTITY_SENSE_PRESENT;
} else {
- 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
- */
- *state = SPAPR_DR_ENTITY_SENSE_EMPTY;
- } else {
- *state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
- }
+ return SPAPR_DR_ENTITY_SENSE_UNUSABLE;
}
-
- trace_spapr_drc_entity_sense(spapr_drc_index(drc), *state);
- return RTAS_OUT_SUCCESS;
}
static void prop_get_index(Object *obj, Visitor *v, const char *name,
@@ -445,7 +435,6 @@ static void reset(DeviceState *d)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- sPAPRDREntitySense state;
trace_spapr_drc_reset(spapr_drc_index(drc));
@@ -477,8 +466,7 @@ static void reset(DeviceState *d)
}
}
- drck->entity_sense(drc, &state);
- if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
+ if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
drck->set_signalled(drc);
}
}
@@ -488,8 +476,7 @@ static bool spapr_drc_needed(void *opaque)
sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
bool rc = false;
- sPAPRDREntitySense value;
- drck->entity_sense(drc, &value);
+ sPAPRDREntitySense value = drck->dr_entity_sense(drc);
/* If no dev is plugged in there is no need to migrate the DRC state */
if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
@@ -667,7 +654,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
drck->set_indicator_state = set_indicator_state;
drck->set_allocation_state = set_allocation_state;
drck->get_name = get_name;
- drck->entity_sense = entity_sense;
drck->attach = attach;
drck->detach = detach;
drck->release_pending = release_pending;
@@ -678,6 +664,20 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->user_creatable = false;
}
+static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->dr_entity_sense = physical_entity_sense;
+}
+
+static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
+{
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+
+ drck->dr_entity_sense = logical_entity_sense;
+}
+
static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
{
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
@@ -716,6 +716,7 @@ static const TypeInfo spapr_drc_physical_info = {
.name = TYPE_SPAPR_DRC_PHYSICAL,
.parent = TYPE_SPAPR_DR_CONNECTOR,
.instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_physical_class_init,
.abstract = true,
};
@@ -723,6 +724,7 @@ static const TypeInfo spapr_drc_logical_info = {
.name = TYPE_SPAPR_DRC_LOGICAL,
.parent = TYPE_SPAPR_DR_CONNECTOR,
.instance_size = sizeof(sPAPRDRConnector),
+ .class_init = spapr_drc_logical_class_init,
.abstract = true,
};
@@ -1010,7 +1012,7 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
goto out;
}
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- ret = drck->entity_sense(drc, &sensor_state);
+ sensor_state = drck->dr_entity_sense(drc);
out:
rtas_st(rets, 0, ret);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 0c181bb..5cba8f9 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1481,7 +1481,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
PCI_DEVFN(slotnr, i));
func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
- func_drck->entity_sense(func_drc, &state);
+ state = func_drck->dr_entity_sense(func_drc);
if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
spapr_hotplug_req_add_by_index(func_drc);
@@ -1522,7 +1522,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
PCI_DEVFN(slotnr, i));
func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
- func_drck->entity_sense(func_drc, &state);
+ state = func_drck->dr_entity_sense(func_drc);
if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
&& !func_drck->release_pending(func_drc)) {
error_setg(errp,
@@ -1548,7 +1548,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
PCI_DEVFN(slotnr, i));
func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
- func_drck->entity_sense(func_drc, &state);
+ state = func_drck->dr_entity_sense(func_drc);
if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
spapr_hotplug_req_remove_by_index(func_drc);
}
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 43d265f..4979397 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -44,7 +44,6 @@ spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", sta
spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
-spapr_drc_entity_sense(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_awaiting_isolated(uint32_t index) "drc: 0x%"PRIx32
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index c88e1be..f892b94 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -214,6 +214,8 @@ typedef struct sPAPRDRConnectorClass {
sPAPRDRConnectorTypeShift typeshift;
const char *typename; /* used in device tree, PAPR 13.5.2.6 & C.6.1 */
+ sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
+
/* accessors for guest-visible (generally via RTAS) DR state */
uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
sPAPRDRIsolationState state);
@@ -223,8 +225,6 @@ typedef struct sPAPRDRConnectorClass {
sPAPRDRAllocationState state);
const char *(*get_name)(sPAPRDRConnector *drc);
- uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
-
/* QEMU interfaces for managing hotplug operations */
void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
int fdt_start_offset, bool coldplug, Error **errp);
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] spapr: Clean up DR entity sense handling
2017-06-06 8:32 ` [Qemu-devel] [PATCH 1/7] spapr: Clean up DR entity sense handling David Gibson
@ 2017-06-06 16:19 ` Michael Roth
0 siblings, 0 replies; 17+ messages in thread
From: Michael Roth @ 2017-06-06 16:19 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 03:32:15)
> DRC classes have an entity_sense method to determine (in a specific PAPR
> sense) the presence or absence of a device plugged into a DRC. However,
> we only have one implementation of the method, which explicitly tests for
> different DRC types. This changes it to instead have different method
> implementations for the two cases: "logical" and "physical" DRCs.
>
> While we're at it, the entity sense method always returns RTAS_OUT_SUCCESS,
> and the interesting value is returned via pass-by-reference. Simplify this
> to directly return the value we care about
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr_drc.c | 76 ++++++++++++++++++++++++----------------------
> hw/ppc/spapr_pci.c | 6 ++--
> hw/ppc/trace-events | 1 -
> include/hw/ppc/spapr_drc.h | 4 +--
> 4 files changed, 44 insertions(+), 43 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 39e7f30..da3779e 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -185,39 +185,29 @@ static void set_signalled(sPAPRDRConnector *drc)
> * based on the current allocation/indicator/power states
> * for the DR connector.
> */
> -static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state)
> +static sPAPRDREntitySense physical_entity_sense(sPAPRDRConnector *drc)
> {
> - if (drc->dev) {
> - 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.
> - * Otherwise, report the state as USABLE/PRESENT,
> - * as we would for PCI.
> - */
> - *state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> - } else {
> - /* this assumes all PCI devices are assigned to
> - * a 'live insertion' power domain, where QEMU
> - * manages power state automatically as opposed
> - * to the guest. present, non-PCI resources are
> - * unaffected by power state.
> - */
> - *state = SPAPR_DR_ENTITY_SENSE_PRESENT;
> - }
> + /* this assumes all PCI devices are assigned to a 'live insertion'
> + * power domain, where QEMU manages power state automatically as
> + * opposed to the guest. present, non-PCI resources are unaffected
> + * by power state.
> + */
> + return drc->dev ? SPAPR_DR_ENTITY_SENSE_PRESENT
> + : SPAPR_DR_ENTITY_SENSE_EMPTY;
> +}
> +
> +static sPAPRDREntitySense logical_entity_sense(sPAPRDRConnector *drc)
> +{
> + /* for logical DR, we return a state of UNUSABLE iff the
> + * allocation state UNUSABLE. Otherwise, report the state as
> + * USABLE/PRESENT, as we would for PCI.
> + */
Now that it's outside of the drc->dev block this comment isn't quite
accurate. And any rewordings I come up with in this new context
basically end up just describing the code (nice), so I think it should
just be dropped.
Otherwise:
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> + if (drc->dev
> + && (drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE)) {
> + return SPAPR_DR_ENTITY_SENSE_PRESENT;
> } else {
> - 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
> - */
> - *state = SPAPR_DR_ENTITY_SENSE_EMPTY;
> - } else {
> - *state = SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> - }
> + return SPAPR_DR_ENTITY_SENSE_UNUSABLE;
> }
> -
> - trace_spapr_drc_entity_sense(spapr_drc_index(drc), *state);
> - return RTAS_OUT_SUCCESS;
> }
>
> static void prop_get_index(Object *obj, Visitor *v, const char *name,
> @@ -445,7 +435,6 @@ static void reset(DeviceState *d)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - sPAPRDREntitySense state;
>
> trace_spapr_drc_reset(spapr_drc_index(drc));
>
> @@ -477,8 +466,7 @@ static void reset(DeviceState *d)
> }
> }
>
> - drck->entity_sense(drc, &state);
> - if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> + if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> drck->set_signalled(drc);
> }
> }
> @@ -488,8 +476,7 @@ static bool spapr_drc_needed(void *opaque)
> sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> bool rc = false;
> - sPAPRDREntitySense value;
> - drck->entity_sense(drc, &value);
> + sPAPRDREntitySense value = drck->dr_entity_sense(drc);
>
> /* If no dev is plugged in there is no need to migrate the DRC state */
> if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
> @@ -667,7 +654,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> drck->set_indicator_state = set_indicator_state;
> drck->set_allocation_state = set_allocation_state;
> drck->get_name = get_name;
> - drck->entity_sense = entity_sense;
> drck->attach = attach;
> drck->detach = detach;
> drck->release_pending = release_pending;
> @@ -678,6 +664,20 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->user_creatable = false;
> }
>
> +static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->dr_entity_sense = physical_entity_sense;
> +}
> +
> +static void spapr_drc_logical_class_init(ObjectClass *k, void *data)
> +{
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +
> + drck->dr_entity_sense = logical_entity_sense;
> +}
> +
> static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> {
> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> @@ -716,6 +716,7 @@ static const TypeInfo spapr_drc_physical_info = {
> .name = TYPE_SPAPR_DRC_PHYSICAL,
> .parent = TYPE_SPAPR_DR_CONNECTOR,
> .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_physical_class_init,
> .abstract = true,
> };
>
> @@ -723,6 +724,7 @@ static const TypeInfo spapr_drc_logical_info = {
> .name = TYPE_SPAPR_DRC_LOGICAL,
> .parent = TYPE_SPAPR_DR_CONNECTOR,
> .instance_size = sizeof(sPAPRDRConnector),
> + .class_init = spapr_drc_logical_class_init,
> .abstract = true,
> };
>
> @@ -1010,7 +1012,7 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> goto out;
> }
> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - ret = drck->entity_sense(drc, &sensor_state);
> + sensor_state = drck->dr_entity_sense(drc);
>
> out:
> rtas_st(rets, 0, ret);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 0c181bb..5cba8f9 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1481,7 +1481,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
> PCI_DEVFN(slotnr, i));
> func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
> - func_drck->entity_sense(func_drc, &state);
> + state = func_drck->dr_entity_sense(func_drc);
>
> if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> spapr_hotplug_req_add_by_index(func_drc);
> @@ -1522,7 +1522,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
> PCI_DEVFN(slotnr, i));
> func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
> - func_drck->entity_sense(func_drc, &state);
> + state = func_drck->dr_entity_sense(func_drc);
> if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
> && !func_drck->release_pending(func_drc)) {
> error_setg(errp,
> @@ -1548,7 +1548,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> func_drc = spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus),
> PCI_DEVFN(slotnr, i));
> func_drck = SPAPR_DR_CONNECTOR_GET_CLASS(func_drc);
> - func_drck->entity_sense(func_drc, &state);
> + state = func_drck->dr_entity_sense(func_drc);
> if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> spapr_hotplug_req_remove_by_index(func_drc);
> }
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 43d265f..4979397 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -44,7 +44,6 @@ spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", sta
> spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_set_configured_skipping(uint32_t index) "drc: 0x%"PRIx32", isolated device"
> -spapr_drc_entity_sense(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_awaiting_isolated(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index c88e1be..f892b94 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -214,6 +214,8 @@ typedef struct sPAPRDRConnectorClass {
> sPAPRDRConnectorTypeShift typeshift;
> const char *typename; /* used in device tree, PAPR 13.5.2.6 & C.6.1 */
>
> + sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
> +
> /* accessors for guest-visible (generally via RTAS) DR state */
> uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> sPAPRDRIsolationState state);
> @@ -223,8 +225,6 @@ typedef struct sPAPRDRConnectorClass {
> sPAPRDRAllocationState state);
> const char *(*get_name)(sPAPRDRConnector *drc);
>
> - uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state);
> -
> /* QEMU interfaces for managing hotplug operations */
> void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> int fdt_start_offset, bool coldplug, Error **errp);
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/7] spapr: Abolish DRC get_name method
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
2017-06-06 8:32 ` [Qemu-devel] [PATCH 1/7] spapr: Clean up DR entity sense handling David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 16:21 ` Michael Roth
2017-06-06 8:32 ` [Qemu-devel] [PATCH 3/7] spapr: Assign DRC names from owners David Gibson
` (4 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
DRConnectorClass has a get_name method, however:
* There's only one implementation, and only ever likely to be one
* There's exactly one caller, which is (now) local
* The implementation is trivial
So just open-code what we need.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 11 ++---------
include/hw/ppc/spapr_drc.h | 1 -
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index da3779e..9140b48 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -167,11 +167,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
return RTAS_OUT_SUCCESS;
}
-static const char *get_name(sPAPRDRConnector *drc)
-{
- return drc->name;
-}
-
/* has the guest been notified of device attachment? */
static void set_signalled(sPAPRDRConnector *drc)
{
@@ -221,8 +216,7 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
static char *prop_get_name(Object *obj, Error **errp)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- return g_strdup(drck->get_name(drc));
+ return g_strdup(drc->name);
}
static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
@@ -653,7 +647,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
drck->set_isolation_state = set_isolation_state;
drck->set_indicator_state = set_indicator_state;
drck->set_allocation_state = set_allocation_state;
- drck->get_name = get_name;
drck->attach = attach;
drck->detach = detach;
drck->release_pending = release_pending;
@@ -848,7 +841,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
g_array_append_val(drc_power_domains, drc_power_domain);
/* ibm,drc-names */
- drc_names = g_string_append(drc_names, drck->get_name(drc));
+ drc_names = g_string_append(drc_names, drc->name);
drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
/* ibm,drc-types */
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index f892b94..795b7dd 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -223,7 +223,6 @@ typedef struct sPAPRDRConnectorClass {
sPAPRDRIndicatorState state);
uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
sPAPRDRAllocationState state);
- const char *(*get_name)(sPAPRDRConnector *drc);
/* QEMU interfaces for managing hotplug operations */
void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] spapr: Abolish DRC get_name method
2017-06-06 8:32 ` [Qemu-devel] [PATCH 2/7] spapr: Abolish DRC get_name method David Gibson
@ 2017-06-06 16:21 ` Michael Roth
0 siblings, 0 replies; 17+ messages in thread
From: Michael Roth @ 2017-06-06 16:21 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 03:32:16)
> DRConnectorClass has a get_name method, however:
> * There's only one implementation, and only ever likely to be one
> * There's exactly one caller, which is (now) local
> * The implementation is trivial
>
> So just open-code what we need.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 11 ++---------
> include/hw/ppc/spapr_drc.h | 1 -
> 2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index da3779e..9140b48 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -167,11 +167,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> return RTAS_OUT_SUCCESS;
> }
>
> -static const char *get_name(sPAPRDRConnector *drc)
> -{
> - return drc->name;
> -}
> -
> /* has the guest been notified of device attachment? */
> static void set_signalled(sPAPRDRConnector *drc)
> {
> @@ -221,8 +216,7 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
> static char *prop_get_name(Object *obj, Error **errp)
> {
> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - return g_strdup(drck->get_name(drc));
> + return g_strdup(drc->name);
> }
>
> static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> @@ -653,7 +647,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> drck->set_isolation_state = set_isolation_state;
> drck->set_indicator_state = set_indicator_state;
> drck->set_allocation_state = set_allocation_state;
> - drck->get_name = get_name;
> drck->attach = attach;
> drck->detach = detach;
> drck->release_pending = release_pending;
> @@ -848,7 +841,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> g_array_append_val(drc_power_domains, drc_power_domain);
>
> /* ibm,drc-names */
> - drc_names = g_string_append(drc_names, drck->get_name(drc));
> + drc_names = g_string_append(drc_names, drc->name);
> drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
>
> /* ibm,drc-types */
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index f892b94..795b7dd 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -223,7 +223,6 @@ typedef struct sPAPRDRConnectorClass {
> sPAPRDRIndicatorState state);
> uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
> sPAPRDRAllocationState state);
> - const char *(*get_name)(sPAPRDRConnector *drc);
>
> /* QEMU interfaces for managing hotplug operations */
> void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/7] spapr: Assign DRC names from owners
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
2017-06-06 8:32 ` [Qemu-devel] [PATCH 1/7] spapr: Clean up DR entity sense handling David Gibson
2017-06-06 8:32 ` [Qemu-devel] [PATCH 2/7] spapr: Abolish DRC get_name method David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 8:32 ` [Qemu-devel] [PATCH 4/7] spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state() David Gibson
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
At present, spapr_dr_connector_new assigns a name to the drc based on its
type and ID. The few places the DRC name are used, however, are supposed
to give some sort of human-useful information to correlate guest-side
problems with host-side resources (for PCI devices it's supposed to be
a physical location code).
The owners who set up the DRCs are in a much better position to supply this
information than the DRC core. We already have a "name" R/O property, so
make it R/W and set from the callers. We change the "name" property from
a new-style QOM one to the property list form, simply because it's a bit
less verbose that way.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 16 +++++++++---
hw/ppc/spapr_drc.c | 61 ++++++++++++----------------------------------
hw/ppc/spapr_pci.c | 29 ++++++++++++++++++++--
include/hw/ppc/spapr_drc.h | 5 ++--
4 files changed, 57 insertions(+), 54 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 86e6228..671cdbb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1909,11 +1909,14 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
for (i = 0; i < nr_lmbs; i++) {
sPAPRDRConnector *drc;
+ char *drc_name;
uint64_t addr;
addr = i * lmb_size + spapr->hotplug_memory.base;
+ drc_name = g_strdup_printf("LMB %"PRId64, addr / lmb_size);
drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
- addr/lmb_size);
+ addr / lmb_size, drc_name, &error_abort);
+ g_free(drc_name);
qemu_register_reset(spapr_drc_reset, drc);
}
}
@@ -2008,9 +2011,14 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
int core_id = i * smp_threads;
if (mc->has_hotpluggable_cpus) {
- sPAPRDRConnector *drc =
- spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
- (core_id / smp_threads) * smt);
+ int id = (core_id / smp_threads) * smt;
+ char *drc_name;
+ sPAPRDRConnector *drc;
+
+ drc_name = g_strdup_printf("CPU %d", id);
+ drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
+ id, drc_name, &error_abort);
+ g_free(drc_name);
qemu_register_reset(spapr_drc_reset, drc);
}
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9140b48..1fc51c9 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -213,12 +213,6 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name,
visit_type_uint32(v, name, &value, errp);
}
-static char *prop_get_name(Object *obj, Error **errp)
-{
- sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
- return g_strdup(drc->name);
-}
-
static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -564,9 +558,11 @@ static void unrealize(DeviceState *d, Error **errp)
}
sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
- uint32_t id)
+ uint32_t id, const char *name,
+ Error **errp)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(type));
+ Error *local_err = NULL;
char *prop_name;
drc->id = id;
@@ -574,48 +570,16 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
spapr_drc_index(drc));
object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
- object_property_set_bool(OBJECT(drc), true, "realized", NULL);
g_free(prop_name);
- /* human-readable name for a DRC to encode into the DT
- * description. this is mainly only used within a guest in place
- * of the unique DRC index.
- *
- * in the case of VIO/PCI devices, it corresponds to a
- * "location code" that maps a logical device/function (DRC index)
- * to a physical (or virtual in the case of VIO) location in the
- * system by chaining together the "location label" for each
- * encapsulating component.
- *
- * since this is more to do with diagnosing physical hardware
- * issues than guest compatibility, we choose location codes/DRC
- * names that adhere to the documented format, but avoid encoding
- * the entire topology information into the label/code, instead
- * just using the location codes based on the labels for the
- * endpoints (VIO/PCI adaptor connectors), which is basically
- * just "C" followed by an integer ID.
- *
- * 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 (spapr_drc_type(drc)) {
- case SPAPR_DR_CONNECTOR_TYPE_CPU:
- drc->name = g_strdup_printf("CPU %d", id);
- break;
- case SPAPR_DR_CONNECTOR_TYPE_PHB:
- drc->name = g_strdup_printf("PHB %d", id);
- break;
- case SPAPR_DR_CONNECTOR_TYPE_VIO:
- case SPAPR_DR_CONNECTOR_TYPE_PCI:
- drc->name = g_strdup_printf("C%d", id);
- break;
- case SPAPR_DR_CONNECTOR_TYPE_LMB:
- drc->name = g_strdup_printf("LMB %d", id);
- break;
- default:
- g_assert(false);
+ object_property_set_str(OBJECT(drc), name, "name", &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return NULL;
}
+ object_property_set_bool(OBJECT(drc), true, "realized", NULL);
+
/* PCI slot always start in a USABLE state, and stay there */
if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
@@ -631,16 +595,21 @@ static void spapr_dr_connector_instance_init(Object *obj)
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_str(obj, "name", prop_get_name, NULL, NULL);
object_property_add(obj, "fdt", "struct", prop_get_fdt,
NULL, NULL, NULL, NULL);
}
+static Property spapr_drc_properties[] = {
+ DEFINE_PROP_STRING("name", sPAPRDRConnector, name),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
{
DeviceClass *dk = DEVICE_CLASS(k);
sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
+ dk->props = spapr_drc_properties;
dk->reset = reset;
dk->realize = realize;
dk->unrealize = unrealize;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5cba8f9..967d7c3 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1570,10 +1570,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
sPAPRTCETable *tcet;
const unsigned windows_supported =
sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
+ Error *local_err = NULL;
if (sphb->index != (uint32_t)-1) {
sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
- Error *local_err = NULL;
if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != (uint32_t)-1)
|| (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported == 2)
@@ -1759,8 +1759,33 @@ 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++) {
+ int id = sphb->index << 16 | i;
+ char *drc_name;
+
+ /* Name for a DRC for the DT. For PCI devices, it is a
+ * "location code" mapping a logical device/function (DRC
+ * index) to a physical location in the system.
+ *
+ * This is more to do with diagnosing physical hardware
+ * issues than guest compatibility, so choose names that
+ * adhere to the documented format, but avoid encoding the
+ * entire topology information into the label/code,
+ * instead just using the location codes based on the
+ * labels for the endpoints (VIO/PCI adaptor connectors),
+ * which is basically just "C" followed by an integer ID.
+ *
+ * DRC names as documented by PAPR+ v2.7, 13.5.2.4
+ * location codes as documented by PAPR+ v2.7, 12.3.1.5
+ */
+
+ drc_name = g_strdup_printf("C%d", id);
spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI,
- (sphb->index << 16) | i);
+ id, drc_name, &local_err);
+ g_free(drc_name);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
}
}
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 795b7dd..d437e0a 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -184,7 +184,7 @@ typedef struct sPAPRDRConnector {
uint32_t id;
Object *owner;
- const char *name;
+ char *name;
/* sensor/indicator states */
uint32_t isolation_state;
@@ -236,7 +236,8 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc);
sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc);
sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
- uint32_t id);
+ uint32_t id, const char *name,
+ Error **errp);
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,
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH 4/7] spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state()
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
` (2 preceding siblings ...)
2017-06-06 8:32 ` [Qemu-devel] [PATCH 3/7] spapr: Assign DRC names from owners David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 17:20 ` Michael Roth
2017-06-06 8:32 ` [Qemu-devel] [PATCH 5/7] spapr: Clean up RTAS set-indicator David Gibson
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
With some combinations of migration and hotplug we can lost temporary state
indicating how many DRCs (guest side hotplug handles) are still connected
to a DIMM object in the process of removal. When we hit that situation
spapr_recover_pending_dimm_state() is used to scan more extensively and
work out the right number.
It does this using drc->indicator state to determine what state of
disconnection the DRC is in. However, this is not safe, because the
indicator state is guest settable - in fact it's more-or-less a purely
guest->host notification mechanism which should have no bearing on the
internals of hotplug state management.
So, replace the test for this with a test on drc->dev, which is a purely
qemu side managed variable, and updated the same BQL critical section as
the indicator state.
This does introduce an off-by-one change, because the indicator state was
updated before the call to spapr_lmb_release() on the current DRC, whereas
drc->dev is updated afterwards. That's corrected by always decrementing
the nr_lmbs value instead of only doing so in the case where we didn't
have to recover information.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 671cdbb..e8cecaf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2683,7 +2683,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
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) {
+ if (drc->dev) {
avail_lmbs++;
}
addr += SPAPR_MEMORY_BLOCK_SIZE;
@@ -2707,10 +2707,11 @@ void spapr_lmb_release(DeviceState *dev)
* during the unplug process. In this case recover it. */
if (ds == NULL) {
ds = spapr_recover_pending_dimm_state(spapr, PC_DIMM(dev));
- if (ds->nr_lmbs) {
- return;
- }
- } else if (--ds->nr_lmbs) {
+ /* The DRC being examined by the caller at least must be counted */
+ g_assert(ds->nr_lmbs);
+ }
+
+ if (--ds->nr_lmbs) {
return;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state()
2017-06-06 8:32 ` [Qemu-devel] [PATCH 4/7] spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state() David Gibson
@ 2017-06-06 17:20 ` Michael Roth
0 siblings, 0 replies; 17+ messages in thread
From: Michael Roth @ 2017-06-06 17:20 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 03:32:18)
> With some combinations of migration and hotplug we can lost temporary state
> indicating how many DRCs (guest side hotplug handles) are still connected
> to a DIMM object in the process of removal. When we hit that situation
> spapr_recover_pending_dimm_state() is used to scan more extensively and
> work out the right number.
>
> It does this using drc->indicator state to determine what state of
> disconnection the DRC is in. However, this is not safe, because the
> indicator state is guest settable - in fact it's more-or-less a purely
> guest->host notification mechanism which should have no bearing on the
> internals of hotplug state management.
>
> So, replace the test for this with a test on drc->dev, which is a purely
> qemu side managed variable, and updated the same BQL critical section as
> the indicator state.
>
> This does introduce an off-by-one change, because the indicator state was
> updated before the call to spapr_lmb_release() on the current DRC, whereas
> drc->dev is updated afterwards. That's corrected by always decrementing
> the nr_lmbs value instead of only doing so in the case where we didn't
> have to recover information.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 671cdbb..e8cecaf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2683,7 +2683,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> 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) {
> + if (drc->dev) {
> avail_lmbs++;
> }
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> @@ -2707,10 +2707,11 @@ void spapr_lmb_release(DeviceState *dev)
> * during the unplug process. In this case recover it. */
> if (ds == NULL) {
> ds = spapr_recover_pending_dimm_state(spapr, PC_DIMM(dev));
> - if (ds->nr_lmbs) {
> - return;
> - }
> - } else if (--ds->nr_lmbs) {
> + /* The DRC being examined by the caller at least must be counted */
> + g_assert(ds->nr_lmbs);
> + }
> +
> + if (--ds->nr_lmbs) {
> return;
> }
>
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 5/7] spapr: Clean up RTAS set-indicator
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
` (3 preceding siblings ...)
2017-06-06 8:32 ` [Qemu-devel] [PATCH 4/7] spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state() David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 20:50 ` Michael Roth
2017-06-06 8:32 ` [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator David Gibson
2017-06-06 8:32 ` [Qemu-devel] [PATCH 7/7] spapr: Change DRC attach & detach methods to functions David Gibson
6 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
In theory the RTAS set-indicator call can be used for a number of
"indicators" defined by PAPR. In practice the only ones we're ever likely
to implement are those used for Dynamic Reconfiguration (i.e. hotplug).
Because of this, the current implementation determines the associated DRC
object, before dispatching based on the type of indicator.
However, this means we also need a check that we're dealing with a DR
related indicator at all, which duplicates some of the logic from the
switch further down.
Even though it means a bit of code duplication, things work out cleaner if
we delegate the DRC lookup to the individual indicator type functions -
and it also allows some further cleanups.
While we're there, remove references to "sensor", a copy/paste artefact
from the related, but distinct "get-sensor" call.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 84 ++++++++++++++++++++++++++++-------------------------
hw/ppc/trace-events | 2 --
2 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1fc51c9..6c2fa93 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -869,74 +869,78 @@ out:
* RTAS calls
*/
-static bool sensor_type_is_dr(uint32_t sensor_type)
+static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
{
- switch (sensor_type) {
- case RTAS_SENSOR_TYPE_ISOLATION_STATE:
- case RTAS_SENSOR_TYPE_DR:
- case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
- return true;
+ sPAPRDRConnector *drc = spapr_drc_by_index(idx);
+ sPAPRDRConnectorClass *drck;
+
+ if (!drc) {
+ return RTAS_OUT_PARAM_ERROR;
}
- return false;
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ return drck->set_isolation_state(drc, state);
}
-static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
- uint32_t token, uint32_t nargs,
- target_ulong args, uint32_t nret,
- target_ulong rets)
+static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
{
- uint32_t sensor_type;
- uint32_t sensor_index;
- uint32_t sensor_state;
- uint32_t ret = RTAS_OUT_SUCCESS;
- sPAPRDRConnector *drc;
+ sPAPRDRConnector *drc = spapr_drc_by_index(idx);
sPAPRDRConnectorClass *drck;
- if (nargs != 3 || nret != 1) {
- ret = RTAS_OUT_PARAM_ERROR;
- goto out;
+ if (!drc) {
+ return RTAS_OUT_PARAM_ERROR;
}
- sensor_type = rtas_ld(args, 0);
- sensor_index = rtas_ld(args, 1);
- sensor_state = rtas_ld(args, 2);
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ return drck->set_allocation_state(drc, state);
+}
- if (!sensor_type_is_dr(sensor_type)) {
- goto out_unimplemented;
- }
+static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
+{
+ sPAPRDRConnector *drc = spapr_drc_by_index(idx);
+ sPAPRDRConnectorClass *drck;
- /* if this is a DR sensor we can assume sensor_index == drc_index */
- drc = spapr_drc_by_index(sensor_index);
if (!drc) {
- trace_spapr_rtas_set_indicator_invalid(sensor_index);
+ return RTAS_OUT_PARAM_ERROR;
+ }
+
+ drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ return drck->set_indicator_state(drc, state);
+}
+
+static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+ uint32_t token,
+ uint32_t nargs, target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+ uint32_t type, idx, state;
+ uint32_t ret = RTAS_OUT_SUCCESS;
+
+ if (nargs != 3 || nret != 1) {
ret = RTAS_OUT_PARAM_ERROR;
goto out;
}
- drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- switch (sensor_type) {
+ type = rtas_ld(args, 0);
+ idx = rtas_ld(args, 1);
+ state = rtas_ld(args, 2);
+
+ switch (type) {
case RTAS_SENSOR_TYPE_ISOLATION_STATE:
- ret = drck->set_isolation_state(drc, sensor_state);
+ ret = rtas_set_isolation_state(idx, state);
break;
case RTAS_SENSOR_TYPE_DR:
- ret = drck->set_indicator_state(drc, sensor_state);
+ ret = rtas_set_indicator_state(idx, state);
break;
case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
- ret = drck->set_allocation_state(drc, sensor_state);
+ ret = rtas_set_allocation_state(idx, state);
break;
default:
- goto out_unimplemented;
+ ret = RTAS_OUT_NOT_SUPPORTED;
}
out:
rtas_st(rets, 0, ret);
- return;
-
-out_unimplemented:
- /* currently only DR-related sensors are implemented */
- trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
- rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
}
static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 4979397..581fa85 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -60,8 +60,6 @@ spapr_ovec_parse_vector(int vector, int byte, uint16_t vec_len, uint8_t entry) "
spapr_ovec_populate_dt(int byte, uint16_t vec_len, uint8_t entry) "encoding guest vector byte %3d / %3d: 0x%.2x"
# hw/ppc/spapr_rtas.c
-spapr_rtas_set_indicator_invalid(uint32_t index) "sensor index: 0x%"PRIx32
-spapr_rtas_set_indicator_not_supported(uint32_t index, uint32_t type) "sensor index: 0x%"PRIx32", type: %"PRIu32
spapr_rtas_get_sensor_state_not_supported(uint32_t index, uint32_t type) "sensor index: 0x%"PRIx32", type: %"PRIu32
spapr_rtas_get_sensor_state_invalid(uint32_t index) "sensor index: 0x%"PRIx32
spapr_rtas_ibm_configure_connector_invalid(uint32_t index) "DRC index: 0x%"PRIx32
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] spapr: Clean up RTAS set-indicator
2017-06-06 8:32 ` [Qemu-devel] [PATCH 5/7] spapr: Clean up RTAS set-indicator David Gibson
@ 2017-06-06 20:50 ` Michael Roth
0 siblings, 0 replies; 17+ messages in thread
From: Michael Roth @ 2017-06-06 20:50 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 03:32:19)
> In theory the RTAS set-indicator call can be used for a number of
> "indicators" defined by PAPR. In practice the only ones we're ever likely
> to implement are those used for Dynamic Reconfiguration (i.e. hotplug).
> Because of this, the current implementation determines the associated DRC
> object, before dispatching based on the type of indicator.
>
> However, this means we also need a check that we're dealing with a DR
> related indicator at all, which duplicates some of the logic from the
> switch further down.
>
> Even though it means a bit of code duplication, things work out cleaner if
> we delegate the DRC lookup to the individual indicator type functions -
> and it also allows some further cleanups.
>
> While we're there, remove references to "sensor", a copy/paste artefact
> from the related, but distinct "get-sensor" call.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr_drc.c | 84 ++++++++++++++++++++++++++++-------------------------
> hw/ppc/trace-events | 2 --
> 2 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 1fc51c9..6c2fa93 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -869,74 +869,78 @@ out:
> * RTAS calls
> */
>
> -static bool sensor_type_is_dr(uint32_t sensor_type)
> +static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
> {
> - switch (sensor_type) {
> - case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> - case RTAS_SENSOR_TYPE_DR:
> - case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> - return true;
> + sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> + sPAPRDRConnectorClass *drck;
> +
> + if (!drc) {
> + return RTAS_OUT_PARAM_ERROR;
> }
>
> - return false;
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + return drck->set_isolation_state(drc, state);
> }
>
> -static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> - uint32_t token, uint32_t nargs,
> - target_ulong args, uint32_t nret,
> - target_ulong rets)
> +static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
> {
> - uint32_t sensor_type;
> - uint32_t sensor_index;
> - uint32_t sensor_state;
> - uint32_t ret = RTAS_OUT_SUCCESS;
> - sPAPRDRConnector *drc;
> + sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> sPAPRDRConnectorClass *drck;
>
> - if (nargs != 3 || nret != 1) {
> - ret = RTAS_OUT_PARAM_ERROR;
> - goto out;
> + if (!drc) {
> + return RTAS_OUT_PARAM_ERROR;
> }
>
> - sensor_type = rtas_ld(args, 0);
> - sensor_index = rtas_ld(args, 1);
> - sensor_state = rtas_ld(args, 2);
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + return drck->set_allocation_state(drc, state);
> +}
>
> - if (!sensor_type_is_dr(sensor_type)) {
> - goto out_unimplemented;
> - }
> +static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> +{
> + sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> + sPAPRDRConnectorClass *drck;
>
> - /* if this is a DR sensor we can assume sensor_index == drc_index */
> - drc = spapr_drc_by_index(sensor_index);
> if (!drc) {
> - trace_spapr_rtas_set_indicator_invalid(sensor_index);
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + return drck->set_indicator_state(drc, state);
> +}
> +
> +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> + uint32_t token,
> + uint32_t nargs, target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + uint32_t type, idx, state;
> + uint32_t ret = RTAS_OUT_SUCCESS;
> +
> + if (nargs != 3 || nret != 1) {
> ret = RTAS_OUT_PARAM_ERROR;
> goto out;
> }
> - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> - switch (sensor_type) {
> + type = rtas_ld(args, 0);
> + idx = rtas_ld(args, 1);
> + state = rtas_ld(args, 2);
> +
> + switch (type) {
> case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> - ret = drck->set_isolation_state(drc, sensor_state);
> + ret = rtas_set_isolation_state(idx, state);
> break;
> case RTAS_SENSOR_TYPE_DR:
> - ret = drck->set_indicator_state(drc, sensor_state);
> + ret = rtas_set_indicator_state(idx, state);
> break;
> case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> - ret = drck->set_allocation_state(drc, sensor_state);
> + ret = rtas_set_allocation_state(idx, state);
> break;
> default:
> - goto out_unimplemented;
> + ret = RTAS_OUT_NOT_SUPPORTED;
> }
>
> out:
> rtas_st(rets, 0, ret);
> - return;
> -
> -out_unimplemented:
> - /* currently only DR-related sensors are implemented */
> - trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type);
> - rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> }
>
> static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 4979397..581fa85 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -60,8 +60,6 @@ spapr_ovec_parse_vector(int vector, int byte, uint16_t vec_len, uint8_t entry) "
> spapr_ovec_populate_dt(int byte, uint16_t vec_len, uint8_t entry) "encoding guest vector byte %3d / %3d: 0x%.2x"
>
> # hw/ppc/spapr_rtas.c
> -spapr_rtas_set_indicator_invalid(uint32_t index) "sensor index: 0x%"PRIx32
> -spapr_rtas_set_indicator_not_supported(uint32_t index, uint32_t type) "sensor index: 0x%"PRIx32", type: %"PRIu32
> spapr_rtas_get_sensor_state_not_supported(uint32_t index, uint32_t type) "sensor index: 0x%"PRIx32", type: %"PRIu32
> spapr_rtas_get_sensor_state_invalid(uint32_t index) "sensor index: 0x%"PRIx32
> spapr_rtas_ibm_configure_connector_invalid(uint32_t index) "DRC index: 0x%"PRIx32
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
` (4 preceding siblings ...)
2017-06-06 8:32 ` [Qemu-devel] [PATCH 5/7] spapr: Clean up RTAS set-indicator David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 21:04 ` Michael Roth
2017-06-06 8:32 ` [Qemu-devel] [PATCH 7/7] spapr: Change DRC attach & detach methods to functions David Gibson
6 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
There are 3 types of "indicator" associated with hotplug in the PAPR spec
the "allocation state", "isolation state" and "DR-indicator". The first
two are intimately tied to the various state transitions associated with
hotplug. The DR-indicator, however, is different and simpler.
It's basically just a guest controlled variable which can be used by the
guest to flag state or problems associated with a device. The idea is that
the hypervisor can use it to present information back on management
consoles (on some machines with PowerVM it may even control physical LEDs
on the machine case associated with the relevant device).
For that reason, there's only ever likely to be a single update
implementation so the set_indicator_state method isn't useful. Replace it
with a direct function call.
While we're there, make some small associated cleanups:
* PAPR doesn't use the term "indicator state", just "DR-indicator" and
the allocation state and isolation state are also considered "indicators".
Rename things to be less confusing
* Fold set_indicator_state() and rtas_set_indicator_state() into a single
rtas_set_dr_indicator() function.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_drc.c | 25 ++++++++-----------------
hw/ppc/trace-events | 2 +-
include/hw/ppc/spapr_drc.h | 16 ++++++++--------
3 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 6c2fa93..a4ece2e 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -116,14 +116,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
return RTAS_OUT_SUCCESS;
}
-static uint32_t set_indicator_state(sPAPRDRConnector *drc,
- sPAPRDRIndicatorState state)
-{
- trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
- drc->indicator_state = state;
- return RTAS_OUT_SUCCESS;
-}
-
static uint32_t set_allocation_state(sPAPRDRConnector *drc,
sPAPRDRAllocationState state)
{
@@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
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;
+ drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
drc->dev = d;
drc->fdt = fdt;
@@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
}
}
- drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
+ drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
/* Calling release callbacks based on spapr_drc_type(drc). */
switch (spapr_drc_type(drc)) {
@@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc = {
.fields = (VMStateField []) {
VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
- VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
+ VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
VMSTATE_BOOL(configured, sPAPRDRConnector),
VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
@@ -614,7 +606,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->realize = realize;
dk->unrealize = unrealize;
drck->set_isolation_state = set_isolation_state;
- drck->set_indicator_state = set_indicator_state;
drck->set_allocation_state = set_allocation_state;
drck->attach = attach;
drck->detach = detach;
@@ -895,17 +886,17 @@ static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
return drck->set_allocation_state(drc, state);
}
-static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
+static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
{
sPAPRDRConnector *drc = spapr_drc_by_index(idx);
- sPAPRDRConnectorClass *drck;
if (!drc) {
return RTAS_OUT_PARAM_ERROR;
}
- drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- return drck->set_indicator_state(drc, state);
+ trace_spapr_drc_set_dr_indicator(idx, state);
+ drc->dr_indicator = state;
+ return RTAS_OUT_SUCCESS;
}
static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
@@ -930,7 +921,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
ret = rtas_set_isolation_state(idx, state);
break;
case RTAS_SENSOR_TYPE_DR:
- ret = rtas_set_indicator_state(idx, state);
+ ret = rtas_set_dr_indicator(idx, state);
break;
case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
ret = rtas_set_allocation_state(idx, state);
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 581fa85..3e8e3cf 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -39,7 +39,7 @@ spapr_iommu_ddw_reset(uint64_t buid, uint32_t cfgaddr) "buid=%"PRIx64" addr=%"PR
spapr_drc_set_isolation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: %"PRIx32
spapr_drc_set_isolation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
-spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
+spapr_drc_set_dr_indicator(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index d437e0a..802c708 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -125,7 +125,7 @@ typedef enum {
} sPAPRDRAllocationState;
/*
- * LED/visual indicator state
+ * DR-indicator (LED/visual indicator)
*
* set via set-indicator RTAS calls
* as documented by PAPR+ 2.7 13.5.3.4, Table 177,
@@ -137,10 +137,10 @@ typedef enum {
* action: (currently unused)
*/
typedef enum {
- SPAPR_DR_INDICATOR_STATE_INACTIVE = 0,
- SPAPR_DR_INDICATOR_STATE_ACTIVE = 1,
- SPAPR_DR_INDICATOR_STATE_IDENTIFY = 2,
- SPAPR_DR_INDICATOR_STATE_ACTION = 3,
+ SPAPR_DR_INDICATOR_INACTIVE = 0,
+ SPAPR_DR_INDICATOR_ACTIVE = 1,
+ SPAPR_DR_INDICATOR_IDENTIFY = 2,
+ SPAPR_DR_INDICATOR_ACTION = 3,
} sPAPRDRIndicatorState;
/*
@@ -186,10 +186,12 @@ typedef struct sPAPRDRConnector {
Object *owner;
char *name;
+ /* DR-indicator */
+ uint32_t dr_indicator;
+
/* sensor/indicator states */
uint32_t isolation_state;
uint32_t allocation_state;
- uint32_t indicator_state;
/* configure-connector state */
void *fdt;
@@ -219,8 +221,6 @@ typedef struct sPAPRDRConnectorClass {
/* accessors for guest-visible (generally via RTAS) DR state */
uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
sPAPRDRIsolationState state);
- uint32_t (*set_indicator_state)(sPAPRDRConnector *drc,
- sPAPRDRIndicatorState state);
uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
sPAPRDRAllocationState state);
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
2017-06-06 8:32 ` [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator David Gibson
@ 2017-06-06 21:04 ` Michael Roth
2017-06-07 1:28 ` David Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Michael Roth @ 2017-06-06 21:04 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 03:32:20)
> There are 3 types of "indicator" associated with hotplug in the PAPR spec
> the "allocation state", "isolation state" and "DR-indicator". The first
> two are intimately tied to the various state transitions associated with
> hotplug. The DR-indicator, however, is different and simpler.
>
> It's basically just a guest controlled variable which can be used by the
> guest to flag state or problems associated with a device. The idea is that
> the hypervisor can use it to present information back on management
> consoles (on some machines with PowerVM it may even control physical LEDs
> on the machine case associated with the relevant device).
>
> For that reason, there's only ever likely to be a single update
> implementation so the set_indicator_state method isn't useful. Replace it
> with a direct function call.
>
> While we're there, make some small associated cleanups:
> * PAPR doesn't use the term "indicator state", just "DR-indicator" and
> the allocation state and isolation state are also considered "indicators".
> Rename things to be less confusing
> * Fold set_indicator_state() and rtas_set_indicator_state() into a single
> rtas_set_dr_indicator() function.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr_drc.c | 25 ++++++++-----------------
> hw/ppc/trace-events | 2 +-
> include/hw/ppc/spapr_drc.h | 16 ++++++++--------
> 3 files changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 6c2fa93..a4ece2e 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -116,14 +116,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> return RTAS_OUT_SUCCESS;
> }
>
> -static uint32_t set_indicator_state(sPAPRDRConnector *drc,
> - sPAPRDRIndicatorState state)
> -{
> - trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
> - drc->indicator_state = state;
> - return RTAS_OUT_SUCCESS;
> -}
> -
> static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> sPAPRDRAllocationState state)
> {
> @@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> 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;
> + drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>
> drc->dev = d;
> drc->fdt = fdt;
> @@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> }
> }
>
> - drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> + drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
>
> /* Calling release callbacks based on spapr_drc_type(drc). */
> switch (spapr_drc_type(drc)) {
> @@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc = {
> .fields = (VMStateField []) {
> VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> - VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> + VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> VMSTATE_BOOL(configured, sPAPRDRConnector),
> VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> @@ -614,7 +606,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->realize = realize;
> dk->unrealize = unrealize;
> drck->set_isolation_state = set_isolation_state;
> - drck->set_indicator_state = set_indicator_state;
> drck->set_allocation_state = set_allocation_state;
> drck->attach = attach;
> drck->detach = detach;
> @@ -895,17 +886,17 @@ static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
> return drck->set_allocation_state(drc, state);
> }
>
> -static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> +static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> {
> sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> - sPAPRDRConnectorClass *drck;
>
> if (!drc) {
> return RTAS_OUT_PARAM_ERROR;
> }
>
> - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - return drck->set_indicator_state(drc, state);
> + trace_spapr_drc_set_dr_indicator(idx, state);
> + drc->dr_indicator = state;
> + return RTAS_OUT_SUCCESS;
> }
>
> static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> @@ -930,7 +921,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> ret = rtas_set_isolation_state(idx, state);
> break;
> case RTAS_SENSOR_TYPE_DR:
> - ret = rtas_set_indicator_state(idx, state);
> + ret = rtas_set_dr_indicator(idx, state);
> break;
> case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> ret = rtas_set_allocation_state(idx, state);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 581fa85..3e8e3cf 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -39,7 +39,7 @@ spapr_iommu_ddw_reset(uint64_t buid, uint32_t cfgaddr) "buid=%"PRIx64" addr=%"PR
> spapr_drc_set_isolation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: %"PRIx32
> spapr_drc_set_isolation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> +spapr_drc_set_dr_indicator(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
Since this only tracks the changes to dr_indicator via RTAS (as was also
the case previously), it should probably be changed to an RTAS trace
while we're here.
In either case:
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index d437e0a..802c708 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -125,7 +125,7 @@ typedef enum {
> } sPAPRDRAllocationState;
>
> /*
> - * LED/visual indicator state
> + * DR-indicator (LED/visual indicator)
> *
> * set via set-indicator RTAS calls
> * as documented by PAPR+ 2.7 13.5.3.4, Table 177,
> @@ -137,10 +137,10 @@ typedef enum {
> * action: (currently unused)
> */
> typedef enum {
> - SPAPR_DR_INDICATOR_STATE_INACTIVE = 0,
> - SPAPR_DR_INDICATOR_STATE_ACTIVE = 1,
> - SPAPR_DR_INDICATOR_STATE_IDENTIFY = 2,
> - SPAPR_DR_INDICATOR_STATE_ACTION = 3,
> + SPAPR_DR_INDICATOR_INACTIVE = 0,
> + SPAPR_DR_INDICATOR_ACTIVE = 1,
> + SPAPR_DR_INDICATOR_IDENTIFY = 2,
> + SPAPR_DR_INDICATOR_ACTION = 3,
> } sPAPRDRIndicatorState;
>
> /*
> @@ -186,10 +186,12 @@ typedef struct sPAPRDRConnector {
> Object *owner;
> char *name;
>
> + /* DR-indicator */
> + uint32_t dr_indicator;
> +
> /* sensor/indicator states */
> uint32_t isolation_state;
> uint32_t allocation_state;
> - uint32_t indicator_state;
>
> /* configure-connector state */
> void *fdt;
> @@ -219,8 +221,6 @@ typedef struct sPAPRDRConnectorClass {
> /* accessors for guest-visible (generally via RTAS) DR state */
> uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> sPAPRDRIsolationState state);
> - uint32_t (*set_indicator_state)(sPAPRDRConnector *drc,
> - sPAPRDRIndicatorState state);
> uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
> sPAPRDRAllocationState state);
>
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
2017-06-06 21:04 ` Michael Roth
@ 2017-06-07 1:28 ` David Gibson
2017-06-07 23:11 ` Michael Roth
0 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-07 1:28 UTC (permalink / raw)
To: Michael Roth; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
[-- Attachment #1: Type: text/plain, Size: 9034 bytes --]
On Tue, Jun 06, 2017 at 04:04:33PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-06 03:32:20)
> > There are 3 types of "indicator" associated with hotplug in the PAPR spec
> > the "allocation state", "isolation state" and "DR-indicator". The first
> > two are intimately tied to the various state transitions associated with
> > hotplug. The DR-indicator, however, is different and simpler.
> >
> > It's basically just a guest controlled variable which can be used by the
> > guest to flag state or problems associated with a device. The idea is that
> > the hypervisor can use it to present information back on management
> > consoles (on some machines with PowerVM it may even control physical LEDs
> > on the machine case associated with the relevant device).
> >
> > For that reason, there's only ever likely to be a single update
> > implementation so the set_indicator_state method isn't useful. Replace it
> > with a direct function call.
> >
> > While we're there, make some small associated cleanups:
> > * PAPR doesn't use the term "indicator state", just "DR-indicator" and
> > the allocation state and isolation state are also considered "indicators".
> > Rename things to be less confusing
> > * Fold set_indicator_state() and rtas_set_indicator_state() into a single
> > rtas_set_dr_indicator() function.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr_drc.c | 25 ++++++++-----------------
> > hw/ppc/trace-events | 2 +-
> > include/hw/ppc/spapr_drc.h | 16 ++++++++--------
> > 3 files changed, 17 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 6c2fa93..a4ece2e 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -116,14 +116,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > return RTAS_OUT_SUCCESS;
> > }
> >
> > -static uint32_t set_indicator_state(sPAPRDRConnector *drc,
> > - sPAPRDRIndicatorState state)
> > -{
> > - trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
> > - drc->indicator_state = state;
> > - return RTAS_OUT_SUCCESS;
> > -}
> > -
> > static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > sPAPRDRAllocationState state)
> > {
> > @@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > 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;
> > + drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> >
> > drc->dev = d;
> > drc->fdt = fdt;
> > @@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > }
> > }
> >
> > - drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> > + drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> >
> > /* Calling release callbacks based on spapr_drc_type(drc). */
> > switch (spapr_drc_type(drc)) {
> > @@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc = {
> > .fields = (VMStateField []) {
> > VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> > VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> > - VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> > + VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > @@ -614,7 +606,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > dk->realize = realize;
> > dk->unrealize = unrealize;
> > drck->set_isolation_state = set_isolation_state;
> > - drck->set_indicator_state = set_indicator_state;
> > drck->set_allocation_state = set_allocation_state;
> > drck->attach = attach;
> > drck->detach = detach;
> > @@ -895,17 +886,17 @@ static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
> > return drck->set_allocation_state(drc, state);
> > }
> >
> > -static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> > +static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> > {
> > sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> > - sPAPRDRConnectorClass *drck;
> >
> > if (!drc) {
> > return RTAS_OUT_PARAM_ERROR;
> > }
> >
> > - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > - return drck->set_indicator_state(drc, state);
> > + trace_spapr_drc_set_dr_indicator(idx, state);
> > + drc->dr_indicator = state;
> > + return RTAS_OUT_SUCCESS;
> > }
> >
> > static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > @@ -930,7 +921,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > ret = rtas_set_isolation_state(idx, state);
> > break;
> > case RTAS_SENSOR_TYPE_DR:
> > - ret = rtas_set_indicator_state(idx, state);
> > + ret = rtas_set_dr_indicator(idx, state);
> > break;
> > case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > ret = rtas_set_allocation_state(idx, state);
> > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > index 581fa85..3e8e3cf 100644
> > --- a/hw/ppc/trace-events
> > +++ b/hw/ppc/trace-events
> > @@ -39,7 +39,7 @@ spapr_iommu_ddw_reset(uint64_t buid, uint32_t cfgaddr) "buid=%"PRIx64" addr=%"PR
> > spapr_drc_set_isolation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: %"PRIx32
> > spapr_drc_set_isolation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> > spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
> > -spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> > +spapr_drc_set_dr_indicator(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
>
> Since this only tracks the changes to dr_indicator via RTAS (as was also
> the case previously), it should probably be changed to an RTAS trace
> while we're here.
That doesn't follow for me. Yes, it's only triggered by RTAS, but
it's really about a DRC event. It's when debugging DRC things that
you're going to care about it.
>
> In either case:
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> > spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> > spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> > spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index d437e0a..802c708 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -125,7 +125,7 @@ typedef enum {
> > } sPAPRDRAllocationState;
> >
> > /*
> > - * LED/visual indicator state
> > + * DR-indicator (LED/visual indicator)
> > *
> > * set via set-indicator RTAS calls
> > * as documented by PAPR+ 2.7 13.5.3.4, Table 177,
> > @@ -137,10 +137,10 @@ typedef enum {
> > * action: (currently unused)
> > */
> > typedef enum {
> > - SPAPR_DR_INDICATOR_STATE_INACTIVE = 0,
> > - SPAPR_DR_INDICATOR_STATE_ACTIVE = 1,
> > - SPAPR_DR_INDICATOR_STATE_IDENTIFY = 2,
> > - SPAPR_DR_INDICATOR_STATE_ACTION = 3,
> > + SPAPR_DR_INDICATOR_INACTIVE = 0,
> > + SPAPR_DR_INDICATOR_ACTIVE = 1,
> > + SPAPR_DR_INDICATOR_IDENTIFY = 2,
> > + SPAPR_DR_INDICATOR_ACTION = 3,
> > } sPAPRDRIndicatorState;
> >
> > /*
> > @@ -186,10 +186,12 @@ typedef struct sPAPRDRConnector {
> > Object *owner;
> > char *name;
> >
> > + /* DR-indicator */
> > + uint32_t dr_indicator;
> > +
> > /* sensor/indicator states */
> > uint32_t isolation_state;
> > uint32_t allocation_state;
> > - uint32_t indicator_state;
> >
> > /* configure-connector state */
> > void *fdt;
> > @@ -219,8 +221,6 @@ typedef struct sPAPRDRConnectorClass {
> > /* accessors for guest-visible (generally via RTAS) DR state */
> > uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> > sPAPRDRIsolationState state);
> > - uint32_t (*set_indicator_state)(sPAPRDRConnector *drc,
> > - sPAPRDRIndicatorState state);
> > uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
> > sPAPRDRAllocationState state);
> >
>
--
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] 17+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
2017-06-07 1:28 ` David Gibson
@ 2017-06-07 23:11 ` Michael Roth
2017-06-08 1:08 ` David Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Michael Roth @ 2017-06-07 23:11 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 20:28:51)
> On Tue, Jun 06, 2017 at 04:04:33PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2017-06-06 03:32:20)
> > > There are 3 types of "indicator" associated with hotplug in the PAPR spec
> > > the "allocation state", "isolation state" and "DR-indicator". The first
> > > two are intimately tied to the various state transitions associated with
> > > hotplug. The DR-indicator, however, is different and simpler.
> > >
> > > It's basically just a guest controlled variable which can be used by the
> > > guest to flag state or problems associated with a device. The idea is that
> > > the hypervisor can use it to present information back on management
> > > consoles (on some machines with PowerVM it may even control physical LEDs
> > > on the machine case associated with the relevant device).
> > >
> > > For that reason, there's only ever likely to be a single update
> > > implementation so the set_indicator_state method isn't useful. Replace it
> > > with a direct function call.
> > >
> > > While we're there, make some small associated cleanups:
> > > * PAPR doesn't use the term "indicator state", just "DR-indicator" and
> > > the allocation state and isolation state are also considered "indicators".
> > > Rename things to be less confusing
> > > * Fold set_indicator_state() and rtas_set_indicator_state() into a single
> > > rtas_set_dr_indicator() function.
> > >
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > hw/ppc/spapr_drc.c | 25 ++++++++-----------------
> > > hw/ppc/trace-events | 2 +-
> > > include/hw/ppc/spapr_drc.h | 16 ++++++++--------
> > > 3 files changed, 17 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 6c2fa93..a4ece2e 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -116,14 +116,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > > return RTAS_OUT_SUCCESS;
> > > }
> > >
> > > -static uint32_t set_indicator_state(sPAPRDRConnector *drc,
> > > - sPAPRDRIndicatorState state)
> > > -{
> > > - trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
> > > - drc->indicator_state = state;
> > > - return RTAS_OUT_SUCCESS;
> > > -}
> > > -
> > > static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > > sPAPRDRAllocationState state)
> > > {
> > > @@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > > 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;
> > > + drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> > >
> > > drc->dev = d;
> > > drc->fdt = fdt;
> > > @@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > > }
> > > }
> > >
> > > - drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> > > + drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> > >
> > > /* Calling release callbacks based on spapr_drc_type(drc). */
> > > switch (spapr_drc_type(drc)) {
> > > @@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc = {
> > > .fields = (VMStateField []) {
> > > VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> > > VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> > > - VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> > > + VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > @@ -614,7 +606,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > > dk->realize = realize;
> > > dk->unrealize = unrealize;
> > > drck->set_isolation_state = set_isolation_state;
> > > - drck->set_indicator_state = set_indicator_state;
> > > drck->set_allocation_state = set_allocation_state;
> > > drck->attach = attach;
> > > drck->detach = detach;
> > > @@ -895,17 +886,17 @@ static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
> > > return drck->set_allocation_state(drc, state);
> > > }
> > >
> > > -static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> > > +static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> > > {
> > > sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> > > - sPAPRDRConnectorClass *drck;
> > >
> > > if (!drc) {
> > > return RTAS_OUT_PARAM_ERROR;
> > > }
> > >
> > > - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > - return drck->set_indicator_state(drc, state);
> > > + trace_spapr_drc_set_dr_indicator(idx, state);
> > > + drc->dr_indicator = state;
> > > + return RTAS_OUT_SUCCESS;
> > > }
> > >
> > > static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > @@ -930,7 +921,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > ret = rtas_set_isolation_state(idx, state);
> > > break;
> > > case RTAS_SENSOR_TYPE_DR:
> > > - ret = rtas_set_indicator_state(idx, state);
> > > + ret = rtas_set_dr_indicator(idx, state);
> > > break;
> > > case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > > ret = rtas_set_allocation_state(idx, state);
> > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > index 581fa85..3e8e3cf 100644
> > > --- a/hw/ppc/trace-events
> > > +++ b/hw/ppc/trace-events
> > > @@ -39,7 +39,7 @@ spapr_iommu_ddw_reset(uint64_t buid, uint32_t cfgaddr) "buid=%"PRIx64" addr=%"PR
> > > spapr_drc_set_isolation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: %"PRIx32
> > > spapr_drc_set_isolation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> > > spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
> > > -spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> > > +spapr_drc_set_dr_indicator(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> >
> > Since this only tracks the changes to dr_indicator via RTAS (as was also
> > the case previously), it should probably be changed to an RTAS trace
> > while we're here.
>
> That doesn't follow for me. Yes, it's only triggered by RTAS, but
> it's really about a DRC event. It's when debugging DRC things that
> you're going to care about it.
My concern is more on the trace output side of things. As it stands it
gives a false sense that you're seeing a full accounting of the state
changes when really it's only a particular call-site that's being
traced. Making it spapr_drc_set_dr_indicator_rtas would have been a
better suggestion though.
Then again, the issue is not unique to this particular value, so maybe
a general rework of how we handle tracing would be better left for when
the dust settles a bit instead of trying to patch it up along the way.
>
> >
> > In either case:
> >
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > > spapr_drc_set_allocation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> > > spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> > > spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
> > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > index d437e0a..802c708 100644
> > > --- a/include/hw/ppc/spapr_drc.h
> > > +++ b/include/hw/ppc/spapr_drc.h
> > > @@ -125,7 +125,7 @@ typedef enum {
> > > } sPAPRDRAllocationState;
> > >
> > > /*
> > > - * LED/visual indicator state
> > > + * DR-indicator (LED/visual indicator)
> > > *
> > > * set via set-indicator RTAS calls
> > > * as documented by PAPR+ 2.7 13.5.3.4, Table 177,
> > > @@ -137,10 +137,10 @@ typedef enum {
> > > * action: (currently unused)
> > > */
> > > typedef enum {
> > > - SPAPR_DR_INDICATOR_STATE_INACTIVE = 0,
> > > - SPAPR_DR_INDICATOR_STATE_ACTIVE = 1,
> > > - SPAPR_DR_INDICATOR_STATE_IDENTIFY = 2,
> > > - SPAPR_DR_INDICATOR_STATE_ACTION = 3,
> > > + SPAPR_DR_INDICATOR_INACTIVE = 0,
> > > + SPAPR_DR_INDICATOR_ACTIVE = 1,
> > > + SPAPR_DR_INDICATOR_IDENTIFY = 2,
> > > + SPAPR_DR_INDICATOR_ACTION = 3,
> > > } sPAPRDRIndicatorState;
> > >
> > > /*
> > > @@ -186,10 +186,12 @@ typedef struct sPAPRDRConnector {
> > > Object *owner;
> > > char *name;
> > >
> > > + /* DR-indicator */
> > > + uint32_t dr_indicator;
> > > +
> > > /* sensor/indicator states */
> > > uint32_t isolation_state;
> > > uint32_t allocation_state;
> > > - uint32_t indicator_state;
> > >
> > > /* configure-connector state */
> > > void *fdt;
> > > @@ -219,8 +221,6 @@ typedef struct sPAPRDRConnectorClass {
> > > /* accessors for guest-visible (generally via RTAS) DR state */
> > > uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> > > sPAPRDRIsolationState state);
> > > - uint32_t (*set_indicator_state)(sPAPRDRConnector *drc,
> > > - sPAPRDRIndicatorState state);
> > > uint32_t (*set_allocation_state)(sPAPRDRConnector *drc,
> > > sPAPRDRAllocationState state);
> > >
> >
>
> --
> 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] 17+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
2017-06-07 23:11 ` Michael Roth
@ 2017-06-08 1:08 ` David Gibson
0 siblings, 0 replies; 17+ messages in thread
From: David Gibson @ 2017-06-08 1:08 UTC (permalink / raw)
To: Michael Roth; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
[-- Attachment #1: Type: text/plain, Size: 8092 bytes --]
On Wed, Jun 07, 2017 at 06:11:03PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-06 20:28:51)
> > On Tue, Jun 06, 2017 at 04:04:33PM -0500, Michael Roth wrote:
> > > Quoting David Gibson (2017-06-06 03:32:20)
> > > > There are 3 types of "indicator" associated with hotplug in the PAPR spec
> > > > the "allocation state", "isolation state" and "DR-indicator". The first
> > > > two are intimately tied to the various state transitions associated with
> > > > hotplug. The DR-indicator, however, is different and simpler.
> > > >
> > > > It's basically just a guest controlled variable which can be used by the
> > > > guest to flag state or problems associated with a device. The idea is that
> > > > the hypervisor can use it to present information back on management
> > > > consoles (on some machines with PowerVM it may even control physical LEDs
> > > > on the machine case associated with the relevant device).
> > > >
> > > > For that reason, there's only ever likely to be a single update
> > > > implementation so the set_indicator_state method isn't useful. Replace it
> > > > with a direct function call.
> > > >
> > > > While we're there, make some small associated cleanups:
> > > > * PAPR doesn't use the term "indicator state", just "DR-indicator" and
> > > > the allocation state and isolation state are also considered "indicators".
> > > > Rename things to be less confusing
> > > > * Fold set_indicator_state() and rtas_set_indicator_state() into a single
> > > > rtas_set_dr_indicator() function.
> > > >
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > > hw/ppc/spapr_drc.c | 25 ++++++++-----------------
> > > > hw/ppc/trace-events | 2 +-
> > > > include/hw/ppc/spapr_drc.h | 16 ++++++++--------
> > > > 3 files changed, 17 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > index 6c2fa93..a4ece2e 100644
> > > > --- a/hw/ppc/spapr_drc.c
> > > > +++ b/hw/ppc/spapr_drc.c
> > > > @@ -116,14 +116,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> > > > return RTAS_OUT_SUCCESS;
> > > > }
> > > >
> > > > -static uint32_t set_indicator_state(sPAPRDRConnector *drc,
> > > > - sPAPRDRIndicatorState state)
> > > > -{
> > > > - trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
> > > > - drc->indicator_state = state;
> > > > - return RTAS_OUT_SUCCESS;
> > > > -}
> > > > -
> > > > static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > > > sPAPRDRAllocationState state)
> > > > {
> > > > @@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > > > 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;
> > > > + drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> > > >
> > > > drc->dev = d;
> > > > drc->fdt = fdt;
> > > > @@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> > > > }
> > > > }
> > > >
> > > > - drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> > > > + drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> > > >
> > > > /* Calling release callbacks based on spapr_drc_type(drc). */
> > > > switch (spapr_drc_type(drc)) {
> > > > @@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc = {
> > > > .fields = (VMStateField []) {
> > > > VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> > > > VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> > > > - VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> > > > + VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > > > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > > VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > > @@ -614,7 +606,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > > > dk->realize = realize;
> > > > dk->unrealize = unrealize;
> > > > drck->set_isolation_state = set_isolation_state;
> > > > - drck->set_indicator_state = set_indicator_state;
> > > > drck->set_allocation_state = set_allocation_state;
> > > > drck->attach = attach;
> > > > drck->detach = detach;
> > > > @@ -895,17 +886,17 @@ static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
> > > > return drck->set_allocation_state(drc, state);
> > > > }
> > > >
> > > > -static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> > > > +static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> > > > {
> > > > sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> > > > - sPAPRDRConnectorClass *drck;
> > > >
> > > > if (!drc) {
> > > > return RTAS_OUT_PARAM_ERROR;
> > > > }
> > > >
> > > > - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > > - return drck->set_indicator_state(drc, state);
> > > > + trace_spapr_drc_set_dr_indicator(idx, state);
> > > > + drc->dr_indicator = state;
> > > > + return RTAS_OUT_SUCCESS;
> > > > }
> > > >
> > > > static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > > @@ -930,7 +921,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > > ret = rtas_set_isolation_state(idx, state);
> > > > break;
> > > > case RTAS_SENSOR_TYPE_DR:
> > > > - ret = rtas_set_indicator_state(idx, state);
> > > > + ret = rtas_set_dr_indicator(idx, state);
> > > > break;
> > > > case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > > > ret = rtas_set_allocation_state(idx, state);
> > > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > > index 581fa85..3e8e3cf 100644
> > > > --- a/hw/ppc/trace-events
> > > > +++ b/hw/ppc/trace-events
> > > > @@ -39,7 +39,7 @@ spapr_iommu_ddw_reset(uint64_t buid, uint32_t cfgaddr) "buid=%"PRIx64" addr=%"PR
> > > > spapr_drc_set_isolation_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: %"PRIx32
> > > > spapr_drc_set_isolation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
> > > > spapr_drc_set_isolation_state_deferring(uint32_t index) "drc: 0x%"PRIx32
> > > > -spapr_drc_set_indicator_state(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> > > > +spapr_drc_set_dr_indicator(uint32_t index, int state) "drc: 0x%"PRIx32", state: 0x%x"
> > >
> > > Since this only tracks the changes to dr_indicator via RTAS (as was also
> > > the case previously), it should probably be changed to an RTAS trace
> > > while we're here.
> >
> > That doesn't follow for me. Yes, it's only triggered by RTAS, but
> > it's really about a DRC event. It's when debugging DRC things that
> > you're going to care about it.
>
> My concern is more on the trace output side of things. As it stands it
> gives a false sense that you're seeing a full accounting of the state
> changes when really it's only a particular call-site that's being
> traced. Making it spapr_drc_set_dr_indicator_rtas would have been a
> better suggestion though.
Oh, I see. That's not new though - the only other dr-indicator state
changes already didn't go through that call path.
> Then again, the issue is not unique to this particular value, so maybe
> a general rework of how we handle tracing would be better left for when
> the dust settles a bit instead of trying to patch it up along the
> way.
Yeah, I think that makes sense.
--
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] 17+ messages in thread
* [Qemu-devel] [PATCH 7/7] spapr: Change DRC attach & detach methods to functions
2017-06-06 8:32 [Qemu-devel] [PATCH 0/7] spapr: DRC cleanups (part III) David Gibson
` (5 preceding siblings ...)
2017-06-06 8:32 ` [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator David Gibson
@ 2017-06-06 8:32 ` David Gibson
2017-06-06 21:15 ` Michael Roth
6 siblings, 1 reply; 17+ messages in thread
From: David Gibson @ 2017-06-06 8:32 UTC (permalink / raw)
To: mdroth
Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug,
David Gibson
DRC objects have attach & detach methods, but there's only one
implementation. Although there are some differences in its behaviour for
different DRC types, the overall structure is the same, so while we might
want different method implementations for some parts, we're unlikely to
want them for the top-level functions.
So, replace them with direct function calls.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 19 ++++++-------------
hw/ppc/spapr_drc.c | 18 ++++++------------
hw/ppc/spapr_pci.c | 9 +++------
include/hw/ppc/spapr_drc.h | 7 ++++---
4 files changed, 19 insertions(+), 34 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e8cecaf..37466df 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2533,7 +2533,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
Error **errp)
{
sPAPRDRConnector *drc;
- sPAPRDRConnectorClass *drck;
uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
int i, fdt_offset, fdt_size;
void *fdt;
@@ -2548,10 +2547,10 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
fdt_offset = spapr_populate_memory_node(fdt, node, addr,
SPAPR_MEMORY_BLOCK_SIZE);
- drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
+ spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
addr += SPAPR_MEMORY_BLOCK_SIZE;
if (!dev->hotplugged) {
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
/* guests expect coldplugged LMBs to be pre-allocated */
drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
@@ -2564,7 +2563,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
if (dedicated_hp_event_source) {
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,
spapr_drc_index(drc));
@@ -2749,7 +2747,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
uint64_t addr_start, addr;
int i;
sPAPRDRConnector *drc;
- sPAPRDRConnectorClass *drck;
sPAPRDIMMState *ds;
addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
@@ -2769,14 +2766,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
- drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- drck->detach(drc, dev, errp);
+ spapr_drc_detach(drc, dev, errp);
addr += 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));
out:
@@ -2831,7 +2826,6 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
{
int index;
sPAPRDRConnector *drc;
- sPAPRDRConnectorClass *drck;
Error *local_err = NULL;
CPUCore *cc = CPU_CORE(dev);
int smt = kvmppc_smt_threads();
@@ -2849,8 +2843,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
g_assert(drc);
- drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- drck->detach(drc, dev, &local_err);
+ spapr_drc_detach(drc, dev, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
@@ -2894,8 +2887,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
}
if (drc) {
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
- drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
+ spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged,
+ &local_err);
if (local_err) {
g_free(fdt);
error_propagate(errp, local_err);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a4ece2e..c73fae0 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -49,8 +49,6 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
static uint32_t set_isolation_state(sPAPRDRConnector *drc,
sPAPRDRIsolationState state)
{
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-
trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
/* if the guest is configuring a device attached to this DRC, we
@@ -105,7 +103,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
uint32_t drc_index = spapr_drc_index(drc);
if (drc->configured) {
trace_spapr_drc_set_isolation_state_finalizing(drc_index);
- drck->detach(drc, DEVICE(drc->dev), NULL);
+ spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
} else {
trace_spapr_drc_set_isolation_state_deferring(drc_index);
}
@@ -119,8 +117,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
static uint32_t set_allocation_state(sPAPRDRConnector *drc,
sPAPRDRAllocationState state)
{
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-
trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
if (state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
@@ -151,7 +147,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
uint32_t drc_index = spapr_drc_index(drc);
trace_spapr_drc_set_allocation_state_finalizing(drc_index);
- drck->detach(drc, DEVICE(drc->dev), NULL);
+ spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
} else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
drc->awaiting_allocation = false;
}
@@ -281,8 +277,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
} while (fdt_depth != 0);
}
-static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
- int fdt_start_offset, bool coldplug, Error **errp)
+void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
+ int fdt_start_offset, bool coldplug, Error **errp)
{
trace_spapr_drc_attach(spapr_drc_index(drc));
@@ -333,7 +329,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
NULL, 0, NULL);
}
-static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
+void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
{
trace_spapr_drc_detach(spapr_drc_index(drc));
@@ -436,7 +432,7 @@ static void reset(DeviceState *d)
* force removal if we are
*/
if (drc->awaiting_release) {
- drck->detach(drc, DEVICE(drc->dev), NULL);
+ spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
}
/* non-PCI devices may be awaiting a transition to UNUSABLE */
@@ -607,8 +603,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->unrealize = unrealize;
drck->set_isolation_state = set_isolation_state;
drck->set_allocation_state = set_allocation_state;
- drck->attach = attach;
- drck->detach = detach;
drck->release_pending = release_pending;
drck->set_signalled = set_signalled;
/*
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 967d7c3..46e736d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1349,7 +1349,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
PCIDevice *pdev,
Error **errp)
{
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
DeviceState *dev = DEVICE(pdev);
void *fdt = NULL;
int fdt_start_offset = 0, fdt_size;
@@ -1361,8 +1360,8 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
goto out;
}
- drck->attach(drc, DEVICE(pdev),
- fdt, fdt_start_offset, !dev->hotplugged, errp);
+ spapr_drc_attach(drc, DEVICE(pdev),
+ fdt, fdt_start_offset, !dev->hotplugged, errp);
out:
if (*errp) {
g_free(fdt);
@@ -1391,9 +1390,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
PCIDevice *pdev,
Error **errp)
{
- sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-
- drck->detach(drc, DEVICE(pdev), errp);
+ spapr_drc_detach(drc, DEVICE(pdev), errp);
}
static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 802c708..ec0256b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -225,9 +225,6 @@ typedef struct sPAPRDRConnectorClass {
sPAPRDRAllocationState state);
/* QEMU interfaces for managing hotplug operations */
- void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
- int fdt_start_offset, bool coldplug, Error **errp);
- void (*detach)(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
bool (*release_pending)(sPAPRDRConnector *drc);
void (*set_signalled)(sPAPRDRConnector *drc);
} sPAPRDRConnectorClass;
@@ -243,4 +240,8 @@ 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);
+void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
+ int fdt_start_offset, bool coldplug, Error **errp);
+void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
+
#endif /* HW_SPAPR_DRC_H */
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] spapr: Change DRC attach & detach methods to functions
2017-06-06 8:32 ` [Qemu-devel] [PATCH 7/7] spapr: Change DRC attach & detach methods to functions David Gibson
@ 2017-06-06 21:15 ` Michael Roth
0 siblings, 0 replies; 17+ messages in thread
From: Michael Roth @ 2017-06-06 21:15 UTC (permalink / raw)
To: David Gibson; +Cc: lvivier, qemu-ppc, qemu-devel, bharata, sursingh, groug
Quoting David Gibson (2017-06-06 03:32:21)
> DRC objects have attach & detach methods, but there's only one
> implementation. Although there are some differences in its behaviour for
> different DRC types, the overall structure is the same, so while we might
> want different method implementations for some parts, we're unlikely to
> want them for the top-level functions.
>
> So, replace them with direct function calls.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 19 ++++++-------------
> hw/ppc/spapr_drc.c | 18 ++++++------------
> hw/ppc/spapr_pci.c | 9 +++------
> include/hw/ppc/spapr_drc.h | 7 ++++---
> 4 files changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e8cecaf..37466df 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2533,7 +2533,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> Error **errp)
> {
> sPAPRDRConnector *drc;
> - sPAPRDRConnectorClass *drck;
> uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
> int i, fdt_offset, fdt_size;
> void *fdt;
> @@ -2548,10 +2547,10 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> fdt_offset = spapr_populate_memory_node(fdt, node, addr,
> SPAPR_MEMORY_BLOCK_SIZE);
>
> - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> + spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> addr += SPAPR_MEMORY_BLOCK_SIZE;
> if (!dev->hotplugged) {
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> /* guests expect coldplugged LMBs to be pre-allocated */
> drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
> drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> @@ -2564,7 +2563,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> if (dedicated_hp_event_source) {
> 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,
> spapr_drc_index(drc));
> @@ -2749,7 +2747,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> uint64_t addr_start, addr;
> int i;
> sPAPRDRConnector *drc;
> - sPAPRDRConnectorClass *drck;
> sPAPRDIMMState *ds;
>
> addr_start = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> @@ -2769,14 +2766,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> addr / SPAPR_MEMORY_BLOCK_SIZE);
> g_assert(drc);
>
> - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - drck->detach(drc, dev, errp);
> + spapr_drc_detach(drc, dev, errp);
> addr += 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));
> out:
> @@ -2831,7 +2826,6 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> {
> int index;
> sPAPRDRConnector *drc;
> - sPAPRDRConnectorClass *drck;
> Error *local_err = NULL;
> CPUCore *cc = CPU_CORE(dev);
> int smt = kvmppc_smt_threads();
> @@ -2849,8 +2843,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
> g_assert(drc);
>
> - drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - drck->detach(drc, dev, &local_err);
> + spapr_drc_detach(drc, dev, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -2894,8 +2887,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> }
>
> if (drc) {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> - drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> + spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged,
> + &local_err);
> if (local_err) {
> g_free(fdt);
> error_propagate(errp, local_err);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a4ece2e..c73fae0 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -49,8 +49,6 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
> static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> sPAPRDRIsolationState state)
> {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -
> trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
>
> /* if the guest is configuring a device attached to this DRC, we
> @@ -105,7 +103,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> uint32_t drc_index = spapr_drc_index(drc);
> if (drc->configured) {
> trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> - drck->detach(drc, DEVICE(drc->dev), NULL);
> + spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> } else {
> trace_spapr_drc_set_isolation_state_deferring(drc_index);
> }
> @@ -119,8 +117,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> sPAPRDRAllocationState state)
> {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -
> trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
>
> if (state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
> @@ -151,7 +147,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> uint32_t drc_index = spapr_drc_index(drc);
> trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> - drck->detach(drc, DEVICE(drc->dev), NULL);
> + spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
> drc->awaiting_allocation = false;
> }
> @@ -281,8 +277,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> } while (fdt_depth != 0);
> }
>
> -static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> - int fdt_start_offset, bool coldplug, Error **errp)
> +void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> + int fdt_start_offset, bool coldplug, Error **errp)
> {
> trace_spapr_drc_attach(spapr_drc_index(drc));
>
> @@ -333,7 +329,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> NULL, 0, NULL);
> }
>
> -static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> +void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> {
> trace_spapr_drc_detach(spapr_drc_index(drc));
>
> @@ -436,7 +432,7 @@ static void reset(DeviceState *d)
> * force removal if we are
> */
> if (drc->awaiting_release) {
> - drck->detach(drc, DEVICE(drc->dev), NULL);
> + spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> }
>
> /* non-PCI devices may be awaiting a transition to UNUSABLE */
> @@ -607,8 +603,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->unrealize = unrealize;
> drck->set_isolation_state = set_isolation_state;
> drck->set_allocation_state = set_allocation_state;
> - drck->attach = attach;
> - drck->detach = detach;
> drck->release_pending = release_pending;
> drck->set_signalled = set_signalled;
> /*
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 967d7c3..46e736d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1349,7 +1349,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> PCIDevice *pdev,
> Error **errp)
> {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> DeviceState *dev = DEVICE(pdev);
> void *fdt = NULL;
> int fdt_start_offset = 0, fdt_size;
> @@ -1361,8 +1360,8 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> goto out;
> }
>
> - drck->attach(drc, DEVICE(pdev),
> - fdt, fdt_start_offset, !dev->hotplugged, errp);
> + spapr_drc_attach(drc, DEVICE(pdev),
> + fdt, fdt_start_offset, !dev->hotplugged, errp);
> out:
> if (*errp) {
> g_free(fdt);
> @@ -1391,9 +1390,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
> PCIDevice *pdev,
> Error **errp)
> {
> - sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -
> - drck->detach(drc, DEVICE(pdev), errp);
> + spapr_drc_detach(drc, DEVICE(pdev), errp);
> }
>
> static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 802c708..ec0256b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -225,9 +225,6 @@ typedef struct sPAPRDRConnectorClass {
> sPAPRDRAllocationState state);
>
> /* QEMU interfaces for managing hotplug operations */
> - void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> - int fdt_start_offset, bool coldplug, Error **errp);
> - void (*detach)(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
> bool (*release_pending)(sPAPRDRConnector *drc);
> void (*set_signalled)(sPAPRDRConnector *drc);
> } sPAPRDRConnectorClass;
> @@ -243,4 +240,8 @@ 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);
>
> +void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> + int fdt_start_offset, bool coldplug, Error **errp);
> +void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
> +
> #endif /* HW_SPAPR_DRC_H */
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 17+ messages in thread