[Qemu-devel] [RESEND RFC 0/6] AMD XGBE KVM platform passthrough

[Qemu-devel] [RESEND RFC 0/6] AMD XGBE KVM platform passthrough

[Eric Auger]

I am resending this RFC from Oct 12, after kernel 4.4-rc1 and
QEMU 2.5-rc1, hoping things have calmed down a little bit.

This RFC 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. Generating the device tree node for
the guest is the challenging and controversary part of this series.

- 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 proposes a way to parse the host device
tree blob and retrieve host values to feed guest representation. Current
approach relies on dtc binary availability plus libfdt usage.
Other alternatives were discussed in:
http://www.spinics.net/lists/kvm-arm/msg16648.html.

- Currently host booted with ACPI is not supported.

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

Best Regards

Eric


Eric Auger (6):
  hw/vfio/platform: amd-xgbe device
  device_tree: introduce load_device_tree_from_sysfs
  device_tree: introduce qemu_fdt_node_path
  device_tree: introduce qemu_fdt_getprop_optional
  hw/arm/sysbus-fdt: helpers for clock node generation
  hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

 device_tree.c                   |  97 ++++++++++++++
 hw/arm/sysbus-fdt.c             | 273 ++++++++++++++++++++++++++++++++++++++++
 hw/vfio/Makefile.objs           |   1 +
 hw/vfio/amd-xgbe.c              |  55 ++++++++
 include/hw/vfio/vfio-amd-xgbe.h |  51 ++++++++
 include/sysemu/device_tree.h    |   6 +
 6 files changed, 483 insertions(+)
 create mode 100644 hw/vfio/amd-xgbe.c
 create mode 100644 include/hw/vfio/vfio-amd-xgbe.h

-- 
1.8.3.2

[Qemu-devel] [RESEND RFC 1/6] 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>
---
 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.8.3.2

[Qemu-devel] [RESEND RFC 2/6] device_tree: introduce load_device_tree_from_sysfs

[Eric Auger]

This function returns the host device tree blob from sysfs
(/sys/firmware/devicetree/base).

This has a runtime dependency on the dtc binary. This functionality
is useful for platform device passthrough where the host device tree
needs to be parsed to feed information into the guest device tree.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h |  1 +
 2 files changed, 41 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index a9f5f8e..58a5329 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -117,6 +117,46 @@ fail:
     return NULL;
 }
 
+/**
+ * load_device_tree_from_sysfs
+ *
+ * extract the dt blob from host sysfs
+ * this has a runtime dependency on the dtc binary
+ */
+void *load_device_tree_from_sysfs(void)
+{
+    char cmd[] = "dtc -I fs -O dtb /sys/firmware/devicetree/base";
+    FILE *pipe;
+    void *fdt;
+    int ret, actual_dt_size;
+
+    pipe = popen(cmd, "r");
+    if (!pipe) {
+        error_report("%s: Error when executing dtc", __func__);
+        return NULL;
+    }
+    fdt = g_malloc0(FDT_MAX_SIZE);
+    actual_dt_size = fread(fdt, 1, FDT_MAX_SIZE, pipe);
+    pclose(pipe);
+
+    if (actual_dt_size == 0) {
+        error_report("%s: could not copy host device tree in memory: %m",
+                     __func__);
+        goto fail;
+    }
+    ret = fdt_check_header(fdt);
+    if (ret) {
+        error_report("%s: Host dt file loaded into memory is invalid: %s",
+                     __func__, fdt_strerror(ret));
+        goto fail;
+    }
+    return fdt;
+
+fail:
+    g_free(fdt);
+    return NULL;
+}
+
 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..307e53d 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -16,6 +16,7 @@
 
 void *create_device_tree(int *sizep);
 void *load_device_tree(const char *filename_path, int *sizep);
+void *load_device_tree_from_sysfs(void);
 
 int qemu_fdt_setprop(void *fdt, const char *node_path,
                      const char *property, const void *val, int size);
-- 
1.8.3.2

[Qemu-devel] [RESEND RFC 3/6] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

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

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h |  3 +++
 2 files changed, 43 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index 58a5329..f184e3c 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -171,6 +171,46 @@ static int findnode_nofail(void *fdt, const char *node_path)
     return offset;
 }
 
+/**
+ * qemu_fdt_node_path
+ *
+ * return the node path of a device, referred to by its node name
+ * and its compat string
+ * fdt: pointer to the dt blob
+ * name: name of the device
+ * compat: compatibility string of the device
+ *
+ * returns the node path
+ */
+int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+                       char **node_path)
+{
+    int offset = 0, len;
+    const char *iter_name;
+    char path[256];
+    int ret;
+
+    *node_path = NULL;
+    while (1) {
+        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
+        if (offset == -FDT_ERR_NOTFOUND) {
+            break;
+        }
+        iter_name = fdt_get_name(fdt, offset, &len);
+        if (!strncmp(iter_name, name, len)) {
+            goto found;
+        }
+    }
+    return offset;
+
+found:
+    ret = fdt_get_path(fdt, offset, path, 256);
+    if (!ret) {
+        *node_path = g_strdup(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 307e53d..f9e6e6e 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -18,6 +18,9 @@ void *create_device_tree(int *sizep);
 void *load_device_tree(const char *filename_path, int *sizep);
 void *load_device_tree_from_sysfs(void);
 
+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.8.3.2

[Qemu-devel] [RESEND RFC 4/6] device_tree: introduce qemu_fdt_getprop_optional

[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 is what this new qemu_fdt_getprop_optional function does.

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

diff --git a/device_tree.c b/device_tree.c
index f184e3c..a318683 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -280,6 +280,23 @@ const void *qemu_fdt_getprop(void *fdt, const char *node_path,
     return r;
 }
 
+const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
+                             const char *property, bool optional, int *lenp)
+{
+    int len;
+    const void *r;
+    if (!lenp) {
+        lenp = &len;
+    }
+    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
+    if (!r && !optional) {
+        error_report("%s: Couldn't get %s/%s: %s", __func__,
+                     node_path, property, fdt_strerror(*lenp));
+        exit(1);
+    }
+    return r;
+}
+
 uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
                                const char *property)
 {
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index f9e6e6e..10cbe8e 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -34,6 +34,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
                              const char *target_node_path);
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
                              const char *property, int *lenp);
+const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
+                             const char *property, bool optional, int *lenp);
 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.8.3.2

[Qemu-devel] [RESEND RFC 5/6] 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 from 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 a they get used

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 9d28797..22e5801 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,115 @@ typedef struct NodeCreationPair {
     int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
 } NodeCreationPair;
 
+/* helpers */
+
+struct HostProperty {
+    const char *name;
+    bool optional;
+};
+
+typedef struct HostProperty HostProperty;
+
+/**
+ * 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 exits. Not found optional ones are ignored.
+ * props: array of HostProperty to copy
+ * nb_props: number of properties in the array
+ * host_dt: host device tree blob
+ * guest_dt: guest device tree blob
+ * node_path: host dt node path where the property is supposed to be
+              found
+ * nodename: guest node name the properties should be added to
+ *
+ */
+static void inherit_properties(HostProperty *props, int nb_props,
+                               void *host_fdt, void *guest_fdt,
+                               char *node_path, char *nodename)
+{
+    int i, prop_len;
+    const void *r;
+
+    for (i = 0; i < nb_props; i++) {
+        r = qemu_fdt_getprop_optional(host_fdt, node_path,
+                             props[i].name,
+                             props[i].optional,
+                             &prop_len);
+        if (r) {
+            qemu_fdt_setprop(guest_fdt, nodename,
+                             props[i].name, r, prop_len);
+        }
+    }
+}
+
+/* clock properties whose values are copied/pasted from host */
+static HostProperty clock_inherited_properties[] = {
+    {"compatible", 0},
+    {"#clock-cells", 0},
+    {"clock-frequency", 1},
+    {"clock-output-names", 1},
+};
+
+/**
+ * fdt_build_clock_node
+ *
+ * Build a guest clock node, used as a dependency from a passthrough'ed
+ * device. Most information are retrieved from the host fellow node.
+ * Also check the host clock is a fixed one.
+ *
+ * host_fdt: host device tree blob from which info are retrieved
+ * guest_fdt: guest device tree blob where the clock node is added
+ * host_phandle: phandle of the clock in host device tree
+ * guest_phandle: phandle to assign to the guest node
+ */
+int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                                uint32_t host_phandle,
+                                uint32_t guest_phandle);
+int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                                uint32_t host_phandle,
+                                uint32_t guest_phandle)
+{
+    char node_path[256];
+    char *nodename;
+    const void *r;
+    int ret, prop_len;
+
+    ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
+    if (ret <= 0) {
+        error_report("not able to locate clock handle %d in host device tree\n",
+                     host_phandle);
+        goto out;
+    }
+    ret = fdt_get_path(host_fdt, ret, node_path, 256);
+    if (ret < 0) {
+        error_report("not able to retrieve node path for clock handle %d\n",
+                     host_phandle);
+        goto out;
+    }
+
+    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len);
+    if (strcmp(r, "fixed-clock")) {
+        error_report("clock handle %d is not a fixed clock\n", host_phandle);
+        ret = -1;
+        goto out;
+    }
+
+    nodename = strrchr(node_path, '/');
+    qemu_fdt_add_subnode(guest_fdt, nodename);
+
+    inherit_properties(clock_inherited_properties,
+                       ARRAY_SIZE(clock_inherited_properties),
+                       host_fdt, guest_fdt,
+                       node_path, nodename);
+
+    qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
+
+out:
+    return ret;
+}
+
 /* Device Specific Code */
 
 /**
-- 
1.8.3.2

[Qemu-devel] [RESEND RFC 6/6] 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.

Property values are inherited from host device tree. It is mandated
host uses device tree, dtc binary is installed, and the host also uses a
combined XGBE/PHY representation (>= 4.2 host kernel).

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

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 22e5801..b13d380 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -22,6 +22,7 @@
  */
 
 #include <libfdt.h>
+#include <linux/vfio.h>
 #include "hw/arm/sysbus-fdt.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
@@ -29,8 +30,10 @@
 #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"
 
+
 /*
  * internal struct that contains the information to create dynamic
  * sysbus device node
@@ -120,10 +123,7 @@ static HostProperty clock_inherited_properties[] = {
  * host_phandle: phandle of the clock in host device tree
  * guest_phandle: phandle to assign to the guest node
  */
-int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
-                                uint32_t host_phandle,
-                                uint32_t guest_phandle);
-int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+static int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
                                 uint32_t host_phandle,
                                 uint32_t guest_phandle)
 {
@@ -166,6 +166,21 @@ out:
     return ret;
 }
 
+/**
+ * sysfs_to_dt_name
+ *
+ * convert the name found in sysfs into the node name
+ * for instance e0900000.xgmac is converted into xgmac@e0900000
+ */
+static char *sysfs_to_dt_name(const char *sysfs_name)
+{
+    gchar **substrings =  g_strsplit(sysfs_name, ".", 2);
+    char *dt_name;
+
+    dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
+    return dt_name;
+}
+
 /* Device Specific Code */
 
 /**
@@ -233,9 +248,157 @@ fail_reg:
     return ret;
 }
 
+
+/* AMD xgbe properties whose values are copied/pasted from host */
+static HostProperty amd_xgbe_inherited_properties[] = {
+    {"compatible", 0},
+    {"dma-coherent", 1},
+    {"amd,per-channel-interrupt", 1},
+    {"phy-mode", 0},
+    {"mac-address", 1},
+    {"amd,speed-set", 0},
+    {"amd,serdes-blwc", 1},
+    {"amd,serdes-cdr-rate", 1},
+    {"amd,serdes-pq-skew", 1},
+    {"amd,serdes-tx-amp", 1},
+    {"amd,serdes-dfe-tap-config", 1},
+    {"amd,serdes-dfe-tap-enable", 1},
+    {"clock-names", 0},
+};
+
+/**
+ * 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();
+    if (!host_fdt) {
+        goto stop;
+    }
+    dt_name = sysfs_to_dt_name(vbasedev->name);
+    ret = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat, &node_path);
+    g_free(dt_name);
+
+    if (ret) {
+        goto stop;
+    }
+
+    /* generate nodes for DMA_CLK and PTP_CLK */
+    r = qemu_fdt_getprop(host_fdt, node_path, "clocks", &prop_len);
+    if (prop_len != 8) {
+        goto stop;
+    }
+    host_clock_phandles = (uint32_t *)r;
+    host_clock_phandles[0] = be32_to_cpu(host_clock_phandles[0]);
+    host_clock_phandles[1] = be32_to_cpu(host_clock_phandles[1]);
+    guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
+    guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
+
+    ret = fdt_build_clock_node(host_fdt, guest_fdt,
+                               host_clock_phandles[0],
+                               guest_clock_phandles[0]);
+    if (ret) {
+        goto stop;
+    }
+
+    ret = fdt_build_clock_node(host_fdt, guest_fdt,
+                               host_clock_phandles[1],
+                               guest_clock_phandles[1]);
+    if (ret) {
+        goto stop;
+    }
+
+    /* 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));
+    }
+    ret = qemu_fdt_setprop(guest_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;
+    }
+
+    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);
+        }
+    }
+    ret = qemu_fdt_setprop(guest_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(host_fdt);
+    g_free(node_path);
+    g_free(irq_attr);
+fail_reg:
+    g_free(reg_attr);
+    g_free(nodename);
+    return ret;
+stop:
+    exit(1);
+}
+
 /* list of supported dynamic sysbus devices */
 static const NodeCreationPair add_fdt_node_functions[] = {
     {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
+    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
     {"", NULL}, /* last element */
 };
 
-- 
1.8.3.2

Re: [Qemu-devel] [RESEND RFC 0/6] AMD XGBE KVM platform passthrough

[Alex Williamson]

On Thu, 2015-11-19 at 15:22 +0000, Eric Auger wrote:
> I am resending this RFC from Oct 12, after kernel 4.4-rc1 and
> QEMU 2.5-rc1, hoping things have calmed down a little bit.
> 
> This RFC 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. Generating the device tree node for
> the guest is the challenging and controversary part of this series.
> 
> - 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 proposes a way to parse the host device
> tree blob and retrieve host values to feed guest representation. Current
> approach relies on dtc binary availability plus libfdt usage.
> Other alternatives were discussed in:
> http://www.spinics.net/lists/kvm-arm/msg16648.html.
> 
> - Currently host booted with ACPI is not supported.

I won't pretend to know all the politics in the ARM space, but doesn't
this last bullet sort of imply that this is dead-on-arrival code?  Maybe
not in the embedded space, but certainly in the server space, I thought
ACPI was declared the winner.  Thanks,

Alex

Re: [Qemu-devel] [RESEND RFC 0/6] AMD XGBE KVM platform passthrough

[Eric Auger]

Hi Alex,
On 11/20/2015 12:44 AM, Alex Williamson wrote:
> On Thu, 2015-11-19 at 15:22 +0000, Eric Auger wrote:
>> I am resending this RFC from Oct 12, after kernel 4.4-rc1 and
>> QEMU 2.5-rc1, hoping things have calmed down a little bit.
>>
>> This RFC 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. Generating the device tree node for
>> the guest is the challenging and controversary part of this series.
>>
>> - 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 proposes a way to parse the host device
>> tree blob and retrieve host values to feed guest representation. Current
>> approach relies on dtc binary availability plus libfdt usage.
>> Other alternatives were discussed in:
>> http://www.spinics.net/lists/kvm-arm/msg16648.html.
>>
>> - Currently host booted with ACPI is not supported.
> 
> I won't pretend to know all the politics in the ARM space, but doesn't
> this last bullet sort of imply that this is dead-on-arrival code?  Maybe
> not in the embedded space, but certainly in the server space, I thought
> ACPI was declared the winner.  Thanks,

When the code was written, no ACPI description was available yet
including IOMMU. Now I think there is a specification that would enable
the description of the system (IORT/DSDT tables) and I will investigate
whether we have one ready for the HW I am using and mainlined.

Nethertheless we had a discussion end of Sept with Marc Zyngier,
Christoffer, Will Deacon and other people working in the ARM ecosystem
and we decided starting with FDT host description was a reasonable
choice at this moment. To be fully honest Peter also thought we should
consider the ACPI case from day 0. But I think nobody -AFAIK - has an
ideal solution about how to address ACPI/DT in a unified way and people
seem to be against introducing IOCTL API.

Among solutions I foresee the 1st one below was considered the simplest
and chosen:
1) rely on external applications to decode/parse dt/ACPI table
2) build a unified fs representation for dt/ACPI
3) create unified IOCTL API to retrieve dt/ACPI info (attempted by VOSYS
but at VFIO level)

The idea of this series is
- to rely on external dtc binary to build the blob from
/sys/firmware/devicetree/base
- introduce some helpers using libfdt that manipulate the host dt blob
- use those helpers in sysbus-fdt.c to build the clock and xgbe nodes
for the guest

Assuming we have an ACPI description I guess we would/could use a
similar approach to parse/decode the ACPI table from QEMU (relying on
acpidump, acpixtract, iasl, ../.. combination). So the current proposal
brings a solution for embedded world and can be easily reused for other
devices. Next step is to propose a similar approach for ACPI. Now I
would like to make sure the open approach is accepted (with external
dependency on dtc binary as we would have ext dependency on ACPI utilities).

Best Regards

Eric


> 
> Alex
> 

Re: [Qemu-devel] [RESEND RFC 0/6] AMD XGBE KVM platform passthrough

[Christoffer Dall]

On Thu, Nov 19, 2015 at 11:44 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Thu, 2015-11-19 at 15:22 +0000, Eric Auger wrote:
>> I am resending this RFC from Oct 12, after kernel 4.4-rc1 and
>> QEMU 2.5-rc1, hoping things have calmed down a little bit.
>>
>> This RFC 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. Generating the device tree node for
>> the guest is the challenging and controversary part of this series.
>>
>> - 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 proposes a way to parse the host device
>> tree blob and retrieve host values to feed guest representation. Current
>> approach relies on dtc binary availability plus libfdt usage.
>> Other alternatives were discussed in:
>> http://www.spinics.net/lists/kvm-arm/msg16648.html.
>>
>> - Currently host booted with ACPI is not supported.
>
> I won't pretend to know all the politics in the ARM space, but doesn't
> this last bullet sort of imply that this is dead-on-arrival code?  Maybe
> not in the embedded space, but certainly in the server space, I thought
> ACPI was declared the winner.  Thanks,
>
Hi Alex,

We currently have no solution to accomplish platform device
passthrough on ACPI, because we don't know how to extract and
encapsulate the necessary information from the host about the
passthrough device and its dependency (clocks and phys for example).

There are people interested in this work, in particular in the
networking space, who couldn't care less about ACPI, and for those,
supporting platform device passthrough with DT remains highly
relevant.

Therefore, I think it's useful to consider these patches for review
and for their basic approach.

Thanks,
-Christoffer

Re: [Qemu-devel] [RESEND RFC 1/6] hw/vfio/platform: amd-xgbe device

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

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

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


--
Alex Bennée

Re: [Qemu-devel] [RESEND RFC 2/6] device_tree: introduce load_device_tree_from_sysfs

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> This function returns the host device tree blob from sysfs
> (/sys/firmware/devicetree/base).
>
> This has a runtime dependency on the dtc binary. This functionality
> is useful for platform device passthrough where the host device tree
> needs to be parsed to feed information into the guest device tree.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  1 +
>  2 files changed, 41 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..58a5329 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -117,6 +117,46 @@ fail:
>      return NULL;
>  }
>
> +/**
> + * load_device_tree_from_sysfs
> + *
> + * extract the dt blob from host sysfs
> + * this has a runtime dependency on the dtc binary
> + */
> +void *load_device_tree_from_sysfs(void)
> +{
> +    char cmd[] = "dtc -I fs -O dtb /sys/firmware/devicetree/base";
> +    FILE *pipe;
> +    void *fdt;
> +    int ret, actual_dt_size;
> +
> +    pipe = popen(cmd, "r");
> +    if (!pipe) {
> +        error_report("%s: Error when executing dtc", __func__);
> +        return NULL;
> +    }
> +    fdt = g_malloc0(FDT_MAX_SIZE);
> +    actual_dt_size = fread(fdt, 1, FDT_MAX_SIZE, pipe);
> +    pclose(pipe);

I think this looks OK but I'm wary of anything that calls out to a
external script. However it seems the other popen() case is for
migration and that looks as though it will get removed by the IO Channel
framework stuff.

There may be millage in using g_spawn_sync() as it will return you an
automatically sized buffer.

> +
> +    if (actual_dt_size == 0) {
> +        error_report("%s: could not copy host device tree in memory: %m",
> +                     __func__);
> +        goto fail;
> +    }
> +    ret = fdt_check_header(fdt);
> +    if (ret) {
> +        error_report("%s: Host dt file loaded into memory is invalid: %s",
> +                     __func__, fdt_strerror(ret));
> +        goto fail;
> +    }
> +    return fdt;
> +
> +fail:
> +    g_free(fdt);
> +    return NULL;
> +}
> +
>  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..307e53d 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -16,6 +16,7 @@
>
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
> +void *load_device_tree_from_sysfs(void);
>
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);


--
Alex Bennée

Re: [Qemu-devel] [RESEND RFC 2/6] device_tree: introduce load_device_tree_from_sysfs

[Thomas Huth]

On 19/11/15 16:22, Eric Auger wrote:
> This function returns the host device tree blob from sysfs
> (/sys/firmware/devicetree/base).
> 
> This has a runtime dependency on the dtc binary. This functionality
> is useful for platform device passthrough where the host device tree
> needs to be parsed to feed information into the guest device tree.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  1 +
>  2 files changed, 41 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..58a5329 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -117,6 +117,46 @@ fail:
>      return NULL;
>  }
>  
> +/**
> + * load_device_tree_from_sysfs
> + *
> + * extract the dt blob from host sysfs
> + * this has a runtime dependency on the dtc binary
> + */
> +void *load_device_tree_from_sysfs(void)
> +{
> +    char cmd[] = "dtc -I fs -O dtb /sys/firmware/devicetree/base";
> +    FILE *pipe;
> +    void *fdt;
> +    int ret, actual_dt_size;
> +
> +    pipe = popen(cmd, "r");
> +    if (!pipe) {
> +        error_report("%s: Error when executing dtc", __func__);
> +        return NULL;
> +    }

The Device Tree Compiler binary is normally only installed on
developer's machines, so I somewhat doubt that it is a good idea to rely
on the availability of that binary in QEMU? Maybe you should rather
extend libfdt to support such a feature?

 Thomas

Re: [Qemu-devel] [RESEND RFC 3/6] device_tree: introduce qemu_fdt_node_path

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> This new helper routine returns the node path of a device
> referred to by its name and compat string.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  3 +++
>  2 files changed, 43 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index 58a5329..f184e3c 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -171,6 +171,46 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>
> +/**
> + * qemu_fdt_node_path
> + *
> + * return the node path of a device, referred to by its node name
> + * and its compat string
> + * fdt: pointer to the dt blob
> + * name: name of the device
> + * compat: compatibility string of the device
> + *
> + * returns the node path
> + */
> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                       char **node_path)
> +{
> +    int offset = 0, len;
> +    const char *iter_name;
> +    char path[256];
> +    int ret;
> +
> +    *node_path = NULL;
> +    while (1) {
> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> +        if (offset == -FDT_ERR_NOTFOUND) {

Is this not the only error code fdt_node_offset_by_compatible() won't
return?

> +            break;
> +        }
> +        iter_name = fdt_get_name(fdt, offset, &len);
> +        if (!strncmp(iter_name, name, len)) {

is it possible for fdt_get_name to fail here and give you NULL and -len?

> +            goto found;
> +        }
> +    }
> +    return offset;
> +
> +found:
> +    ret = fdt_get_path(fdt, offset, path, 256);
> +    if (!ret) {
> +        *node_path = g_strdup(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 307e53d..f9e6e6e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  void *load_device_tree_from_sysfs(void);
>
> +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,


--
Alex Bennée

Re: [Qemu-devel] [RESEND RFC 4/6] device_tree: introduce qemu_fdt_getprop_optional

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> 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 is what this new qemu_fdt_getprop_optional function does.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  device_tree.c                | 17 +++++++++++++++++
>  include/sysemu/device_tree.h |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index f184e3c..a318683 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -280,6 +280,23 @@ const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>      return r;
>  }
>
> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
> +                             const char *property, bool optional, int *lenp)
> +{
> +    int len;
> +    const void *r;
> +    if (!lenp) {
> +        lenp = &len;
> +    }
> +    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
> +    if (!r && !optional) {
> +        error_report("%s: Couldn't get %s/%s: %s", __func__,
> +                     node_path, property, fdt_strerror(*lenp));
> +        exit(1);
> +    }
> +    return r;
> +}
> +
>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>                                 const char *property)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f9e6e6e..10cbe8e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -34,6 +34,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>                               const char *target_node_path);
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>                               const char *property, int *lenp);
> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
> +                             const char *property, bool optional, int *lenp);
>  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);


--
Alex Bennée

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

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> 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 from 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 a they get used
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  hw/arm/sysbus-fdt.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 9d28797..22e5801 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,115 @@ typedef struct NodeCreationPair {
>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>  } NodeCreationPair;
>
> +/* helpers */
> +
> +struct HostProperty {
> +    const char *name;
> +    bool optional;
> +};
> +
> +typedef struct HostProperty HostProperty;
> +
> +/**
> + * 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 exits. Not found optional ones are ignored.
> + * props: array of HostProperty to copy
> + * nb_props: number of properties in the array
> + * host_dt: host device tree blob
> + * guest_dt: guest device tree blob
> + * node_path: host dt node path where the property is supposed to be
> +              found
> + * nodename: guest node name the properties should be added to
> + *
> + */
> +static void inherit_properties(HostProperty *props, int nb_props,
> +                               void *host_fdt, void *guest_fdt,
> +                               char *node_path, char *nodename)
> +{
> +    int i, prop_len;
> +    const void *r;
> +
> +    for (i = 0; i < nb_props; i++) {
> +        r = qemu_fdt_getprop_optional(host_fdt, node_path,
> +                             props[i].name,
> +                             props[i].optional,
> +                             &prop_len);

indentation?

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

I'm seeing double here ;-)

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


--
Alex Bennée

Re: [Qemu-devel] [RESEND RFC 6/6] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> 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.
>
> Property values are inherited from host device tree. It is mandated
> host uses device tree, dtc binary is installed, and the host also uses a
> combined XGBE/PHY representation (>= 4.2 host kernel).

I might ping you later about this as I'm running 4.3 but can't see the
device. However my board could be a very old rev.

>
> 2 clock nodes (dma and ptp) also are created. It is mandated those clocks
> are fixed on host side.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  hw/arm/sysbus-fdt.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 167 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 22e5801..b13d380 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -22,6 +22,7 @@
>   */
>
>  #include <libfdt.h>
> +#include <linux/vfio.h>
>  #include "hw/arm/sysbus-fdt.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/device_tree.h"
> @@ -29,8 +30,10 @@
>  #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"
>
> +
>  /*
>   * internal struct that contains the information to create dynamic
>   * sysbus device node
> @@ -120,10 +123,7 @@ static HostProperty clock_inherited_properties[] = {
>   * host_phandle: phandle of the clock in host device tree
>   * guest_phandle: phandle to assign to the guest node
>   */
> -int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> -                                uint32_t host_phandle,
> -                                uint32_t guest_phandle);
> -int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> +static int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>                                  uint32_t host_phandle,
>                                  uint32_t guest_phandle)
>  {

This looks like it should be squashed with 5/6.

> @@ -166,6 +166,21 @@ out:
>      return ret;
>  }
>
> +/**
> + * sysfs_to_dt_name
> + *
> + * convert the name found in sysfs into the node name
> + * for instance e0900000.xgmac is converted into xgmac@e0900000
> + */
> +static char *sysfs_to_dt_name(const char *sysfs_name)
> +{
> +    gchar **substrings =  g_strsplit(sysfs_name, ".", 2);
> +    char *dt_name;
> +
> +    dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);

You need to g_strfreev(substrings) once you are done.

> +    return dt_name;
> +}
> +
>  /* Device Specific Code */
>
>  /**
> @@ -233,9 +248,157 @@ fail_reg:
>      return ret;
>  }
>
> +
> +/* AMD xgbe properties whose values are copied/pasted from host */
> +static HostProperty amd_xgbe_inherited_properties[] = {
> +    {"compatible", 0},
> +    {"dma-coherent", 1},
> +    {"amd,per-channel-interrupt", 1},
> +    {"phy-mode", 0},
> +    {"mac-address", 1},
> +    {"amd,speed-set", 0},
> +    {"amd,serdes-blwc", 1},
> +    {"amd,serdes-cdr-rate", 1},
> +    {"amd,serdes-pq-skew", 1},
> +    {"amd,serdes-tx-amp", 1},
> +    {"amd,serdes-dfe-tap-config", 1},
> +    {"amd,serdes-dfe-tap-enable", 1},
> +    {"clock-names", 0},
> +};
> +
> +/**
> + * 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();
> +    if (!host_fdt) {
> +        goto stop;
> +    }
> +    dt_name = sysfs_to_dt_name(vbasedev->name);
> +    ret = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat, &node_path);
> +    g_free(dt_name);
> +
> +    if (ret) {
> +        goto stop;
> +    }
> +
> +    /* generate nodes for DMA_CLK and PTP_CLK */
> +    r = qemu_fdt_getprop(host_fdt, node_path, "clocks", &prop_len);
> +    if (prop_len != 8) {
> +        goto stop;
> +    }
> +    host_clock_phandles = (uint32_t *)r;
> +    host_clock_phandles[0] = be32_to_cpu(host_clock_phandles[0]);
> +    host_clock_phandles[1] = be32_to_cpu(host_clock_phandles[1]);
> +    guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
> +    guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
> +
> +    ret = fdt_build_clock_node(host_fdt, guest_fdt,
> +                               host_clock_phandles[0],
> +                               guest_clock_phandles[0]);

Hmm a little comment about the endianess here would be worthwhile. It
seems odd to have a fdt function return stuff in be32 form but need it
in host cpu form in other calls.

As you only use host_clock_phandles[] once you could push the endian
conversion to the call rather than the in-place swizzle.:

        ret = fdt_build_clock_node(host_fdt, guest_fdt,
                                   be32_to_cpu(host_clock_phandles[0]),
                                   guest_clock_phandles[0]);


> +    if (ret) {
> +        goto stop;
> +    }
> +
> +    ret = fdt_build_clock_node(host_fdt, guest_fdt,
> +                               host_clock_phandles[1],
> +                               guest_clock_phandles[1]);
> +    if (ret) {
> +        goto stop;
> +    }
> +
> +    /* 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));
> +    }
> +    ret = qemu_fdt_setprop(guest_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;
> +    }
> +
> +    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);
> +        }
> +    }
> +    ret = qemu_fdt_setprop(guest_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(host_fdt);
> +    g_free(node_path);
> +    g_free(irq_attr);
> +fail_reg:
> +    g_free(reg_attr);
> +    g_free(nodename);
> +    return ret;
> +stop:
> +    exit(1);
> +}
> +
>  /* list of supported dynamic sysbus devices */
>  static const NodeCreationPair add_fdt_node_functions[] = {
>      {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
> +    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
>      {"", NULL}, /* last element */
>  };


--
Alex Bennée

Re: [Qemu-devel] [RESEND RFC 4/6] device_tree: introduce qemu_fdt_getprop_optional

[Peter Crosthwaite]

On Thu, Nov 19, 2015 at 7:22 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 is what this new qemu_fdt_getprop_optional function does.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  device_tree.c                | 17 +++++++++++++++++
>  include/sysemu/device_tree.h |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index f184e3c..a318683 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -280,6 +280,23 @@ const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>      return r;
>  }
>
> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
> +                             const char *property, bool optional, int *lenp)
> +{
> +    int len;
> +    const void *r;
> +    if (!lenp) {
> +        lenp = &len;
> +    }
> +    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
> +    if (!r && !optional) {
> +        error_report("%s: Couldn't get %s/%s: %s", __func__,
> +                     node_path, property, fdt_strerror(*lenp));
> +        exit(1);
> +    }
> +    return r;
> +}
> +

The real problem here is that the device-tree API is self-asserting.
This looks the old _nofail system that we removed but just named in
reverse. The correct solution here is to use the Error API properly.
Convert qemu_fdt_getprop to accept an Error **, and all existing users
are converted to pass &error_fatal. This will preserve existing
behaviour. Then to use the API with your optional semantic your pass
NULL for the Error **.

Regards,
Peter

>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>                                 const char *property)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f9e6e6e..10cbe8e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -34,6 +34,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>                               const char *target_node_path);
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>                               const char *property, int *lenp);
> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
> +                             const char *property, bool optional, int *lenp);
>  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.8.3.2
>
>

Re: [Qemu-devel] [RESEND RFC 2/6] device_tree: introduce load_device_tree_from_sysfs

[Eric Auger]

Hi Thomas, Alex,
On 11/26/2015 11:57 AM, Thomas Huth wrote:
> On 19/11/15 16:22, Eric Auger wrote:
>> This function returns the host device tree blob from sysfs
>> (/sys/firmware/devicetree/base).
>>
>> This has a runtime dependency on the dtc binary. This functionality
>> is useful for platform device passthrough where the host device tree
>> needs to be parsed to feed information into the guest device tree.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
>>  include/sysemu/device_tree.h |  1 +
>>  2 files changed, 41 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index a9f5f8e..58a5329 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -117,6 +117,46 @@ fail:
>>      return NULL;
>>  }
>>  
>> +/**
>> + * load_device_tree_from_sysfs
>> + *
>> + * extract the dt blob from host sysfs
>> + * this has a runtime dependency on the dtc binary
>> + */
>> +void *load_device_tree_from_sysfs(void)
>> +{
>> +    char cmd[] = "dtc -I fs -O dtb /sys/firmware/devicetree/base";
>> +    FILE *pipe;
>> +    void *fdt;
>> +    int ret, actual_dt_size;
>> +
>> +    pipe = popen(cmd, "r");
>> +    if (!pipe) {
>> +        error_report("%s: Error when executing dtc", __func__);
>> +        return NULL;
>> +    }
> 
> The Device Tree Compiler binary is normally only installed on
> developer's machines, so I somewhat doubt that it is a good idea to rely
> on the availability of that binary in QEMU? Maybe you should rather
> extend libfdt to support such a feature?

Sorry for the delay and thank you for your review/comments. According to
your feedbacks I give up my plan about using dtc binary.

Thanks

Eric
> 
>  Thomas
> 

Re: [Qemu-devel] [RESEND RFC 3/6] device_tree: introduce qemu_fdt_node_path

[Eric Auger]

On 11/26/2015 01:06 PM, Alex Bennée wrote:
> 
> Eric Auger <eric.auger@linaro.org> writes:
> 
>> This new helper routine returns the node path of a device
>> referred to by its name and compat string.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  device_tree.c                | 40 ++++++++++++++++++++++++++++++++++++++++
>>  include/sysemu/device_tree.h |  3 +++
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index 58a5329..f184e3c 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -171,6 +171,46 @@ static int findnode_nofail(void *fdt, const char *node_path)
>>      return offset;
>>  }
>>
>> +/**
>> + * qemu_fdt_node_path
>> + *
>> + * return the node path of a device, referred to by its node name
>> + * and its compat string
>> + * fdt: pointer to the dt blob
>> + * name: name of the device
>> + * compat: compatibility string of the device
>> + *
>> + * returns the node path
>> + */
>> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> +                       char **node_path)
>> +{
>> +    int offset = 0, len;
>> +    const char *iter_name;
>> +    char path[256];
>> +    int ret;
>> +
>> +    *node_path = NULL;
>> +    while (1) {
>> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
>> +        if (offset == -FDT_ERR_NOTFOUND) {
> 
> Is this not the only error code fdt_node_offset_by_compatible() won't
> return?
> 
>> +            break;
>> +        }
>> +        iter_name = fdt_get_name(fdt, offset, &len);
>> +        if (!strncmp(iter_name, name, len)) {
> 
> is it possible for fdt_get_name to fail here and give you NULL and -len?
I agree with both of your comments. Thanks for spotting my wrong
handling of the errors.

Thank you for your time!

Eric
> 
>> +            goto found;
>> +        }
>> +    }
>> +    return offset;
>> +
>> +found:
>> +    ret = fdt_get_path(fdt, offset, path, 256);
>> +    if (!ret) {
>> +        *node_path = g_strdup(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 307e53d..f9e6e6e 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep);
>>  void *load_device_tree(const char *filename_path, int *sizep);
>>  void *load_device_tree_from_sysfs(void);
>>
>> +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,
> 
> 
> --
> Alex Bennée
> 

Re: [Qemu-devel] [RESEND RFC 4/6] device_tree: introduce qemu_fdt_getprop_optional

[Eric Auger]

Hi Peter,
On 11/27/2015 08:38 PM, Peter Crosthwaite wrote:
> On Thu, Nov 19, 2015 at 7:22 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 is what this new qemu_fdt_getprop_optional function does.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  device_tree.c                | 17 +++++++++++++++++
>>  include/sysemu/device_tree.h |  2 ++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index f184e3c..a318683 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -280,6 +280,23 @@ const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>>      return r;
>>  }
>>
>> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
>> +                             const char *property, bool optional, int *lenp)
>> +{
>> +    int len;
>> +    const void *r;
>> +    if (!lenp) {
>> +        lenp = &len;
>> +    }
>> +    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
>> +    if (!r && !optional) {
>> +        error_report("%s: Couldn't get %s/%s: %s", __func__,
>> +                     node_path, property, fdt_strerror(*lenp));
>> +        exit(1);
>> +    }
>> +    return r;
>> +}
>> +
> 
> The real problem here is that the device-tree API is self-asserting.
> This looks the old _nofail system that we removed but just named in
> reverse. The correct solution here is to use the Error API properly.
> Convert qemu_fdt_getprop to accept an Error **, and all existing users
> are converted to pass &error_fatal. This will preserve existing
> behaviour. Then to use the API with your optional semantic your pass
> NULL for the Error **.
OK I will implement your proposal. Thanks for your time.

Best Regards

Eric
> 
> Regards,
> Peter
> 
>>  uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>>                                 const char *property)
>>  {
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index f9e6e6e..10cbe8e 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -34,6 +34,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>>                               const char *target_node_path);
>>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>>                               const char *property, int *lenp);
>> +const void *qemu_fdt_getprop_optional(void *fdt, const char *node_path,
>> +                             const char *property, bool optional, int *lenp);
>>  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.8.3.2
>>
>>

Re: [Qemu-devel] [RESEND RFC 6/6] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation

[Eric Auger]

On 11/26/2015 06:14 PM, Alex Bennée wrote:
> 
> Eric Auger <eric.auger@linaro.org> writes:
> 
>> 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.
>>
>> Property values are inherited from host device tree. It is mandated
>> host uses device tree, dtc binary is installed, and the host also uses a
>> combined XGBE/PHY representation (>= 4.2 host kernel).
> 
> I might ping you later about this as I'm running 4.3 but can't see the
> device. However my board could be a very old rev.
On my side it is in the smb directory.

/sys/firmware/devicetree/base/smb/xgmac@*
> 
>>
>> 2 clock nodes (dma and ptp) also are created. It is mandated those clocks
>> are fixed on host side.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  hw/arm/sysbus-fdt.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 167 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 22e5801..b13d380 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -22,6 +22,7 @@
>>   */
>>
>>  #include <libfdt.h>
>> +#include <linux/vfio.h>
>>  #include "hw/arm/sysbus-fdt.h"
>>  #include "qemu/error-report.h"
>>  #include "sysemu/device_tree.h"
>> @@ -29,8 +30,10 @@
>>  #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"
>>
>> +
>>  /*
>>   * internal struct that contains the information to create dynamic
>>   * sysbus device node
>> @@ -120,10 +123,7 @@ static HostProperty clock_inherited_properties[] = {
>>   * host_phandle: phandle of the clock in host device tree
>>   * guest_phandle: phandle to assign to the guest node
>>   */
>> -int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> -                                uint32_t host_phandle,
>> -                                uint32_t guest_phandle);
>> -int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +static int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>>                                  uint32_t host_phandle,
>>                                  uint32_t guest_phandle)
>>  {
> 
> This looks like it should be squashed with 5/6.
Sure ;-)
> 
>> @@ -166,6 +166,21 @@ out:
>>      return ret;
>>  }
>>
>> +/**
>> + * sysfs_to_dt_name
>> + *
>> + * convert the name found in sysfs into the node name
>> + * for instance e0900000.xgmac is converted into xgmac@e0900000
>> + */
>> +static char *sysfs_to_dt_name(const char *sysfs_name)
>> +{
>> +    gchar **substrings =  g_strsplit(sysfs_name, ".", 2);
>> +    char *dt_name;
>> +
>> +    dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
> 
> You need to g_strfreev(substrings) once you are done.
sure!
> 
>> +    return dt_name;
>> +}
>> +
>>  /* Device Specific Code */
>>
>>  /**
>> @@ -233,9 +248,157 @@ fail_reg:
>>      return ret;
>>  }
>>
>> +
>> +/* AMD xgbe properties whose values are copied/pasted from host */
>> +static HostProperty amd_xgbe_inherited_properties[] = {
>> +    {"compatible", 0},
>> +    {"dma-coherent", 1},
>> +    {"amd,per-channel-interrupt", 1},
>> +    {"phy-mode", 0},
>> +    {"mac-address", 1},
>> +    {"amd,speed-set", 0},
>> +    {"amd,serdes-blwc", 1},
>> +    {"amd,serdes-cdr-rate", 1},
>> +    {"amd,serdes-pq-skew", 1},
>> +    {"amd,serdes-tx-amp", 1},
>> +    {"amd,serdes-dfe-tap-config", 1},
>> +    {"amd,serdes-dfe-tap-enable", 1},
>> +    {"clock-names", 0},
>> +};
>> +
>> +/**
>> + * 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();
>> +    if (!host_fdt) {
>> +        goto stop;
>> +    }
>> +    dt_name = sysfs_to_dt_name(vbasedev->name);
>> +    ret = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat, &node_path);
>> +    g_free(dt_name);
>> +
>> +    if (ret) {
>> +        goto stop;
>> +    }
>> +
>> +    /* generate nodes for DMA_CLK and PTP_CLK */
>> +    r = qemu_fdt_getprop(host_fdt, node_path, "clocks", &prop_len);
>> +    if (prop_len != 8) {
>> +        goto stop;
>> +    }
>> +    host_clock_phandles = (uint32_t *)r;
>> +    host_clock_phandles[0] = be32_to_cpu(host_clock_phandles[0]);
>> +    host_clock_phandles[1] = be32_to_cpu(host_clock_phandles[1]);
>> +    guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
>> +    guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
>> +
>> +    ret = fdt_build_clock_node(host_fdt, guest_fdt,
>> +                               host_clock_phandles[0],
>> +                               guest_clock_phandles[0]);
> 
> Hmm a little comment about the endianess here would be worthwhile. It
> seems odd to have a fdt function return stuff in be32 form but need it
> in host cpu form in other calls.
> 
> As you only use host_clock_phandles[] once you could push the endian
> conversion to the call rather than the in-place swizzle.:
OK

Thanks!

Eric
> 
>         ret = fdt_build_clock_node(host_fdt, guest_fdt,
>                                    be32_to_cpu(host_clock_phandles[0]),
>                                    guest_clock_phandles[0]);
> 
> 
>> +    if (ret) {
>> +        goto stop;
>> +    }
>> +
>> +    ret = fdt_build_clock_node(host_fdt, guest_fdt,
>> +                               host_clock_phandles[1],
>> +                               guest_clock_phandles[1]);
>> +    if (ret) {
>> +        goto stop;
>> +    }
>> +
>> +    /* 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));
>> +    }
>> +    ret = qemu_fdt_setprop(guest_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;
>> +    }
>> +
>> +    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);
>> +        }
>> +    }
>> +    ret = qemu_fdt_setprop(guest_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(host_fdt);
>> +    g_free(node_path);
>> +    g_free(irq_attr);
>> +fail_reg:
>> +    g_free(reg_attr);
>> +    g_free(nodename);
>> +    return ret;
>> +stop:
>> +    exit(1);
>> +}
>> +
>>  /* list of supported dynamic sysbus devices */
>>  static const NodeCreationPair add_fdt_node_functions[] = {
>>      {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
>> +    {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
>>      {"", NULL}, /* last element */
>>  };
> 
> 
> --
> Alex Bennée
> 

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

[Eric Auger]

Hi Alex (B),
On 11/26/2015 05:06 PM, Alex Bennée wrote:
> 
> Eric Auger <eric.auger@linaro.org> writes:
> 
>> 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 from 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 a they get used
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  hw/arm/sysbus-fdt.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 110 insertions(+)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 9d28797..22e5801 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,115 @@ typedef struct NodeCreationPair {
>>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>  } NodeCreationPair;
>>
>> +/* helpers */
>> +
>> +struct HostProperty {
>> +    const char *name;
>> +    bool optional;
>> +};
>> +
>> +typedef struct HostProperty HostProperty;
>> +
>> +/**
>> + * 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 exits. Not found optional ones are ignored.
>> + * props: array of HostProperty to copy
>> + * nb_props: number of properties in the array
>> + * host_dt: host device tree blob
>> + * guest_dt: guest device tree blob
>> + * node_path: host dt node path where the property is supposed to be
>> +              found
>> + * nodename: guest node name the properties should be added to
>> + *
>> + */
>> +static void inherit_properties(HostProperty *props, int nb_props,
>> +                               void *host_fdt, void *guest_fdt,
>> +                               char *node_path, char *nodename)
>> +{
>> +    int i, prop_len;
>> +    const void *r;
>> +
>> +    for (i = 0; i < nb_props; i++) {
>> +        r = qemu_fdt_getprop_optional(host_fdt, node_path,
>> +                             props[i].name,
>> +                             props[i].optional,
>> +                             &prop_len);
> 
> indentation?
> 
>> +        if (r) {
>> +            qemu_fdt_setprop(guest_fdt, nodename,
>> +                             props[i].name, r, prop_len);
>> +        }
>> +    }
>> +}
>> +
>> +/* clock properties whose values are copied/pasted from host */
>> +static HostProperty clock_inherited_properties[] = {
>> +    {"compatible", 0},
>> +    {"#clock-cells", 0},
>> +    {"clock-frequency", 1},
>> +    {"clock-output-names", 1},
>> +};
>> +
>> +/**
>> + * fdt_build_clock_node
>> + *
>> + * Build a guest clock node, used as a dependency from a passthrough'ed
>> + * device. Most information are retrieved from the host fellow node.
>> + * Also check the host clock is a fixed one.
>> + *
>> + * host_fdt: host device tree blob from which info are retrieved
>> + * guest_fdt: guest device tree blob where the clock node is added
>> + * host_phandle: phandle of the clock in host device tree
>> + * guest_phandle: phandle to assign to the guest node
>> + */
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +                                uint32_t host_phandle,
>> +                                uint32_t guest_phandle);
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +                                uint32_t host_phandle,
>> +                                uint32_t guest_phandle)
> 
> I'm seeing double here ;-)
I am coming back to you wrt this comment. This function will become
static but in that patch it is not used yet so compilation fails (the
function starts being used in next path file). That's why I did not use
the static here. Then the compiler complains the function needs to be
pre-declared. Hence the declaration here. In next patch that disappears
and becomes static. Is that acceptable? If I squash this patch in next
one, this becomes a huge one.

Best Regards

Eric
> 
>> +{
>> +    char node_path[256];
>> +    char *nodename;
>> +    const void *r;
>> +    int ret, prop_len;
>> +
>> +    ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>> +    if (ret <= 0) {
>> +        error_report("not able to locate clock handle %d in host device tree\n",
>> +                     host_phandle);
>> +        goto out;
>> +    }
>> +    ret = fdt_get_path(host_fdt, ret, node_path, 256);
>> +    if (ret < 0) {
>> +        error_report("not able to retrieve node path for clock handle %d\n",
>> +                     host_phandle);
>> +        goto out;
>> +    }
>> +
>> +    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len);
>> +    if (strcmp(r, "fixed-clock")) {
>> +        error_report("clock handle %d is not a fixed clock\n", host_phandle);
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    nodename = strrchr(node_path, '/');
>> +    qemu_fdt_add_subnode(guest_fdt, nodename);
>> +
>> +    inherit_properties(clock_inherited_properties,
>> +                       ARRAY_SIZE(clock_inherited_properties),
>> +                       host_fdt, guest_fdt,
>> +                       node_path, nodename);
>> +
>> +    qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
>> +
>> +out:
>> +    return ret;
>> +}
>> +
>>  /* Device Specific Code */
>>
>>  /**
> 
> 
> --
> Alex Bennée
> 

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

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> Hi Alex (B),
> On 11/26/2015 05:06 PM, Alex Bennée wrote:
>>
>> Eric Auger <eric.auger@linaro.org> writes:
>>
>>> 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 from 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 a they get used
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>> ---
>>>  hw/arm/sysbus-fdt.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 110 insertions(+)
>>>
>>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>> index 9d28797..22e5801 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,115 @@ typedef struct NodeCreationPair {
>>>      int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>>  } NodeCreationPair;
>>>
>>> +/* helpers */
>>> +
>>> +struct HostProperty {
>>> +    const char *name;
>>> +    bool optional;
>>> +};
>>> +
>>> +typedef struct HostProperty HostProperty;
>>> +
>>> +/**
>>> + * 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 exits. Not found optional ones are ignored.
>>> + * props: array of HostProperty to copy
>>> + * nb_props: number of properties in the array
>>> + * host_dt: host device tree blob
>>> + * guest_dt: guest device tree blob
>>> + * node_path: host dt node path where the property is supposed to be
>>> +              found
>>> + * nodename: guest node name the properties should be added to
>>> + *
>>> + */
>>> +static void inherit_properties(HostProperty *props, int nb_props,
>>> +                               void *host_fdt, void *guest_fdt,
>>> +                               char *node_path, char *nodename)
>>> +{
>>> +    int i, prop_len;
>>> +    const void *r;
>>> +
>>> +    for (i = 0; i < nb_props; i++) {
>>> +        r = qemu_fdt_getprop_optional(host_fdt, node_path,
>>> +                             props[i].name,
>>> +                             props[i].optional,
>>> +                             &prop_len);
>>
>> indentation?
>>
>>> +        if (r) {
>>> +            qemu_fdt_setprop(guest_fdt, nodename,
>>> +                             props[i].name, r, prop_len);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +/* clock properties whose values are copied/pasted from host */
>>> +static HostProperty clock_inherited_properties[] = {
>>> +    {"compatible", 0},
>>> +    {"#clock-cells", 0},
>>> +    {"clock-frequency", 1},
>>> +    {"clock-output-names", 1},
>>> +};
>>> +
>>> +/**
>>> + * fdt_build_clock_node
>>> + *
>>> + * Build a guest clock node, used as a dependency from a passthrough'ed
>>> + * device. Most information are retrieved from the host fellow node.
>>> + * Also check the host clock is a fixed one.
>>> + *
>>> + * host_fdt: host device tree blob from which info are retrieved
>>> + * guest_fdt: guest device tree blob where the clock node is added
>>> + * host_phandle: phandle of the clock in host device tree
>>> + * guest_phandle: phandle to assign to the guest node
>>> + */
>>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>>> +                                uint32_t host_phandle,
>>> +                                uint32_t guest_phandle);
>>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>>> +                                uint32_t host_phandle,
>>> +                                uint32_t guest_phandle)
>>
>> I'm seeing double here ;-)
> I am coming back to you wrt this comment. This function will become
> static but in that patch it is not used yet so compilation fails (the
> function starts being used in next path file). That's why I did not use
> the static here. Then the compiler complains the function needs to be
> pre-declared. Hence the declaration here. In next patch that disappears
> and becomes static. Is that acceptable? If I squash this patch in next
> one, this becomes a huge one.

Usually I would expect to see a pre-declaration of a function at the
head of the file and only if it is used before the actual definition of
the function. It doesn't make sense to pre-declare right before the
actual function definition itself.

I'm surprised to hear the compiler complained, especially as nothing was
calling this function in this patch.

>
> Best Regards
>
> Eric
>>
>>> +{
>>> +    char node_path[256];
>>> +    char *nodename;
>>> +    const void *r;
>>> +    int ret, prop_len;
>>> +
>>> +    ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>>> +    if (ret <= 0) {
>>> +        error_report("not able to locate clock handle %d in host device tree\n",
>>> +                     host_phandle);
>>> +        goto out;
>>> +    }
>>> +    ret = fdt_get_path(host_fdt, ret, node_path, 256);
>>> +    if (ret < 0) {
>>> +        error_report("not able to retrieve node path for clock handle %d\n",
>>> +                     host_phandle);
>>> +        goto out;
>>> +    }
>>> +
>>> +    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len);
>>> +    if (strcmp(r, "fixed-clock")) {
>>> +        error_report("clock handle %d is not a fixed clock\n", host_phandle);
>>> +        ret = -1;
>>> +        goto out;
>>> +    }
>>> +
>>> +    nodename = strrchr(node_path, '/');
>>> +    qemu_fdt_add_subnode(guest_fdt, nodename);
>>> +
>>> +    inherit_properties(clock_inherited_properties,
>>> +                       ARRAY_SIZE(clock_inherited_properties),
>>> +                       host_fdt, guest_fdt,
>>> +                       node_path, nodename);
>>> +
>>> +    qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
>>> +
>>> +out:
>>> +    return ret;
>>> +}
>>> +
>>>  /* Device Specific Code */
>>>
>>>  /**
>>
>>
>> --
>> Alex Bennée
>>


--
Alex Bennée

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

[Peter Maydell]

On 17 December 2015 at 13:28, Alex Bennée <alex.bennee@linaro.org> wrote:
> Usually I would expect to see a pre-declaration of a function at the
> head of the file and only if it is used before the actual definition of
> the function. It doesn't make sense to pre-declare right before the
> actual function definition itself.
>
> I'm surprised to hear the compiler complained, especially as nothing was
> calling this function in this patch.

The compiler complains if it sees a function which is not static
and for which it hasn't previously seen a prototype, because
generally this means that either (a) the function is file-local
only and should have been declared static or (b) the function is
not file-local but you forgot to put a prototype in a header so
that other files can call it. (This is -Wmissing-prototypes.)

thanks
-- PMM

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

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 17 December 2015 at 13:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Usually I would expect to see a pre-declaration of a function at the
>> head of the file and only if it is used before the actual definition of
>> the function. It doesn't make sense to pre-declare right before the
>> actual function definition itself.
>>
>> I'm surprised to hear the compiler complained, especially as nothing was
>> calling this function in this patch.
>
> The compiler complains if it sees a function which is not static
> and for which it hasn't previously seen a prototype, because
> generally this means that either (a) the function is file-local
> only and should have been declared static or (b) the function is
> not file-local but you forgot to put a prototype in a header so
> that other files can call it. (This is -Wmissing-prototypes.)


Ahh I see now. I guess if its declared static in this patch and not
used its going to complain about an unused function as well? Maybe that
suggests the patch should just be merged with patch where it is actually
used?

>
> thanks
> -- PMM


--
Alex Bennée

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

[Eric Auger]

Hi Alex,
On 12/17/2015 04:13 PM, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 17 December 2015 at 13:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> Usually I would expect to see a pre-declaration of a function at the
>>> head of the file and only if it is used before the actual definition of
>>> the function. It doesn't make sense to pre-declare right before the
>>> actual function definition itself.
>>>
>>> I'm surprised to hear the compiler complained, especially as nothing was
>>> calling this function in this patch.
>>
>> The compiler complains if it sees a function which is not static
>> and for which it hasn't previously seen a prototype, because
>> generally this means that either (a) the function is file-local
>> only and should have been declared static or (b) the function is
>> not file-local but you forgot to put a prototype in a header so
>> that other files can call it. (This is -Wmissing-prototypes.)
> 
> 
> Ahh I see now. I guess if its declared static in this patch and not
> used its going to complain about an unused function as well? Maybe that
> suggests the patch should just be merged with patch where it is actually
> used?

my fear is that it becomes too big for review then. I suggest we wait
for other comments and I will follow the consensus if any. I just wanted
to emphasize I did not ignore your comment but I just don't know how to
handle it at best ;-)

Thanks for your time!

Regards

Eric
> 
>>
>> thanks
>> -- PMM
> 
> 
> --
> Alex Bennée
> 

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

[Alex Bennée]

Eric Auger <eric.auger@linaro.org> writes:

> Hi Alex,
> On 12/17/2015 04:13 PM, Alex Bennée wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 17 December 2015 at 13:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>> Usually I would expect to see a pre-declaration of a function at the
>>>> head of the file and only if it is used before the actual definition of
>>>> the function. It doesn't make sense to pre-declare right before the
>>>> actual function definition itself.
>>>>
>>>> I'm surprised to hear the compiler complained, especially as nothing was
>>>> calling this function in this patch.
>>>
>>> The compiler complains if it sees a function which is not static
>>> and for which it hasn't previously seen a prototype, because
>>> generally this means that either (a) the function is file-local
>>> only and should have been declared static or (b) the function is
>>> not file-local but you forgot to put a prototype in a header so
>>> that other files can call it. (This is -Wmissing-prototypes.)
>>
>>
>> Ahh I see now. I guess if its declared static in this patch and not
>> used its going to complain about an unused function as well? Maybe that
>> suggests the patch should just be merged with patch where it is actually
>> used?
>
> my fear is that it becomes too big for review then.

It's a valid concern although I think in this case your patches are
fairly well contained.

> I suggest we wait
> for other comments and I will follow the consensus if any. I just wanted
> to emphasize I did not ignore your comment but I just don't know how to
> handle it at best ;-)

It's OK, there is often dark compromise involved in keeping the compiler
gods happy - as long as it compiles clean ;-)

>
> Thanks for your time!
>
> Regards
>
> Eric
>>
>>>
>>> thanks
>>> -- PMM
>>
>>
>> --
>> Alex Bennée
>>


--
Alex Bennée