Re: [Qemu-devel] [PATCH 03/14] spapr_pci: add get-sensor-state RTAS interface

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Alexey Kardashevskiy (2013-12-04 20:47:52)
> On 12/05/2013 12:19 PM, 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_pci.c     |   72 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |    6 +++-
> >  2 files changed, 77 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 64077f9..95336f7 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -498,6 +498,77 @@ static void rtas_get_power_level(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >      rtas_st(rets, 1, 100);
> >  }
> >  
> > +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> > +                                  uint32_t token, uint32_t nargs,
> > +                                  target_ulong args, uint32_t nret,
> > +                                  target_ulong rets)
> > +{
> > +    uint32_t sensor = rtas_ld(args, 0);
> > +    uint32_t drc_index = rtas_ld(args, 1);
> > +    uint32_t sensor_state = 0, decoded = 0, ccode = NO_SUCH_INDICATOR;
> > +    uint32_t shift = 0, mask = 0;
> > +    DrcEntry *drc_entry = NULL;
> > +
> > +    if (drc_index == 0) {  /* platform state sensor/indicator */
> > +        sensor_state = spapr->state;
> > +    } else { /* we should have a drc entry */
> > +        drc_entry = spapr_find_drc_entry(drc_index);
> > +        if (!drc_entry) {
> > +            g_warning("unable to find DRC entry for index %x", drc_index);
> 
> 
> I did not see QEMU using g_warning(), it is error_report() or custom
> defined DPRINTF or fprintf(stderr,...) or a trace point from ./trace-events
> (traces which can be switched on/off in run-time).
> 
> It the case like this, I'd make it DPRINTF or trace-event, the current RTAS
> handlers do not print error messages on errors by default.

Yah, DPRINTF seems reasonable, invalid guest-specified params aren't really
a QEMU issue, this is more for debugging.

> 
> 
> 
> > +            sensor_state = 0; /* empty */
> > +            /* ccode is already set to -3 */
> > +            rtas_st(rets, 0, ccode);
> > +            return;
> > +        }
> > +        sensor_state = drc_entry->state;
> > +    }
> > +    switch (sensor) {
> > +    case 9:  /* EPOW */
> > +        shift = INDICATOR_EPOW_SHIFT;
> > +        mask = INDICATOR_EPOW_MASK;
> > +        break;
> > +    case 9001: /* Isolation state */
> > +        /* encode the new value into the correct bit field */
> > +        shift = INDICATOR_ISOLATION_SHIFT;
> > +        mask = INDICATOR_ISOLATION_MASK;
> > +        break;
> > +    case 9002: /* DR */
> > +        shift = INDICATOR_DR_SHIFT;
> > +        mask = INDICATOR_DR_MASK;
> > +        break;
> > +    case 9003: /* entity sense */
> > +        shift = SENSOR_ENTITY_SENSE_SHIFT;
> > +        mask = SENSOR_ENTITY_SENSE_MASK;
> > +        break;
> > +    case 9005: /* global interrupt */
> > +        shift = INDICATOR_GLOBAL_INTERRUPT_SHIFT;
> > +        mask = INDICATOR_GLOBAL_INTERRUPT_MASK;
> > +        break;
> > +    case 9006: /* error log */
> > +        shift = INDICATOR_ERROR_LOG_SHIFT;
> > +        mask = INDICATOR_ERROR_LOG_MASK;
> > +        break;
> > +    case 9007: /* identify */
> > +        shift = INDICATOR_IDENTIFY_SHIFT;
> > +        mask = INDICATOR_IDENTIFY_MASK;
> > +        break;
> > +    case 9009: /* reset */
> > +        shift = INDICATOR_RESET_SHIFT;
> > +        mask = INDICATOR_RESET_MASK;
> > +        break;
> > +    default:
> > +        g_warning("rtas_get_sensor_state: sensor not implemented: %d",
> > +                  sensor);
> > +        rtas_st(rets, 0, -3);
> > +        return;
> > +    }
> > +
> > +    decoded = DECODE_DRC_STATE(sensor_state, mask, shift);
> > +    ccode = 0;
> > +    rtas_st(rets, 0, ccode);
> > +    rtas_st(rets, 1, decoded);
> > +}
> > +
> >  static int pci_spapr_swizzle(int slot, int pin)
> >  {
> >      return (slot + pin) % PCI_NUM_PINS;
> > @@ -961,6 +1032,7 @@ void spapr_pci_rtas_init(void)
> >      spapr_rtas_register("set-indicator", rtas_set_indicator);
> >      spapr_rtas_register("set-power-level", rtas_set_power_level);
> >      spapr_rtas_register("get-power-level", rtas_get_power_level);
> > +    spapr_rtas_register("get-sensor-state", rtas_get_sensor_state);
> >  }
> >  
> >  static void spapr_pci_register_types(void)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index d8c7de4..cadf95f 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -302,7 +302,7 @@ typedef struct sPAPREnvironment {
> >  #define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
> >  #define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
> >  
> > -/* For set-indicator RTAS interface */
> > +/* For set-indicator/get-sensor-state RTAS interfaces */
> >  #define INDICATOR_ISOLATION_MASK            0x0001   /* 9001 one bit */
> >  #define INDICATOR_GLOBAL_INTERRUPT_MASK     0x0002   /* 9005 one bit */
> >  #define INDICATOR_ERROR_LOG_MASK            0x0004   /* 9006 one bit */
> > @@ -311,6 +311,7 @@ typedef struct sPAPREnvironment {
> >  #define INDICATOR_DR_MASK                   0x00e0   /* 9002 three bits */
> >  #define INDICATOR_ALLOCATION_MASK           0x0300   /* 9003 two bits */
> >  #define INDICATOR_EPOW_MASK                 0x1c00   /* 9 three bits */
> > +#define SENSOR_ENTITY_SENSE_MASK            0xe000   /* 9003 three bits */
> 
> 
> This one starts with "SENSOR_" when others with "INDICATOR_". Is it for a
> reason?

This mostly due to how it was transcribed from PAPR documentation, we call
them "sensors" WRT to get-sensor-state, and "indicators" for
set-indicator-state.

But since we re-use the INDICATOR_* values for get-sensor-state as well, I
agree we should probably stick with the INDICATOR_ prefix.

> 
> 
> >  #define INDICATOR_ISOLATION_SHIFT           0x00     /* bit 0 */
> >  #define INDICATOR_GLOBAL_INTERRUPT_SHIFT    0x01     /* bit 1 */
> > @@ -320,8 +321,11 @@ typedef struct sPAPREnvironment {
> >  #define INDICATOR_DR_SHIFT                  0x05     /* bits 5-7 */
> >  #define INDICATOR_ALLOCATION_SHIFT          0x08     /* bits 8-9 */
> >  #define INDICATOR_EPOW_SHIFT                0x0a     /* bits 10-12 */
> > +#define SENSOR_ENTITY_SENSE_SHIFT           0x0d     /* bits 13-15 */
> >  
> >  #define NO_SUCH_INDICATOR -3
> > +#define DR_ENTITY_SENSE_EMPTY 0
> > +#define DR_ENTITY_SENSE_PRESENT 1
> >  
> >  #define DECODE_DRC_STATE(state, m, s)                  \
> >      ((((uint32_t)(state) & (uint32_t)(m))) >> (s))
> > 
> 
> 
> -- 
> Alexey