Re: [Qemu-devel] [PATCH 04/12] spapr_pci: add set-indicator RTAS interface

[Alexander Graf <agraf@suse.de>]

On 01.10.14 00:08, Michael Roth wrote:
> Quoting Alexander Graf (2014-08-26 06:36:57)
>> On 19.08.14 02:21, 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     | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/ppc/spapr.h |   3 ++
>>>  2 files changed, 122 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 924d488..23a3477 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -36,6 +36,16 @@
>>>  
>>>  #include "hw/pci/pci_bus.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
>>> +
>>>  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>>>  #define RTAS_QUERY_FN           0
>>>  #define RTAS_CHANGE_FN          1
>>> @@ -47,6 +57,31 @@
>>>  #define RTAS_TYPE_MSI           1
>>>  #define RTAS_TYPE_MSIX          2
>>>  
>>> +/* For set-indicator RTAS interface */
>>> +#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 */
>>> +#define INDICATOR_IDENTIFY_MASK             0x0008   /* 9007 one bit */
>>> +#define INDICATOR_RESET_MASK                0x0010   /* 9009 one bit */
>>> +#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 INDICATOR_ISOLATION_SHIFT           0x00     /* bit 0 */
>>> +#define INDICATOR_GLOBAL_INTERRUPT_SHIFT    0x01     /* bit 1 */
>>> +#define INDICATOR_ERROR_LOG_SHIFT           0x02     /* bit 2 */
>>> +#define INDICATOR_IDENTIFY_SHIFT            0x03     /* bit 3 */
>>> +#define INDICATOR_RESET_SHIFT               0x04     /* bit 4 */
>>> +#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 DECODE_DRC_STATE(state, m, s)                  \
>>> +    ((((uint32_t)(state) & (uint32_t)(m))) >> (s))
>>> +
>>> +#define ENCODE_DRC_STATE(val, m, s) \
>>> +    (((uint32_t)(val) << (s)) & (uint32_t)(m))
>>> +
>>>  static sPAPRPHBState *find_phb(sPAPREnvironment *spapr, uint64_t buid)
>>>  {
>>>      sPAPRPHBState *sphb;
>>> @@ -402,6 +437,80 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>>>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>>>  }
>>>  
>>> +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 indicator = rtas_ld(args, 0);
>>> +    uint32_t drc_index = rtas_ld(args, 1);
>>> +    uint32_t indicator_state = rtas_ld(args, 2);
>>> +    uint32_t encoded = 0, shift = 0, mask = 0;
>>> +    uint32_t *pind;
>>> +    sPAPRDrcEntry *drc_entry = NULL;
>>
>> This rtas call does not have any idea what a PHB is. Why does it live in
>> spapr_pci.c?
> 
> spapr_rtas.c does seem like a better home
> 
>>
>>> +
>>> +    if (drc_index == 0) { /* platform indicator */
>>> +        pind = &spapr->state;
>>> +    } else {
>>> +        drc_entry = spapr_find_drc_entry(drc_index);
>>> +        if (!drc_entry) {
>>> +            DPRINTF("rtas_set_indicator: unable to find drc_entry for %x",
>>> +                    drc_index);
>>> +            rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>> +            return;
>>> +        }
>>> +        pind = &drc_entry->state;
>>> +    }
>>> +
>>> +    switch (indicator) {
>>> +    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: /* Allocation State */
>>> +        shift = INDICATOR_ALLOCATION_SHIFT;
>>> +        mask = INDICATOR_ALLOCATION_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:
>>> +        DPRINTF("rtas_set_indicator: indicator not implemented: %d",
>>> +                indicator);
>>> +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>> +        return;
>>> +    }
>>> +
>>> +    encoded = ENCODE_DRC_STATE(indicator_state, mask, shift);
>>> +    /* clear the current indicator value */
>>> +    *pind &= ~mask;
>>> +    /* set the new value */
>>> +    *pind |= encoded;
>>> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>> +}
>>> +
>>>  static int pci_spapr_swizzle(int slot, int pin)
>>>  {
>>>      return (slot + pin) % PCI_NUM_PINS;
>>> @@ -624,6 +733,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>>          sphb->lsi_table[i].irq = irq;
>>>      }
>>>  
>>> +    /* make sure the platform EPOW sensor is initialized - the
>>> +     * guest will probe it when there is a hotplug event.
>>> +     */
>>> +    spapr->state &= ~(uint32_t)INDICATOR_EPOW_MASK;
>>> +    spapr->state |= ENCODE_DRC_STATE(0,
>>> +                                     INDICATOR_EPOW_MASK,
>>> +                                     INDICATOR_EPOW_SHIFT);
>>> +
>>>      if (!info->finish_realize) {
>>>          error_setg(errp, "finish_realize not defined");
>>>          return;
>>> @@ -1056,6 +1173,8 @@ void spapr_pci_rtas_init(void)
>>>          spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
>>>                              rtas_ibm_change_msi);
>>>      }
>>> +    spapr_rtas_register(RTAS_SET_INDICATOR, "set-indicator",
>>> +                        rtas_set_indicator);
>>>  }
>>>  
>>>  static void spapr_pci_register_types(void)
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index 0ac1a19..fac85f8 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -72,6 +72,9 @@ typedef struct sPAPREnvironment {
>>>  
>>>      /* state for Dynamic Reconfiguration Connectors */
>>>      sPAPRDrcEntry drc_table[SPAPR_DRC_TABLE_SIZE];
>>> +
>>> +    /* Platform state - sensors and indicators */
>>> +    uint32_t state;
>>
>> Do you think it'd be possible to create a special DRC device that
>> contains all of its tables and global state and also exposes sensors and
>> indicators? That device could then get linked via qom links to the PHBs
>> for their slots.
> 
> Sorry for the delay, I've been going back through the code with this
> suggestion in mind and there does seem to be a lot of state that
> can be nicely encapsulated by modeling the DR Connectors as a QOM
> "device" (though I haven't gone as far as to make them actual
> DeviceState's since it's more of a firmware abstraction than real
> hardware)
> 
> I'm not sure what the best way to plumb things together is, as a first
> run, since each DRC must have a index drc_index as per spec, I've moved
> put them under /machine/DRConnector as a flat list, where top-level
> PHB/CPU/MEMORY DRCs would be allocated statically during sPAPR machine
> init (since the corresponding DRC indexes/types/etc are hard-coded into
> the top-level of the boot-time DT anyway, though I guess we could also
> allocate these on the fly...seems messier though than just plugging new
> resources into existing DRCs)
> 
> PHB's in turn will associate themselves with a DRC via an attach/detach
> method as part of realize (and in the future, hotplug hooks, though
> that's not part of the series). The PHBs in turn will create a DRC for each
> hotpluggable PCI slot.
> 
> Creation is via:
> 
> sPAPRDRConnector *spapr_dr_connector_new(sPAPRDRConnectorType type,
>                                          uint32_t id);
> 
> where the code computes the drc index based on <type> (one of phb, cpu, pci,
> memory, etc) and <id>, and sticks them under /machine/dr-Connector/<drc_index>
> 
> Any pci/phb/cpu hotplug hooks can then fetch the DRC via type/id,
> and hotplug/unplug via attach()/detach() methods. attach() adds
> the attached/hotplugged DeviceState as a link property of the
> DRC object, and sets the initial sensor state.
> 
> rtas calls can fetch DRCs via drc_index, and set/get sensor state
> via DRC sensor get/set methods.
> 
> Hotplug event delivery still lives outside of DRC implementation for now. I
> thought of moving them into DRC, but decisions like whether we should
> emit events during coldplug/initial boot seemed to require pushing
> a lot of general machine state into DRCs and making the encapsulation
> seem superficial.
> 
> Things end up looking like this (2xxxxxxx are PHBs, 4xxxxxxx are PCI slots):
> 
> mdroth@loki:~/w/qom/machine/dr-connector$ ls
> 20000000  40000018  40000038  40000058  40000078  40000098  400000b8  400000d8  400000f8
> 40000000  40000020  40000040  40000060  40000080  400000a0  400000c0  400000e0  type
> 40000008  40000028  40000048  40000068  40000088  400000a8  400000c8  400000e8
> 40000010  40000030  40000050  40000070  40000090  400000b0  400000d0  400000f0
> mdroth@loki:~/w/qom/machine/dr-connector$ cd 40000000/
> mdroth@loki:~/w/qom/machine/dr-connector/40000000$ ls -l
> total 0
> -rw-r--r-- 1 mdroth mdroth 4096 Dec 31  1969 allocation-state
> lrwxr-xr-x 2 mdroth mdroth 4096 Dec 31  1969 device -> ../../../machine/peripheral/hp0
> -rw-r--r-- 1 mdroth mdroth 4096 Dec 31  1969 drc-index
> -rw-r--r-- 1 mdroth mdroth 4096 Dec 31  1969 entity-sense
> -rw-r--r-- 1 mdroth mdroth 4096 Dec 31  1969 indicator-state
> -rw-r--r-- 1 mdroth mdroth 4096 Dec 31  1969 isolation-state
> -rw-r--r-- 1 mdroth mdroth 4096 Dec 31  1969 type
> mdroth@loki:~/w/qom/machine/dr-connector/40000000$ cat allocation-state 
> 1
> mdroth@loki:~/w/qom/machine/dr-connector/40000000$ cat indicator-state 
> 1
> mdroth@loki:~/w/qom/machine/dr-connector/40000000$ cat ../../../machine/peripheral/hp0/type 
> virtio-net-pci
> mdroth@loki:~/w/qom/machine/dr-connector/40000000$
> 
> Hopefully this is sort of the approach you were thinking of?

This look quite neat so far, looking forward to the patches :).


Alex