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 <da...@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 <mdr...@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 >