Re: [Qemu-devel] [PATCH v8 07/16] spapr_rtas: add ibm, configure-connector RTAS interface

[Michael Roth]

Quoting David Gibson (2015-04-28 02:23:43)
> On Wed, Apr 22, 2015 at 01:28:11AM -0500, Michael Roth wrote:
> > This interface is used to fetch an OF device-tree nodes that describes a
> > newly-attached device to guest. It is called multiple times to walk the
> > device-tree node and fetch individual properties into a 'workarea'/buffer
> > provided by the guest.
> > 
> > The device-tree is generated by QEMU and passed to an sPAPRDRConnector 
> > during
> > the initial hotplug operation, and the state of these RTAS calls is tracked 
> > by
> > the sPAPRDRConnector. When the last of these properties is successfully
> > fetched, we report as special return value to the guest and transition
> > the device to a 'configured' state on the QEMU/DRC side.
> > 
> > See docs/specs/ppc-spapr-hotplug.txt for a complete description of
> > this interface.
> > 
> > Signed-off-by: Michael Roth <address@hidden>
> 
> Apart from some details noted below,
> 
> Reviewed-by: David Gibson <address@hidden>
> 
> > ---
> >  hw/ppc/spapr.c         |   4 ++
> >  hw/ppc/spapr_rtas.c    | 179 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  14 ++++
> >  3 files changed, 197 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7febff7..6b68ebc 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1650,6 +1650,10 @@ static void ppc_spapr_init(MachineState *machine)
> >                                              boot_device, kernel_cmdline,
> >                                              spapr->epow_irq);
> >      assert(spapr->fdt_skel != NULL);
> > +
> > +    /* used by RTAS */
> > +    QTAILQ_INIT(&spapr->ccs_list);
> > +    qemu_register_reset(spapr_ccs_reset_hook, spapr);
> 
> It seems kind of awkward to have this list in spapr, rather than just
> having a pointer to the ccs in the drc, but whatever.

Well, it ended up being a bit wonky storing it in the DRC, since we end up
with the issue in v7 with needing to register callbacks for the ccs for
things like reset. But it may be doable to just have a way to
register/fetch the ccs opaque from the DRC instead of having a list in
spapr. The current version makes the separation between CCS/DRC clearer
though, so if you're ok with this approach I think I'd rather stick with
this, at least for now.

> 
> >  }
> >  
> >  static int spapr_kvm_type(const char *vm_type)
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index f80beb2..874380d 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -47,6 +47,43 @@
> >      do { } while (0)
> >  #endif
> >  
> > +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPREnvironment 
> > *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(sPAPREnvironment *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(sPAPREnvironment *spapr,
> > +                             sPAPRConfigureConnectorState *ccs)
> > +{
> > +    QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> > +    g_free(ccs);
> > +}
> > +
> > +void spapr_ccs_reset_hook(void *opaque)
> > +{
> > +    sPAPREnvironment *spapr = opaque;
> > +    sPAPRConfigureConnectorState *ccs, *ccs_tmp;
> > +
> > +    QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) {
> > +        spapr_ccs_remove(spapr, ccs);
> > +    }
> > +}
> >  
> >  static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment 
> > *spapr,
> >                                     uint32_t token, uint32_t nargs,
> > @@ -355,6 +392,18 @@ static void rtas_set_indicator(PowerPCCPU *cpu, 
> > sPAPREnvironment *spapr,
> >  
> >      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);
> > +            }
> > +        }
> >          drck->set_isolation_state(drc, sensor_state);
> >          break;
> >      case RTAS_SENSOR_TYPE_DR:
> > @@ -418,6 +467,134 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu, 
> > sPAPREnvironment *spapr,
> >      rtas_st(rets, 1, entity_sense);
> >  }
> >  
> > +/* 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 rtas_ibm_configure_connector(PowerPCCPU *cpu,
> > +                                         sPAPREnvironment *spapr,
> > +                                         uint32_t token, uint32_t nargs,
> > +                                         target_ulong args, uint32_t nret,
> > +                                         target_ulong rets)
> > +{
> > +    uint64_t wa_addr;
> > +    uint64_t wa_offset;
> > +    uint32_t drc_index;
> > +    sPAPRDRConnector *drc;
> > +    sPAPRDRConnectorClass *drck;
> > +    sPAPRConfigureConnectorState *ccs;
> > +    sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE;
> > +    int rc;
> > +    const void *fdt;
> > +
> > +    if (nargs != 2 || nret != 1) {
> > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +        return;
> > +    }
> > +
> > +    wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
> > +
> > +    drc_index = rtas_ld(wa_addr, 0);
> > +    drc = spapr_dr_connector_by_index(drc_index);
> > +    if (!drc) {
> > +        DPRINTF("rtas_ibm_configure_connector: invalid DRC index: %xh\n",
> > +                drc_index);
> > +        rc = RTAS_OUT_PARAM_ERROR;
> > +        goto out;
> > +    }
> > +
> > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +    fdt = drck->get_fdt(drc, NULL);
> > +
> > +    ccs = spapr_ccs_find(spapr, drc_index);
> > +    if (!ccs) {
> > +        ccs = g_new0(sPAPRConfigureConnectorState, 1);
> > +        (void)drck->get_fdt(drc, &ccs->fdt_offset);
> 
> So, I think we can avoid the awkwardness with getting fdt_offset out
> by making the DT fragment have the relevant node at the root, instead
> of as an immediate subnode of the root.  But that's a cleanup we can
> probably do later.

Yah, for PHB hotplug for instance I ended up re-using spapr_populate_pci_dt,
which add PHB dt info as a subnode. That could be re-worked a bit of course,
but maybe sticking with the more general version we have currently is safer
for now.

> 
> Alternatively fdt_first_subnode() could be used to find the starting offset.
> 
> > +        ccs->drc_index = drc_index;
> > +        spapr_ccs_add(spapr, ccs);
> > +    }
> > +
> > +    do {
> > +        uint32_t tag;
> > +        const char *name;
> > +        const struct fdt_property *prop;
> > +        int fdt_offset_next, prop_len;
> > +
> > +        tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next);
> > +
> > +        switch (tag) {
> > +        case FDT_BEGIN_NODE:
> > +            ccs->fdt_depth++;
> > +            name = fdt_get_name(fdt, ccs->fdt_offset, NULL);
> > +
> > +            /* provide the name of the next OF node */
> > +            wa_offset = CC_VAL_DATA_OFFSET;
> > +            rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
> > +            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - 
> > wa_offset,
> > +                                  (uint8_t *)name, strlen(name) + 1);
> > +            resp = SPAPR_DR_CC_RESPONSE_NEXT_CHILD;
> > +            break;
> > +        case FDT_END_NODE:
> > +            ccs->fdt_depth--;
> > +            if (ccs->fdt_depth == 0) {
> > +                /* done sending the device tree, don't need to track
> > +                 * the state anymore
> > +                 */
> > +                drck->set_configured(drc);
> > +                spapr_ccs_remove(spapr, ccs);
> > +                ccs = NULL;
> > +                resp = SPAPR_DR_CC_RESPONSE_SUCCESS;
> > +            } else {
> > +                resp = SPAPR_DR_CC_RESPONSE_PREV_PARENT;
> > +            }
> > +            break;
> > +        case FDT_PROP:
> > +            prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset,
> > +                                              &prop_len);
> > +            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> > +
> > +            /* provide the name of the next OF property */
> > +            wa_offset = CC_VAL_DATA_OFFSET;
> > +            rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
> > +            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - 
> > wa_offset,
> > +                                  (uint8_t *)name, strlen(name) + 1);
> > +
> > +            /* provide the length and value of the OF property. data gets
> > +             * placed immediately after NULL terminator of the OF 
> > property's
> > +             * name string
> > +             */
> > +            wa_offset += strlen(name) + 1,
> > +            rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
> > +            rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
> > +            rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - 
> > wa_offset,
> > +                                  (uint8_t *)((struct fdt_property 
> > *)prop)->data,
> > +                                  prop_len);
> > +            resp = SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY;
> > +            break;
> > +        case FDT_END:
> > +            resp = SPAPR_DR_CC_RESPONSE_ERROR;
> > +        default:
> > +            /* keep seeking for an actionable tag */
> > +            break;
> > +        }
> > +        if (ccs) {
> > +            ccs->fdt_offset = fdt_offset_next;
> > +        }
> > +    } while (resp == SPAPR_DR_CC_RESPONSE_CONTINUE);
> > +
> > +    rc = resp;
> > +out:
> > +    rtas_st(rets, 0, rc);
> > +}
> > +
> >  static struct rtas_call {
> >      const char *name;
> >      spapr_rtas_fn fn;
> > @@ -551,6 +728,8 @@ static void core_rtas_register_types(void)
> >                          rtas_set_indicator);
> >      spapr_rtas_register(RTAS_GET_SENSOR_STATE, "get-sensor-state",
> >                          rtas_get_sensor_state);
> > +    spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, 
> > "ibm,configure-connector",
> > +                        rtas_ibm_configure_connector);
> >  }
> >  
> >  type_init(core_rtas_register_types)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index f046a89..b021ead 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -7,6 +7,7 @@
> >  struct VIOsPAPRBus;
> >  struct sPAPRPHBState;
> >  struct sPAPRNVRAM;
> > +typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
> >  
> >  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >  
> > @@ -39,6 +40,9 @@ typedef struct sPAPREnvironment {
> >      bool htab_first_pass;
> >      int htab_fd;
> >      bool htab_fd_stale;
> > +
> > +    /* RTAS state */
> > +    QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
> >  } sPAPREnvironment;
> >  
> >  #define H_SUCCESS         0
> > @@ -544,6 +548,16 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const 
> > char *propname,
> >                        sPAPRTCETable *tcet);
> >  void spapr_pci_switch_vga(bool big_endian);
> >  
> > +/* rtas-configure-connector state */
> > +struct sPAPRConfigureConnectorState {
> > +    uint32_t drc_index;
> > +    int fdt_offset;
> > +    int fdt_depth;
> > +    QTAILQ_ENTRY(sPAPRConfigureConnectorState) next;
> > +};
> > +
> > +void spapr_ccs_reset_hook(void *opaque);
> > +
> >  #define TYPE_SPAPR_RTC "spapr-rtc"
> >  
> >  void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
> 
> -- 
> 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