Re: [Qemu-devel] [PATCH v6 10/15] spapr_drc: add spapr_drc_populate_dt()

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

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

On Thu, Feb 26, 2015 at 09:11:10PM -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: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

This makes me wonder if I should implement an
fdt_setprop_inplace_partial() to avoid fdt creators needing to mess
about with temporary allocations like this does.  But that's a project
for another day.

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

> ---
>  hw/ppc/spapr_drc.c         | 165 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_drc.h |   2 +
>  2 files changed, 167 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index bf22e1d..2a3195c 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -583,3 +583,168 @@ sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type,
>              (get_type_shift(type) << DRC_INDEX_TYPE_SHIFT) |
>              (id & DRC_INDEX_ID_MASK));
>  }
> +
> +/* 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 char *spapr_drc_get_type_str(sPAPRDRConnectorType type)
> +{
> +    char *type_str = NULL;
> +
> +    switch (type) {
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        type_str = g_strdup_printf("CPU");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> +        type_str = g_strdup_printf("PHB");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> +        type_str = g_strdup_printf("SLOT");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> +        type_str = g_strdup_printf("28");
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> +        type_str = g_strdup_printf("MEM");

So, at present this always returns a string literal, which means you
could avoid the strdup()s and the free in the caller.  That would
break down if you ever need to return something computed here though.

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