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

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

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

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 <mdroth@linux.vnet.ibm.com>

Apart from some details noted below,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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.

>  }
>  
>  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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]