Re: [Qemu-devel] [PATCH v4 04/17] spapr_rtas: add set-indicator RTAS interface

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

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

On Tue, Dec 23, 2014 at 06:30:18AM -0600, Michael Roth wrote:
> From: Mike Day <ncmike@ncultra.org>
> 
> Signed-off-by: Mike Day <ncmike@ncultra.org>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_rtas.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index a2fb533..6aa325f 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -35,6 +35,18 @@
>  #include "qapi-event.h"
>  
>  #include <libfdt.h>
> +#include "hw/ppc/spapr_drc.h"
> +
> +/* #define DEBUG_SPAPR */
> +
> +#ifdef DEBUG_SPAPR
> +#define DPRINTF(fmt, ...) \
> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
>  
>  static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                     uint32_t token, uint32_t nargs,
> @@ -311,6 +323,72 @@ static void rtas_get_power_level(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      rtas_st(rets, 1, 100);
>  }
>  
> +/*
> + * indicator/sensor types
> + * as defined by PAPR+ 2.7 7.3.5.4, Table 41
> + *
> + * NOTE: currently only DR-related sensors are implemented here
> + */
> +#define RTAS_SENSOR_TYPE_ISOLATION_STATE 9001
> +#define RTAS_SENSOR_TYPE_DR 9002
> +#define RTAS_SENSOR_TYPE_ALLOCATION_STATE 9003
> +#define RTAS_SENSOR_TYPE_ENTITY_SENSE RTAS_SENSOR_TYPE_ALLOCATION_STATE

These should probably go in a header.

> +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, sPAPREnvironment *spapr,
> +                               uint32_t token, uint32_t nargs,
> +                               target_ulong args, uint32_t nret,
> +                               target_ulong rets)
> +{
> +    uint32_t sensor_type = rtas_ld(args, 0);
> +    uint32_t sensor_index = rtas_ld(args, 1);
> +    uint32_t sensor_state = rtas_ld(args, 2);

You must validate nargs anr nret before reading the RTAS parameters.

> +    sPAPRDRConnector *drc;
> +    sPAPRDRConnectorClass *drck;
> +
> +    if (sensor_type_is_dr(sensor_type)) {
> +        /* if this is a DR sensor we can assume sensor_index == drc_index */
> +        drc = spapr_dr_connector_by_index(sensor_index);
> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);

I'd reverse the sense of the if, and move these initializations after
the if.  That makes it more obvious that these can't be uninitialized
when you use them below.

> +    } else {
> +        /* currently only DR-related sensors are implemented */
> +        goto out_unimplemented;
> +    }
> +
> +    switch (sensor_type) {
> +    case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> +        drck->set_isolation_state(drc, sensor_state);
> +        break;
> +    case RTAS_SENSOR_TYPE_DR:
> +        drck->set_indicator_state(drc, sensor_state);
> +        break;
> +    case RTAS_SENSOR_TYPE_ALLOCATION_STATE:
> +        drck->set_allocation_state(drc, sensor_state);
> +        break;
> +    default:
> +        goto out_unimplemented;
> +    }
> +
> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    return;
> +
> +out_unimplemented:
> +    DPRINTF("rtas_set_indicator: sensor/indicator not implemented: %d\n",
> +            sensor_type);
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
>  static struct rtas_call {
>      const char *name;
>      spapr_rtas_fn fn;
> @@ -444,6 +522,8 @@ static void core_rtas_register_types(void)
>                          rtas_set_power_level);
>      spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level",
>                          rtas_get_power_level);
> +    spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
> +                        rtas_set_indicator);
>  }
>  
>  type_init(core_rtas_register_types)

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]