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

[Michael Roth]

Quoting David Gibson (2015-01-18 23:15:28)
> 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 <address@hidden>
> > ---
> >  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.

I'm not sure I understand the suggestion. We do make use of the
drc->id values to generate this, though those don't really
correspond to "id"/DeviceState->id properties as specified on
the command-line. There's currently no plans to create the DRCs via
-device since the IDs are dependent on/chosen by the parent devices in
in this case (DRC IDs for PCI slots inherit/encode parent bus/controller
index for example). Did you have something else in mind?

> 
> > + * 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