Re: [Qemu-devel] [PATCH 5/7] spapr: Clean up RTAS set-indicator

[Michael Roth]

Quoting David Gibson (2017-06-06 03:32:19)
> In theory the RTAS set-indicator call can be used for a number of
> "indicators" defined by PAPR.  In practice the only ones we're ever likely
> to implement are those used for Dynamic Reconfiguration (i.e. hotplug).
> Because of this, the current implementation determines the associated DRC
> object, before dispatching based on the type of indicator.
> 
> However, this means we also need a check that we're dealing with a DR
> related indicator at all, which duplicates some of the logic from the
> switch further down.
> 
> Even though it means a bit of code duplication, things work out cleaner if
> we delegate the DRC lookup to the individual indicator type functions -
> and it also allows some further cleanups.
> 
> While we're there, remove references to "sensor", a copy/paste artefact
> from the related, but distinct "get-sensor" call.
> 
> Signed-off-by: David Gibson <address@hidden>

Reviewed-by: Michael Roth <address@hidden>

> ---
>  hw/ppc/spapr_drc.c  | 84 
> ++++++++++++++++++++++++++++-------------------------
>  hw/ppc/trace-events |  2 --
>  2 files changed, 44 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 1fc51c9..6c2fa93 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -869,74 +869,78 @@ out:
>   * RTAS calls
>   */
> 
> -static bool sensor_type_is_dr(uint32_t sensor_type)
> +static uint32_t rtas_set_isolation_state(uint32_t idx, uint32_t state)
>  {
> -    switch (sensor_type) {
> -    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> -    case RTAS_SENSOR_TYPE_DR:
> -    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> -        return true;
> +    sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> +    sPAPRDRConnectorClass *drck;
> +
> +    if (!drc) {
> +        return RTAS_OUT_PARAM_ERROR;
>      }
> 
> -    return false;
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    return drck->set_isolation_state(drc, state);
>  }
> 
> -static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> -                               uint32_t token, uint32_t nargs,
> -                               target_ulong args, uint32_t nret,
> -                               target_ulong rets)
> +static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
>  {
> -    uint32_t sensor_type;
> -    uint32_t sensor_index;
> -    uint32_t sensor_state;
> -    uint32_t ret = RTAS_OUT_SUCCESS;
> -    sPAPRDRConnector *drc;
> +    sPAPRDRConnector *drc = spapr_drc_by_index(idx);
>      sPAPRDRConnectorClass *drck;
> 
> -    if (nargs != 3 || nret != 1) {
> -        ret = RTAS_OUT_PARAM_ERROR;
> -        goto out;
> +    if (!drc) {
> +        return RTAS_OUT_PARAM_ERROR;
>      }
> 
> -    sensor_type = rtas_ld(args, 0);
> -    sensor_index = rtas_ld(args, 1);
> -    sensor_state = rtas_ld(args, 2);
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    return drck->set_allocation_state(drc, state);
> +}
> 
> -    if (!sensor_type_is_dr(sensor_type)) {
> -        goto out_unimplemented;
> -    }
> +static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> +{
> +    sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> +    sPAPRDRConnectorClass *drck;
> 
> -    /* if this is a DR sensor we can assume sensor_index == drc_index */
> -    drc = spapr_drc_by_index(sensor_index);
>      if (!drc) {
> -        trace_spapr_rtas_set_indicator_invalid(sensor_index);
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
> +
> +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    return drck->set_indicator_state(drc, state);
> +}
> +
> +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 type, idx, state;
> +    uint32_t ret = RTAS_OUT_SUCCESS;
> +
> +    if (nargs != 3 || nret != 1) {
>          ret = RTAS_OUT_PARAM_ERROR;
>          goto out;
>      }
> -    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> 
> -    switch (sensor_type) {
> +    type = rtas_ld(args, 0);
> +    idx = rtas_ld(args, 1);
> +    state = rtas_ld(args, 2);
> +
> +    switch (type) {
>      case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> -        ret = drck->set_isolation_state(drc, sensor_state);
> +        ret = rtas_set_isolation_state(idx, state);
>          break;
>      case RTAS_SENSOR_TYPE_DR:
> -        ret = drck->set_indicator_state(drc, sensor_state);
> +        ret = rtas_set_indicator_state(idx, state);
>          break;
>      case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> -        ret = drck->set_allocation_state(drc, sensor_state);
> +        ret = rtas_set_allocation_state(idx, state);
>          break;
>      default:
> -        goto out_unimplemented;
> +        ret = RTAS_OUT_NOT_SUPPORTED;
>      }
> 
>  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,
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 4979397..581fa85 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -60,8 +60,6 @@ spapr_ovec_parse_vector(int vector, int byte, uint16_t 
> vec_len, uint8_t entry) "
>  spapr_ovec_populate_dt(int byte, uint16_t vec_len, uint8_t entry) "encoding 
> guest vector byte %3d / %3d: 0x%.2x"
> 
>  # hw/ppc/spapr_rtas.c
> -spapr_rtas_set_indicator_invalid(uint32_t index) "sensor index: 0x%"PRIx32
> -spapr_rtas_set_indicator_not_supported(uint32_t index, uint32_t type) 
> "sensor index: 0x%"PRIx32", type: %"PRIu32
>  spapr_rtas_get_sensor_state_not_supported(uint32_t index, uint32_t type) 
> "sensor index: 0x%"PRIx32", type: %"PRIu32
>  spapr_rtas_get_sensor_state_invalid(uint32_t index) "sensor index: 0x%"PRIx32
>  spapr_rtas_ibm_configure_connector_invalid(uint32_t index) "DRC index: 
> 0x%"PRIx32
> -- 
> 2.9.4
>