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 <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.
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 <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);
> >
>
--
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
signature.asc
Description: PGP signature
