Re: [Qemu-ppc] [PATCH 6/7] spapr: Clean up handling of DR-indicator

[David Gibson]

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 <address@hidden>
> > > > ---
> > > >  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

signature.asc
Description: PGP signature