Re: [Qemu-devel] [PATCH 5/6] hw/arm/sysbus-fdt: helpers for clock node generation

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger <address@hidden> wrote:
> Some passthrough'ed devices depend on clock nodes. Those need to be
> generated in the guest device tree. This patch introduces some helpers
> to build a clock node from information retrieved in the host device tree.
>
> - inherit_properties copies properties from a host device tree node to
>   a guest device tree node
> - fdt_build_clock_node builds a guest clock node and checks the host
>   fellow clock is a fixed one.
>
> fdt_build_clock_node will become static as soon as it gets used. A
> dummy pre-declaration is needed for compilation of this patch.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> RFC -> v1:
> - use the new proto of qemu_fdt_getprop
> - remove newline in error_report
> - fix some style issues
> ---
>  hw/arm/sysbus-fdt.c | 111 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 9d28797..c2d813b 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -21,6 +21,7 @@
>   *
>   */
>
> +#include <libfdt.h>
>  #include "hw/arm/sysbus-fdt.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/device_tree.h"
> @@ -56,6 +57,116 @@ typedef struct NodeCreationPair {
>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>  } NodeCreationPair;
>
> +/* helpers */
> +
> +struct HostProperty {
> +    const char *name;
> +    bool optional;
> +};
> +
> +typedef struct HostProperty HostProperty;

You can combine the typedef and the struct declaration if you like.

> +
> +/**
> + * inherit_properties
> + *
> + * copies properties listed in an array from host device tree to
> + * guest device tree. If a non optional property is not found, the
> + * function self-asserts. An optional property is ignored if not found
> + * in the host device tree.
> + * @props: array of HostProperty to copy
> + * @nb_props: number of properties in the array
> + * @host_dt: host device tree blob
> + * @guest_dt: guest device tree blob
> + * @node_path: host dt node path where the property is supposed to be
> +              found
> + * @nodename: guest node name the properties should be added to
> + */
> +static void inherit_properties(HostProperty *props, int nb_props,
> +                               void *host_fdt, void *guest_fdt,
> +                               char *node_path, char *nodename)
> +{
> +    int i, prop_len;
> +    const void *r;
> +
> +    for (i = 0; i < nb_props; i++) {
> +        r = qemu_fdt_getprop(host_fdt, node_path,
> +                             props[i].name,
> +                             &prop_len,
> +                             props[i].optional ? NULL : &error_fatal);

We'll get an error here if the host device tree doesn't match
up correctly, right? Is the error message going to be sufficiently
informative to the end user about what's gone wrong? (What does
it end up looking like?)

> +        if (r) {
> +            qemu_fdt_setprop(guest_fdt, nodename,
> +                             props[i].name, r, prop_len);
> +        }
> +    }
> +}
> +
> +/* clock properties whose values are copied/pasted from host */
> +static HostProperty clock_inherited_properties[] = {
> +    {"compatible", 0},
> +    {"#clock-cells", 0},
> +    {"clock-frequency", 1},
> +    {"clock-output-names", 1},
> +};
> +
> +/**
> + * fdt_build_clock_node
> + *
> + * Build a guest clock node, used as a dependency from a passthrough'ed
> + * device. Most information are retrieved from the host clock node.
> + * Also check the host clock is a fixed one.
> + *
> + * @host_fdt: host device tree blob from which info are retrieved
> + * @guest_fdt: guest device tree blob where the clock node is added
> + * @host_phandle: phandle of the clock in host device tree
> + * @guest_phandle: phandle to assign to the guest node
> + */
> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> +                         uint32_t host_phandle,
> +                         uint32_t guest_phandle);
> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> +                         uint32_t host_phandle,
> +                         uint32_t guest_phandle)
> +{
> +    char node_path[256];

Please don't use hardcoded fixed buffer lengths (see previous patch
review comments).

> +    char *nodename;
> +    const void *r;
> +    int ret, prop_len;
> +
> +    ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
> +    if (ret <= 0) {
> +        error_report("not able to locate clock handle %d in host device 
> tree",
> +                     host_phandle);
> +        goto out;
> +    }

If we're going to return to the caller in the error case rather than just
exiting here, I think we should return the error to the caller via an
Error** rather than calling error_report() directly ourselves.

> +    ret = fdt_get_path(host_fdt, ret, node_path, 256);
> +    if (ret < 0) {
> +        error_report("not able to retrieve node path for clock handle %d",
> +                     host_phandle);
> +        goto out;
> +    }
> +
> +    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
> +                         &error_fatal);
> +    if (strcmp(r, "fixed-clock")) {
> +        error_report("clock handle %d is not a fixed clock", host_phandle);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    nodename = strrchr(node_path, '/');
> +    qemu_fdt_add_subnode(guest_fdt, nodename);
> +
> +    inherit_properties(clock_inherited_properties,
> +                       ARRAY_SIZE(clock_inherited_properties),
> +                       host_fdt, guest_fdt,
> +                       node_path, nodename);
> +
> +    qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
> +
> +out:
> +    return ret;
> +}
> +
>  /* Device Specific Code */

thanks
-- PMM