Re: [Qemu-devel] [PATCH v4 10/17] spapr_drc: add spapr_drc_populate_dt()

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

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

On Tue, Dec 23, 2014 at 06:30:24AM -0600, Michael Roth wrote:
> This function handles generation of ibm,drc-* array device tree
> properties to describe DRC topology to guests. This will by used
> by the guest to direct RTAS calls to manage any dynamic resources
> we associate with a particular DR Connector as part of
> hotplug/unplug.
> 
> Since general management of boot-time device trees are handled
> outside of sPAPRDRConnector, we insert these values blindly given
> an FDT and offset. A mask of sPAPRDRConnector types is given to
> instruct us on what types of connectors entries should be generated
> for, since descriptions for different connectors may live in
> different parts of the device tree.
> 
> Based on code originally written by Nathan Fontenot.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_drc.c         | 225 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_drc.h |   3 +-
>  2 files changed, 227 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index f81c6d1..b162184 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -501,3 +501,228 @@ sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
>              (get_type_shift(type) << DRC_INDEX_TYPE_SHIFT) |
>              (id & DRC_INDEX_ID_MASK));
>  }
> +
> +/* internal helper to gather up DRC info specific to populating DRC
> + * topology information in the device tree.
> + */
> +typedef struct DRConnectorDTInfo {
> +    char drc_type[64];
> +    char drc_name[64];
> +    uint32_t drc_index;
> +    uint32_t drc_power_domain;
> +} DRConnectorDTInfo;
> +
> +/* generate a string the describes the DRC to encode into the
> + * device tree.
> + *
> + * as documented by PAPR+ v2.7, 13.5.2.6 and C.6.1
> + */
> +static void spapr_drc_get_type_str(char *buf, sPAPRDRConnectorType type)
> +{
> +    switch (type) {
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        sprintf(buf, "CPU");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> +        sprintf(buf, "PHB");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> +        sprintf(buf, "SLOT");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> +        sprintf(buf, "28");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> +        sprintf(buf, "MEM");
> +        break;
> +    default:
> +        g_assert(false);
> +    }

So this case is simple enough that you can probably get away with it,
but still - interfaces that involve writing to a buffer without any
length checks make me very nervous.

> +}
> +
> +/* generate a human-readable name for a DRC to encode into the DT
> + * description. this is mainly only used within a guest in place
> + * of the unique DRC index.
> + *
> + * in the case of VIO/PCI devices, it corresponds to a
> + * "location code" that maps a logical device/function (DRC index)
> + * to a physical (or virtual in the case of VIO) location in the
> + * system by chaining together the "location label" for each
> + * encapsulating component.
> + *
> + * since this is more to do with diagnosing physical hardware
> + * issues than guest compatibility, we choose location codes/DRC
> + * names that adhere to the documented format, but avoid encoding
> + * the entire topology information into the label/code, instead
> + * just using the location codes based on the labels for the
> + * endpoints (VIO/PCI adaptor connectors), which is basically
> + * just "C" followed by an integer ID.

Hrm.. would it make sense to include here the qemu "id" value on the
DRC device?  That will make names which are matchable to specific
elements on the qemu command line, which about as close an equivalent
to a physical location as I can think of.

> + * DRC names as documented by PAPR+ v2.7, 13.5.2.4
> + * location codes as documented by PAPR+ v2.7, 12.3.1.5
> + */
> +static void spapr_drc_get_name_str(char *buf,
> +                                   sPAPRDRConnectorType type,
> +                                   uint32_t drc_index)
> +{
> +    uint32_t id = drc_index & DRC_INDEX_ID_MASK;
> +
> +    switch (type) {
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        sprintf(buf, "CPU %d", id);
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> +        sprintf(buf, "PHB %d", id);
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> +        sprintf(buf, "C%d", id);
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> +        sprintf(buf, "LMB %d", id);
> +        break;
> +    default:
> +        g_assert(false);
> +    }
> +}
> +
> +static DRConnectorDTInfo *spapr_dr_connector_get_info(uint32_t drc_type_mask,
> +                                                      unsigned int *count)
> +{
> +    Object *root_container;
> +    ObjectProperty *prop;
> +    GArray *drc_info_list = g_array_new(false, true,
> +                                        sizeof(DRConnectorDTInfo));
> +
> +    /* aliases for all DRConnector objects will be rooted in QOM
> +     * composition tree at /dr-connector
> +     */
> +    root_container = container_get(object_get_root(), "/dr-connector");
> +
> +    QTAILQ_FOREACH(prop, &root_container->properties, node) {
> +        Object *obj;
> +        sPAPRDRConnector *drc;
> +        sPAPRDRConnectorClass *drck;
> +        DRConnectorDTInfo drc_info;
> +
> +        if (!strstart(prop->type, "link<", NULL)) {
> +            continue;
> +        }
> +
> +        obj = object_property_get_link(root_container, prop->name, NULL);
> +        drc = SPAPR_DR_CONNECTOR(obj);
> +        drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> +        if ((drc->type & drc_type_mask) == 0) {
> +            continue;
> +        }
> +
> +        drc_info.drc_index = drck->get_index(drc);
> +        drc_info.drc_power_domain = -1;
> +        spapr_drc_get_type_str(drc_info.drc_type, drc->type);
> +        spapr_drc_get_name_str(drc_info.drc_name, drc->type,
> +                               drck->get_index(drc));
> +        g_array_append_val(drc_info_list, drc_info);
> +    }
> +
> +    if (count) {
> +        *count = drc_info_list->len;
> +    }
> +
> +    /* if count is zero, free everything, including internal storage
> +     * for array
> +     */
> +    return (DRConnectorDTInfo *)g_array_free(drc_info_list, count == 0);
> +}
> +
> +/**
> + * spapr_drc_populate_dt
> + *
> + * @fdt: libfdt device tree
> + * @path: path in the DT to generate properties
> + * @drc_type_mask: mask of sPAPRDRConnectorType values corresponding
> + *   to the types of DRCs to generate entries for
> + *
> + * generate OF properties to describe DRC topology/indices to guests
> + *
> + * as documented in PAPR+ v2.1, 13.5.2
> + */
> +int spapr_drc_populate_dt(void *fdt, int fdt_offset, uint32_t drc_type_mask)
> +{
> +    DRConnectorDTInfo *drc_info_list;
> +    unsigned int i, count;
> +    char *char_buf;
> +    uint32_t *char_buf_count;
> +    uint32_t *int_buf;
> +    int char_buf_offset, ret;
> +
> +    drc_info_list =
> +        spapr_dr_connector_get_info(drc_type_mask, &count);

This is the only call to spapr_dt_connector_get_info().  I don't see a
lot of point in splitting it out from this function, since it involves
a not particular easy to work with array encoding of the information.
Why not go direct from the drc objects to the fdt.

> +    if (!count) {
> +        return 0;
> +    }
> +
> +    int_buf = g_new0(uint32_t, count + 1);
> +    int_buf[0] = cpu_to_be32(count);
> +    char_buf = g_new0(char, count * 128 + sizeof(uint32_t));
> +    char_buf_count = (uint32_t *)&char_buf[0];
> +    *char_buf_count = cpu_to_be32(count);
> +
> +    /* ibm,drc-indexes */
> +    for (i = 0; i < count; i++) {
> +        int_buf[i + 1] = cpu_to_be32(drc_info_list[i].drc_index);
> +    }
> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-indexes", int_buf,
> +                      (count + 1) * sizeof(uint32_t));
> +    if (ret) {
> +        fprintf(stderr, "Couldn't create ibm,drc-indexes property\n");
> +        goto out;
> +    }
> +
> +    /* ibm,drc-power-domains */
> +    for (i = 0; i < count; i++) {
> +        int_buf[i + 1] = cpu_to_be32(drc_info_list[i].drc_power_domain);
> +    }
> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-power-domains", int_buf,
> +                      (count + 1) * sizeof(uint32_t));
> +    if (ret) {
> +        fprintf(stderr, "Couldn't finalize ibm,drc-power-domains property\n");
> +        goto out;
> +    }
> +
> +    /* ibm,drc-names */
> +    char_buf_offset = sizeof(uint32_t);
> +
> +    for (i = 0; i < count; i++) {
> +        strcpy(char_buf + char_buf_offset, drc_info_list[i].drc_name);
> +        char_buf_offset += strlen(drc_info_list[i].drc_name) + 1;
> +    }
> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-names", char_buf,
> +                      char_buf_offset);
> +    if (ret) {
> +        fprintf(stderr, "Couldn't finalize ibm,drc-names property\n");
> +        goto out;
> +    }
> +
> +    /* ibm,drc-types */
> +    char_buf_offset = sizeof(uint32_t);
> +
> +    for (i = 0; i < count; i++) {
> +        strcpy(char_buf + char_buf_offset, drc_info_list[i].drc_type);
> +        char_buf_offset += strlen(drc_info_list[i].drc_type) + 1;
> +    }
> +
> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,drc-types", char_buf,
> +                      char_buf_offset);
> +    if (ret) {
> +        fprintf(stderr, "Couldn't finalize ibm,drc-types property\n");
> +        goto out;
> +    }
> +
> +out:
> +    g_free(int_buf);
> +    g_free(char_buf);
> +    g_free(drc_info_list);
> +    return ret;
> +}
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 63ec687..5c70140 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -193,9 +193,10 @@ typedef struct sPAPRDRConnectorClass {
>  
>  sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>                                           sPAPRDRConnectorType type,
> -                                         uint32_t token);
> +                                         uint32_t id);
>  sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index);
>  sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
>                                             uint32_t id);
> +int spapr_drc_populate_dt(void *fdt, int fdt_offset, uint32_t drc_type_mask);
>  
>  #endif /* __HW_SPAPR_DRC_H__ */

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