[Qemu-devel] [PATCH v2 0/7] AMD XGBE KVM platform passthrough

[Qemu-devel] [PATCH v2 0/7] AMD XGBE KVM platform passthrough

[Eric Auger]

This series allows to set up AMD XGBE passthrough. This was tested on AMD
Seattle.

The first upstreamed device supporting KVM platform passthrough was the
Calxeda Midway XGMAC. Compared to this latter, the XGBE XGMAC exposes a
much more complex device tree node.

- First There are 2 device tree node formats:
one where XGBE and PHY are described in separate nodes and another one
that combines both description in a single node (only supported by 4.2
onwards kernels). Only the combined description is supported for passthrough,
meaning the host must be >= 4.2 and must feature a device tree with a combined
description. The guest will also be exposed with a combined description,
meaning only >= 4.2 guest are supported. It is not planned to support
separate node representation since assignment of the PHY is less
straigtforward.

- the XGMAC/PHY node depends on 2 clock nodes (DMA and PTP).
The code checks those clocks are fixed to make sure they cannot be
switched off at some point after the native driver gets unbound.

- there are many property values to populate on guest side. Most of them
cannot be hardcoded. That series implements host device tree blob extraction
from the host /proc/device-tree (inspired from dtc implementation)
and retrieve host property values to populate guest dtb.

- the case where the host uses ACPI is not yet covered since there is
  no usable ACPI description for this HW yet.

The patches can be found at
https://git.linaro.org/people/eric.auger/qemu.git/shortlog/refs/heads/v2.5.0-xgbe-v2

Previous version can be found at
https://git.linaro.org/people/eric.auger/qemu.git/shortlog/refs/heads/v2.5.0-xgbe-v1

HISTORY:
v1 -> v2:
- take into account Peter's comments:
  - add CONFIG_LINUX protection
  - improve error handling and messages
  - no fixed size buffer anymore
  - fix read_fstree error handling
- use /proc/device-tree symlink instead of /sys/firmware/devicetree/base
- added hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check
- see individual commits for full details

RFC -> v1:
- no dependency anymore on dtc binary: load_device_tree_from_sysfs is
  self-contained and implements something similar to dtc read_fstree.
- take into account Alex' comments
- remove qemu_fdt_getprop_optional and use error API instead

Best Regards

Eric


Eric Auger (7):
  hw/vfio/platform: amd-xgbe device
  device_tree: introduce load_device_tree_from_sysfs
  device_tree: introduce qemu_fdt_node_path
  device_tree: qemu_fdt_getprop converted to use the error API
  hw/arm/sysbus-fdt: helpers for clock node generation
  hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation
  hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check

 device_tree.c                   | 148 ++++++++++++++++++-
 hw/arm/sysbus-fdt.c             | 306 ++++++++++++++++++++++++++++++++++++++--
 hw/vfio/Makefile.objs           |   1 +
 hw/vfio/amd-xgbe.c              |  55 ++++++++
 include/hw/vfio/vfio-amd-xgbe.h |  51 +++++++
 include/sysemu/device_tree.h    |  32 ++++-
 6 files changed, 577 insertions(+), 16 deletions(-)
 create mode 100644 hw/vfio/amd-xgbe.c
 create mode 100644 include/hw/vfio/vfio-amd-xgbe.h

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/7] hw/vfio/platform: amd-xgbe device

[Eric Auger]

This patch introduces the amd-xgbe VFIO platform device. It
allows the guest to do passthrough on a device exposing an
"amd,xgbe-seattle-v1a" compat string.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Alex Benné<alex.bennee@linaro.org>

---
RFC -> v1:
- add Alex' R-b
---
 hw/vfio/Makefile.objs           |  1 +
 hw/vfio/amd-xgbe.c              | 55 +++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-amd-xgbe.h | 51 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 hw/vfio/amd-xgbe.c
 create mode 100644 include/hw/vfio/vfio-amd-xgbe.h

diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index d324863..ceddbb8 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
 obj-$(CONFIG_PCI) += pci.o pci-quirks.o
 obj-$(CONFIG_SOFTMMU) += platform.o
 obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
+obj-$(CONFIG_SOFTMMU) += amd-xgbe.o
 endif
diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
new file mode 100644
index 0000000..53451eb
--- /dev/null
+++ b/hw/vfio/amd-xgbe.c
@@ -0,0 +1,55 @@
+/*
+ * AMD XGBE VFIO device
+ *
+ * Copyright Linaro Limited, 2015
+ *
+ * Authors:
+ *  Eric Auger <eric.auger@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/vfio/vfio-amd-xgbe.h"
+
+static void amd_xgbe_realize(DeviceState *dev, Error **errp)
+{
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
+    VFIOAmdXgbeDeviceClass *k = VFIO_AMD_XGBE_DEVICE_GET_CLASS(dev);
+
+    vdev->compat = g_strdup("amd,xgbe-seattle-v1a");
+
+    k->parent_realize(dev, errp);
+}
+
+static const VMStateDescription vfio_platform_amd_xgbe_vmstate = {
+    .name = TYPE_VFIO_AMD_XGBE,
+    .unmigratable = 1,
+};
+
+static void vfio_amd_xgbe_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VFIOAmdXgbeDeviceClass *vcxc =
+        VFIO_AMD_XGBE_DEVICE_CLASS(klass);
+    vcxc->parent_realize = dc->realize;
+    dc->realize = amd_xgbe_realize;
+    dc->desc = "VFIO AMD XGBE";
+    dc->vmsd = &vfio_platform_amd_xgbe_vmstate;
+}
+
+static const TypeInfo vfio_amd_xgbe_dev_info = {
+    .name = TYPE_VFIO_AMD_XGBE,
+    .parent = TYPE_VFIO_PLATFORM,
+    .instance_size = sizeof(VFIOAmdXgbeDevice),
+    .class_init = vfio_amd_xgbe_class_init,
+    .class_size = sizeof(VFIOAmdXgbeDeviceClass),
+};
+
+static void register_amd_xgbe_dev_type(void)
+{
+    type_register_static(&vfio_amd_xgbe_dev_info);
+}
+
+type_init(register_amd_xgbe_dev_type)
diff --git a/include/hw/vfio/vfio-amd-xgbe.h b/include/hw/vfio/vfio-amd-xgbe.h
new file mode 100644
index 0000000..9fff65e
--- /dev/null
+++ b/include/hw/vfio/vfio-amd-xgbe.h
@@ -0,0 +1,51 @@
+/*
+ * VFIO AMD XGBE device
+ *
+ * Copyright Linaro Limited, 2015
+ *
+ * Authors:
+ *  Eric Auger <eric.auger@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef HW_VFIO_VFIO_AMD_XGBE_H
+#define HW_VFIO_VFIO_AMD_XGBE_H
+
+#include "hw/vfio/vfio-platform.h"
+
+#define TYPE_VFIO_AMD_XGBE "vfio-amd-xgbe"
+
+/**
+ * This device exposes:
+ * - 5 MMIO regions: MAC, PCS, SerDes Rx/Tx regs,
+     SerDes Integration Registers 1/2 & 2/2
+ * - 2 level sensitive IRQs and optional DMA channel IRQs
+ */
+struct VFIOAmdXgbeDevice {
+    VFIOPlatformDevice vdev;
+};
+
+typedef struct VFIOAmdXgbeDevice VFIOAmdXgbeDevice;
+
+struct VFIOAmdXgbeDeviceClass {
+    /*< private >*/
+    VFIOPlatformDeviceClass parent_class;
+    /*< public >*/
+    DeviceRealize parent_realize;
+};
+
+typedef struct VFIOAmdXgbeDeviceClass VFIOAmdXgbeDeviceClass;
+
+#define VFIO_AMD_XGBE_DEVICE(obj) \
+     OBJECT_CHECK(VFIOAmdXgbeDevice, (obj), TYPE_VFIO_AMD_XGBE)
+#define VFIO_AMD_XGBE_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(VFIOAmdXgbeDeviceClass, (klass), \
+                        TYPE_VFIO_AMD_XGBE)
+#define VFIO_AMD_XGBE_DEVICE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(VFIOAmdXgbeDeviceClass, (obj), \
+                      TYPE_VFIO_AMD_XGBE)
+
+#endif
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/7] device_tree: introduce load_device_tree_from_sysfs

[Eric Auger]

This function returns the host device tree blob from sysfs
(/proc/device-tree). It uses a recursive function inspired
from dtc read_fstree.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v1 -> v2:
- do not implement/expose read_fstree and load_device_tree_from_sysfs
  if CONFIG_LINUX is not defined (lstat is not implemeted in mingw)
- correct indentation in read_fstree
- use /proc/device-tree symlink instead of /sys/firmware/devicetree/base
  path (kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-ofw)
- use g_file_get_contents in read_fstree
- introduce SYSFS_DT_BASEDIR macro and use strlen
- exit on error in load_device_tree_from_sysfs
- user error_setg

RFC -> v1:
- remove runtime dependency on dtc binary and introduce read_fstree
---
 device_tree.c                | 100 +++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h |   3 ++
 2 files changed, 103 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index a9f5f8e..b262c2d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -17,6 +17,9 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdlib.h>
+#ifdef CONFIG_LINUX
+#include <dirent.h>
+#endif
 
 #include "qemu-common.h"
 #include "qemu/error-report.h"
@@ -117,6 +120,103 @@ fail:
     return NULL;
 }
 
+#ifdef CONFIG_LINUX
+
+#define SYSFS_DT_BASEDIR "/proc/device-tree"
+
+/**
+ * read_fstree: this function is inspired from dtc read_fstree
+ * @fdt: preallocated fdt blob buffer, to be populated
+ * @dirname: directory to scan under SYSFS_DT_BASEDIR
+ * the search is recursive and the tree is searched down to the
+ * leafs (property files).
+ *
+ * the function self-asserts in case of error
+ */
+static void read_fstree(void *fdt, const char *dirname)
+{
+    DIR *d;
+    struct dirent *de;
+    struct stat st;
+    const char *root_dir = SYSFS_DT_BASEDIR;
+    char *parent_node;
+
+    if (strstr(dirname, root_dir) != dirname) {
+        error_report("%s: %s must be searched within %s",
+                     __func__, dirname, root_dir);
+        exit(1);
+    }
+    parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];
+
+    d = opendir(dirname);
+    if (!d) {
+        error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
+    }
+
+    while ((de = readdir(d)) != NULL) {
+        char *tmpnam;
+
+        if (!g_strcmp0(de->d_name, ".")
+            || !g_strcmp0(de->d_name, "..")) {
+            continue;
+        }
+
+        tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
+
+        if (lstat(tmpnam, &st) < 0) {
+            error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
+        }
+
+        if (S_ISREG(st.st_mode)) {
+            gchar *val;
+            gsize len;
+
+            if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
+                error_setg(&error_fatal, "%s not able to extract info from %s",
+                           __func__, tmpnam);
+            }
+
+            if (strlen(parent_node) > 0) {
+                qemu_fdt_setprop(fdt, parent_node,
+                                 de->d_name, val, len);
+            } else {
+                qemu_fdt_setprop(fdt, "/", de->d_name, val, len);
+            }
+            g_free(val);
+        } else if (S_ISDIR(st.st_mode)) {
+            char *node_name;
+
+            node_name = g_strdup_printf("%s/%s",
+                                        parent_node, de->d_name);
+            qemu_fdt_add_subnode(fdt, node_name);
+            g_free(node_name);
+            read_fstree(fdt, tmpnam);
+        }
+
+        g_free(tmpnam);
+    }
+
+    closedir(d);
+}
+
+/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
+void *load_device_tree_from_sysfs(void)
+{
+    void *host_fdt;
+    int host_fdt_size;
+
+    host_fdt = create_device_tree(&host_fdt_size);
+    read_fstree(host_fdt, SYSFS_DT_BASEDIR);
+    if (fdt_check_header(host_fdt)) {
+        error_setg(&error_fatal,
+                   "%s host device tree extracted into memory is invalid",
+                   __func__);
+    }
+    return host_fdt;
+}
+
+#endif /* CONFIG_LINUX */
+
 static int findnode_nofail(void *fdt, const char *node_path)
 {
     int offset;
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 359e143..fdf25a4 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -16,6 +16,9 @@
 
 void *create_device_tree(int *sizep);
 void *load_device_tree(const char *filename_path, int *sizep);
+#ifdef CONFIG_LINUX
+void *load_device_tree_from_sysfs(void);
+#endif
 
 int qemu_fdt_setprop(void *fdt, const char *node_path,
                      const char *property, const void *val, int size);
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/7] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

This new helper routine returns the node path of a device
referred to by its node name and compat string.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- move doc comment in header file
- do not use a fixed size buffer
- break on errors in while loop
- use strcmp instead of strncmp

RFC -> v1:
- improve error handling according to Alex' comments
---
 device_tree.c                | 37 +++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h | 14 ++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index b262c2d..8441e01 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -231,6 +231,43 @@ static int findnode_nofail(void *fdt, const char *node_path)
     return offset;
 }
 
+int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+                       char **node_path)
+{
+    int offset, len, ret;
+    const char *iter_name;
+    unsigned int path_len = 16;
+    char *path;
+
+    *node_path = NULL;
+    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
+
+    while (offset >= 0) {
+        iter_name = fdt_get_name(fdt, offset, &len);
+        if (!iter_name) {
+            offset = len;
+            break;
+        }
+        if (!strcmp(iter_name, name)) {
+            goto found;
+        }
+        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
+    }
+    return offset;
+
+found:
+    path = g_malloc(path_len);
+    while ((ret = fdt_get_path(fdt, offset, path, path_len))
+            == -FDT_ERR_NOSPACE) {
+        path_len += 16;
+        path = g_realloc(path, path_len);
+    }
+    if (!ret) {
+        *node_path = path;
+    }
+    return ret;
+}
+
 int qemu_fdt_setprop(void *fdt, const char *node_path,
                      const char *property, const void *val, int size)
 {
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index fdf25a4..269cb1c 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -20,6 +20,20 @@ void *load_device_tree(const char *filename_path, int *sizep);
 void *load_device_tree_from_sysfs(void);
 #endif
 
+/**
+ * qemu_fdt_node_path: return the node path of a device, given its
+ * node name and its compat string
+ * @fdt: pointer to the dt blob
+ * @name: device node name
+ * @compat: compatibility string of the device
+ * @node_path: returned node path
+ *
+ * upon success, the path is output at node_path address
+ * returns 0 on success, < 0 on failure
+ */
+int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+                       char **node_path);
+
 int qemu_fdt_setprop(void *fdt, const char *node_path,
                      const char *property, const void *val, int size);
 int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
-- 
1.9.1

[Qemu-devel] [PATCH v2 4/7] device_tree: qemu_fdt_getprop converted to use the error API

[Eric Auger]

Current qemu_fdt_getprop exits if the property is not found. It is
sometimes needed to read an optional property, in which case we do
not wish to exit but simply returns a null value.

This patch converts qemu_fdt_getprop to accept an Error **, and existing
users are converted to pass &error_fatal. This preserves the existing
behaviour. Then to use the API with your optional semantic a null
parameter can be conveyed.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- add a doc comment in the header file

RFC -> v1:
- get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
  that consists in using the error API

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 device_tree.c                | 11 ++++++-----
 include/sysemu/device_tree.h | 15 ++++++++++++++-
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 8441e01..6ecc9da 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -321,18 +321,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
 }
 
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
-                             const char *property, int *lenp)
+                             const char *property, int *lenp, Error **errp)
 {
     int len;
     const void *r;
+
     if (!lenp) {
         lenp = &len;
     }
     r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
     if (!r) {
-        error_report("%s: Couldn't get %s/%s: %s", __func__,
-                     node_path, property, fdt_strerror(*lenp));
-        exit(1);
+        error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
+                  node_path, property, fdt_strerror(*lenp));
     }
     return r;
 }
@@ -341,7 +341,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
                                const char *property)
 {
     int len;
-    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
+    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
+                                         &error_fatal);
     if (len != 4) {
         error_report("%s: %s/%s not 4 bytes long (not a cell?)",
                      __func__, node_path, property);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 269cb1c..4d7cbb9 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -45,8 +45,21 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
 int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
                              const char *property,
                              const char *target_node_path);
+/**
+ * qemu_fdt_getprop: retrieve the value of a given property
+ * @fdt: pointer to the device tree blob
+ * @node_path: node path
+ * @property: name of the property to find
+ * @lenp: fdt error if any or length of the property on success
+ * @errp: handle to an error object
+ *
+ * returns a pointer to the property on success and NULL on failure
+ * in case errp is set to &error_fatal, the function auto-asserts
+ * on error (legacy behavior)
+ */
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
-                             const char *property, int *lenp);
+                             const char *property, int *lenp,
+                             Error **errp);
 uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
                                const char *property);
 uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
-- 
1.9.1

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

[Eric Auger]

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 <eric.auger@linaro.org>

---

v1 -> v2:
- inherit properties now outputs an error message in case
  qemu_fdt_getprop fails for an existing optional property
- no hardcoded fixed buffer length
- fdt_build_clock_node becomes void and auto-asserts on error
- use boolean values when defining the clock properties

RFC -> v1:
- use the new proto of qemu_fdt_getprop
- remove newline in error_report
- fix some style issues
---
 hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 9d28797..a1cf57b 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,125 @@ typedef struct NodeCreationPair {
     int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
 } NodeCreationPair;
 
+/* helpers */
+
+typedef struct HostProperty {
+    const char *name;
+    bool optional;
+} HostProperty;
+
+/**
+ * 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;
+    Error *err = NULL;
+
+    for (i = 0; i < nb_props; i++) {
+        r = qemu_fdt_getprop(host_fdt, node_path,
+                             props[i].name,
+                             &prop_len,
+                             props[i].optional ? &err : &error_fatal);
+        if (r) {
+            qemu_fdt_setprop(guest_fdt, nodename,
+                             props[i].name, r, prop_len);
+        } else {
+            if (prop_len != -FDT_ERR_NOTFOUND) {
+                /* optional property not returned although property exists */
+                error_report_err(err);
+            } else {
+                error_free(err);
+            }
+        }
+    }
+}
+
+/* clock properties whose values are copied/pasted from host */
+static HostProperty clock_inherited_properties[] = {
+    {"compatible", false},
+    {"#clock-cells", false},
+    {"clock-frequency", true},
+    {"clock-output-names", true},
+};
+
+/**
+ * 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
+ */
+void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                         uint32_t host_phandle,
+                         uint32_t guest_phandle);
+void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                         uint32_t host_phandle,
+                         uint32_t guest_phandle)
+{
+    char *node_path = NULL;
+    char *nodename;
+    const void *r;
+    int ret, node_offset, prop_len, path_len = 16;
+
+    node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
+    if (node_offset <= 0) {
+        error_setg(&error_fatal,
+                   "not able to locate clock handle %d in host device tree",
+                   host_phandle);
+    }
+    node_path = g_malloc(path_len);
+    while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
+            == -FDT_ERR_NOSPACE) {
+        path_len += 16;
+        node_path = g_realloc(node_path, path_len);
+    }
+    if (ret < 0) {
+        error_setg(&error_fatal,
+                   "not able to retrieve node path for clock handle %d",
+                   host_phandle);
+    }
+
+    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
+                         &error_fatal);
+    if (strcmp(r, "fixed-clock")) {
+        error_setg(&error_fatal,
+                   "clock handle %d is not a fixed clock", host_phandle);
+    }
+
+    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);
+
+    g_free(node_path);
+}
+
 /* Device Specific Code */
 
 /**
-- 
1.9.1

[Qemu-devel] [PATCH v2 6/7] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Eric Auger]

This patch allows the instantiation of the vfio-amd-xgbe device
from the QEMU command line (-device vfio-amd-xgbe,host="<device>").

The guest is exposed with a device tree node that combines the description
of both XGBE and PHY (representation supported from 4.2 onwards kernel):
Documentation/devicetree/bindings/net/amd-xgbe.txt.

There are 5 register regions, 6 interrupts including 4 optional
edge-sensitive per-channel interrupts.

Some property values are inherited from host device tree. Host device tree
must feature a combined XGBE/PHY representation (>= 4.2 host kernel).

2 clock nodes (dma and ptp) also are created. It is checked those clocks
are fixed on host side.

AMD XGBE node creation function has a dependency on vfio Linux header and
more generally node creation function for VFIO platform devices only make
sense with CONFIG_LINUX so let's protect this code with #ifdef CONFIG_LINUX.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v1 -> v2:
- add CONFIG_LINUX protection
- improves robustness in sysfs_to_dt_name
- output messages to end-user on misc failures and self-exits:
  error management becomes more violent than before assuming if
  the end-user wants passthrough we must exit if the device cannot
  be instantiated
- fix misc style issues
- remove qemu_fdt_setprop returned value check since it self-asserts

RFC -> v1:
- use qemu_fdt_getprop with Error **
- free substrings in sysfs_to_dt_name
- add some comments related to endianess in add_amd_xgbe_fdt_node
- reword commit message (dtc binary dependency has disappeared)
- check the host device has 5 regions meaning this is a combined
  XGBE/PHY device
---
 hw/arm/sysbus-fdt.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 181 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index a1cf57b..66fa766 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -22,6 +22,10 @@
  */
 
 #include <libfdt.h>
+#include "qemu-common.h"
+#ifdef CONFIG_LINUX
+#include <linux/vfio.h>
+#endif
 #include "hw/arm/sysbus-fdt.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
@@ -29,6 +33,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/vfio/vfio-amd-xgbe.h"
 #include "hw/arm/fdt.h"
 
 /*
@@ -64,6 +69,8 @@ typedef struct HostProperty {
     bool optional;
 } HostProperty;
 
+#ifdef CONFIG_LINUX
+
 /**
  * inherit_properties
  *
@@ -126,12 +133,9 @@ static HostProperty clock_inherited_properties[] = {
  * @host_phandle: phandle of the clock in host device tree
  * @guest_phandle: phandle to assign to the guest node
  */
-void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
-                         uint32_t host_phandle,
-                         uint32_t guest_phandle);
-void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
-                         uint32_t host_phandle,
-                         uint32_t guest_phandle)
+static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                                uint32_t host_phandle,
+                                uint32_t guest_phandle)
 {
     char *node_path = NULL;
     char *nodename;
@@ -176,6 +180,28 @@ void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
     g_free(node_path);
 }
 
+/**
+ * sysfs_to_dt_name: convert the name found in sysfs into the node name
+ * for instance e0900000.xgmac is converted into xgmac@e0900000
+ * @sysfs_name: directory name in sysfs
+ *
+ * returns the device tree name upon success or NULL in case the sysfs name
+ * does not match the expected format
+ */
+static char *sysfs_to_dt_name(const char *sysfs_name)
+{
+    gchar **substrings =  g_strsplit(sysfs_name, ".", 2);
+    char *dt_name = NULL;
+
+    if (!substrings || !substrings[1] || !substrings[0]) {
+        goto out;
+    }
+    dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
+out:
+    g_strfreev(substrings);
+    return dt_name;
+}
+
 /* Device Specific Code */
 
 /**
@@ -243,9 +269,158 @@ fail_reg:
     return ret;
 }
 
+
+/* AMD xgbe properties whose values are copied/pasted from host */
+static HostProperty amd_xgbe_inherited_properties[] = {
+    {"compatible", false},
+    {"dma-coherent", true},
+    {"amd,per-channel-interrupt", true},
+    {"phy-mode", false},
+    {"mac-address", true},
+    {"amd,speed-set", false},
+    {"amd,serdes-blwc", true},
+    {"amd,serdes-cdr-rate", true},
+    {"amd,serdes-pq-skew", true},
+    {"amd,serdes-tx-amp", true},
+    {"amd,serdes-dfe-tap-config", true},
+    {"amd,serdes-dfe-tap-enable", true},
+    {"clock-names", false},
+};
+
+/**
+ * add_amd_xgbe_fdt_node
+ *
+ * Generates the combined xgbe/phy node following kernel >=4.2
+ * binding documentation:
+ * Documentation/devicetree/bindings/net/amd-xgbe.txt:
+ * Also 2 clock nodes are created (dma and ptp)
+ */
+static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformBusFDTData *data = opaque;
+    PlatformBusDevice *pbus = data->pbus;
+    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
+    VFIOINTp *intp;
+    const char *parent_node = data->pbus_node_name;
+    char *node_path, *nodename, *dt_name;
+    void *guest_fdt = data->fdt, *host_fdt;
+    const void *r;
+    int i, ret = -1, prop_len;
+    uint32_t *irq_attr, *reg_attr, *host_clock_phandles;
+    uint64_t mmio_base, irq_number;
+    uint32_t guest_clock_phandles[2];
+
+    host_fdt = load_device_tree_from_sysfs();
+
+    dt_name = sysfs_to_dt_name(vbasedev->name);
+    if (!dt_name) {
+        error_setg(&error_fatal, "%s incorrect sysfs device name %s",
+                    __func__, vbasedev->name);
+    }
+    ret = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat, &node_path);
+    g_free(dt_name);
+
+    if (ret) {
+        error_setg(&error_fatal, "%s unable to retrieve node path for %s",
+                   __func__, dt_name);
+    }
+
+    if (vbasedev->num_regions != 5) {
+        error_setg(&error_fatal, "%s Does the host dt node combine XGBE/PHY?",
+                   __func__);
+    }
+
+    /* generate nodes for DMA_CLK and PTP_CLK */
+    r = qemu_fdt_getprop(host_fdt, node_path, "clocks",
+                         &prop_len, &error_fatal);
+    if (prop_len != 8) {
+        error_setg(&error_fatal, "%s clocks property should contain 2 handles",
+                   __func__);
+    }
+    host_clock_phandles = (uint32_t *)r;
+    guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
+    guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
+
+    /**
+     * clock handles fetched from host dt are in be32 layout whereas
+     * rest of the code uses cpu layout. Also guest clock handles are
+     * in cpu layout.
+     */
+    fdt_build_clock_node(host_fdt, guest_fdt,
+                         be32_to_cpu(host_clock_phandles[0]),
+                         guest_clock_phandles[0]);
+
+    fdt_build_clock_node(host_fdt, guest_fdt,
+                         be32_to_cpu(host_clock_phandles[1]),
+                         guest_clock_phandles[1]);
+
+    /* combined XGBE/PHY node */
+    mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+    nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
+                               vbasedev->name, mmio_base);
+    qemu_fdt_add_subnode(guest_fdt, nodename);
+
+    inherit_properties(amd_xgbe_inherited_properties,
+                       ARRAY_SIZE(amd_xgbe_inherited_properties),
+                       host_fdt, guest_fdt,
+                       node_path, nodename);
+
+    qemu_fdt_setprop_cells(guest_fdt, nodename, "clocks",
+                           guest_clock_phandles[0],
+                           guest_clock_phandles[1]);
+
+    reg_attr = g_new(uint32_t, vbasedev->num_regions * 2);
+    for (i = 0; i < vbasedev->num_regions; i++) {
+        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
+        reg_attr[2 * i] = cpu_to_be32(mmio_base);
+        reg_attr[2 * i + 1] = cpu_to_be32(
+                                memory_region_size(&vdev->regions[i]->mem));
+    }
+    qemu_fdt_setprop(guest_fdt, nodename, "reg", reg_attr,
+                     vbasedev->num_regions * 2 * sizeof(uint32_t));
+
+    irq_attr = g_new(uint32_t, vbasedev->num_irqs * 3);
+    for (i = 0; i < vbasedev->num_irqs; i++) {
+        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
+                         + data->irq_start;
+        irq_attr[3 * i] = cpu_to_be32(GIC_FDT_IRQ_TYPE_SPI);
+        irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
+        /*
+          * General device interrupt and PCS auto-negociation interrupts are
+          * level-sensitive while the 4 per-channel interrupts are edge
+          * sensitive
+          */
+        QLIST_FOREACH(intp, &vdev->intp_list, next) {
+            if (intp->pin == i) {
+                break;
+            }
+        }
+        if (intp->flags & VFIO_IRQ_INFO_AUTOMASKED) {
+            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+        } else {
+            irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+        }
+    }
+    qemu_fdt_setprop(guest_fdt, nodename, "interrupts",
+                     irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
+
+    g_free(host_fdt);
+    g_free(node_path);
+    g_free(irq_attr);
+    g_free(reg_attr);
+    g_free(nodename);
+    return ret;
+}
+
+#endif /* CONFIG_LINUX */
+
 /* list of supported dynamic sysbus devices */
 static const NodeCreationPair add_fdt_node_functions[] = {
+#ifdef CONFIG_LINUX
     {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
+    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
+#endif
     {"", NULL}, /* last element */
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 7/7] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check

[Eric Auger]

qemu_fdt_setprop self-exists in case of error hence no need to check
the returned value.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/arm/sysbus-fdt.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 66fa766..68d7e53 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -241,12 +241,8 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
         reg_attr[2 * i + 1] = cpu_to_be32(
                                 memory_region_size(&vdev->regions[i]->mem));
     }
-    ret = qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
-                           vbasedev->num_regions * 2 * sizeof(uint32_t));
-    if (ret) {
-        error_report("could not set reg property of node %s", nodename);
-        goto fail_reg;
-    }
+    qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
+                     vbasedev->num_regions * 2 * sizeof(uint32_t));
 
     irq_attr = g_new(uint32_t, vbasedev->num_irqs * 3);
     for (i = 0; i < vbasedev->num_irqs; i++) {
@@ -256,14 +252,9 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
         irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
         irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
     }
-    ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
+    qemu_fdt_setprop(fdt, nodename, "interrupts",
                      irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
-    if (ret) {
-        error_report("could not set interrupts property of node %s",
-                     nodename);
-    }
     g_free(irq_attr);
-fail_reg:
     g_free(reg_attr);
     g_free(nodename);
     return ret;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 4/7] device_tree: qemu_fdt_getprop converted to use the error API

[Peter Crosthwaite]

On Wed, Jan 6, 2016 at 7:13 AM, Eric Auger <eric.auger@linaro.org> wrote:
> Current qemu_fdt_getprop exits if the property is not found. It is
> sometimes needed to read an optional property, in which case we do
> not wish to exit but simply returns a null value.
>
> This patch converts qemu_fdt_getprop to accept an Error **, and existing
> users are converted to pass &error_fatal. This preserves the existing
> behaviour. Then to use the API with your optional semantic a null
> parameter can be conveyed.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> v1 -> v2:
> - add a doc comment in the header file
>
> RFC -> v1:
> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
>   that consists in using the error API
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  device_tree.c                | 11 ++++++-----
>  include/sysemu/device_tree.h | 15 ++++++++++++++-
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 8441e01..6ecc9da 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -321,18 +321,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>  }
>
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
> -                             const char *property, int *lenp)
> +                             const char *property, int *lenp, Error **errp)
>  {
>      int len;
>      const void *r;
> +
>      if (!lenp) {
>          lenp = &len;
>      }
>      r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
>      if (!r) {
> -        error_report("%s: Couldn't get %s/%s: %s", __func__,
> -                     node_path, property, fdt_strerror(*lenp));
> -        exit(1);
> +        error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
> +                  node_path, property, fdt_strerror(*lenp));
>      }
>      return r;
>  }
> @@ -341,7 +341,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>                                 const char *property)
>  {
>      int len;
> -    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
> +    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
> +                                         &error_fatal);

The _cell variant is now inconsistent with the regular _getprop. Can
we convert them both together and fix the clients to error_fatal?
(there are only 4 of them).

This would become the standard error_propagate pattern.

>      if (len != 4) {
>          error_report("%s: %s/%s not 4 bytes long (not a cell?)",
>                       __func__, node_path, property);

And this would also convert to error_setg.

Regards,
Peter

> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 269cb1c..4d7cbb9 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -45,8 +45,21 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>  int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>                               const char *property,
>                               const char *target_node_path);
> +/**
> + * qemu_fdt_getprop: retrieve the value of a given property
> + * @fdt: pointer to the device tree blob
> + * @node_path: node path
> + * @property: name of the property to find
> + * @lenp: fdt error if any or length of the property on success
> + * @errp: handle to an error object
> + *
> + * returns a pointer to the property on success and NULL on failure
> + * in case errp is set to &error_fatal, the function auto-asserts
> + * on error (legacy behavior)
> + */
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
> -                             const char *property, int *lenp);
> +                             const char *property, int *lenp,
> +                             Error **errp);
>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>                                 const char *property);
>  uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
> --
> 1.9.1
>

Re: [Qemu-devel] [PATCH v2 4/7] device_tree: qemu_fdt_getprop converted to use the error API

[Eric Auger]

Hi Peter,
On 01/07/2016 01:20 AM, Peter Crosthwaite wrote:
> On Wed, Jan 6, 2016 at 7:13 AM, Eric Auger <eric.auger@linaro.org> wrote:
>> Current qemu_fdt_getprop exits if the property is not found. It is
>> sometimes needed to read an optional property, in which case we do
>> not wish to exit but simply returns a null value.
>>
>> This patch converts qemu_fdt_getprop to accept an Error **, and existing
>> users are converted to pass &error_fatal. This preserves the existing
>> behaviour. Then to use the API with your optional semantic a null
>> parameter can be conveyed.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - add a doc comment in the header file
>>
>> RFC -> v1:
>> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
>>   that consists in using the error API
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  device_tree.c                | 11 ++++++-----
>>  include/sysemu/device_tree.h | 15 ++++++++++++++-
>>  2 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index 8441e01..6ecc9da 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -321,18 +321,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>>  }
>>
>>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>> -                             const char *property, int *lenp)
>> +                             const char *property, int *lenp, Error **errp)
>>  {
>>      int len;
>>      const void *r;
>> +
>>      if (!lenp) {
>>          lenp = &len;
>>      }
>>      r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
>>      if (!r) {
>> -        error_report("%s: Couldn't get %s/%s: %s", __func__,
>> -                     node_path, property, fdt_strerror(*lenp));
>> -        exit(1);
>> +        error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
>> +                  node_path, property, fdt_strerror(*lenp));
>>      }
>>      return r;
>>  }
>> @@ -341,7 +341,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>>                                 const char *property)
>>  {
>>      int len;
>> -    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
>> +    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
>> +                                         &error_fatal);
> 
> The _cell variant is now inconsistent with the regular _getprop. Can
> we convert them both together and fix the clients to error_fatal?
> (there are only 4 of them).

Yes sure

Best Regards

Eric
> 
> This would become the standard error_propagate pattern.
> 
>>      if (len != 4) {
>>          error_report("%s: %s/%s not 4 bytes long (not a cell?)",
>>                       __func__, node_path, property);
> 
> And this would also convert to error_setg.
> 
> Regards,
> Peter
> 
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index 269cb1c..4d7cbb9 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -45,8 +45,21 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>>  int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>>                               const char *property,
>>                               const char *target_node_path);
>> +/**
>> + * qemu_fdt_getprop: retrieve the value of a given property
>> + * @fdt: pointer to the device tree blob
>> + * @node_path: node path
>> + * @property: name of the property to find
>> + * @lenp: fdt error if any or length of the property on success
>> + * @errp: handle to an error object
>> + *
>> + * returns a pointer to the property on success and NULL on failure
>> + * in case errp is set to &error_fatal, the function auto-asserts
>> + * on error (legacy behavior)
>> + */
>>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>> -                             const char *property, int *lenp);
>> +                             const char *property, int *lenp,
>> +                             Error **errp);
>>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>>                                 const char *property);
>>  uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
>> --
>> 1.9.1
>>

Re: [Qemu-devel] [PATCH v2 3/7] device_tree: introduce qemu_fdt_node_path

[David Gibson]

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

On Wed, Jan 06, 2016 at 03:13:21PM +0000, Eric Auger wrote:
> This new helper routine returns the node path of a device
> referred to by its node name and compat string.

What if there are multiple nodes matching the name and compat?

> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v1 -> v2:
> - move doc comment in header file
> - do not use a fixed size buffer
> - break on errors in while loop
> - use strcmp instead of strncmp
> 
> RFC -> v1:
> - improve error handling according to Alex' comments
> ---
>  device_tree.c                | 37 +++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h | 14 ++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index b262c2d..8441e01 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -231,6 +231,43 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>  
> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                       char **node_path)
> +{
> +    int offset, len, ret;
> +    const char *iter_name;
> +    unsigned int path_len = 16;
> +    char *path;
> +
> +    *node_path = NULL;
> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
> +
> +    while (offset >= 0) {
> +        iter_name = fdt_get_name(fdt, offset, &len);
> +        if (!iter_name) {
> +            offset = len;
> +            break;
> +        }
> +        if (!strcmp(iter_name, name)) {
> +            goto found;
> +        }
> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> +    }
> +    return offset;
> +
> +found:
> +    path = g_malloc(path_len);
> +    while ((ret = fdt_get_path(fdt, offset, path, path_len))
> +            == -FDT_ERR_NOSPACE) {
> +        path_len += 16;
> +        path = g_realloc(path, path_len);
> +    }
> +    if (!ret) {
> +        *node_path = path;
> +    }
> +    return ret;
> +}
> +
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index fdf25a4..269cb1c 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -20,6 +20,20 @@ void *load_device_tree(const char *filename_path, int *sizep);
>  void *load_device_tree_from_sysfs(void);
>  #endif
>  
> +/**
> + * qemu_fdt_node_path: return the node path of a device, given its
> + * node name and its compat string
> + * @fdt: pointer to the dt blob
> + * @name: device node name
> + * @compat: compatibility string of the device
> + * @node_path: returned node path
> + *
> + * upon success, the path is output at node_path address
> + * returns 0 on success, < 0 on failure
> + */
> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                       char **node_path);
> +
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

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

On Wed, Jan 06, 2016 at 03:13:23PM +0000, Eric Auger 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

I dislike the name, since the first thing I think when I see "inherit"
is that it's about a node inheriting a property from an ancestor node,
not the guest inheriting properties from the host.  Maybe
"passthrough_properties()"?

> - 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 <eric.auger@linaro.org>
> 
> ---
> 
> v1 -> v2:
> - inherit properties now outputs an error message in case
>   qemu_fdt_getprop fails for an existing optional property
> - no hardcoded fixed buffer length
> - fdt_build_clock_node becomes void and auto-asserts on error
> - use boolean values when defining the clock properties
> 
> RFC -> v1:
> - use the new proto of qemu_fdt_getprop
> - remove newline in error_report
> - fix some style issues
> ---
>  hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 9d28797..a1cf57b 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,125 @@ typedef struct NodeCreationPair {
>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>  } NodeCreationPair;
>  
> +/* helpers */
> +
> +typedef struct HostProperty {
> +    const char *name;
> +    bool optional;
> +} HostProperty;
> +
> +/**
> + * 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;
> +    Error *err = NULL;
> +
> +    for (i = 0; i < nb_props; i++) {
> +        r = qemu_fdt_getprop(host_fdt, node_path,
> +                             props[i].name,
> +                             &prop_len,
> +                             props[i].optional ? &err : &error_fatal);
> +        if (r) {
> +            qemu_fdt_setprop(guest_fdt, nodename,
> +                             props[i].name, r, prop_len);
> +        } else {
> +            if (prop_len != -FDT_ERR_NOTFOUND) {
> +                /* optional property not returned although property exists */
> +                error_report_err(err);
> +            } else {
> +                error_free(err);
> +            }
> +        }
> +    }
> +}
> +
> +/* clock properties whose values are copied/pasted from host */
> +static HostProperty clock_inherited_properties[] = {
> +    {"compatible", false},
> +    {"#clock-cells", false},
> +    {"clock-frequency", true},
> +    {"clock-output-names", true},
> +};
> +
> +/**
> + * 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
> + */
> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> +                         uint32_t host_phandle,
> +                         uint32_t guest_phandle);
> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> +                         uint32_t host_phandle,
> +                         uint32_t guest_phandle)
> +{
> +    char *node_path = NULL;
> +    char *nodename;
> +    const void *r;
> +    int ret, node_offset, prop_len, path_len = 16;
> +
> +    node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
> +    if (node_offset <= 0) {
> +        error_setg(&error_fatal,
> +                   "not able to locate clock handle %d in host device tree",
> +                   host_phandle);
> +    }
> +    node_path = g_malloc(path_len);
> +    while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
> +            == -FDT_ERR_NOSPACE) {
> +        path_len += 16;
> +        node_path = g_realloc(node_path, path_len);
> +    }
> +    if (ret < 0) {
> +        error_setg(&error_fatal,
> +                   "not able to retrieve node path for clock handle %d",
> +                   host_phandle);
> +    }
> +
> +    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
> +                         &error_fatal);
> +    if (strcmp(r, "fixed-clock")) {
> +        error_setg(&error_fatal,
> +                   "clock handle %d is not a fixed clock", host_phandle);
> +    }
> +
> +    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);
> +
> +    g_free(node_path);
> +}
> +
>  /* Device Specific Code */
>  
>  /**

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 7/7] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check

[David Gibson]

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

On Wed, Jan 06, 2016 at 03:13:25PM +0000, Eric Auger wrote:
> qemu_fdt_setprop self-exists in case of error hence no need to check
> the returned value.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>

This change is fine, but in general I'm disinclined to invest too much
in the qemu interfaces for manipulating flattened trees.

I think our device tree manipulation in qemu is now complicated enough
that we should move towards using an unflattened (i.e. pointer based)
DT representation inside qemu, which is generally more suitable for
complex manipulation.

That would then get flattened into a blob for the guest in a single
pass at reset time.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Eric Auger]

Hi David,
On 01/11/2016 03:41 AM, David Gibson wrote:
> On Wed, Jan 06, 2016 at 03:13:23PM +0000, Eric Auger 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
> 
> I dislike the name, since the first thing I think when I see "inherit"
> is that it's about a node inheriting a property from an ancestor node,
> not the guest inheriting properties from the host.  Maybe
> "passthrough_properties()"?
No Problem, I will rename the function

Best Regards

Eric

> 
>> - 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 <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - inherit properties now outputs an error message in case
>>   qemu_fdt_getprop fails for an existing optional property
>> - no hardcoded fixed buffer length
>> - fdt_build_clock_node becomes void and auto-asserts on error
>> - use boolean values when defining the clock properties
>>
>> RFC -> v1:
>> - use the new proto of qemu_fdt_getprop
>> - remove newline in error_report
>> - fix some style issues
>> ---
>>  hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 120 insertions(+)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 9d28797..a1cf57b 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,125 @@ typedef struct NodeCreationPair {
>>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>  } NodeCreationPair;
>>  
>> +/* helpers */
>> +
>> +typedef struct HostProperty {
>> +    const char *name;
>> +    bool optional;
>> +} HostProperty;
>> +
>> +/**
>> + * 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;
>> +    Error *err = NULL;
>> +
>> +    for (i = 0; i < nb_props; i++) {
>> +        r = qemu_fdt_getprop(host_fdt, node_path,
>> +                             props[i].name,
>> +                             &prop_len,
>> +                             props[i].optional ? &err : &error_fatal);
>> +        if (r) {
>> +            qemu_fdt_setprop(guest_fdt, nodename,
>> +                             props[i].name, r, prop_len);
>> +        } else {
>> +            if (prop_len != -FDT_ERR_NOTFOUND) {
>> +                /* optional property not returned although property exists */
>> +                error_report_err(err);
>> +            } else {
>> +                error_free(err);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +/* clock properties whose values are copied/pasted from host */
>> +static HostProperty clock_inherited_properties[] = {
>> +    {"compatible", false},
>> +    {"#clock-cells", false},
>> +    {"clock-frequency", true},
>> +    {"clock-output-names", true},
>> +};
>> +
>> +/**
>> + * 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
>> + */
>> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +                         uint32_t host_phandle,
>> +                         uint32_t guest_phandle);
>> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +                         uint32_t host_phandle,
>> +                         uint32_t guest_phandle)
>> +{
>> +    char *node_path = NULL;
>> +    char *nodename;
>> +    const void *r;
>> +    int ret, node_offset, prop_len, path_len = 16;
>> +
>> +    node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>> +    if (node_offset <= 0) {
>> +        error_setg(&error_fatal,
>> +                   "not able to locate clock handle %d in host device tree",
>> +                   host_phandle);
>> +    }
>> +    node_path = g_malloc(path_len);
>> +    while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
>> +            == -FDT_ERR_NOSPACE) {
>> +        path_len += 16;
>> +        node_path = g_realloc(node_path, path_len);
>> +    }
>> +    if (ret < 0) {
>> +        error_setg(&error_fatal,
>> +                   "not able to retrieve node path for clock handle %d",
>> +                   host_phandle);
>> +    }
>> +
>> +    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
>> +                         &error_fatal);
>> +    if (strcmp(r, "fixed-clock")) {
>> +        error_setg(&error_fatal,
>> +                   "clock handle %d is not a fixed clock", host_phandle);
>> +    }
>> +
>> +    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);
>> +
>> +    g_free(node_path);
>> +}
>> +
>>  /* Device Specific Code */
>>  
>>  /**
> 

Re: [Qemu-devel] [PATCH v2 3/7] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

Hi David,
On 01/11/2016 03:38 AM, David Gibson wrote:
> On Wed, Jan 06, 2016 at 03:13:21PM +0000, Eric Auger wrote:
>> This new helper routine returns the node path of a device
>> referred to by its node name and compat string.
> 
> What if there are multiple nodes matching the name and compat?
The function would return the first one. I can improve the doc comment.
Do you think it is a problem stopping at the first one? Is it a real
life test case I have to handle here?

Thanks

Eric
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - move doc comment in header file
>> - do not use a fixed size buffer
>> - break on errors in while loop
>> - use strcmp instead of strncmp
>>
>> RFC -> v1:
>> - improve error handling according to Alex' comments
>> ---
>>  device_tree.c                | 37 +++++++++++++++++++++++++++++++++++++
>>  include/sysemu/device_tree.h | 14 ++++++++++++++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index b262c2d..8441e01 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -231,6 +231,43 @@ static int findnode_nofail(void *fdt, const char *node_path)
>>      return offset;
>>  }
>>  
>> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> +                       char **node_path)
>> +{
>> +    int offset, len, ret;
>> +    const char *iter_name;
>> +    unsigned int path_len = 16;
>> +    char *path;
>> +
>> +    *node_path = NULL;
>> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
>> +
>> +    while (offset >= 0) {
>> +        iter_name = fdt_get_name(fdt, offset, &len);
>> +        if (!iter_name) {
>> +            offset = len;
>> +            break;
>> +        }
>> +        if (!strcmp(iter_name, name)) {
>> +            goto found;
>> +        }
>> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
>> +    }
>> +    return offset;
>> +
>> +found:
>> +    path = g_malloc(path_len);
>> +    while ((ret = fdt_get_path(fdt, offset, path, path_len))
>> +            == -FDT_ERR_NOSPACE) {
>> +        path_len += 16;
>> +        path = g_realloc(path, path_len);
>> +    }
>> +    if (!ret) {
>> +        *node_path = path;
>> +    }
>> +    return ret;
>> +}
>> +
>>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>>                       const char *property, const void *val, int size)
>>  {
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index fdf25a4..269cb1c 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -20,6 +20,20 @@ void *load_device_tree(const char *filename_path, int *sizep);
>>  void *load_device_tree_from_sysfs(void);
>>  #endif
>>  
>> +/**
>> + * qemu_fdt_node_path: return the node path of a device, given its
>> + * node name and its compat string
>> + * @fdt: pointer to the dt blob
>> + * @name: device node name
>> + * @compat: compatibility string of the device
>> + * @node_path: returned node path
>> + *
>> + * upon success, the path is output at node_path address
>> + * returns 0 on success, < 0 on failure
>> + */
>> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> +                       char **node_path);
>> +
>>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>>                       const char *property, const void *val, int size);
>>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
> 

Re: [Qemu-devel] [PATCH v2 7/7] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check

[Eric Auger]

Hi David,
On 01/11/2016 03:45 AM, David Gibson wrote:
> On Wed, Jan 06, 2016 at 03:13:25PM +0000, Eric Auger wrote:
>> qemu_fdt_setprop self-exists in case of error hence no need to check
>> the returned value.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> This change is fine, but in general I'm disinclined to invest too much
> in the qemu interfaces for manipulating flattened trees.
> 
> I think our device tree manipulation in qemu is now complicated enough
> that we should move towards using an unflattened (i.e. pointer based)
> DT representation inside qemu, which is generally more suitable for
> complex manipulation.
OK. Is there any user-space library available for un-flattened tree
manipulation? I only found references to kernel unflattened tree
manipulations (drivers/of/fdt.c, include/linux/of.h) and dtc flattree.c.

Besides the indicated direction do I understand correctly that you do
not reject the series?

Best Regards

Eric
> 
> That would then get flattened into a blob for the guest in a single
> pass at reset time.
> 

Re: [Qemu-devel] [PATCH v2 3/7] device_tree: introduce qemu_fdt_node_path

[David Gibson]

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

On Mon, Jan 11, 2016 at 11:35:50AM +0100, Eric Auger wrote:
> Hi David,
> On 01/11/2016 03:38 AM, David Gibson wrote:
> > On Wed, Jan 06, 2016 at 03:13:21PM +0000, Eric Auger wrote:
> >> This new helper routine returns the node path of a device
> >> referred to by its node name and compat string.
> > 
> > What if there are multiple nodes matching the name and compat?
> The function would return the first one. I can improve the doc comment.
> Do you think it is a problem stopping at the first one? Is it a real
> life test case I have to handle here?

Well, I don't know of a specific system which will have this, but it's
absolutely possible to get this situation:  e.g. two different PCI
busses, both of which have their own slot 0 populated with different
instances of the same device.

Whether it's possible for platform devices will depend on the
platform's specific bus toplogies, but you certainly can't rule it out
in general.

I could consider adding a new libfdt function like
fdt_node_offset_by_compatible() that searches by name as well.  It's
just I'm not sure that matching by name and compatible isn't a sign of
a poor approach in the caller.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 7/7] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check

[David Gibson]

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

On Mon, Jan 11, 2016 at 12:18:59PM +0100, Eric Auger wrote:
> Hi David,
> On 01/11/2016 03:45 AM, David Gibson wrote:
> > On Wed, Jan 06, 2016 at 03:13:25PM +0000, Eric Auger wrote:
> >> qemu_fdt_setprop self-exists in case of error hence no need to check
> >> the returned value.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> > 
> > This change is fine, but in general I'm disinclined to invest too much
> > in the qemu interfaces for manipulating flattened trees.
> > 
> > I think our device tree manipulation in qemu is now complicated enough
> > that we should move towards using an unflattened (i.e. pointer based)
> > DT representation inside qemu, which is generally more suitable for
> > complex manipulation.
> OK. Is there any user-space library available for un-flattened tree
> manipulation? I only found references to kernel unflattened tree
> manipulations (drivers/of/fdt.c, include/linux/of.h) and dtc flattree.c.

Not that I'm aware of.  I've sometimes thought of making one as
another companion project to dtc.  Or it would be reasonably
straightforward to build a qemu specific one using qemu's existing
list routines.

> Besides the indicated direction do I understand correctly that you do
> not reject the series?

Yes, that's correct.  I think working with unflattened trees is
something we should head towards somewhere in the future, but that's
certainly not a reason to hold up real improvements based on the
existing flattened tree code.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 3/7] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

Hi David,
On 01/12/2016 05:28 AM, David Gibson wrote:
> On Mon, Jan 11, 2016 at 11:35:50AM +0100, Eric Auger wrote:
>> Hi David,
>> On 01/11/2016 03:38 AM, David Gibson wrote:
>>> On Wed, Jan 06, 2016 at 03:13:21PM +0000, Eric Auger wrote:
>>>> This new helper routine returns the node path of a device
>>>> referred to by its node name and compat string.
>>>
>>> What if there are multiple nodes matching the name and compat?
>> The function would return the first one. I can improve the doc comment.
>> Do you think it is a problem stopping at the first one? Is it a real
>> life test case I have to handle here?
> 
> Well, I don't know of a specific system which will have this, but it's
> absolutely possible to get this situation:  e.g. two different PCI
> busses, both of which have their own slot 0 populated with different
> instances of the same device.
> 
> Whether it's possible for platform devices will depend on the
> platform's specific bus toplogies, but you certainly can't rule it out
> in general.
OK I will handle that case then. I hope I will be able to test it.
> 
> I could consider adding a new libfdt function like
> fdt_node_offset_by_compatible() that searches by name as well.  It's
> just I'm not sure that matching by name and compatible isn't a sign of
> a poor approach in the caller.
well I can't really comment. That looked the most straightforward to me
given the current libfdt API. But not sure it's worth to invest in a new
function in libfdt

Thanks!

Eric
> 

Re: [Qemu-devel] [PATCH v2 3/7] device_tree: introduce qemu_fdt_node_path

[David Gibson]

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

On Tue, Jan 12, 2016 at 06:02:00PM +0100, Eric Auger wrote:
> Hi David,
> On 01/12/2016 05:28 AM, David Gibson wrote:
> > On Mon, Jan 11, 2016 at 11:35:50AM +0100, Eric Auger wrote:
> >> Hi David,
> >> On 01/11/2016 03:38 AM, David Gibson wrote:
> >>> On Wed, Jan 06, 2016 at 03:13:21PM +0000, Eric Auger wrote:
> >>>> This new helper routine returns the node path of a device
> >>>> referred to by its node name and compat string.
> >>>
> >>> What if there are multiple nodes matching the name and compat?
> >> The function would return the first one. I can improve the doc comment.
> >> Do you think it is a problem stopping at the first one? Is it a real
> >> life test case I have to handle here?
> > 
> > Well, I don't know of a specific system which will have this, but it's
> > absolutely possible to get this situation:  e.g. two different PCI
> > busses, both of which have their own slot 0 populated with different
> > instances of the same device.
> > 
> > Whether it's possible for platform devices will depend on the
> > platform's specific bus toplogies, but you certainly can't rule it out
> > in general.
> OK I will handle that case then. I hope I will be able to test it.

One case where this might occur in practice for platform devices is if
you have a system built with multiple instances of a SoC.  A
peripheral on SoC 0 is likely to have the same name and compatible as
the same peripheral on Soc 1, just inside a different parent node.

> > I could consider adding a new libfdt function like
> > fdt_node_offset_by_compatible() that searches by name as well.  It's
> > just I'm not sure that matching by name and compatible isn't a sign of
> > a poor approach in the caller.
> well I can't really comment. That looked the most straightforward to me
> given the current libfdt API. But not sure it's worth to invest in a new
> function in libfdt
> 
> Thanks!
> 
> Eric
> > 
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]