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
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
>
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
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
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
© 2016 - 2025 Red Hat, Inc.