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

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

[Eric Auger]

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

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

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

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

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

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

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

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

Best Regards

Eric


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

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

-- 
1.9.1

[Qemu-devel] [PATCH 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>
Reviewed-by: Alex Benné<alex.bennee@linaro.org>

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

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

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

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> This patch introduces the amd-xgbe VFIO platform device. It
> allows the guest to do passthrough on a device exposing an
> "amd,xgbe-seattle-v1a" compat string.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> Reviewed-by: Alex Benné<alex.bennee@linaro.org>

You've typo'd Alex's name here ("Bennée"); would be good to
fix if you need to do another round.

thanks
-- PMM

[Qemu-devel] [PATCH 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). It uses a recursive function
inspired from dtc read_fstree.

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

---

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

diff --git a/device_tree.c b/device_tree.c
index a9f5f8e..e556a99 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -17,6 +17,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <dirent.h>
 
 #include "qemu-common.h"
 #include "qemu/error-report.h"
@@ -117,6 +118,107 @@ fail:
     return NULL;
 }
 
+/**
+ * read_fstree: this function is inspired from dtc read_fstree
+ * @fdt: preallocated fdt blob buffer, to be populated
+ * @dirname: directory to scan under /sys/firmware/devicetree/base
+ * the search is recursive and the tree is search down to the
+ * leafs (property files).
+ *
+ * the function self-asserts in case of error
+ */
+static void read_fstree(void *fdt, const char *dirname)
+{
+        DIR *d;
+        struct dirent *de;
+        struct stat st;
+        const char *root_dir = "/sys/firmware/devicetree/base";
+        char *parent_node;
+
+        if (strstr(dirname, root_dir) != dirname) {
+            error_report("%s: %s must be searched within %s",
+                         __func__, dirname, root_dir);
+            exit(1);
+        }
+        parent_node = (char *)&dirname[29];
+
+        d = opendir(dirname);
+        if (!d) {
+                error_report("%s cannot open %s", __func__, dirname);
+                exit(1);
+        }
+
+        while ((de = readdir(d)) != NULL) {
+                char *tmpnam;
+
+                if (!g_strcmp0(de->d_name, ".")
+                    || !g_strcmp0(de->d_name, "..")) {
+                        continue;
+                }
+
+                tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
+
+                if (lstat(tmpnam, &st) < 0) {
+                        error_report("%s cannot lstat %s", __func__, tmpnam);
+                        exit(1);
+                }
+
+                if (S_ISREG(st.st_mode)) {
+                    int ret, size = st.st_size;
+                    void *val = g_malloc0(size);
+                    FILE *pfile;
+
+                    pfile = fopen(tmpnam, "r");
+                    if (!pfile) {
+                        error_report("%s cannot open %s", __func__, tmpnam);
+                        exit(1);
+                    }
+                    ret = fread(val, 1, size, pfile);
+                    if (ferror(pfile) || ret < size) {
+                        error_report("%s fail reading %s", __func__, tmpnam);
+                        exit(1);
+                    }
+                    fclose(pfile);
+
+                    if (strlen(parent_node) > 0) {
+                        qemu_fdt_setprop(fdt, parent_node,
+                                         de->d_name, val, size);
+                    } else {
+                        qemu_fdt_setprop(fdt, "/", de->d_name, val, size);
+                    }
+                    g_free(val);
+                } else if (S_ISDIR(st.st_mode)) {
+                        char *node_name;
+
+                        node_name = g_strdup_printf("%s/%s",
+                                                    parent_node, de->d_name);
+                        qemu_fdt_add_subnode(fdt, node_name);
+                        g_free(node_name);
+                        read_fstree(fdt, tmpnam);
+                }
+
+                g_free(tmpnam);
+        }
+
+        closedir(d);
+}
+
+/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
+void *load_device_tree_from_sysfs(void)
+{
+    void *host_fdt;
+    int host_fdt_size;
+
+    host_fdt = create_device_tree(&host_fdt_size);
+    read_fstree(host_fdt, "/sys/firmware/devicetree/base");
+    if (fdt_check_header(host_fdt)) {
+        error_report("%s host device tree extracted into memory is invalid",
+                     __func__);
+        g_free(host_fdt);
+    }
+    return host_fdt;
+}
+
 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.9.1

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

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> This function returns the host device tree blob from sysfs
> (/sys/firmware/devicetree/base). It uses a recursive function
> inspired from dtc read_fstree.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC -> v1:
> - remove runtime dependency on dtc binary and introduce read_fstree
> ---
>  device_tree.c                | 102 +++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |   1 +
>  2 files changed, 103 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..e556a99 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -17,6 +17,7 @@
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#include <dirent.h>

Does this code compile on non-Linux hosts? (You've put it in a file
which is built everywhere, but it's definitely semantically Linux
specific.)

>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -117,6 +118,107 @@ fail:
>      return NULL;
>  }
>
> +/**
> + * read_fstree: this function is inspired from dtc read_fstree
> + * @fdt: preallocated fdt blob buffer, to be populated
> + * @dirname: directory to scan under /sys/firmware/devicetree/base
> + * the search is recursive and the tree is search down to the
> + * leafs (property files).
> + *
> + * the function self-asserts in case of error
> + */
> +static void read_fstree(void *fdt, const char *dirname)
> +{
> +        DIR *d;
> +        struct dirent *de;

Indent here doesn't match QEMU coding style, which is four-space.

> +        struct stat st;
> +        const char *root_dir = "/sys/firmware/devicetree/base";

You use this string twice and its length once so it would be nice
to have it in a #define.

> +        char *parent_node;
> +
> +        if (strstr(dirname, root_dir) != dirname) {
> +            error_report("%s: %s must be searched within %s",
> +                         __func__, dirname, root_dir);
> +            exit(1);
> +        }
> +        parent_node = (char *)&dirname[29];

I think 29 here should be strlen(SYSFS_DT_BASEDIR) or whatever
you want to call it.

> +
> +        d = opendir(dirname);
> +        if (!d) {
> +                error_report("%s cannot open %s", __func__, dirname);
> +                exit(1);
> +        }
> +
> +        while ((de = readdir(d)) != NULL) {
> +                char *tmpnam;
> +
> +                if (!g_strcmp0(de->d_name, ".")
> +                    || !g_strcmp0(de->d_name, "..")) {
> +                        continue;
> +                }

If you used glib g_dir_open/g_dir_read_name/g_dir_close it would
automatically skip '.' and '..' for you, but I'm not sure the
benefit is enough to bother redoing this code now.

> +
> +                tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
> +
> +                if (lstat(tmpnam, &st) < 0) {
> +                        error_report("%s cannot lstat %s", __func__, tmpnam);
> +                        exit(1);
> +                }
> +
> +                if (S_ISREG(st.st_mode)) {
> +                    int ret, size = st.st_size;
> +                    void *val = g_malloc0(size);
> +                    FILE *pfile;
> +
> +                    pfile = fopen(tmpnam, "r");
> +                    if (!pfile) {
> +                        error_report("%s cannot open %s", __func__, tmpnam);
> +                        exit(1);
> +                    }
> +                    ret = fread(val, 1, size, pfile);
> +                    if (ferror(pfile) || ret < size) {
> +                        error_report("%s fail reading %s", __func__, tmpnam);
> +                        exit(1);
> +                    }
> +                    fclose(pfile);

This looks like it's reimplementing g_file_get_contents().

> +
> +                    if (strlen(parent_node) > 0) {
> +                        qemu_fdt_setprop(fdt, parent_node,
> +                                         de->d_name, val, size);
> +                    } else {
> +                        qemu_fdt_setprop(fdt, "/", de->d_name, val, size);
> +                    }
> +                    g_free(val);
> +                } else if (S_ISDIR(st.st_mode)) {
> +                        char *node_name;
> +
> +                        node_name = g_strdup_printf("%s/%s",
> +                                                    parent_node, de->d_name);
> +                        qemu_fdt_add_subnode(fdt, node_name);
> +                        g_free(node_name);
> +                        read_fstree(fdt, tmpnam);
> +                }
> +
> +                g_free(tmpnam);
> +        }
> +
> +        closedir(d);
> +}
> +
> +/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
> +void *load_device_tree_from_sysfs(void)
> +{
> +    void *host_fdt;
> +    int host_fdt_size;
> +
> +    host_fdt = create_device_tree(&host_fdt_size);
> +    read_fstree(host_fdt, "/sys/firmware/devicetree/base");
> +    if (fdt_check_header(host_fdt)) {
> +        error_report("%s host device tree extracted into memory is invalid",
> +                     __func__);
> +        g_free(host_fdt);

Why do we exit-on-error for the errors inside read_fstree() but
plough on (returning a pointer to freed memory!) in this case?

> +    }
> +    return host_fdt;
> +}
> +
>  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.9.1

thanks
-- PMM

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

[Eric Auger]

Hi Peter,
On 12/18/2015 03:10 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
>> This function returns the host device tree blob from sysfs
>> (/sys/firmware/devicetree/base). It uses a recursive function
>> inspired from dtc read_fstree.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> RFC -> v1:
>> - remove runtime dependency on dtc binary and introduce read_fstree
>> ---
>>  device_tree.c                | 102 +++++++++++++++++++++++++++++++++++++++++++
>>  include/sysemu/device_tree.h |   1 +
>>  2 files changed, 103 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index a9f5f8e..e556a99 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -17,6 +17,7 @@
>>  #include <fcntl.h>
>>  #include <unistd.h>
>>  #include <stdlib.h>
>> +#include <dirent.h>
> 
> Does this code compile on non-Linux hosts? (You've put it in a file
> which is built everywhere, but it's definitely semantically Linux
> specific.)

I struggled quite a lot while cross-compiling all dependencies for W32
(~ http://wiki.qemu.org/Hosts/W32).

Eventually device_tree.c compiles but there is a link issue since lstat
does not seem to be available with MinGW

But there is definitively a problem with hw/arm/sysbus-fdt.c which is
not compiling due to the inclusion of #include <linux/vfio.h>

So thanks for raising the concern.

With respect to read_fstree, what is your sugestion: shall I keep it in
device_tree.c while protecting it with a CONFIG_LINUX or is it better to
move it, for instance in hw/arm/sysbus-fdt.c?

> 
>>  #include "qemu-common.h"
>>  #include "qemu/error-report.h"
>> @@ -117,6 +118,107 @@ fail:
>>      return NULL;
>>  }
>>
>> +/**
>> + * read_fstree: this function is inspired from dtc read_fstree
>> + * @fdt: preallocated fdt blob buffer, to be populated
>> + * @dirname: directory to scan under /sys/firmware/devicetree/base
>> + * the search is recursive and the tree is search down to the
>> + * leafs (property files).
>> + *
>> + * the function self-asserts in case of error
>> + */
>> +static void read_fstree(void *fdt, const char *dirname)
>> +{
>> +        DIR *d;
>> +        struct dirent *de;
> 
> Indent here doesn't match QEMU coding style, which is four-space.
OK
> 
>> +        struct stat st;
>> +        const char *root_dir = "/sys/firmware/devicetree/base";
> 
> You use this string twice and its length once so it would be nice
> to have it in a #define.
OK
> 
>> +        char *parent_node;
>> +
>> +        if (strstr(dirname, root_dir) != dirname) {
>> +            error_report("%s: %s must be searched within %s",
>> +                         __func__, dirname, root_dir);
>> +            exit(1);
>> +        }
>> +        parent_node = (char *)&dirname[29];
> 
> I think 29 here should be strlen(SYSFS_DT_BASEDIR) or whatever
> you want to call it.
OK
> 
>> +
>> +        d = opendir(dirname);
>> +        if (!d) {
>> +                error_report("%s cannot open %s", __func__, dirname);
>> +                exit(1);
>> +        }
>> +
>> +        while ((de = readdir(d)) != NULL) {
>> +                char *tmpnam;
>> +
>> +                if (!g_strcmp0(de->d_name, ".")
>> +                    || !g_strcmp0(de->d_name, "..")) {
>> +                        continue;
>> +                }
> 
> If you used glib g_dir_open/g_dir_read_name/g_dir_close it would
> automatically skip '.' and '..' for you, but I'm not sure the
> benefit is enough to bother redoing this code now.
OK thanks for the hint
> 
>> +
>> +                tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
>> +
>> +                if (lstat(tmpnam, &st) < 0) {
>> +                        error_report("%s cannot lstat %s", __func__, tmpnam);
>> +                        exit(1);
>> +                }
>> +
>> +                if (S_ISREG(st.st_mode)) {
>> +                    int ret, size = st.st_size;
>> +                    void *val = g_malloc0(size);
>> +                    FILE *pfile;
>> +
>> +                    pfile = fopen(tmpnam, "r");
>> +                    if (!pfile) {
>> +                        error_report("%s cannot open %s", __func__, tmpnam);
>> +                        exit(1);
>> +                    }
>> +                    ret = fread(val, 1, size, pfile);
>> +                    if (ferror(pfile) || ret < size) {
>> +                        error_report("%s fail reading %s", __func__, tmpnam);
>> +                        exit(1);
>> +                    }
>> +                    fclose(pfile);
> 
> This looks like it's reimplementing g_file_get_contents().
OK
> 
>> +
>> +                    if (strlen(parent_node) > 0) {
>> +                        qemu_fdt_setprop(fdt, parent_node,
>> +                                         de->d_name, val, size);
>> +                    } else {
>> +                        qemu_fdt_setprop(fdt, "/", de->d_name, val, size);
>> +                    }
>> +                    g_free(val);
>> +                } else if (S_ISDIR(st.st_mode)) {
>> +                        char *node_name;
>> +
>> +                        node_name = g_strdup_printf("%s/%s",
>> +                                                    parent_node, de->d_name);
>> +                        qemu_fdt_add_subnode(fdt, node_name);
>> +                        g_free(node_name);
>> +                        read_fstree(fdt, tmpnam);
>> +                }
>> +
>> +                g_free(tmpnam);
>> +        }
>> +
>> +        closedir(d);
>> +}
>> +
>> +/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
>> +void *load_device_tree_from_sysfs(void)
>> +{
>> +    void *host_fdt;
>> +    int host_fdt_size;
>> +
>> +    host_fdt = create_device_tree(&host_fdt_size);
>> +    read_fstree(host_fdt, "/sys/firmware/devicetree/base");
>> +    if (fdt_check_header(host_fdt)) {
>> +        error_report("%s host device tree extracted into memory is invalid",
>> +                     __func__);
>> +        g_free(host_fdt);
> 
> Why do we exit-on-error for the errors inside read_fstree() but
> plough on (returning a pointer to freed memory!) in this case?
Yes I can do that. I was doing something similar as in load_device_tree

Best Regards

Eric
> 
>> +    }
>> +    return host_fdt;
>> +}
>> +
>>  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.9.1
> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On 4 January 2016 at 17:37, Eric Auger <eric.auger@linaro.org> wrote:
> Hi Peter,
> On 12/18/2015 03:10 PM, Peter Maydell wrote:
>> Does this code compile on non-Linux hosts? (You've put it in a file
>> which is built everywhere, but it's definitely semantically Linux
>> specific.)
>
> I struggled quite a lot while cross-compiling all dependencies for W32
> (~ http://wiki.qemu.org/Hosts/W32).
>
> Eventually device_tree.c compiles but there is a link issue since lstat
> does not seem to be available with MinGW
>
> But there is definitively a problem with hw/arm/sysbus-fdt.c which is
> not compiling due to the inclusion of #include <linux/vfio.h>
>
> So thanks for raising the concern.
>
> With respect to read_fstree, what is your sugestion: shall I keep it in
> device_tree.c while protecting it with a CONFIG_LINUX or is it better to
> move it, for instance in hw/arm/sysbus-fdt.c?

I don't have a strong opinion, but I don't think this code
is arm-specific, so hw/arm doesn't sound quite right.
A CONFIG_LINUX ifdef might be simplest if there's no obvious
other file to put this.

thanks
-- PMM

[Qemu-devel] [PATCH 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 node name and compat string.

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

---

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

diff --git a/device_tree.c b/device_tree.c
index e556a99..b5d7e0b 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path)
     return offset;
 }
 
+/**
+ * qemu_fdt_node_path
+ *
+ * return the node path of a device, given its node name and its
+ * compat string
+ * fdt: pointer to the dt blob
+ * name: device node name
+ * compat: compatibility string of the device
+ *
+ * upon success, the path is output at node_path address
+ * returns 0 on success, < 0 on failure
+ */
+int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+                       char **node_path)
+{
+    int offset, len, ret;
+    const char *iter_name;
+    char path[256];
+
+    *node_path = NULL;
+    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
+    while (offset != -FDT_ERR_NOTFOUND) {
+        if (offset < 0) {
+            continue;
+        }
+        iter_name = fdt_get_name(fdt, offset, &len);
+        if (!iter_name) {
+            continue;
+        }
+
+        if (!strncmp(iter_name, name, len)) {
+            goto found;
+        }
+        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
+    }
+    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.9.1

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

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> This new helper routine returns the node path of a device
> referred to by its node name and compat string.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC -> v1:
> - improve error handling according to Alex' comments
> ---
>  device_tree.c                | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  3 +++
>  2 files changed, 48 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index e556a99..b5d7e0b 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>
> +/**
> + * qemu_fdt_node_path
> + *
> + * return the node path of a device, given its node name and its
> + * compat string
> + * fdt: pointer to the dt blob
> + * name: device node name
> + * compat: compatibility string of the device
> + *
> + * upon success, the path is output at node_path address
> + * returns 0 on success, < 0 on failure
> + */

Can we put the doc comment in the header file, since this is
a globally visible function? Also it would be nice to follow the
doc-comment syntax standards about marking up arguments with '@'
and so on.

> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                       char **node_path)
> +{
> +    int offset, len, ret;
> +    const char *iter_name;
> +    char path[256];

Rather than a fixed buffer size, we should check whether
fdt_get_path returns -FDT_ERR_NOSPACE and if so enlarge the
buffer and try again.

> +
> +    *node_path = NULL;
> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
> +    while (offset != -FDT_ERR_NOTFOUND) {
> +        if (offset < 0) {
> +            continue;

I don't understand this continue -- if the fdt function returned any
error other than -FDT_ERR_NOTFOUND then this will cause us to go
into an infinite loop around this while(). Did you mean 'break' ?
(Though if you just want to break then fixing the while condition
would be better.)

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

This also seems like it ought to be a break, except you need to
set offset to the error code first (which fdt_get_name() will
have returned in 'len').

> +        }
> +
> +        if (!strncmp(iter_name, name, len)) {

Do we really want strncmp rather than strcmp here ? (ie
"find first node whose name has 'name' as a prefix" rather
than "find first node whose name matches 'name').

> +            goto found;
> +        }
> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> +    }
> +    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.9.1

thanks
-- PMM

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

[Eric Auger]

Hi Peter,
On 12/18/2015 03:23 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
>> This new helper routine returns the node path of a device
>> referred to by its node name and compat string.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> RFC -> v1:
>> - improve error handling according to Alex' comments
>> ---
>>  device_tree.c                | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/sysemu/device_tree.h |  3 +++
>>  2 files changed, 48 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index e556a99..b5d7e0b 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path)
>>      return offset;
>>  }
>>
>> +/**
>> + * qemu_fdt_node_path
>> + *
>> + * return the node path of a device, given its node name and its
>> + * compat string
>> + * fdt: pointer to the dt blob
>> + * name: device node name
>> + * compat: compatibility string of the device
>> + *
>> + * upon success, the path is output at node_path address
>> + * returns 0 on success, < 0 on failure
>> + */
> 
> Can we put the doc comment in the header file, since this is
> a globally visible function? Also it would be nice to follow the
> doc-comment syntax standards about marking up arguments with '@'
> and so on.
sure
> 
>> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> +                       char **node_path)
>> +{
>> +    int offset, len, ret;
>> +    const char *iter_name;
>> +    char path[256];
> 
> Rather than a fixed buffer size, we should check whether
> fdt_get_path returns -FDT_ERR_NOSPACE and if so enlarge the
> buffer and try again.
OK
> 
>> +
>> +    *node_path = NULL;
>> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
>> +    while (offset != -FDT_ERR_NOTFOUND) {
>> +        if (offset < 0) {
>> +            continue;
> 
> I don't understand this continue -- if the fdt function returned any
> error other than -FDT_ERR_NOTFOUND then this will cause us to go
> into an infinite loop around this while(). Did you mean 'break' ?
> (Though if you just want to break then fixing the while condition
> would be better.)
My first understanding of the API was fdt_node_offset_by_compatible
would increment the offset even if an error occurred; so I envisioned to
continue parsing the tree, looking for another node with same features.
But I think it is overkill anyway and I will abort.
> 
>> +        }
>> +        iter_name = fdt_get_name(fdt, offset, &len);
>> +        if (!iter_name) {
>> +            continue;
> 
> This also seems like it ought to be a break, except you need to
> set offset to the error code first (which fdt_get_name() will
> have returned in 'len').
yes will set offset to len and then break;
> 
>> +        }
>> +
>> +        if (!strncmp(iter_name, name, len)) {
> 
> Do we really want strncmp rather than strcmp here ? (ie
> "find first node whose name has 'name' as a prefix" rather
> than "find first node whose name matches 'name').
true strcmp is OK

Thanks

Eric
> 
>> +            goto found;
>> +        }
>> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
>> +    }
>> +    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.9.1
> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On 5 January 2016 at 16:20, Eric Auger <eric.auger@linaro.org> wrote:
> Hi Peter,
> On 12/18/2015 03:23 PM, Peter Maydell wrote:
>> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
>>> This new helper routine returns the node path of a device
>>> referred to by its node name and compat string.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>

>>> +
>>> +    *node_path = NULL;
>>> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
>>> +    while (offset != -FDT_ERR_NOTFOUND) {
>>> +        if (offset < 0) {
>>> +            continue;
>>
>> I don't understand this continue -- if the fdt function returned any
>> error other than -FDT_ERR_NOTFOUND then this will cause us to go
>> into an infinite loop around this while(). Did you mean 'break' ?
>> (Though if you just want to break then fixing the while condition
>> would be better.)
> My first understanding of the API was fdt_node_offset_by_compatible
> would increment the offset even if an error occurred; so I envisioned to
> continue parsing the tree, looking for another node with same features.

Your code doesn't call fdt_node_offset_by_compatible again
in the case where it's trying to continue, though...

thanks
-- PMM

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

[Eric Auger]

On 01/05/2016 06:55 PM, Peter Maydell wrote:
> On 5 January 2016 at 16:20, Eric Auger <eric.auger@linaro.org> wrote:
>> Hi Peter,
>> On 12/18/2015 03:23 PM, Peter Maydell wrote:
>>> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
>>>> This new helper routine returns the node path of a device
>>>> referred to by its node name and compat string.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
>>>> +
>>>> +    *node_path = NULL;
>>>> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
>>>> +    while (offset != -FDT_ERR_NOTFOUND) {
>>>> +        if (offset < 0) {
>>>> +            continue;
>>>
>>> I don't understand this continue -- if the fdt function returned any
>>> error other than -FDT_ERR_NOTFOUND then this will cause us to go
>>> into an infinite loop around this while(). Did you mean 'break' ?
>>> (Though if you just want to break then fixing the while condition
>>> would be better.)
>> My first understanding of the API was fdt_node_offset_by_compatible
>> would increment the offset even if an error occurred; so I envisioned to
>> continue parsing the tree, looking for another node with same features.
> 
> Your code doesn't call fdt_node_offset_by_compatible again
> in the case where it's trying to continue, though...

I'll be damned, got it now!

Thanks

Eric
> 
> thanks
> -- PMM
> 

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

[Eric Auger]

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

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

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

---

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

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

diff --git a/device_tree.c b/device_tree.c
index b5d7e0b..c3720c2 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -331,18 +331,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
 }
 
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
-                             const char *property, int *lenp)
+                             const char *property, int *lenp, Error **errp)
 {
     int len;
     const void *r;
+
     if (!lenp) {
         lenp = &len;
     }
     r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
     if (!r) {
-        error_report("%s: Couldn't get %s/%s: %s", __func__,
-                     node_path, property, fdt_strerror(*lenp));
-        exit(1);
+        error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
+                  node_path, property, fdt_strerror(*lenp));
     }
     return r;
 }
@@ -351,7 +351,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
                                const char *property)
 {
     int len;
-    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
+    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
+                                         &error_fatal);
     if (len != 4) {
         error_report("%s: %s/%s not 4 bytes long (not a cell?)",
                      __func__, node_path, property);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index f9e6e6e..284fd3b 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -33,7 +33,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
                              const char *property,
                              const char *target_node_path);
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
-                             const char *property, int *lenp);
+                             const char *property, int *lenp,
+                             Error **errp);
 uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
                                const char *property);
 uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
-- 
1.9.1

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

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> Current qemu_fdt_getprop exits if the property is not found. It is
> sometimes needed to read an optional property, in which case we do
> not wish to exit but simply returns a null value.
>
> This patch converts qemu_fdt_getprop to accept an Error **, and existing
> users are converted to pass &error_fatal. This preserves the existing
> behaviour. Then to use the API with your optional semantic a null
> parameter can be conveyed.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC -> v1:
> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
>   that consists in using the error API

This doesn't seem to me like a great way for qemu_fdt_getprop to
report "property not found", because there's no way for the caller
to distinguish "property not found" from "function went wrong
some other way" (since Errors just report human readable strings,
and in any case you're not distinguishing -FDT_ERR_NOTFOUND
from any of the other FDT errors).

If we want to handle "ok if property doesn't exist" then we
could either (a) make the function return NULL on doesn't-exist
and error_report in the other error cases, with the existing
single caller extending its error checking appropriately, or
(b) have the caller that cares about property-may-not-exist
call fdt_getprop() directly.

> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  device_tree.c                | 11 ++++++-----
>  include/sysemu/device_tree.h |  3 ++-
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index b5d7e0b..c3720c2 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -331,18 +331,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>  }
>
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
> -                             const char *property, int *lenp)
> +                             const char *property, int *lenp, Error **errp)
>  {
>      int len;
>      const void *r;
> +
>      if (!lenp) {
>          lenp = &len;
>      }
>      r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
>      if (!r) {
> -        error_report("%s: Couldn't get %s/%s: %s", __func__,
> -                     node_path, property, fdt_strerror(*lenp));
> -        exit(1);
> +        error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
> +                  node_path, property, fdt_strerror(*lenp));
>      }
>      return r;
>  }
> @@ -351,7 +351,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>                                 const char *property)
>  {
>      int len;
> -    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
> +    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
> +                                         &error_fatal);
>      if (len != 4) {
>          error_report("%s: %s/%s not 4 bytes long (not a cell?)",
>                       __func__, node_path, property);
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f9e6e6e..284fd3b 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -33,7 +33,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>                               const char *property,
>                               const char *target_node_path);
>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
> -                             const char *property, int *lenp);
> +                             const char *property, int *lenp,
> +                             Error **errp);

If we change the function it would be nice to add a brief
doc comment while we're touching the prototype in the header.

thanks
-- PMM

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

[Eric Auger]

Hi Peter,
On 12/18/2015 03:36 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
>> Current qemu_fdt_getprop exits if the property is not found. It is
>> sometimes needed to read an optional property, in which case we do
>> not wish to exit but simply returns a null value.
>>
>> This patch converts qemu_fdt_getprop to accept an Error **, and existing
>> users are converted to pass &error_fatal. This preserves the existing
>> behaviour. Then to use the API with your optional semantic a null
>> parameter can be conveyed.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> RFC -> v1:
>> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
>>   that consists in using the error API
> 
> This doesn't seem to me like a great way for qemu_fdt_getprop to
> report "property not found", because there's no way for the caller
> to distinguish "property not found" from "function went wrong
> some other way" (since Errors just report human readable strings,
> and in any case you're not distinguishing -FDT_ERR_NOTFOUND
> from any of the other FDT errors).
Not sure I get what you mean here. In case fdt_getprop fails, as long as
the caller provided a lenp != NULL, *lenp contains the error code so
qemu_fdt_getprop's caller can discriminate a -FDT_ERR_NOTFOUND from any
other errors. Do I miss something?
> 
> If we want to handle "ok if property doesn't exist" then we
> could either (a) make the function return NULL on doesn't-exist
> and error_report in the other error cases, with the existing
> single caller extending its error checking appropriately, or
> (b) have the caller that cares about property-may-not-exist
> call fdt_getprop() directly.
> 
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  device_tree.c                | 11 ++++++-----
>>  include/sysemu/device_tree.h |  3 ++-
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index b5d7e0b..c3720c2 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -331,18 +331,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
>>  }
>>
>>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>> -                             const char *property, int *lenp)
>> +                             const char *property, int *lenp, Error **errp)
>>  {
>>      int len;
>>      const void *r;
>> +
>>      if (!lenp) {
>>          lenp = &len;
>>      }
>>      r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
>>      if (!r) {
>> -        error_report("%s: Couldn't get %s/%s: %s", __func__,
>> -                     node_path, property, fdt_strerror(*lenp));
>> -        exit(1);
>> +        error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
>> +                  node_path, property, fdt_strerror(*lenp));
>>      }
>>      return r;
>>  }
>> @@ -351,7 +351,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
>>                                 const char *property)
>>  {
>>      int len;
>> -    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
>> +    const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
>> +                                         &error_fatal);
>>      if (len != 4) {
>>          error_report("%s: %s/%s not 4 bytes long (not a cell?)",
>>                       __func__, node_path, property);
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index f9e6e6e..284fd3b 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -33,7 +33,8 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
>>                               const char *property,
>>                               const char *target_node_path);
>>  const void *qemu_fdt_getprop(void *fdt, const char *node_path,
>> -                             const char *property, int *lenp);
>> +                             const char *property, int *lenp,
>> +                             Error **errp);
> 
> If we change the function it would be nice to add a brief
> doc comment while we're touching the prototype in the header.
sure

Thanks

Eric
> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On 5 January 2016 at 16:20, Eric Auger <eric.auger@linaro.org> wrote:
> Hi Peter,
> On 12/18/2015 03:36 PM, Peter Maydell wrote:
>> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
>>> Current qemu_fdt_getprop exits if the property is not found. It is
>>> sometimes needed to read an optional property, in which case we do
>>> not wish to exit but simply returns a null value.
>>>
>>> This patch converts qemu_fdt_getprop to accept an Error **, and existing
>>> users are converted to pass &error_fatal. This preserves the existing
>>> behaviour. Then to use the API with your optional semantic a null
>>> parameter can be conveyed.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>>
>>> RFC -> v1:
>>> - get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
>>>   that consists in using the error API
>>
>> This doesn't seem to me like a great way for qemu_fdt_getprop to
>> report "property not found", because there's no way for the caller
>> to distinguish "property not found" from "function went wrong
>> some other way" (since Errors just report human readable strings,
>> and in any case you're not distinguishing -FDT_ERR_NOTFOUND
>> from any of the other FDT errors).
> Not sure I get what you mean here. In case fdt_getprop fails, as long as
> the caller provided a lenp != NULL, *lenp contains the error code so
> qemu_fdt_getprop's caller can discriminate a -FDT_ERR_NOTFOUND from any
> other errors. Do I miss something?

There's no documentation of this behaviour of qemu_fdt_getprop
in either this commit message or in a doc comment in the header,
so I didn't realise that was your intention.

thanks
-- PMM

[Qemu-devel] [PATCH 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 in the host device tree.

- inherit_properties copies properties from a host device tree node to
  a guest device tree node
- fdt_build_clock_node builds a guest clock node and checks the host
  fellow clock is a fixed one.

fdt_build_clock_node will become static as soon as it gets used. A
dummy pre-declaration is needed for compilation of this patch.

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

---

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 9d28797..c2d813b 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -21,6 +21,7 @@
  *
  */
 
+#include <libfdt.h>
 #include "hw/arm/sysbus-fdt.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
@@ -56,6 +57,116 @@ typedef struct NodeCreationPair {
     int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
 } NodeCreationPair;
 
+/* helpers */
+
+struct HostProperty {
+    const char *name;
+    bool optional;
+};
+
+typedef struct HostProperty HostProperty;
+
+/**
+ * inherit_properties
+ *
+ * copies properties listed in an array from host device tree to
+ * guest device tree. If a non optional property is not found, the
+ * function self-asserts. An optional property is ignored if not found
+ * in the host device tree.
+ * @props: array of HostProperty to copy
+ * @nb_props: number of properties in the array
+ * @host_dt: host device tree blob
+ * @guest_dt: guest device tree blob
+ * @node_path: host dt node path where the property is supposed to be
+              found
+ * @nodename: guest node name the properties should be added to
+ */
+static void inherit_properties(HostProperty *props, int nb_props,
+                               void *host_fdt, void *guest_fdt,
+                               char *node_path, char *nodename)
+{
+    int i, prop_len;
+    const void *r;
+
+    for (i = 0; i < nb_props; i++) {
+        r = qemu_fdt_getprop(host_fdt, node_path,
+                             props[i].name,
+                             &prop_len,
+                             props[i].optional ? NULL : &error_fatal);
+        if (r) {
+            qemu_fdt_setprop(guest_fdt, nodename,
+                             props[i].name, r, prop_len);
+        }
+    }
+}
+
+/* clock properties whose values are copied/pasted from host */
+static HostProperty clock_inherited_properties[] = {
+    {"compatible", 0},
+    {"#clock-cells", 0},
+    {"clock-frequency", 1},
+    {"clock-output-names", 1},
+};
+
+/**
+ * fdt_build_clock_node
+ *
+ * Build a guest clock node, used as a dependency from a passthrough'ed
+ * device. Most information are retrieved from the host clock node.
+ * Also check the host clock is a fixed one.
+ *
+ * @host_fdt: host device tree blob from which info are retrieved
+ * @guest_fdt: guest device tree blob where the clock node is added
+ * @host_phandle: phandle of the clock in host device tree
+ * @guest_phandle: phandle to assign to the guest node
+ */
+int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                         uint32_t host_phandle,
+                         uint32_t guest_phandle);
+int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+                         uint32_t host_phandle,
+                         uint32_t guest_phandle)
+{
+    char node_path[256];
+    char *nodename;
+    const void *r;
+    int ret, prop_len;
+
+    ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
+    if (ret <= 0) {
+        error_report("not able to locate clock handle %d in host device tree",
+                     host_phandle);
+        goto out;
+    }
+    ret = fdt_get_path(host_fdt, ret, node_path, 256);
+    if (ret < 0) {
+        error_report("not able to retrieve node path for clock handle %d",
+                     host_phandle);
+        goto out;
+    }
+
+    r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
+                         &error_fatal);
+    if (strcmp(r, "fixed-clock")) {
+        error_report("clock handle %d is not a fixed clock", host_phandle);
+        ret = -1;
+        goto out;
+    }
+
+    nodename = strrchr(node_path, '/');
+    qemu_fdt_add_subnode(guest_fdt, nodename);
+
+    inherit_properties(clock_inherited_properties,
+                       ARRAY_SIZE(clock_inherited_properties),
+                       host_fdt, guest_fdt,
+                       node_path, nodename);
+
+    qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
+
+out:
+    return ret;
+}
+
 /* Device Specific Code */
 
 /**
-- 
1.9.1

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

[Peter Maydell]

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

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

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

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

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

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

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

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

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

thanks
-- PMM

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

[Eric Auger]

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

ok
> 
>> +
>> +/**
>> + * inherit_properties
>> + *
>> + * copies properties listed in an array from host device tree to
>> + * guest device tree. If a non optional property is not found, the
>> + * function self-asserts. An optional property is ignored if not found
>> + * in the host device tree.
>> + * @props: array of HostProperty to copy
>> + * @nb_props: number of properties in the array
>> + * @host_dt: host device tree blob
>> + * @guest_dt: guest device tree blob
>> + * @node_path: host dt node path where the property is supposed to be
>> +              found
>> + * @nodename: guest node name the properties should be added to
>> + */
>> +static void inherit_properties(HostProperty *props, int nb_props,
>> +                               void *host_fdt, void *guest_fdt,
>> +                               char *node_path, char *nodename)
>> +{
>> +    int i, prop_len;
>> +    const void *r;
>> +
>> +    for (i = 0; i < nb_props; i++) {
>> +        r = qemu_fdt_getprop(host_fdt, node_path,
>> +                             props[i].name,
>> +                             &prop_len,
>> +                             props[i].optional ? NULL : &error_fatal);
> 
> We'll get an error here if the host device tree doesn't match
> up correctly, right? Is the error message going to be sufficiently
> informative to the end user about what's gone wrong? (What does
> it end up looking like?)

hum you're right, in case of a mandated property there is auto-assert
with a good error message.
In case of an optional property, there is currently no message output to
the end-user and the host property is ignored/not propagated to the
guest, with potential functional consequences.  I intend to use an error
object instead and print the error message in case the error is !=
FDT_ERR_NOTFOUND.
> 
>> +        if (r) {
>> +            qemu_fdt_setprop(guest_fdt, nodename,
>> +                             props[i].name, r, prop_len);
>> +        }
>> +    }
>> +}
>> +
>> +/* clock properties whose values are copied/pasted from host */
>> +static HostProperty clock_inherited_properties[] = {
>> +    {"compatible", 0},
>> +    {"#clock-cells", 0},
>> +    {"clock-frequency", 1},
>> +    {"clock-output-names", 1},
>> +};
>> +
>> +/**
>> + * fdt_build_clock_node
>> + *
>> + * Build a guest clock node, used as a dependency from a passthrough'ed
>> + * device. Most information are retrieved from the host clock node.
>> + * Also check the host clock is a fixed one.
>> + *
>> + * @host_fdt: host device tree blob from which info are retrieved
>> + * @guest_fdt: guest device tree blob where the clock node is added
>> + * @host_phandle: phandle of the clock in host device tree
>> + * @guest_phandle: phandle to assign to the guest node
>> + */
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +                         uint32_t host_phandle,
>> +                         uint32_t guest_phandle);
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> +                         uint32_t host_phandle,
>> +                         uint32_t guest_phandle)
>> +{
>> +    char node_path[256];
> 
> Please don't use hardcoded fixed buffer lengths (see previous patch
> review comments).
OK
> 
>> +    char *nodename;
>> +    const void *r;
>> +    int ret, prop_len;
>> +
>> +    ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>> +    if (ret <= 0) {
>> +        error_report("not able to locate clock handle %d in host device tree",
>> +                     host_phandle);
>> +        goto out;
>> +    }
> 
> If we're going to return to the caller in the error case rather than just
> exiting here, I think we should return the error to the caller via an
> Error** rather than calling error_report() directly ourselves.
agreed

Thanks!

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

[Qemu-devel] [PATCH 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.

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

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

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

---

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

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index c2d813b..7ec5623 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,6 +30,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/vfio/vfio-amd-xgbe.h"
 #include "hw/arm/fdt.h"
 
 /*
@@ -120,12 +122,9 @@ static HostProperty clock_inherited_properties[] = {
  * @host_phandle: phandle of the clock in host device tree
  * @guest_phandle: phandle to assign to the guest node
  */
-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)
+static 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;
@@ -167,6 +166,22 @@ 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]);
+    g_strfreev(substrings);
+    return dt_name;
+}
+
 /* Device Specific Code */
 
 /**
@@ -234,9 +249,166 @@ 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;
+    }
+
+    /* make sure the asssigned device is a combined XGBE/PHY device */
+    if (vbasedev->num_regions != 5) {
+        goto stop;
+    }
+
+    /* generate nodes for DMA_CLK and PTP_CLK */
+    r = qemu_fdt_getprop(host_fdt, node_path, "clocks",
+                         &prop_len, &error_fatal);
+    if (prop_len != 8) {
+        goto stop;
+    }
+    host_clock_phandles = (uint32_t *)r;
+    guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
+    guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
+
+    /**
+     * clock handles fetched from host dt are in be32 layout whereas
+     * rest of the code uses cpu layout. Also guest clock handles are
+     * in cpu layout.
+     */
+    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,
+                               be32_to_cpu(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.9.1

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

[Peter Maydell]

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> This patch allows the instantiation of the vfio-amd-xgbe device
> from the QEMU command line (-device vfio-amd-xgbe,host="<device>").
>
> The guest is exposed with a device tree node that combines the description
> of both XGBE and PHY (representation supported from 4.2 onwards kernel):
> Documentation/devicetree/bindings/net/amd-xgbe.txt.
>
> There are 5 register regions, 6 interrupts including 4 optional
> edge-sensitive per-channel interrupts.
>
> Some property values are inherited from host device tree. Host device tree
> must feature a combined XGBE/PHY representation (>= 4.2 host kernel).
>
> 2 clock nodes (dma and ptp) also are created. It is checked those clocks
> are fixed on host side.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC -> v1:
> - use qemu_fdt_getprop with Error **
> - free substrings in sysfs_to_dt_name
> - add some comments related to endianess in add_amd_xgbe_fdt_node
> - reword commit message (dtc binary dependency has disappeared)
> - check the host device has 5 regions meaning this is a combined
>   XGBE/PHY device
> ---
>  hw/arm/sysbus-fdt.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 178 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index c2d813b..7ec5623 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,6 +30,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-platform.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
> +#include "hw/vfio/vfio-amd-xgbe.h"
>  #include "hw/arm/fdt.h"
>
>  /*
> @@ -120,12 +122,9 @@ static HostProperty clock_inherited_properties[] = {
>   * @host_phandle: phandle of the clock in host device tree
>   * @guest_phandle: phandle to assign to the guest node
>   */
> -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)
> +static 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;
> @@ -167,6 +166,22 @@ 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]);
> +    g_strfreev(substrings);
> +    return dt_name;

This will crash if you feed it a string without a "." in it.
A bit more robustness would be a good idea.

> +}
> +
>  /* Device Specific Code */
>
>  /**
> @@ -234,9 +249,166 @@ 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},

HostProperty's second field is type 'bool' so these initializers
should be using 'true' and 'false' rather than 1 and 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;
> +    }
> +
> +    /* make sure the asssigned device is a combined XGBE/PHY device */

"assigned"

> +    if (vbasedev->num_regions != 5) {
> +        goto stop;

in this error case will we ever print a useful error message
to the user? (More generally I think the error reporting should
probably pass up Errors and then you can add a hint at the
end when you report it about the host dt blob being wrong
and perhaps how the user might deal with that.)

> +    }
> +
> +    /* generate nodes for DMA_CLK and PTP_CLK */
> +    r = qemu_fdt_getprop(host_fdt, node_path, "clocks",
> +                         &prop_len, &error_fatal);
> +    if (prop_len != 8) {
> +        goto stop;
> +    }
> +    host_clock_phandles = (uint32_t *)r;
> +    guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
> +    guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
> +
> +    /**
> +     * clock handles fetched from host dt are in be32 layout whereas
> +     * rest of the code uses cpu layout. Also guest clock handles are
> +     * in cpu layout.
> +     */
> +    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,
> +                               be32_to_cpu(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 */
>  };

thanks
-- PMM