Re: [Qemu-devel] [PATCH 1/4] spapr: Move DRC RTAS calls into spapr_drc.c

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 10083 bytes --]

On Thu, Jun 01, 2017 at 11:05:37AM -0500, Michael Roth wrote:
> Quoting Laurent Vivier (2017-06-01 08:36:36)
> > On 01/06/2017 03:52, David Gibson wrote:
> > > Currently implementations of the RTAS calls related to DRCs are in
> > > spapr_rtas.c.  They belong better in spapr_drc.c - that way they're closer
> > > to related code, and we'll be able to make some more things local.
> > > 
> > > spapr_rtas.c was intended to contain the RTAS infrastructure and core calls
> > > that don't belong anywhere else, not every RTAS implementation.
> > > 
> > > Code motion only.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr_drc.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  hw/ppc/spapr_rtas.c | 304 -------------------------------------------------
> > >  2 files changed, 315 insertions(+), 311 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index cc2400b..ae8800d 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -27,6 +27,34 @@
> > >  #define DRC_INDEX_TYPE_SHIFT 28
> > >  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
> > >  
> > > +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> > > +                                                    uint32_t drc_index)
> > > +{
> > > +    sPAPRConfigureConnectorState *ccs = NULL;
> > > +
> > > +    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> > > +        if (ccs->drc_index == drc_index) {
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    return ccs;
> > > +}
> > > +
> > > +static void spapr_ccs_add(sPAPRMachineState *spapr,
> > > +                          sPAPRConfigureConnectorState *ccs)
> > > +{
> > > +    g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> > > +    QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> > > +}
> > > +
> > > +static void spapr_ccs_remove(sPAPRMachineState *spapr,
> > > +                             sPAPRConfigureConnectorState *ccs)
> > > +{
> > > +    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> > > +    g_free(ccs);
> > > +}
> > > +
> > >  static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type)
> > >  {
> > >      uint32_t shift = 0;
> > > @@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = {
> > >      .class_init    = spapr_dr_connector_class_init,
> > >  };
> > >  
> > > -static void spapr_drc_register_types(void)
> > > -{
> > > -    type_register_static(&spapr_dr_connector_info);
> > > -}
> > > -
> > > -type_init(spapr_drc_register_types)
> > > -
> > >  /* helper functions for external users */
> > >  
> > >  sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index)
> > > @@ -932,3 +953,290 @@ out:
> > >  
> > >      return ret;
> > >  }
> > > +
> > > +/*
> > > + * RTAS calls
> > > + */
> > > +
> > > +static bool sensor_type_is_dr(uint32_t sensor_type)
> > > +{
> > > +    switch (sensor_type) {
> > > +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> > > +    case RTAS_SENSOR_TYPE_DR:
> > > +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > > +        return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +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 sensor_type;
> > > +    uint32_t sensor_index;
> > > +    uint32_t sensor_state;
> > > +    uint32_t ret = RTAS_OUT_SUCCESS;
> > > +    sPAPRDRConnector *drc;
> > > +    sPAPRDRConnectorClass *drck;
> > > +
> > > +    if (nargs != 3 || nret != 1) {
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +
> > > +    sensor_type = rtas_ld(args, 0);
> > > +    sensor_index = rtas_ld(args, 1);
> > > +    sensor_state = rtas_ld(args, 2);
> > > +
> > > +    if (!sensor_type_is_dr(sensor_type)) {
> > > +        goto out_unimplemented;
> > > +    }
> > > +
> > > +    /* if this is a DR sensor we can assume sensor_index == drc_index */
> > > +    drc = spapr_dr_connector_by_index(sensor_index);
> > > +    if (!drc) {
> > > +        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +
> > > +    switch (sensor_type) {
> > > +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> > > +        /* if the guest is configuring a device attached to this
> > > +         * DRC, we should reset the configuration state at this
> > > +         * point since it may no longer be reliable (guest released
> > > +         * device and needs to start over, or unplug occurred so
> > > +         * the FDT is no longer valid)
> > > +         */
> > > +        if (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > > +            sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> > > +                                                               sensor_index);
> > > +            if (ccs) {
> > > +                spapr_ccs_remove(spapr, ccs);
> > > +            }
> > > +        }
> > > +        ret = drck->set_isolation_state(drc, sensor_state);
> > > +        break;
> > > +    case RTAS_SENSOR_TYPE_DR:
> > > +        ret = drck->set_indicator_state(drc, sensor_state);
> > > +        break;
> > > +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> > > +        ret = drck->set_allocation_state(drc, sensor_state);
> > > +        break;
> > > +    default:
> > > +        goto out_unimplemented;
> > > +    }
> > > +
> > > +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,
> > > +                                  uint32_t token, uint32_t nargs,
> > > +                                  target_ulong args, uint32_t nret,
> > > +                                  target_ulong rets)
> > > +{
> > > +    uint32_t sensor_type;
> > > +    uint32_t sensor_index;
> > > +    uint32_t sensor_state = 0;
> > > +    sPAPRDRConnector *drc;
> > > +    sPAPRDRConnectorClass *drck;
> > > +    uint32_t ret = RTAS_OUT_SUCCESS;
> > > +
> > > +    if (nargs != 2 || nret != 2) {
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +
> > > +    sensor_type = rtas_ld(args, 0);
> > > +    sensor_index = rtas_ld(args, 1);
> > > +
> > > +    if (sensor_type != RTAS_SENSOR_TYPE_ENTITY_SENSE) {
> > > +        /* currently only DR-related sensors are implemented */
> > > +        trace_spapr_rtas_get_sensor_state_not_supported(sensor_index,
> > > +                                                        sensor_type);
> > > +        ret = RTAS_OUT_NOT_SUPPORTED;
> > > +        goto out;
> > > +    }
> > > +
> > > +    drc = spapr_dr_connector_by_index(sensor_index);
> > > +    if (!drc) {
> > > +        trace_spapr_rtas_get_sensor_state_invalid(sensor_index);
> > > +        ret = RTAS_OUT_PARAM_ERROR;
> > > +        goto out;
> > > +    }
> > > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +    ret = drck->entity_sense(drc, &sensor_state);
> > > +
> > > +out:
> > > +    rtas_st(rets, 0, ret);
> > > +    rtas_st(rets, 1, sensor_state);
> > > +}
> > > +
> > > +/* configure-connector work area offsets, int32_t units for field
> > > + * indexes, bytes for field offset/len values.
> > > + *
> > > + * as documented by PAPR+ v2.7, 13.5.3.5
> > > + */
> > > +#define CC_IDX_NODE_NAME_OFFSET 2
> > > +#define CC_IDX_PROP_NAME_OFFSET 2
> > > +#define CC_IDX_PROP_LEN 3
> > > +#define CC_IDX_PROP_DATA_OFFSET 4
> > > +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> > > +#define CC_WA_LEN 4096
> > > +
> > > +static void configure_connector_st(target_ulong addr, target_ulong offset,
> > > +                                   const void *buf, size_t len)
> > > +{
> > > +    cpu_physical_memory_write(ppc64_phys_to_real(addr + offset),
> > > +                              buf, MIN(len, CC_WA_LEN - offset));
> > > +}
> > > +
> > > +void spapr_ccs_reset_hook(void *opaque)
> > > +{
> > > +    sPAPRMachineState *spapr = opaque;
> > > +    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> > > +
> > > +    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> > > +        spapr_ccs_remove(spapr, ccs);
> > > +    }
> > > +}
> > 
> > 
> > Why do you move this function in the middle of the "RTAS calls" whereas
> > previously it was with spapr_ccs_find(), spapr_ccs_add() and
> > spapr_ccs_remove() and it is only used by a reset handler in spapr.c?
> 
> I think it's as good a spot as any after the move, and allows
> spapr_ccs_* functions to remain static.

Yeah, what he said.

> But all the CCS stuff got hung off of sPAPRMachineState in the first
> place due to an attempt to separate "RTAS" state from the DRC state.

Ah, right.  I think that was a mistake - RTAS is just an interface to
various subsystems; it doesn't have state of its own.

> Now
> that we're treating both as internal state, I think a logical follow-up
> would be to drop the CCS list completely and instead hang each instance
> off of the corresponding DRC. At that point we'd probably want to move
> the reset functionality into the DRC reset hook anyway.

I'll look into that.

-- 
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 --]