[Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough

[Qemu-devel] [PATCH v5 0/8] 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-v5

Previous versions can be found at
https://git.linaro.org/people/eric.auger/qemu.git/shortlog/refs/heads/v2.5.0-xgbe-v<n>

HISTORY:

v4 -> v5:
- add Peter's R-b on qemu_fdt_getprop patches + remove comment on
  error_fatal
- renamed inherit_properties* into copied_properties
- qemu_fdt_node_path now returns a char **: it supports the case where
  several nodes match the name/compat
- add_amd_xgbe_fdt_node now checks a single node is found

v3 -> v4:
- explicitly return 0 in node creation functions

v2 -> v3:
- added "device_tree: qemu_fdt_getprop_cell converted to use the error API"

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 (8):
  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
  device_tree: qemu_fdt_getprop_cell 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                   | 177 ++++++++++++++++++++--
 hw/arm/boot.c                   |   6 +-
 hw/arm/sysbus-fdt.c             | 320 ++++++++++++++++++++++++++++++++++++++--
 hw/arm/vexpress.c               |   6 +-
 hw/vfio/Makefile.objs           |   1 +
 hw/vfio/amd-xgbe.c              |  55 +++++++
 include/hw/vfio/vfio-amd-xgbe.h |  51 +++++++
 include/sysemu/device_tree.h    |  44 +++++-
 8 files changed, 631 insertions(+), 29 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 v5 1/8] 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ée <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 v5 2/8] 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 v5 3/8] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

This new helper routine returns a NULL terminated array of
node paths matching a node name and a compat string.

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

---

v4 -> v5:
- support the case where several nodes exist, ie.
  return an array of node paths. Also add Error **
  parameter

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                | 49 ++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h | 14 +++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index b262c2d..3c88a37 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -231,6 +231,55 @@ static int findnode_nofail(void *fdt, const char *node_path)
     return offset;
 }
 
+char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+                          Error **errp)
+{
+    int offset, len, ret;
+    const char *iter_name;
+    unsigned int path_len = 16, n = 0;
+    GSList *path_list = NULL, *iter;
+    char **path_array;
+
+    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)) {
+            char *path;
+
+            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);
+            }
+            path_list = g_slist_prepend(path_list, path);
+            n++;
+        }
+        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
+    }
+
+    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
+        error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
+                   __func__, name, compat, fdt_strerror(offset));
+    }
+
+    path_array = g_new(char *, n + 1);
+    path_array[n--] = NULL;
+
+    for (iter = path_list; iter; iter = iter->next) {
+        path_array[n--] = iter->data;
+    }
+
+    g_slist_free(path_list);
+
+    return path_array;
+}
+
 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..436b5dd 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 paths of nodes matching a given
+ * name and compat string
+ * @fdt: pointer to the dt blob
+ * @name: node name
+ * @compat: compatibility string
+ * @errp: handle to an error object
+ *
+ * returns a newly allocated NULL-terminated array of node paths.
+ * Use g_strfreev() to free it.
+ */
+char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+                          Error **errp);
+
 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 v5 4/8] 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>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

---
v4 -> v5:
- add Peter's R-b
- remove comment about error_fatal

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 | 13 ++++++++++++-
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 3c88a37..45fd76d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -333,18 +333,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;
 }
@@ -353,7 +353,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 436b5dd..123beb5 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -45,8 +45,19 @@ 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
+ */
 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 v5 5/8] device_tree: qemu_fdt_getprop_cell converted to use the error API

[Eric Auger]

This patch aligns the prototype with qemu_fdt_getprop. The caller
can choose whether the function self-asserts on error (passing
&error_fatal as Error ** argument, corresponding to the legacy behavior),
or behaves differently such as simply output a message.

In this later case the caller can use the new lenp parameter to interpret
the error if any.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

---
v4 -> v5:
- Add Peter's R-b
- remove comment about error_fatal

v3 : creation
---
 device_tree.c                | 21 ++++++++++++++-------
 hw/arm/boot.c                |  6 ++++--
 hw/arm/vexpress.c            |  6 ++++--
 include/sysemu/device_tree.h | 14 +++++++++++++-
 4 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 45fd76d..5e56f8e 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -350,15 +350,22 @@ const void *qemu_fdt_getprop(void *fdt, const char *node_path,
 }
 
 uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
-                               const char *property)
+                               const char *property, int *lenp, Error **errp)
 {
     int 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);
-        exit(1);
+    const uint32_t *p;
+
+    if (!lenp) {
+        lenp = &len;
+    }
+    p = qemu_fdt_getprop(fdt, node_path, property, lenp, errp);
+    if (!p) {
+        return 0;
+    } else if (*lenp != 4) {
+        error_setg(errp, "%s: %s/%s not 4 bytes long (not a cell?)",
+                   __func__, node_path, property);
+        *lenp = -EINVAL;
+        return 0;
     }
     return be32_to_cpu(*p);
 }
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 75f69bf..541b74c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -386,8 +386,10 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         return 0;
     }
 
-    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
-    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
+    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
+                                   NULL, &error_fatal);
+    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
+                                   NULL, &error_fatal);
     if (acells == 0 || scells == 0) {
         fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
         goto fail;
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index ea9a984..f6e28dc 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -477,8 +477,10 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
     uint32_t acells, scells, intc;
     const VEDBoardInfo *daughterboard = (const VEDBoardInfo *)info;
 
-    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
-    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
+    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
+                                   NULL, &error_fatal);
+    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
+                                   NULL, &error_fatal);
     intc = find_int_controller(fdt);
     if (!intc) {
         /* Not fatal, we just won't provide virtio. This will
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 123beb5..7897e54 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -58,8 +58,20 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
                              const char *property, int *lenp,
                              Error **errp);
+/**
+ * qemu_fdt_getprop_cell: retrieve the value of a given 4 byte 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 -EINVAL if the property size is different from
+ *        4 bytes, or 4 (expected length of the property) upon success.
+ * @errp: handle to an error object
+ *
+ * returns the property value on success
+ */
 uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
-                               const char *property);
+                               const char *property, int *lenp,
+                               Error **errp);
 uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
 uint32_t qemu_fdt_alloc_phandle(void *fdt);
 int qemu_fdt_nop_node(void *fdt, const char *node_path);
-- 
1.9.1

[Qemu-devel] [PATCH v5 6/8] 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.

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

---
v4 -> v5:
- renamed inherit_properties

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..d85c9e6 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;
+
+/**
+ * copy_properties_from_host
+ *
+ * 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 copy_properties_from_host(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_copied_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);
+
+    copy_properties_from_host(clock_copied_properties,
+                              ARRAY_SIZE(clock_copied_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 v5 7/8] 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>

---

v4 -> v5:
- use new proto for qemu_fdt_node_path, check there is only 1 node
  matching the name/compat.
- renamed inherit_properties*

v3 -> v4:
- add_amd_xgbe_fdt_node explicitly returns 0. Reword comment to emphasize
  the fact the function self-asserts in case of error
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 | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 189 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d85c9e6..bf6ec24 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
+
 /**
  * copy_properties_from_host
  *
@@ -126,12 +133,9 @@ static HostProperty clock_copied_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,166 @@ fail_reg:
     return ret;
 }
 
+
+/* AMD xgbe properties whose values are copied/pasted from host */
+static HostProperty amd_xgbe_copied_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)
+ *
+ * self-asserts in case of error
+ */
+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, 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);
+    }
+    node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
+                                   &error_fatal);
+    if (!node_path[0]) {
+        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
+                   __func__, dt_name, vdev->compat);
+    }
+
+    if (node_path[1]) {
+        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
+                   __func__, dt_name, vdev->compat);
+    }
+
+    g_free(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[0], "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);
+
+    copy_properties_from_host(amd_xgbe_copied_properties,
+                       ARRAY_SIZE(amd_xgbe_copied_properties),
+                       host_fdt, guest_fdt,
+                       node_path[0], 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_strfreev(node_path);
+    g_free(irq_attr);
+    g_free(reg_attr);
+    g_free(nodename);
+    return 0;
+}
+
+#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 v5 8/8] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check

[Eric Auger]

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

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

---

v3 -> v4: fix returned value
---
 hw/arm/sysbus-fdt.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index bf6ec24..d179214 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -216,7 +216,7 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
     PlatformBusDevice *pbus = data->pbus;
     void *fdt = data->fdt;
     const char *parent_node = data->pbus_node_name;
-    int compat_str_len, i, ret = -1;
+    int compat_str_len, i;
     char *nodename;
     uint32_t *irq_attr, *reg_attr;
     uint64_t mmio_base, irq_number;
@@ -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,17 +252,12 @@ 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;
+    return 0;
 }
 
 
-- 
1.9.1

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

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> 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.
>
> - copy_properties_from_host 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>

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

This is just "asserts", not "self-asserts".

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

> +/**
> + * 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);

Don't we want to return here, rather than continuing with the
rest of the function? ("goto err;" with an err: label before
the g_free() at the bottom of the function probably best since
some of the error paths need to free that.)

> +    }
> +    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);
> +
> +    copy_properties_from_host(clock_copied_properties,
> +                              ARRAY_SIZE(clock_copied_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 */

thanks
-- PMM

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

[Eric Auger]

Hi Peter,
On 01/25/2016 03:05 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> 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.
>>
>> - copy_properties_from_host 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>
> 
>> +
>> +/**
>> + * copy_properties_from_host
>> + *
>> + * 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
> 
> This is just "asserts", not "self-asserts".
ok
> 
>> + * 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
>> + */
> 
>> +/**
>> + * 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);
> 
> Don't we want to return here, rather than continuing with the
> rest of the function? ("goto err;" with an err: label before
> the g_free() at the bottom of the function probably best since
> some of the error paths need to free that.)
due to the &error_fatal, we are going to exit here. I thought it is not
worth going further if we can't find the clock node.

Best Regards

Eric
> 
>> +    }
>> +    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);
>> +
>> +    copy_properties_from_host(clock_copied_properties,
>> +                              ARRAY_SIZE(clock_copied_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 */
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> 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).

"leaves"

> + *
> + * the function self-asserts in case of error

"asserts"

> + */
> +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);

Why does this one error_report and exit but other errors below use
error_setg?

> +    }
> +    parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];

What causes us to need this cast to char* ?

> +
> +    d = opendir(dirname);
> +    if (!d) {
> +        error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);

You need to return here (and similarly to bail out properly
in the other error paths below).

> +    }
> +
> +    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);

I don't mind whether we use g_strjoin("/",...) or g_strdup_printf("%s/%s", ...)
to glue together strings with a '/' between them, but can we not use
both methods in the same function, please?

> +            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__);

Should we free the created device tree and return NULL here?

> +    }
> +    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);

Can we have a doc comment for a new global function, please?

> +#endif
>
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);
> --
> 1.9.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> This new helper routine returns a NULL terminated array of
> node paths matching a node name and a compat string.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> v4 -> v5:
> - support the case where several nodes exist, ie.
>   return an array of node paths. Also add Error **
>   parameter
>
> 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                | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h | 14 +++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index b262c2d..3c88a37 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -231,6 +231,55 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>
> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                          Error **errp)
> +{
> +    int offset, len, ret;
> +    const char *iter_name;
> +    unsigned int path_len = 16, n = 0;
> +    GSList *path_list = NULL, *iter;
> +    char **path_array;
> +
> +    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)) {
> +            char *path;
> +
> +            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);
> +            }
> +            path_list = g_slist_prepend(path_list, path);
> +            n++;
> +        }
> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> +    }
> +
> +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> +        error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
> +                   __func__, name, compat, fdt_strerror(offset));

Needs to bail out (freeing memory).

> +    }
> +
> +    path_array = g_new(char *, n + 1);
> +    path_array[n--] = NULL;

0, or '\0'. NULL is a pointer, not the NUL character. (This is a
style issue, not a correctness one.)

> +
> +    for (iter = path_list; iter; iter = iter->next) {
> +        path_array[n--] = iter->data;
> +    }
> +
> +    g_slist_free(path_list);
> +
> +    return path_array;
> +}
> +
>  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..436b5dd 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 paths of nodes matching a given
> + * name and compat string
> + * @fdt: pointer to the dt blob
> + * @name: node name
> + * @compat: compatibility string
> + * @errp: handle to an error object
> + *
> + * returns a newly allocated NULL-terminated array of node paths.

Should say what we return on error (NULL ?)

> + * Use g_strfreev() to free it.
> + */
> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                          Error **errp);
> +
>  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
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> 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>

>  hw/arm/sysbus-fdt.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 189 insertions(+), 6 deletions(-)

I'll let it pass for this patchset, but this file is starting to
get big, and probably needs some kind of split into common functions
in one file and then a separate file for each pass-through device,
if they're all going to require 200-odd lines to deal with.

> +/**
> + * 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]) {

We should check substrings[0] before substrings[1], as otherwise
if we're passed the empty string we'll index off the end of the
vector.

> +        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,166 @@ fail_reg:
>      return ret;
>  }
>
> +
> +/* AMD xgbe properties whose values are copied/pasted from host */
> +static HostProperty amd_xgbe_copied_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)
> + *
> + * self-asserts in case of error

"asserts".

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

"negotiation"

> +          * 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_strfreev(node_path);
> +    g_free(irq_attr);
> +    g_free(reg_attr);
> +    g_free(nodename);
> +    return 0;
> +}
> +
> +#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 */
>  };

thanks
-- PMM

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

[Peter Maydell]

On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote:
> Hi Peter,
> On 01/25/2016 03:05 PM, Peter Maydell wrote:
>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:

>> Don't we want to return here, rather than continuing with the
>> rest of the function? ("goto err;" with an err: label before
>> the g_free() at the bottom of the function probably best since
>> some of the error paths need to free that.)
> due to the &error_fatal, we are going to exit here. I thought it is not
> worth going further if we can't find the clock node.

Yes, sorry, I didn't notice the error_fatal til I got to about
the last patch I reviewed. You can ignore similar comments
I made about some of the other patches.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

Peter,
On 01/25/2016 03:26 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> This new helper routine returns a NULL terminated array of
>> node paths matching a node name and a compat string.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v4 -> v5:
>> - support the case where several nodes exist, ie.
>>   return an array of node paths. Also add Error **
>>   parameter
>>
>> 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                | 49 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/sysemu/device_tree.h | 14 +++++++++++++
>>  2 files changed, 63 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index b262c2d..3c88a37 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -231,6 +231,55 @@ static int findnode_nofail(void *fdt, const char *node_path)
>>      return offset;
>>  }
>>
>> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> +                          Error **errp)
>> +{
>> +    int offset, len, ret;
>> +    const char *iter_name;
>> +    unsigned int path_len = 16, n = 0;
>> +    GSList *path_list = NULL, *iter;
>> +    char **path_array;
>> +
>> +    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)) {
>> +            char *path;
>> +
>> +            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);
>> +            }
>> +            path_list = g_slist_prepend(path_list, path);
>> +            n++;
>> +        }
>> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
>> +    }
>> +
>> +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
>> +        error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
>> +                   __func__, name, compat, fdt_strerror(offset));
> 
> Needs to bail out (freeing memory).
effectively in case I get an error, it's better to free everything and
return a vector with with a single NULL element, see comment below. I
will correct this.
> 
>> +    }
>> +
>> +    path_array = g_new(char *, n + 1);
>> +    path_array[n--] = NULL;
> 
> 0, or '\0'. NULL is a pointer, not the NUL character. (This is a
> style issue, not a correctness one.)
actually I want to put NULL as the last element of my array and not a
NUL char. The function returns a NULL terminated vector as
g_strsplit_set does, for instance.

Thanks

Eric
> 
>> +
>> +    for (iter = path_list; iter; iter = iter->next) {
>> +        path_array[n--] = iter->data;
>> +    }
>> +
>> +    g_slist_free(path_list);
>> +
>> +    return path_array;
>> +}
>> +
>>  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..436b5dd 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 paths of nodes matching a given
>> + * name and compat string
>> + * @fdt: pointer to the dt blob
>> + * @name: node name
>> + * @compat: compatibility string
>> + * @errp: handle to an error object
>> + *
>> + * returns a newly allocated NULL-terminated array of node paths.
> 
> Should say what we return on error (NULL ?)
> 
>> + * Use g_strfreev() to free it.
>> + */
>> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> +                          Error **errp);
>> +
>>  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
>>
> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On 25 January 2016 at 14:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote:
>> Hi Peter,
>> On 01/25/2016 03:05 PM, Peter Maydell wrote:
>>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>
>>> Don't we want to return here, rather than continuing with the
>>> rest of the function? ("goto err;" with an err: label before
>>> the g_free() at the bottom of the function probably best since
>>> some of the error paths need to free that.)
>> due to the &error_fatal, we are going to exit here. I thought it is not
>> worth going further if we can't find the clock node.
>
> Yes, sorry, I didn't notice the error_fatal til I got to about
> the last patch I reviewed. You can ignore similar comments
> I made about some of the other patches.

...so if you fix the 'asserts' comment thing, you can have
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Eric Auger]

On 01/25/2016 03:43 PM, Peter Maydell wrote:
> On 25 January 2016 at 14:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote:
>>> Hi Peter,
>>> On 01/25/2016 03:05 PM, Peter Maydell wrote:
>>>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>>
>>>> Don't we want to return here, rather than continuing with the
>>>> rest of the function? ("goto err;" with an err: label before
>>>> the g_free() at the bottom of the function probably best since
>>>> some of the error paths need to free that.)
>>> due to the &error_fatal, we are going to exit here. I thought it is not
>>> worth going further if we can't find the clock node.
>>
>> Yes, sorry, I didn't notice the error_fatal til I got to about
>> the last patch I reviewed. You can ignore similar comments
>> I made about some of the other patches.
> 
> ...so if you fix the 'asserts' comment thing, you can have
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

OK thanks for the whole review

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs

[Eric Auger]

Hi Peter,
On 01/25/2016 03:13 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> 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).
> 
> "leaves"
OK
> 
>> + *
>> + * the function self-asserts in case of error
> 
> "asserts"
OK
> 
>> + */
>> +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);
> 
> Why does this one error_report and exit but other errors below use
> error_setg?
replaced with error_setg(&error_fatal, ...)
> 
>> +    }
>> +    parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];
> 
> What causes us to need this cast to char* ?
I changed parent_node to a const char * instead of char*
> 
>> +
>> +    d = opendir(dirname);
>> +    if (!d) {
>> +        error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
> 
> You need to return here (and similarly to bail out properly
> in the other error paths below).
> 
>> +    }
>> +
>> +    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);
> 
> I don't mind whether we use g_strjoin("/",...) or g_strdup_printf("%s/%s", ...)
> to glue together strings with a '/' between them, but can we not use
> both methods in the same function, please?
ok
> 
>> +            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__);
> 
> Should we free the created device tree and return NULL here?
> 
>> +    }
>> +    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);
> 
> Can we have a doc comment for a new global function, please?
sure

Thanks

Eric
> 
>> +#endif
>>
>>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>>                       const char *property, const void *val, int size);
>> --
>> 1.9.1
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Eric Auger]

Hi Peter,
On 01/25/2016 03:33 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> 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>
> 
>>  hw/arm/sysbus-fdt.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 189 insertions(+), 6 deletions(-)
> 
> I'll let it pass for this patchset, but this file is starting to
> get big, and probably needs some kind of split into common functions
> in one file and then a separate file for each pass-through device,
> if they're all going to require 200-odd lines to deal with.

That's understood. If you don't mind i would rather introduce that move
in a separate series then.

Thanks

Eric
> 
>> +/**
>> + * 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]) {
> 
> We should check substrings[0] before substrings[1], as otherwise
> if we're passed the empty string we'll index off the end of the
> vector.
> 
>> +        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,166 @@ fail_reg:
>>      return ret;
>>  }
>>
>> +
>> +/* AMD xgbe properties whose values are copied/pasted from host */
>> +static HostProperty amd_xgbe_copied_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)
>> + *
>> + * self-asserts in case of error
> 
> "asserts".
> 
>> +    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
> 
> "negotiation"
> 
>> +          * 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_strfreev(node_path);
>> +    g_free(irq_attr);
>> +    g_free(reg_attr);
>> +    g_free(nodename);
>> +    return 0;
>> +}
>> +
>> +#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 */
>>  };
> 
> thanks
> -- PMM
>