* [Qemu-devel] [PATCH v5 1/8] hw/vfio/platform: amd-xgbe device
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs Eric Auger
` (6 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
This patch introduces the amd-xgbe VFIO platform device. It
allows the guest to do passthrough on a device exposing an
"amd,xgbe-seattle-v1a" compat string.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
RFC -> v1:
- add Alex' R-b
---
hw/vfio/Makefile.objs | 1 +
hw/vfio/amd-xgbe.c | 55 +++++++++++++++++++++++++++++++++++++++++
include/hw/vfio/vfio-amd-xgbe.h | 51 ++++++++++++++++++++++++++++++++++++++
3 files changed, 107 insertions(+)
create mode 100644 hw/vfio/amd-xgbe.c
create mode 100644 include/hw/vfio/vfio-amd-xgbe.h
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index d324863..ceddbb8 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
obj-$(CONFIG_PCI) += pci.o pci-quirks.o
obj-$(CONFIG_SOFTMMU) += platform.o
obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
+obj-$(CONFIG_SOFTMMU) += amd-xgbe.o
endif
diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
new file mode 100644
index 0000000..53451eb
--- /dev/null
+++ b/hw/vfio/amd-xgbe.c
@@ -0,0 +1,55 @@
+/*
+ * AMD XGBE VFIO device
+ *
+ * Copyright Linaro Limited, 2015
+ *
+ * Authors:
+ * Eric Auger <eric.auger@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/vfio/vfio-amd-xgbe.h"
+
+static void amd_xgbe_realize(DeviceState *dev, Error **errp)
+{
+ VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
+ VFIOAmdXgbeDeviceClass *k = VFIO_AMD_XGBE_DEVICE_GET_CLASS(dev);
+
+ vdev->compat = g_strdup("amd,xgbe-seattle-v1a");
+
+ k->parent_realize(dev, errp);
+}
+
+static const VMStateDescription vfio_platform_amd_xgbe_vmstate = {
+ .name = TYPE_VFIO_AMD_XGBE,
+ .unmigratable = 1,
+};
+
+static void vfio_amd_xgbe_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VFIOAmdXgbeDeviceClass *vcxc =
+ VFIO_AMD_XGBE_DEVICE_CLASS(klass);
+ vcxc->parent_realize = dc->realize;
+ dc->realize = amd_xgbe_realize;
+ dc->desc = "VFIO AMD XGBE";
+ dc->vmsd = &vfio_platform_amd_xgbe_vmstate;
+}
+
+static const TypeInfo vfio_amd_xgbe_dev_info = {
+ .name = TYPE_VFIO_AMD_XGBE,
+ .parent = TYPE_VFIO_PLATFORM,
+ .instance_size = sizeof(VFIOAmdXgbeDevice),
+ .class_init = vfio_amd_xgbe_class_init,
+ .class_size = sizeof(VFIOAmdXgbeDeviceClass),
+};
+
+static void register_amd_xgbe_dev_type(void)
+{
+ type_register_static(&vfio_amd_xgbe_dev_info);
+}
+
+type_init(register_amd_xgbe_dev_type)
diff --git a/include/hw/vfio/vfio-amd-xgbe.h b/include/hw/vfio/vfio-amd-xgbe.h
new file mode 100644
index 0000000..9fff65e
--- /dev/null
+++ b/include/hw/vfio/vfio-amd-xgbe.h
@@ -0,0 +1,51 @@
+/*
+ * VFIO AMD XGBE device
+ *
+ * Copyright Linaro Limited, 2015
+ *
+ * Authors:
+ * Eric Auger <eric.auger@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef HW_VFIO_VFIO_AMD_XGBE_H
+#define HW_VFIO_VFIO_AMD_XGBE_H
+
+#include "hw/vfio/vfio-platform.h"
+
+#define TYPE_VFIO_AMD_XGBE "vfio-amd-xgbe"
+
+/**
+ * This device exposes:
+ * - 5 MMIO regions: MAC, PCS, SerDes Rx/Tx regs,
+ SerDes Integration Registers 1/2 & 2/2
+ * - 2 level sensitive IRQs and optional DMA channel IRQs
+ */
+struct VFIOAmdXgbeDevice {
+ VFIOPlatformDevice vdev;
+};
+
+typedef struct VFIOAmdXgbeDevice VFIOAmdXgbeDevice;
+
+struct VFIOAmdXgbeDeviceClass {
+ /*< private >*/
+ VFIOPlatformDeviceClass parent_class;
+ /*< public >*/
+ DeviceRealize parent_realize;
+};
+
+typedef struct VFIOAmdXgbeDeviceClass VFIOAmdXgbeDeviceClass;
+
+#define VFIO_AMD_XGBE_DEVICE(obj) \
+ OBJECT_CHECK(VFIOAmdXgbeDevice, (obj), TYPE_VFIO_AMD_XGBE)
+#define VFIO_AMD_XGBE_DEVICE_CLASS(klass) \
+ OBJECT_CLASS_CHECK(VFIOAmdXgbeDeviceClass, (klass), \
+ TYPE_VFIO_AMD_XGBE)
+#define VFIO_AMD_XGBE_DEVICE_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(VFIOAmdXgbeDeviceClass, (obj), \
+ TYPE_VFIO_AMD_XGBE)
+
+#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 1/8] hw/vfio/platform: amd-xgbe device Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-25 14:13 ` Peter Maydell
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path Eric Auger
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
This function returns the host device tree blob from sysfs
(/proc/device-tree). It uses a recursive function inspired
from dtc read_fstree.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v1 -> v2:
- do not implement/expose read_fstree and load_device_tree_from_sysfs
if CONFIG_LINUX is not defined (lstat is not implemeted in mingw)
- correct indentation in read_fstree
- use /proc/device-tree symlink instead of /sys/firmware/devicetree/base
path (kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-ofw)
- use g_file_get_contents in read_fstree
- introduce SYSFS_DT_BASEDIR macro and use strlen
- exit on error in load_device_tree_from_sysfs
- user error_setg
RFC -> v1:
- remove runtime dependency on dtc binary and introduce read_fstree
---
device_tree.c | 100 +++++++++++++++++++++++++++++++++++++++++++
include/sysemu/device_tree.h | 3 ++
2 files changed, 103 insertions(+)
diff --git a/device_tree.c b/device_tree.c
index a9f5f8e..b262c2d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -17,6 +17,9 @@
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
+#ifdef CONFIG_LINUX
+#include <dirent.h>
+#endif
#include "qemu-common.h"
#include "qemu/error-report.h"
@@ -117,6 +120,103 @@ fail:
return NULL;
}
+#ifdef CONFIG_LINUX
+
+#define SYSFS_DT_BASEDIR "/proc/device-tree"
+
+/**
+ * read_fstree: this function is inspired from dtc read_fstree
+ * @fdt: preallocated fdt blob buffer, to be populated
+ * @dirname: directory to scan under SYSFS_DT_BASEDIR
+ * the search is recursive and the tree is searched down to the
+ * leafs (property files).
+ *
+ * the function self-asserts in case of error
+ */
+static void read_fstree(void *fdt, const char *dirname)
+{
+ DIR *d;
+ struct dirent *de;
+ struct stat st;
+ const char *root_dir = SYSFS_DT_BASEDIR;
+ char *parent_node;
+
+ if (strstr(dirname, root_dir) != dirname) {
+ error_report("%s: %s must be searched within %s",
+ __func__, dirname, root_dir);
+ exit(1);
+ }
+ parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];
+
+ d = opendir(dirname);
+ if (!d) {
+ error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
+ }
+
+ while ((de = readdir(d)) != NULL) {
+ char *tmpnam;
+
+ if (!g_strcmp0(de->d_name, ".")
+ || !g_strcmp0(de->d_name, "..")) {
+ continue;
+ }
+
+ tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
+
+ if (lstat(tmpnam, &st) < 0) {
+ error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
+ }
+
+ if (S_ISREG(st.st_mode)) {
+ gchar *val;
+ gsize len;
+
+ if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
+ error_setg(&error_fatal, "%s not able to extract info from %s",
+ __func__, tmpnam);
+ }
+
+ if (strlen(parent_node) > 0) {
+ qemu_fdt_setprop(fdt, parent_node,
+ de->d_name, val, len);
+ } else {
+ qemu_fdt_setprop(fdt, "/", de->d_name, val, len);
+ }
+ g_free(val);
+ } else if (S_ISDIR(st.st_mode)) {
+ char *node_name;
+
+ node_name = g_strdup_printf("%s/%s",
+ parent_node, de->d_name);
+ qemu_fdt_add_subnode(fdt, node_name);
+ g_free(node_name);
+ read_fstree(fdt, tmpnam);
+ }
+
+ g_free(tmpnam);
+ }
+
+ closedir(d);
+}
+
+/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
+void *load_device_tree_from_sysfs(void)
+{
+ void *host_fdt;
+ int host_fdt_size;
+
+ host_fdt = create_device_tree(&host_fdt_size);
+ read_fstree(host_fdt, SYSFS_DT_BASEDIR);
+ if (fdt_check_header(host_fdt)) {
+ error_setg(&error_fatal,
+ "%s host device tree extracted into memory is invalid",
+ __func__);
+ }
+ return host_fdt;
+}
+
+#endif /* CONFIG_LINUX */
+
static int findnode_nofail(void *fdt, const char *node_path)
{
int offset;
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 359e143..fdf25a4 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -16,6 +16,9 @@
void *create_device_tree(int *sizep);
void *load_device_tree(const char *filename_path, int *sizep);
+#ifdef CONFIG_LINUX
+void *load_device_tree_from_sysfs(void);
+#endif
int qemu_fdt_setprop(void *fdt, const char *node_path,
const char *property, const void *val, int size);
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs Eric Auger
@ 2016-01-25 14:13 ` Peter Maydell
2016-02-01 13:11 ` Eric Auger
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-01-25 14:13 UTC (permalink / raw)
To: Eric Auger
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> This function returns the host device tree blob from sysfs
> (/proc/device-tree). It uses a recursive function inspired
> from dtc read_fstree.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
> v1 -> v2:
> - do not implement/expose read_fstree and load_device_tree_from_sysfs
> if CONFIG_LINUX is not defined (lstat is not implemeted in mingw)
> - correct indentation in read_fstree
> - use /proc/device-tree symlink instead of /sys/firmware/devicetree/base
> path (kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-ofw)
> - use g_file_get_contents in read_fstree
> - introduce SYSFS_DT_BASEDIR macro and use strlen
> - exit on error in load_device_tree_from_sysfs
> - user error_setg
>
> RFC -> v1:
> - remove runtime dependency on dtc binary and introduce read_fstree
> ---
> device_tree.c | 100 +++++++++++++++++++++++++++++++++++++++++++
> include/sysemu/device_tree.h | 3 ++
> 2 files changed, 103 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..b262c2d 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -17,6 +17,9 @@
> #include <fcntl.h>
> #include <unistd.h>
> #include <stdlib.h>
> +#ifdef CONFIG_LINUX
> +#include <dirent.h>
> +#endif
>
> #include "qemu-common.h"
> #include "qemu/error-report.h"
> @@ -117,6 +120,103 @@ fail:
> return NULL;
> }
>
> +#ifdef CONFIG_LINUX
> +
> +#define SYSFS_DT_BASEDIR "/proc/device-tree"
> +
> +/**
> + * read_fstree: this function is inspired from dtc read_fstree
> + * @fdt: preallocated fdt blob buffer, to be populated
> + * @dirname: directory to scan under SYSFS_DT_BASEDIR
> + * the search is recursive and the tree is searched down to the
> + * leafs (property files).
"leaves"
> + *
> + * the function self-asserts in case of error
"asserts"
> + */
> +static void read_fstree(void *fdt, const char *dirname)
> +{
> + DIR *d;
> + struct dirent *de;
> + struct stat st;
> + const char *root_dir = SYSFS_DT_BASEDIR;
> + char *parent_node;
> +
> + if (strstr(dirname, root_dir) != dirname) {
> + error_report("%s: %s must be searched within %s",
> + __func__, dirname, root_dir);
> + exit(1);
Why does this one error_report and exit but other errors below use
error_setg?
> + }
> + parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];
What causes us to need this cast to char* ?
> +
> + d = opendir(dirname);
> + if (!d) {
> + error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
You need to return here (and similarly to bail out properly
in the other error paths below).
> + }
> +
> + while ((de = readdir(d)) != NULL) {
> + char *tmpnam;
> +
> + if (!g_strcmp0(de->d_name, ".")
> + || !g_strcmp0(de->d_name, "..")) {
> + continue;
> + }
> +
> + tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
> +
> + if (lstat(tmpnam, &st) < 0) {
> + error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
> + }
> +
> + if (S_ISREG(st.st_mode)) {
> + gchar *val;
> + gsize len;
> +
> + if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
> + error_setg(&error_fatal, "%s not able to extract info from %s",
> + __func__, tmpnam);
> + }
> +
> + if (strlen(parent_node) > 0) {
> + qemu_fdt_setprop(fdt, parent_node,
> + de->d_name, val, len);
> + } else {
> + qemu_fdt_setprop(fdt, "/", de->d_name, val, len);
> + }
> + g_free(val);
> + } else if (S_ISDIR(st.st_mode)) {
> + char *node_name;
> +
> + node_name = g_strdup_printf("%s/%s",
> + parent_node, de->d_name);
I don't mind whether we use g_strjoin("/",...) or g_strdup_printf("%s/%s", ...)
to glue together strings with a '/' between them, but can we not use
both methods in the same function, please?
> + qemu_fdt_add_subnode(fdt, node_name);
> + g_free(node_name);
> + read_fstree(fdt, tmpnam);
> + }
> +
> + g_free(tmpnam);
> + }
> +
> + closedir(d);
> +}
> +
> +/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
> +void *load_device_tree_from_sysfs(void)
> +{
> + void *host_fdt;
> + int host_fdt_size;
> +
> + host_fdt = create_device_tree(&host_fdt_size);
> + read_fstree(host_fdt, SYSFS_DT_BASEDIR);
> + if (fdt_check_header(host_fdt)) {
> + error_setg(&error_fatal,
> + "%s host device tree extracted into memory is invalid",
> + __func__);
Should we free the created device tree and return NULL here?
> + }
> + return host_fdt;
> +}
> +
> +#endif /* CONFIG_LINUX */
> +
> static int findnode_nofail(void *fdt, const char *node_path)
> {
> int offset;
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 359e143..fdf25a4 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -16,6 +16,9 @@
>
> void *create_device_tree(int *sizep);
> void *load_device_tree(const char *filename_path, int *sizep);
> +#ifdef CONFIG_LINUX
> +void *load_device_tree_from_sysfs(void);
Can we have a doc comment for a new global function, please?
> +#endif
>
> int qemu_fdt_setprop(void *fdt, const char *node_path,
> const char *property, const void *val, int size);
> --
> 1.9.1
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs
2016-01-25 14:13 ` Peter Maydell
@ 2016-02-01 13:11 ` Eric Auger
0 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-02-01 13:11 UTC (permalink / raw)
To: Peter Maydell
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
Hi Peter,
On 01/25/2016 03:13 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> This function returns the host device tree blob from sysfs
>> (/proc/device-tree). It uses a recursive function inspired
>> from dtc read_fstree.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v1 -> v2:
>> - do not implement/expose read_fstree and load_device_tree_from_sysfs
>> if CONFIG_LINUX is not defined (lstat is not implemeted in mingw)
>> - correct indentation in read_fstree
>> - use /proc/device-tree symlink instead of /sys/firmware/devicetree/base
>> path (kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-ofw)
>> - use g_file_get_contents in read_fstree
>> - introduce SYSFS_DT_BASEDIR macro and use strlen
>> - exit on error in load_device_tree_from_sysfs
>> - user error_setg
>>
>> RFC -> v1:
>> - remove runtime dependency on dtc binary and introduce read_fstree
>> ---
>> device_tree.c | 100 +++++++++++++++++++++++++++++++++++++++++++
>> include/sysemu/device_tree.h | 3 ++
>> 2 files changed, 103 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index a9f5f8e..b262c2d 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -17,6 +17,9 @@
>> #include <fcntl.h>
>> #include <unistd.h>
>> #include <stdlib.h>
>> +#ifdef CONFIG_LINUX
>> +#include <dirent.h>
>> +#endif
>>
>> #include "qemu-common.h"
>> #include "qemu/error-report.h"
>> @@ -117,6 +120,103 @@ fail:
>> return NULL;
>> }
>>
>> +#ifdef CONFIG_LINUX
>> +
>> +#define SYSFS_DT_BASEDIR "/proc/device-tree"
>> +
>> +/**
>> + * read_fstree: this function is inspired from dtc read_fstree
>> + * @fdt: preallocated fdt blob buffer, to be populated
>> + * @dirname: directory to scan under SYSFS_DT_BASEDIR
>> + * the search is recursive and the tree is searched down to the
>> + * leafs (property files).
>
> "leaves"
OK
>
>> + *
>> + * the function self-asserts in case of error
>
> "asserts"
OK
>
>> + */
>> +static void read_fstree(void *fdt, const char *dirname)
>> +{
>> + DIR *d;
>> + struct dirent *de;
>> + struct stat st;
>> + const char *root_dir = SYSFS_DT_BASEDIR;
>> + char *parent_node;
>> +
>> + if (strstr(dirname, root_dir) != dirname) {
>> + error_report("%s: %s must be searched within %s",
>> + __func__, dirname, root_dir);
>> + exit(1);
>
> Why does this one error_report and exit but other errors below use
> error_setg?
replaced with error_setg(&error_fatal, ...)
>
>> + }
>> + parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];
>
> What causes us to need this cast to char* ?
I changed parent_node to a const char * instead of char*
>
>> +
>> + d = opendir(dirname);
>> + if (!d) {
>> + error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
>
> You need to return here (and similarly to bail out properly
> in the other error paths below).
>
>> + }
>> +
>> + while ((de = readdir(d)) != NULL) {
>> + char *tmpnam;
>> +
>> + if (!g_strcmp0(de->d_name, ".")
>> + || !g_strcmp0(de->d_name, "..")) {
>> + continue;
>> + }
>> +
>> + tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
>> +
>> + if (lstat(tmpnam, &st) < 0) {
>> + error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
>> + }
>> +
>> + if (S_ISREG(st.st_mode)) {
>> + gchar *val;
>> + gsize len;
>> +
>> + if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
>> + error_setg(&error_fatal, "%s not able to extract info from %s",
>> + __func__, tmpnam);
>> + }
>> +
>> + if (strlen(parent_node) > 0) {
>> + qemu_fdt_setprop(fdt, parent_node,
>> + de->d_name, val, len);
>> + } else {
>> + qemu_fdt_setprop(fdt, "/", de->d_name, val, len);
>> + }
>> + g_free(val);
>> + } else if (S_ISDIR(st.st_mode)) {
>> + char *node_name;
>> +
>> + node_name = g_strdup_printf("%s/%s",
>> + parent_node, de->d_name);
>
> I don't mind whether we use g_strjoin("/",...) or g_strdup_printf("%s/%s", ...)
> to glue together strings with a '/' between them, but can we not use
> both methods in the same function, please?
ok
>
>> + qemu_fdt_add_subnode(fdt, node_name);
>> + g_free(node_name);
>> + read_fstree(fdt, tmpnam);
>> + }
>> +
>> + g_free(tmpnam);
>> + }
>> +
>> + closedir(d);
>> +}
>> +
>> +/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
>> +void *load_device_tree_from_sysfs(void)
>> +{
>> + void *host_fdt;
>> + int host_fdt_size;
>> +
>> + host_fdt = create_device_tree(&host_fdt_size);
>> + read_fstree(host_fdt, SYSFS_DT_BASEDIR);
>> + if (fdt_check_header(host_fdt)) {
>> + error_setg(&error_fatal,
>> + "%s host device tree extracted into memory is invalid",
>> + __func__);
>
> Should we free the created device tree and return NULL here?
>
>> + }
>> + return host_fdt;
>> +}
>> +
>> +#endif /* CONFIG_LINUX */
>> +
>> static int findnode_nofail(void *fdt, const char *node_path)
>> {
>> int offset;
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index 359e143..fdf25a4 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -16,6 +16,9 @@
>>
>> void *create_device_tree(int *sizep);
>> void *load_device_tree(const char *filename_path, int *sizep);
>> +#ifdef CONFIG_LINUX
>> +void *load_device_tree_from_sysfs(void);
>
> Can we have a doc comment for a new global function, please?
sure
Thanks
Eric
>
>> +#endif
>>
>> int qemu_fdt_setprop(void *fdt, const char *node_path,
>> const char *property, const void *val, int size);
>> --
>> 1.9.1
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 1/8] hw/vfio/platform: amd-xgbe device Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-25 14:26 ` Peter Maydell
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 4/8] device_tree: qemu_fdt_getprop converted to use the error API Eric Auger
` (4 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
This new helper routine returns a NULL terminated array of
node paths matching a node name and a compat string.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v4 -> v5:
- support the case where several nodes exist, ie.
return an array of node paths. Also add Error **
parameter
v1 -> v2:
- move doc comment in header file
- do not use a fixed size buffer
- break on errors in while loop
- use strcmp instead of strncmp
RFC -> v1:
- improve error handling according to Alex' comments
---
device_tree.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
include/sysemu/device_tree.h | 14 +++++++++++++
2 files changed, 63 insertions(+)
diff --git a/device_tree.c b/device_tree.c
index b262c2d..3c88a37 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -231,6 +231,55 @@ static int findnode_nofail(void *fdt, const char *node_path)
return offset;
}
+char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+ Error **errp)
+{
+ int offset, len, ret;
+ const char *iter_name;
+ unsigned int path_len = 16, n = 0;
+ GSList *path_list = NULL, *iter;
+ char **path_array;
+
+ offset = fdt_node_offset_by_compatible(fdt, -1, compat);
+
+ while (offset >= 0) {
+ iter_name = fdt_get_name(fdt, offset, &len);
+ if (!iter_name) {
+ offset = len;
+ break;
+ }
+ if (!strcmp(iter_name, name)) {
+ char *path;
+
+ path = g_malloc(path_len);
+ while ((ret = fdt_get_path(fdt, offset, path, path_len))
+ == -FDT_ERR_NOSPACE) {
+ path_len += 16;
+ path = g_realloc(path, path_len);
+ }
+ path_list = g_slist_prepend(path_list, path);
+ n++;
+ }
+ offset = fdt_node_offset_by_compatible(fdt, offset, compat);
+ }
+
+ if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
+ error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
+ __func__, name, compat, fdt_strerror(offset));
+ }
+
+ path_array = g_new(char *, n + 1);
+ path_array[n--] = NULL;
+
+ for (iter = path_list; iter; iter = iter->next) {
+ path_array[n--] = iter->data;
+ }
+
+ g_slist_free(path_list);
+
+ return path_array;
+}
+
int qemu_fdt_setprop(void *fdt, const char *node_path,
const char *property, const void *val, int size)
{
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index fdf25a4..436b5dd 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -20,6 +20,20 @@ void *load_device_tree(const char *filename_path, int *sizep);
void *load_device_tree_from_sysfs(void);
#endif
+/**
+ * qemu_fdt_node_path: return the paths of nodes matching a given
+ * name and compat string
+ * @fdt: pointer to the dt blob
+ * @name: node name
+ * @compat: compatibility string
+ * @errp: handle to an error object
+ *
+ * returns a newly allocated NULL-terminated array of node paths.
+ * Use g_strfreev() to free it.
+ */
+char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
+ Error **errp);
+
int qemu_fdt_setprop(void *fdt, const char *node_path,
const char *property, const void *val, int size);
int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path Eric Auger
@ 2016-01-25 14:26 ` Peter Maydell
2016-01-25 14:41 ` Eric Auger
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-01-25 14:26 UTC (permalink / raw)
To: Eric Auger
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> This new helper routine returns a NULL terminated array of
> node paths matching a node name and a compat string.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> v4 -> v5:
> - support the case where several nodes exist, ie.
> return an array of node paths. Also add Error **
> parameter
>
> v1 -> v2:
> - move doc comment in header file
> - do not use a fixed size buffer
> - break on errors in while loop
> - use strcmp instead of strncmp
>
> RFC -> v1:
> - improve error handling according to Alex' comments
> ---
> device_tree.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> include/sysemu/device_tree.h | 14 +++++++++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index b262c2d..3c88a37 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -231,6 +231,55 @@ static int findnode_nofail(void *fdt, const char *node_path)
> return offset;
> }
>
> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> + Error **errp)
> +{
> + int offset, len, ret;
> + const char *iter_name;
> + unsigned int path_len = 16, n = 0;
> + GSList *path_list = NULL, *iter;
> + char **path_array;
> +
> + offset = fdt_node_offset_by_compatible(fdt, -1, compat);
> +
> + while (offset >= 0) {
> + iter_name = fdt_get_name(fdt, offset, &len);
> + if (!iter_name) {
> + offset = len;
> + break;
> + }
> + if (!strcmp(iter_name, name)) {
> + char *path;
> +
> + path = g_malloc(path_len);
> + while ((ret = fdt_get_path(fdt, offset, path, path_len))
> + == -FDT_ERR_NOSPACE) {
> + path_len += 16;
> + path = g_realloc(path, path_len);
> + }
> + path_list = g_slist_prepend(path_list, path);
> + n++;
> + }
> + offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> + }
> +
> + if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> + error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
> + __func__, name, compat, fdt_strerror(offset));
Needs to bail out (freeing memory).
> + }
> +
> + path_array = g_new(char *, n + 1);
> + path_array[n--] = NULL;
0, or '\0'. NULL is a pointer, not the NUL character. (This is a
style issue, not a correctness one.)
> +
> + for (iter = path_list; iter; iter = iter->next) {
> + path_array[n--] = iter->data;
> + }
> +
> + g_slist_free(path_list);
> +
> + return path_array;
> +}
> +
> int qemu_fdt_setprop(void *fdt, const char *node_path,
> const char *property, const void *val, int size)
> {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index fdf25a4..436b5dd 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -20,6 +20,20 @@ void *load_device_tree(const char *filename_path, int *sizep);
> void *load_device_tree_from_sysfs(void);
> #endif
>
> +/**
> + * qemu_fdt_node_path: return the paths of nodes matching a given
> + * name and compat string
> + * @fdt: pointer to the dt blob
> + * @name: node name
> + * @compat: compatibility string
> + * @errp: handle to an error object
> + *
> + * returns a newly allocated NULL-terminated array of node paths.
Should say what we return on error (NULL ?)
> + * Use g_strfreev() to free it.
> + */
> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> + Error **errp);
> +
> int qemu_fdt_setprop(void *fdt, const char *node_path,
> const char *property, const void *val, int size);
> int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
> --
> 1.9.1
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path
2016-01-25 14:26 ` Peter Maydell
@ 2016-01-25 14:41 ` Eric Auger
0 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-01-25 14:41 UTC (permalink / raw)
To: Peter Maydell
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
Peter,
On 01/25/2016 03:26 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> This new helper routine returns a NULL terminated array of
>> node paths matching a node name and a compat string.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v4 -> v5:
>> - support the case where several nodes exist, ie.
>> return an array of node paths. Also add Error **
>> parameter
>>
>> v1 -> v2:
>> - move doc comment in header file
>> - do not use a fixed size buffer
>> - break on errors in while loop
>> - use strcmp instead of strncmp
>>
>> RFC -> v1:
>> - improve error handling according to Alex' comments
>> ---
>> device_tree.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
>> include/sysemu/device_tree.h | 14 +++++++++++++
>> 2 files changed, 63 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index b262c2d..3c88a37 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -231,6 +231,55 @@ static int findnode_nofail(void *fdt, const char *node_path)
>> return offset;
>> }
>>
>> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> + Error **errp)
>> +{
>> + int offset, len, ret;
>> + const char *iter_name;
>> + unsigned int path_len = 16, n = 0;
>> + GSList *path_list = NULL, *iter;
>> + char **path_array;
>> +
>> + offset = fdt_node_offset_by_compatible(fdt, -1, compat);
>> +
>> + while (offset >= 0) {
>> + iter_name = fdt_get_name(fdt, offset, &len);
>> + if (!iter_name) {
>> + offset = len;
>> + break;
>> + }
>> + if (!strcmp(iter_name, name)) {
>> + char *path;
>> +
>> + path = g_malloc(path_len);
>> + while ((ret = fdt_get_path(fdt, offset, path, path_len))
>> + == -FDT_ERR_NOSPACE) {
>> + path_len += 16;
>> + path = g_realloc(path, path_len);
>> + }
>> + path_list = g_slist_prepend(path_list, path);
>> + n++;
>> + }
>> + offset = fdt_node_offset_by_compatible(fdt, offset, compat);
>> + }
>> +
>> + if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
>> + error_setg(errp, "%s: abort parsing dt for %s/%s: %s",
>> + __func__, name, compat, fdt_strerror(offset));
>
> Needs to bail out (freeing memory).
effectively in case I get an error, it's better to free everything and
return a vector with with a single NULL element, see comment below. I
will correct this.
>
>> + }
>> +
>> + path_array = g_new(char *, n + 1);
>> + path_array[n--] = NULL;
>
> 0, or '\0'. NULL is a pointer, not the NUL character. (This is a
> style issue, not a correctness one.)
actually I want to put NULL as the last element of my array and not a
NUL char. The function returns a NULL terminated vector as
g_strsplit_set does, for instance.
Thanks
Eric
>
>> +
>> + for (iter = path_list; iter; iter = iter->next) {
>> + path_array[n--] = iter->data;
>> + }
>> +
>> + g_slist_free(path_list);
>> +
>> + return path_array;
>> +}
>> +
>> int qemu_fdt_setprop(void *fdt, const char *node_path,
>> const char *property, const void *val, int size)
>> {
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index fdf25a4..436b5dd 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -20,6 +20,20 @@ void *load_device_tree(const char *filename_path, int *sizep);
>> void *load_device_tree_from_sysfs(void);
>> #endif
>>
>> +/**
>> + * qemu_fdt_node_path: return the paths of nodes matching a given
>> + * name and compat string
>> + * @fdt: pointer to the dt blob
>> + * @name: node name
>> + * @compat: compatibility string
>> + * @errp: handle to an error object
>> + *
>> + * returns a newly allocated NULL-terminated array of node paths.
>
> Should say what we return on error (NULL ?)
>
>> + * Use g_strfreev() to free it.
>> + */
>> +char **qemu_fdt_node_path(void *fdt, const char *name, char *compat,
>> + Error **errp);
>> +
>> int qemu_fdt_setprop(void *fdt, const char *node_path,
>> const char *property, const void *val, int size);
>> int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
>> --
>> 1.9.1
>>
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v5 4/8] device_tree: qemu_fdt_getprop converted to use the error API
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
` (2 preceding siblings ...)
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 3/8] device_tree: introduce qemu_fdt_node_path Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 5/8] device_tree: qemu_fdt_getprop_cell " Eric Auger
` (3 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
Current qemu_fdt_getprop exits if the property is not found. It is
sometimes needed to read an optional property, in which case we do
not wish to exit but simply returns a null value.
This patch converts qemu_fdt_getprop to accept an Error **, and existing
users are converted to pass &error_fatal. This preserves the existing
behaviour. Then to use the API with your optional semantic a null
parameter can be conveyed.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
v4 -> v5:
- add Peter's R-b
- remove comment about error_fatal
v1 -> v2:
- add a doc comment in the header file
RFC -> v1:
- get rid of qemu_fdt_getprop_optional and implement Peter's suggestion
that consists in using the error API
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
device_tree.c | 11 ++++++-----
include/sysemu/device_tree.h | 13 ++++++++++++-
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/device_tree.c b/device_tree.c
index 3c88a37..45fd76d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -333,18 +333,18 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
}
const void *qemu_fdt_getprop(void *fdt, const char *node_path,
- const char *property, int *lenp)
+ const char *property, int *lenp, Error **errp)
{
int len;
const void *r;
+
if (!lenp) {
lenp = &len;
}
r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
if (!r) {
- error_report("%s: Couldn't get %s/%s: %s", __func__,
- node_path, property, fdt_strerror(*lenp));
- exit(1);
+ error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__,
+ node_path, property, fdt_strerror(*lenp));
}
return r;
}
@@ -353,7 +353,8 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
const char *property)
{
int len;
- const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len);
+ const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
+ &error_fatal);
if (len != 4) {
error_report("%s: %s/%s not 4 bytes long (not a cell?)",
__func__, node_path, property);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 436b5dd..123beb5 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -45,8 +45,19 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
const char *property,
const char *target_node_path);
+/**
+ * qemu_fdt_getprop: retrieve the value of a given property
+ * @fdt: pointer to the device tree blob
+ * @node_path: node path
+ * @property: name of the property to find
+ * @lenp: fdt error if any or length of the property on success
+ * @errp: handle to an error object
+ *
+ * returns a pointer to the property on success and NULL on failure
+ */
const void *qemu_fdt_getprop(void *fdt, const char *node_path,
- const char *property, int *lenp);
+ const char *property, int *lenp,
+ Error **errp);
uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
const char *property);
uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v5 5/8] device_tree: qemu_fdt_getprop_cell converted to use the error API
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
` (3 preceding siblings ...)
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 4/8] device_tree: qemu_fdt_getprop converted to use the error API Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation Eric Auger
` (2 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
This patch aligns the prototype with qemu_fdt_getprop. The caller
can choose whether the function self-asserts on error (passing
&error_fatal as Error ** argument, corresponding to the legacy behavior),
or behaves differently such as simply output a message.
In this later case the caller can use the new lenp parameter to interpret
the error if any.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
v4 -> v5:
- Add Peter's R-b
- remove comment about error_fatal
v3 : creation
---
device_tree.c | 21 ++++++++++++++-------
hw/arm/boot.c | 6 ++++--
hw/arm/vexpress.c | 6 ++++--
include/sysemu/device_tree.h | 14 +++++++++++++-
4 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/device_tree.c b/device_tree.c
index 45fd76d..5e56f8e 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -350,15 +350,22 @@ const void *qemu_fdt_getprop(void *fdt, const char *node_path,
}
uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
- const char *property)
+ const char *property, int *lenp, Error **errp)
{
int len;
- const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len,
- &error_fatal);
- if (len != 4) {
- error_report("%s: %s/%s not 4 bytes long (not a cell?)",
- __func__, node_path, property);
- exit(1);
+ const uint32_t *p;
+
+ if (!lenp) {
+ lenp = &len;
+ }
+ p = qemu_fdt_getprop(fdt, node_path, property, lenp, errp);
+ if (!p) {
+ return 0;
+ } else if (*lenp != 4) {
+ error_setg(errp, "%s: %s/%s not 4 bytes long (not a cell?)",
+ __func__, node_path, property);
+ *lenp = -EINVAL;
+ return 0;
}
return be32_to_cpu(*p);
}
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 75f69bf..541b74c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -386,8 +386,10 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
return 0;
}
- acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
- scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
+ acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
+ NULL, &error_fatal);
+ scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
+ NULL, &error_fatal);
if (acells == 0 || scells == 0) {
fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
goto fail;
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index ea9a984..f6e28dc 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -477,8 +477,10 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
uint32_t acells, scells, intc;
const VEDBoardInfo *daughterboard = (const VEDBoardInfo *)info;
- acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
- scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
+ acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
+ NULL, &error_fatal);
+ scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
+ NULL, &error_fatal);
intc = find_int_controller(fdt);
if (!intc) {
/* Not fatal, we just won't provide virtio. This will
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 123beb5..7897e54 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -58,8 +58,20 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
const void *qemu_fdt_getprop(void *fdt, const char *node_path,
const char *property, int *lenp,
Error **errp);
+/**
+ * qemu_fdt_getprop_cell: retrieve the value of a given 4 byte property
+ * @fdt: pointer to the device tree blob
+ * @node_path: node path
+ * @property: name of the property to find
+ * @lenp: fdt error if any or -EINVAL if the property size is different from
+ * 4 bytes, or 4 (expected length of the property) upon success.
+ * @errp: handle to an error object
+ *
+ * returns the property value on success
+ */
uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path,
- const char *property);
+ const char *property, int *lenp,
+ Error **errp);
uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
uint32_t qemu_fdt_alloc_phandle(void *fdt);
int qemu_fdt_nop_node(void *fdt, const char *node_path);
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
` (4 preceding siblings ...)
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 5/8] device_tree: qemu_fdt_getprop_cell " Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-25 14:05 ` Peter Maydell
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation Eric Auger
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 8/8] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check Eric Auger
7 siblings, 1 reply; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
Some passthrough'ed devices depend on clock nodes. Those need to be
generated in the guest device tree. This patch introduces some helpers
to build a clock node from information retrieved in the host device tree.
- copy_properties_from_host copies properties from a host device tree
node to a guest device tree node
- fdt_build_clock_node builds a guest clock node and checks the host
fellow clock is a fixed one.
fdt_build_clock_node will become static as soon as it gets used. A
dummy pre-declaration is needed for compilation of this patch.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v4 -> v5:
- renamed inherit_properties
v1 -> v2:
- inherit properties now outputs an error message in case
qemu_fdt_getprop fails for an existing optional property
- no hardcoded fixed buffer length
- fdt_build_clock_node becomes void and auto-asserts on error
- use boolean values when defining the clock properties
RFC -> v1:
- use the new proto of qemu_fdt_getprop
- remove newline in error_report
- fix some style issues
---
hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 120 insertions(+)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 9d28797..d85c9e6 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -21,6 +21,7 @@
*
*/
+#include <libfdt.h>
#include "hw/arm/sysbus-fdt.h"
#include "qemu/error-report.h"
#include "sysemu/device_tree.h"
@@ -56,6 +57,125 @@ typedef struct NodeCreationPair {
int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
} NodeCreationPair;
+/* helpers */
+
+typedef struct HostProperty {
+ const char *name;
+ bool optional;
+} HostProperty;
+
+/**
+ * copy_properties_from_host
+ *
+ * copies properties listed in an array from host device tree to
+ * guest device tree. If a non optional property is not found, the
+ * function self-asserts. An optional property is ignored if not found
+ * in the host device tree.
+ * @props: array of HostProperty to copy
+ * @nb_props: number of properties in the array
+ * @host_dt: host device tree blob
+ * @guest_dt: guest device tree blob
+ * @node_path: host dt node path where the property is supposed to be
+ found
+ * @nodename: guest node name the properties should be added to
+ */
+static void copy_properties_from_host(HostProperty *props, int nb_props,
+ void *host_fdt, void *guest_fdt,
+ char *node_path, char *nodename)
+{
+ int i, prop_len;
+ const void *r;
+ Error *err = NULL;
+
+ for (i = 0; i < nb_props; i++) {
+ r = qemu_fdt_getprop(host_fdt, node_path,
+ props[i].name,
+ &prop_len,
+ props[i].optional ? &err : &error_fatal);
+ if (r) {
+ qemu_fdt_setprop(guest_fdt, nodename,
+ props[i].name, r, prop_len);
+ } else {
+ if (prop_len != -FDT_ERR_NOTFOUND) {
+ /* optional property not returned although property exists */
+ error_report_err(err);
+ } else {
+ error_free(err);
+ }
+ }
+ }
+}
+
+/* clock properties whose values are copied/pasted from host */
+static HostProperty clock_copied_properties[] = {
+ {"compatible", false},
+ {"#clock-cells", false},
+ {"clock-frequency", true},
+ {"clock-output-names", true},
+};
+
+/**
+ * fdt_build_clock_node
+ *
+ * Build a guest clock node, used as a dependency from a passthrough'ed
+ * device. Most information are retrieved from the host clock node.
+ * Also check the host clock is a fixed one.
+ *
+ * @host_fdt: host device tree blob from which info are retrieved
+ * @guest_fdt: guest device tree blob where the clock node is added
+ * @host_phandle: phandle of the clock in host device tree
+ * @guest_phandle: phandle to assign to the guest node
+ */
+void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+ uint32_t host_phandle,
+ uint32_t guest_phandle);
+void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+ uint32_t host_phandle,
+ uint32_t guest_phandle)
+{
+ char *node_path = NULL;
+ char *nodename;
+ const void *r;
+ int ret, node_offset, prop_len, path_len = 16;
+
+ node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
+ if (node_offset <= 0) {
+ error_setg(&error_fatal,
+ "not able to locate clock handle %d in host device tree",
+ host_phandle);
+ }
+ node_path = g_malloc(path_len);
+ while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
+ == -FDT_ERR_NOSPACE) {
+ path_len += 16;
+ node_path = g_realloc(node_path, path_len);
+ }
+ if (ret < 0) {
+ error_setg(&error_fatal,
+ "not able to retrieve node path for clock handle %d",
+ host_phandle);
+ }
+
+ r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
+ &error_fatal);
+ if (strcmp(r, "fixed-clock")) {
+ error_setg(&error_fatal,
+ "clock handle %d is not a fixed clock", host_phandle);
+ }
+
+ nodename = strrchr(node_path, '/');
+ qemu_fdt_add_subnode(guest_fdt, nodename);
+
+ copy_properties_from_host(clock_copied_properties,
+ ARRAY_SIZE(clock_copied_properties),
+ host_fdt, guest_fdt,
+ node_path, nodename);
+
+ qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
+
+ g_free(node_path);
+}
+
/* Device Specific Code */
/**
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation Eric Auger
@ 2016-01-25 14:05 ` Peter Maydell
2016-01-25 14:09 ` Eric Auger
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-01-25 14:05 UTC (permalink / raw)
To: Eric Auger
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> Some passthrough'ed devices depend on clock nodes. Those need to be
> generated in the guest device tree. This patch introduces some helpers
> to build a clock node from information retrieved in the host device tree.
>
> - copy_properties_from_host copies properties from a host device tree
> node to a guest device tree node
> - fdt_build_clock_node builds a guest clock node and checks the host
> fellow clock is a fixed one.
>
> fdt_build_clock_node will become static as soon as it gets used. A
> dummy pre-declaration is needed for compilation of this patch.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> +
> +/**
> + * copy_properties_from_host
> + *
> + * copies properties listed in an array from host device tree to
> + * guest device tree. If a non optional property is not found, the
> + * function self-asserts. An optional property is ignored if not found
This is just "asserts", not "self-asserts".
> + * in the host device tree.
> + * @props: array of HostProperty to copy
> + * @nb_props: number of properties in the array
> + * @host_dt: host device tree blob
> + * @guest_dt: guest device tree blob
> + * @node_path: host dt node path where the property is supposed to be
> + found
> + * @nodename: guest node name the properties should be added to
> + */
> +/**
> + * fdt_build_clock_node
> + *
> + * Build a guest clock node, used as a dependency from a passthrough'ed
> + * device. Most information are retrieved from the host clock node.
> + * Also check the host clock is a fixed one.
> + *
> + * @host_fdt: host device tree blob from which info are retrieved
> + * @guest_fdt: guest device tree blob where the clock node is added
> + * @host_phandle: phandle of the clock in host device tree
> + * @guest_phandle: phandle to assign to the guest node
> + */
> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> + uint32_t host_phandle,
> + uint32_t guest_phandle);
> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
> + uint32_t host_phandle,
> + uint32_t guest_phandle)
> +{
> + char *node_path = NULL;
> + char *nodename;
> + const void *r;
> + int ret, node_offset, prop_len, path_len = 16;
> +
> + node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
> + if (node_offset <= 0) {
> + error_setg(&error_fatal,
> + "not able to locate clock handle %d in host device tree",
> + host_phandle);
Don't we want to return here, rather than continuing with the
rest of the function? ("goto err;" with an err: label before
the g_free() at the bottom of the function probably best since
some of the error paths need to free that.)
> + }
> + node_path = g_malloc(path_len);
> + while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
> + == -FDT_ERR_NOSPACE) {
> + path_len += 16;
> + node_path = g_realloc(node_path, path_len);
> + }
> + if (ret < 0) {
> + error_setg(&error_fatal,
> + "not able to retrieve node path for clock handle %d",
> + host_phandle);
> + }
> +
> + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
> + &error_fatal);
> + if (strcmp(r, "fixed-clock")) {
> + error_setg(&error_fatal,
> + "clock handle %d is not a fixed clock", host_phandle);
> + }
> +
> + nodename = strrchr(node_path, '/');
> + qemu_fdt_add_subnode(guest_fdt, nodename);
> +
> + copy_properties_from_host(clock_copied_properties,
> + ARRAY_SIZE(clock_copied_properties),
> + host_fdt, guest_fdt,
> + node_path, nodename);
> +
> + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
> +
> + g_free(node_path);
> +}
> +
> /* Device Specific Code */
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation
2016-01-25 14:05 ` Peter Maydell
@ 2016-01-25 14:09 ` Eric Auger
2016-01-25 14:34 ` Peter Maydell
0 siblings, 1 reply; 20+ messages in thread
From: Eric Auger @ 2016-01-25 14:09 UTC (permalink / raw)
To: Peter Maydell
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
Hi Peter,
On 01/25/2016 03:05 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> Some passthrough'ed devices depend on clock nodes. Those need to be
>> generated in the guest device tree. This patch introduces some helpers
>> to build a clock node from information retrieved in the host device tree.
>>
>> - copy_properties_from_host copies properties from a host device tree
>> node to a guest device tree node
>> - fdt_build_clock_node builds a guest clock node and checks the host
>> fellow clock is a fixed one.
>>
>> fdt_build_clock_node will become static as soon as it gets used. A
>> dummy pre-declaration is needed for compilation of this patch.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
>> +
>> +/**
>> + * copy_properties_from_host
>> + *
>> + * copies properties listed in an array from host device tree to
>> + * guest device tree. If a non optional property is not found, the
>> + * function self-asserts. An optional property is ignored if not found
>
> This is just "asserts", not "self-asserts".
ok
>
>> + * in the host device tree.
>> + * @props: array of HostProperty to copy
>> + * @nb_props: number of properties in the array
>> + * @host_dt: host device tree blob
>> + * @guest_dt: guest device tree blob
>> + * @node_path: host dt node path where the property is supposed to be
>> + found
>> + * @nodename: guest node name the properties should be added to
>> + */
>
>> +/**
>> + * fdt_build_clock_node
>> + *
>> + * Build a guest clock node, used as a dependency from a passthrough'ed
>> + * device. Most information are retrieved from the host clock node.
>> + * Also check the host clock is a fixed one.
>> + *
>> + * @host_fdt: host device tree blob from which info are retrieved
>> + * @guest_fdt: guest device tree blob where the clock node is added
>> + * @host_phandle: phandle of the clock in host device tree
>> + * @guest_phandle: phandle to assign to the guest node
>> + */
>> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> + uint32_t host_phandle,
>> + uint32_t guest_phandle);
>> +void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> + uint32_t host_phandle,
>> + uint32_t guest_phandle)
>> +{
>> + char *node_path = NULL;
>> + char *nodename;
>> + const void *r;
>> + int ret, node_offset, prop_len, path_len = 16;
>> +
>> + node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>> + if (node_offset <= 0) {
>> + error_setg(&error_fatal,
>> + "not able to locate clock handle %d in host device tree",
>> + host_phandle);
>
> Don't we want to return here, rather than continuing with the
> rest of the function? ("goto err;" with an err: label before
> the g_free() at the bottom of the function probably best since
> some of the error paths need to free that.)
due to the &error_fatal, we are going to exit here. I thought it is not
worth going further if we can't find the clock node.
Best Regards
Eric
>
>> + }
>> + node_path = g_malloc(path_len);
>> + while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
>> + == -FDT_ERR_NOSPACE) {
>> + path_len += 16;
>> + node_path = g_realloc(node_path, path_len);
>> + }
>> + if (ret < 0) {
>> + error_setg(&error_fatal,
>> + "not able to retrieve node path for clock handle %d",
>> + host_phandle);
>> + }
>> +
>> + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
>> + &error_fatal);
>> + if (strcmp(r, "fixed-clock")) {
>> + error_setg(&error_fatal,
>> + "clock handle %d is not a fixed clock", host_phandle);
>> + }
>> +
>> + nodename = strrchr(node_path, '/');
>> + qemu_fdt_add_subnode(guest_fdt, nodename);
>> +
>> + copy_properties_from_host(clock_copied_properties,
>> + ARRAY_SIZE(clock_copied_properties),
>> + host_fdt, guest_fdt,
>> + node_path, nodename);
>> +
>> + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
>> +
>> + g_free(node_path);
>> +}
>> +
>> /* Device Specific Code */
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation
2016-01-25 14:09 ` Eric Auger
@ 2016-01-25 14:34 ` Peter Maydell
2016-01-25 14:43 ` Peter Maydell
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-01-25 14:34 UTC (permalink / raw)
To: Eric Auger
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote:
> Hi Peter,
> On 01/25/2016 03:05 PM, Peter Maydell wrote:
>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> Don't we want to return here, rather than continuing with the
>> rest of the function? ("goto err;" with an err: label before
>> the g_free() at the bottom of the function probably best since
>> some of the error paths need to free that.)
> due to the &error_fatal, we are going to exit here. I thought it is not
> worth going further if we can't find the clock node.
Yes, sorry, I didn't notice the error_fatal til I got to about
the last patch I reviewed. You can ignore similar comments
I made about some of the other patches.
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation
2016-01-25 14:34 ` Peter Maydell
@ 2016-01-25 14:43 ` Peter Maydell
2016-01-25 14:51 ` Eric Auger
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-01-25 14:43 UTC (permalink / raw)
To: Eric Auger
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 25 January 2016 at 14:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote:
>> Hi Peter,
>> On 01/25/2016 03:05 PM, Peter Maydell wrote:
>>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>
>>> Don't we want to return here, rather than continuing with the
>>> rest of the function? ("goto err;" with an err: label before
>>> the g_free() at the bottom of the function probably best since
>>> some of the error paths need to free that.)
>> due to the &error_fatal, we are going to exit here. I thought it is not
>> worth going further if we can't find the clock node.
>
> Yes, sorry, I didn't notice the error_fatal til I got to about
> the last patch I reviewed. You can ignore similar comments
> I made about some of the other patches.
...so if you fix the 'asserts' comment thing, you can have
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation
2016-01-25 14:43 ` Peter Maydell
@ 2016-01-25 14:51 ` Eric Auger
0 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-01-25 14:51 UTC (permalink / raw)
To: Peter Maydell
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 01/25/2016 03:43 PM, Peter Maydell wrote:
> On 25 January 2016 at 14:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 January 2016 at 14:09, Eric Auger <eric.auger@linaro.org> wrote:
>>> Hi Peter,
>>> On 01/25/2016 03:05 PM, Peter Maydell wrote:
>>>> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>>
>>>> Don't we want to return here, rather than continuing with the
>>>> rest of the function? ("goto err;" with an err: label before
>>>> the g_free() at the bottom of the function probably best since
>>>> some of the error paths need to free that.)
>>> due to the &error_fatal, we are going to exit here. I thought it is not
>>> worth going further if we can't find the clock node.
>>
>> Yes, sorry, I didn't notice the error_fatal til I got to about
>> the last patch I reviewed. You can ignore similar comments
>> I made about some of the other patches.
>
> ...so if you fix the 'asserts' comment thing, you can have
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
OK thanks for the whole review
Eric
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
` (5 preceding siblings ...)
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 6/8] hw/arm/sysbus-fdt: helpers for clock node generation Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
2016-01-25 14:33 ` Peter Maydell
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 8/8] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check Eric Auger
7 siblings, 1 reply; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
This patch allows the instantiation of the vfio-amd-xgbe device
from the QEMU command line (-device vfio-amd-xgbe,host="<device>").
The guest is exposed with a device tree node that combines the description
of both XGBE and PHY (representation supported from 4.2 onwards kernel):
Documentation/devicetree/bindings/net/amd-xgbe.txt.
There are 5 register regions, 6 interrupts including 4 optional
edge-sensitive per-channel interrupts.
Some property values are inherited from host device tree. Host device tree
must feature a combined XGBE/PHY representation (>= 4.2 host kernel).
2 clock nodes (dma and ptp) also are created. It is checked those clocks
are fixed on host side.
AMD XGBE node creation function has a dependency on vfio Linux header and
more generally node creation function for VFIO platform devices only make
sense with CONFIG_LINUX so let's protect this code with #ifdef CONFIG_LINUX.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v4 -> v5:
- use new proto for qemu_fdt_node_path, check there is only 1 node
matching the name/compat.
- renamed inherit_properties*
v3 -> v4:
- add_amd_xgbe_fdt_node explicitly returns 0. Reword comment to emphasize
the fact the function self-asserts in case of error
v1 -> v2:
- add CONFIG_LINUX protection
- improves robustness in sysfs_to_dt_name
- output messages to end-user on misc failures and self-exits:
error management becomes more violent than before assuming if
the end-user wants passthrough we must exit if the device cannot
be instantiated
- fix misc style issues
- remove qemu_fdt_setprop returned value check since it self-asserts
RFC -> v1:
- use qemu_fdt_getprop with Error **
- free substrings in sysfs_to_dt_name
- add some comments related to endianess in add_amd_xgbe_fdt_node
- reword commit message (dtc binary dependency has disappeared)
- check the host device has 5 regions meaning this is a combined
XGBE/PHY device
---
hw/arm/sysbus-fdt.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 189 insertions(+), 6 deletions(-)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d85c9e6..bf6ec24 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -22,6 +22,10 @@
*/
#include <libfdt.h>
+#include "qemu-common.h"
+#ifdef CONFIG_LINUX
+#include <linux/vfio.h>
+#endif
#include "hw/arm/sysbus-fdt.h"
#include "qemu/error-report.h"
#include "sysemu/device_tree.h"
@@ -29,6 +33,7 @@
#include "sysemu/sysemu.h"
#include "hw/vfio/vfio-platform.h"
#include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/vfio/vfio-amd-xgbe.h"
#include "hw/arm/fdt.h"
/*
@@ -64,6 +69,8 @@ typedef struct HostProperty {
bool optional;
} HostProperty;
+#ifdef CONFIG_LINUX
+
/**
* copy_properties_from_host
*
@@ -126,12 +133,9 @@ static HostProperty clock_copied_properties[] = {
* @host_phandle: phandle of the clock in host device tree
* @guest_phandle: phandle to assign to the guest node
*/
-void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
- uint32_t host_phandle,
- uint32_t guest_phandle);
-void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
- uint32_t host_phandle,
- uint32_t guest_phandle)
+static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
+ uint32_t host_phandle,
+ uint32_t guest_phandle)
{
char *node_path = NULL;
char *nodename;
@@ -176,6 +180,28 @@ void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
g_free(node_path);
}
+/**
+ * sysfs_to_dt_name: convert the name found in sysfs into the node name
+ * for instance e0900000.xgmac is converted into xgmac@e0900000
+ * @sysfs_name: directory name in sysfs
+ *
+ * returns the device tree name upon success or NULL in case the sysfs name
+ * does not match the expected format
+ */
+static char *sysfs_to_dt_name(const char *sysfs_name)
+{
+ gchar **substrings = g_strsplit(sysfs_name, ".", 2);
+ char *dt_name = NULL;
+
+ if (!substrings || !substrings[1] || !substrings[0]) {
+ goto out;
+ }
+ dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
+out:
+ g_strfreev(substrings);
+ return dt_name;
+}
+
/* Device Specific Code */
/**
@@ -243,9 +269,166 @@ fail_reg:
return ret;
}
+
+/* AMD xgbe properties whose values are copied/pasted from host */
+static HostProperty amd_xgbe_copied_properties[] = {
+ {"compatible", false},
+ {"dma-coherent", true},
+ {"amd,per-channel-interrupt", true},
+ {"phy-mode", false},
+ {"mac-address", true},
+ {"amd,speed-set", false},
+ {"amd,serdes-blwc", true},
+ {"amd,serdes-cdr-rate", true},
+ {"amd,serdes-pq-skew", true},
+ {"amd,serdes-tx-amp", true},
+ {"amd,serdes-dfe-tap-config", true},
+ {"amd,serdes-dfe-tap-enable", true},
+ {"clock-names", false},
+};
+
+/**
+ * add_amd_xgbe_fdt_node
+ *
+ * Generates the combined xgbe/phy node following kernel >=4.2
+ * binding documentation:
+ * Documentation/devicetree/bindings/net/amd-xgbe.txt:
+ * Also 2 clock nodes are created (dma and ptp)
+ *
+ * self-asserts in case of error
+ */
+static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+ PlatformBusFDTData *data = opaque;
+ PlatformBusDevice *pbus = data->pbus;
+ VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
+ VFIODevice *vbasedev = &vdev->vbasedev;
+ VFIOINTp *intp;
+ const char *parent_node = data->pbus_node_name;
+ char **node_path, *nodename, *dt_name;
+ void *guest_fdt = data->fdt, *host_fdt;
+ const void *r;
+ int i, prop_len;
+ uint32_t *irq_attr, *reg_attr, *host_clock_phandles;
+ uint64_t mmio_base, irq_number;
+ uint32_t guest_clock_phandles[2];
+
+ host_fdt = load_device_tree_from_sysfs();
+
+ dt_name = sysfs_to_dt_name(vbasedev->name);
+ if (!dt_name) {
+ error_setg(&error_fatal, "%s incorrect sysfs device name %s",
+ __func__, vbasedev->name);
+ }
+ node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
+ &error_fatal);
+ if (!node_path[0]) {
+ error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
+ __func__, dt_name, vdev->compat);
+ }
+
+ if (node_path[1]) {
+ error_setg(&error_fatal, "%s more than one node matching %s/%s!",
+ __func__, dt_name, vdev->compat);
+ }
+
+ g_free(dt_name);
+
+ if (vbasedev->num_regions != 5) {
+ error_setg(&error_fatal, "%s Does the host dt node combine XGBE/PHY?",
+ __func__);
+ }
+
+ /* generate nodes for DMA_CLK and PTP_CLK */
+ r = qemu_fdt_getprop(host_fdt, node_path[0], "clocks",
+ &prop_len, &error_fatal);
+ if (prop_len != 8) {
+ error_setg(&error_fatal, "%s clocks property should contain 2 handles",
+ __func__);
+ }
+ host_clock_phandles = (uint32_t *)r;
+ guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
+ guest_clock_phandles[1] = qemu_fdt_alloc_phandle(guest_fdt);
+
+ /**
+ * clock handles fetched from host dt are in be32 layout whereas
+ * rest of the code uses cpu layout. Also guest clock handles are
+ * in cpu layout.
+ */
+ fdt_build_clock_node(host_fdt, guest_fdt,
+ be32_to_cpu(host_clock_phandles[0]),
+ guest_clock_phandles[0]);
+
+ fdt_build_clock_node(host_fdt, guest_fdt,
+ be32_to_cpu(host_clock_phandles[1]),
+ guest_clock_phandles[1]);
+
+ /* combined XGBE/PHY node */
+ mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+ nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
+ vbasedev->name, mmio_base);
+ qemu_fdt_add_subnode(guest_fdt, nodename);
+
+ copy_properties_from_host(amd_xgbe_copied_properties,
+ ARRAY_SIZE(amd_xgbe_copied_properties),
+ host_fdt, guest_fdt,
+ node_path[0], nodename);
+
+ qemu_fdt_setprop_cells(guest_fdt, nodename, "clocks",
+ guest_clock_phandles[0],
+ guest_clock_phandles[1]);
+
+ reg_attr = g_new(uint32_t, vbasedev->num_regions * 2);
+ for (i = 0; i < vbasedev->num_regions; i++) {
+ mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
+ reg_attr[2 * i] = cpu_to_be32(mmio_base);
+ reg_attr[2 * i + 1] = cpu_to_be32(
+ memory_region_size(&vdev->regions[i]->mem));
+ }
+ qemu_fdt_setprop(guest_fdt, nodename, "reg", reg_attr,
+ vbasedev->num_regions * 2 * sizeof(uint32_t));
+
+ irq_attr = g_new(uint32_t, vbasedev->num_irqs * 3);
+ for (i = 0; i < vbasedev->num_irqs; i++) {
+ irq_number = platform_bus_get_irqn(pbus, sbdev , i)
+ + data->irq_start;
+ irq_attr[3 * i] = cpu_to_be32(GIC_FDT_IRQ_TYPE_SPI);
+ irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
+ /*
+ * General device interrupt and PCS auto-negociation interrupts are
+ * level-sensitive while the 4 per-channel interrupts are edge
+ * sensitive
+ */
+ QLIST_FOREACH(intp, &vdev->intp_list, next) {
+ if (intp->pin == i) {
+ break;
+ }
+ }
+ if (intp->flags & VFIO_IRQ_INFO_AUTOMASKED) {
+ irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+ } else {
+ irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+ }
+ }
+ qemu_fdt_setprop(guest_fdt, nodename, "interrupts",
+ irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
+
+ g_free(host_fdt);
+ g_strfreev(node_path);
+ g_free(irq_attr);
+ g_free(reg_attr);
+ g_free(nodename);
+ return 0;
+}
+
+#endif /* CONFIG_LINUX */
+
/* list of supported dynamic sysbus devices */
static const NodeCreationPair add_fdt_node_functions[] = {
+#ifdef CONFIG_LINUX
{TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
+ {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
+#endif
{"", NULL}, /* last element */
};
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation Eric Auger
@ 2016-01-25 14:33 ` Peter Maydell
2016-02-01 13:11 ` Eric Auger
0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2016-01-25 14:33 UTC (permalink / raw)
To: Eric Auger
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
> This patch allows the instantiation of the vfio-amd-xgbe device
> from the QEMU command line (-device vfio-amd-xgbe,host="<device>").
>
> The guest is exposed with a device tree node that combines the description
> of both XGBE and PHY (representation supported from 4.2 onwards kernel):
> Documentation/devicetree/bindings/net/amd-xgbe.txt.
>
> There are 5 register regions, 6 interrupts including 4 optional
> edge-sensitive per-channel interrupts.
>
> Some property values are inherited from host device tree. Host device tree
> must feature a combined XGBE/PHY representation (>= 4.2 host kernel).
>
> 2 clock nodes (dma and ptp) also are created. It is checked those clocks
> are fixed on host side.
>
> AMD XGBE node creation function has a dependency on vfio Linux header and
> more generally node creation function for VFIO platform devices only make
> sense with CONFIG_LINUX so let's protect this code with #ifdef CONFIG_LINUX.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> hw/arm/sysbus-fdt.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 189 insertions(+), 6 deletions(-)
I'll let it pass for this patchset, but this file is starting to
get big, and probably needs some kind of split into common functions
in one file and then a separate file for each pass-through device,
if they're all going to require 200-odd lines to deal with.
> +/**
> + * sysfs_to_dt_name: convert the name found in sysfs into the node name
> + * for instance e0900000.xgmac is converted into xgmac@e0900000
> + * @sysfs_name: directory name in sysfs
> + *
> + * returns the device tree name upon success or NULL in case the sysfs name
> + * does not match the expected format
> + */
> +static char *sysfs_to_dt_name(const char *sysfs_name)
> +{
> + gchar **substrings = g_strsplit(sysfs_name, ".", 2);
> + char *dt_name = NULL;
> +
> + if (!substrings || !substrings[1] || !substrings[0]) {
We should check substrings[0] before substrings[1], as otherwise
if we're passed the empty string we'll index off the end of the
vector.
> + goto out;
> + }
> + dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
> +out:
> + g_strfreev(substrings);
> + return dt_name;
> +}
> +
> /* Device Specific Code */
>
> /**
> @@ -243,9 +269,166 @@ fail_reg:
> return ret;
> }
>
> +
> +/* AMD xgbe properties whose values are copied/pasted from host */
> +static HostProperty amd_xgbe_copied_properties[] = {
> + {"compatible", false},
> + {"dma-coherent", true},
> + {"amd,per-channel-interrupt", true},
> + {"phy-mode", false},
> + {"mac-address", true},
> + {"amd,speed-set", false},
> + {"amd,serdes-blwc", true},
> + {"amd,serdes-cdr-rate", true},
> + {"amd,serdes-pq-skew", true},
> + {"amd,serdes-tx-amp", true},
> + {"amd,serdes-dfe-tap-config", true},
> + {"amd,serdes-dfe-tap-enable", true},
> + {"clock-names", false},
> +};
> +
> +/**
> + * add_amd_xgbe_fdt_node
> + *
> + * Generates the combined xgbe/phy node following kernel >=4.2
> + * binding documentation:
> + * Documentation/devicetree/bindings/net/amd-xgbe.txt:
> + * Also 2 clock nodes are created (dma and ptp)
> + *
> + * self-asserts in case of error
"asserts".
> + for (i = 0; i < vbasedev->num_irqs; i++) {
> + irq_number = platform_bus_get_irqn(pbus, sbdev , i)
> + + data->irq_start;
> + irq_attr[3 * i] = cpu_to_be32(GIC_FDT_IRQ_TYPE_SPI);
> + irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
> + /*
> + * General device interrupt and PCS auto-negociation interrupts are
"negotiation"
> + * level-sensitive while the 4 per-channel interrupts are edge
> + * sensitive
> + */
> + QLIST_FOREACH(intp, &vdev->intp_list, next) {
> + if (intp->pin == i) {
> + break;
> + }
> + }
> + if (intp->flags & VFIO_IRQ_INFO_AUTOMASKED) {
> + irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> + } else {
> + irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> + }
> + }
> + qemu_fdt_setprop(guest_fdt, nodename, "interrupts",
> + irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
> +
> + g_free(host_fdt);
> + g_strfreev(node_path);
> + g_free(irq_attr);
> + g_free(reg_attr);
> + g_free(nodename);
> + return 0;
> +}
> +
> +#endif /* CONFIG_LINUX */
> +
> /* list of supported dynamic sysbus devices */
> static const NodeCreationPair add_fdt_node_functions[] = {
> +#ifdef CONFIG_LINUX
> {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
> + {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
> +#endif
> {"", NULL}, /* last element */
> };
thanks
-- PMM
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation
2016-01-25 14:33 ` Peter Maydell
@ 2016-02-01 13:11 ` Eric Auger
0 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-02-01 13:11 UTC (permalink / raw)
To: Peter Maydell
Cc: Baptiste Reynal, Thomas Huth, eric.auger, Patch Tracking,
Peter Crosthwaite, QEMU Developers, Alex Williamson, qemu-arm,
Suravee Suthikulpanit, Paolo Bonzini, thomas.lendacky,
Alex Bennée, Christoffer Dall, David Gibson
Hi Peter,
On 01/25/2016 03:33 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> wrote:
>> This patch allows the instantiation of the vfio-amd-xgbe device
>> from the QEMU command line (-device vfio-amd-xgbe,host="<device>").
>>
>> The guest is exposed with a device tree node that combines the description
>> of both XGBE and PHY (representation supported from 4.2 onwards kernel):
>> Documentation/devicetree/bindings/net/amd-xgbe.txt.
>>
>> There are 5 register regions, 6 interrupts including 4 optional
>> edge-sensitive per-channel interrupts.
>>
>> Some property values are inherited from host device tree. Host device tree
>> must feature a combined XGBE/PHY representation (>= 4.2 host kernel).
>>
>> 2 clock nodes (dma and ptp) also are created. It is checked those clocks
>> are fixed on host side.
>>
>> AMD XGBE node creation function has a dependency on vfio Linux header and
>> more generally node creation function for VFIO platform devices only make
>> sense with CONFIG_LINUX so let's protect this code with #ifdef CONFIG_LINUX.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
>> hw/arm/sysbus-fdt.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 189 insertions(+), 6 deletions(-)
>
> I'll let it pass for this patchset, but this file is starting to
> get big, and probably needs some kind of split into common functions
> in one file and then a separate file for each pass-through device,
> if they're all going to require 200-odd lines to deal with.
That's understood. If you don't mind i would rather introduce that move
in a separate series then.
Thanks
Eric
>
>> +/**
>> + * sysfs_to_dt_name: convert the name found in sysfs into the node name
>> + * for instance e0900000.xgmac is converted into xgmac@e0900000
>> + * @sysfs_name: directory name in sysfs
>> + *
>> + * returns the device tree name upon success or NULL in case the sysfs name
>> + * does not match the expected format
>> + */
>> +static char *sysfs_to_dt_name(const char *sysfs_name)
>> +{
>> + gchar **substrings = g_strsplit(sysfs_name, ".", 2);
>> + char *dt_name = NULL;
>> +
>> + if (!substrings || !substrings[1] || !substrings[0]) {
>
> We should check substrings[0] before substrings[1], as otherwise
> if we're passed the empty string we'll index off the end of the
> vector.
>
>> + goto out;
>> + }
>> + dt_name = g_strdup_printf("%s@%s", substrings[1], substrings[0]);
>> +out:
>> + g_strfreev(substrings);
>> + return dt_name;
>> +}
>> +
>> /* Device Specific Code */
>>
>> /**
>> @@ -243,9 +269,166 @@ fail_reg:
>> return ret;
>> }
>>
>> +
>> +/* AMD xgbe properties whose values are copied/pasted from host */
>> +static HostProperty amd_xgbe_copied_properties[] = {
>> + {"compatible", false},
>> + {"dma-coherent", true},
>> + {"amd,per-channel-interrupt", true},
>> + {"phy-mode", false},
>> + {"mac-address", true},
>> + {"amd,speed-set", false},
>> + {"amd,serdes-blwc", true},
>> + {"amd,serdes-cdr-rate", true},
>> + {"amd,serdes-pq-skew", true},
>> + {"amd,serdes-tx-amp", true},
>> + {"amd,serdes-dfe-tap-config", true},
>> + {"amd,serdes-dfe-tap-enable", true},
>> + {"clock-names", false},
>> +};
>> +
>> +/**
>> + * add_amd_xgbe_fdt_node
>> + *
>> + * Generates the combined xgbe/phy node following kernel >=4.2
>> + * binding documentation:
>> + * Documentation/devicetree/bindings/net/amd-xgbe.txt:
>> + * Also 2 clock nodes are created (dma and ptp)
>> + *
>> + * self-asserts in case of error
>
> "asserts".
>
>> + for (i = 0; i < vbasedev->num_irqs; i++) {
>> + irq_number = platform_bus_get_irqn(pbus, sbdev , i)
>> + + data->irq_start;
>> + irq_attr[3 * i] = cpu_to_be32(GIC_FDT_IRQ_TYPE_SPI);
>> + irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
>> + /*
>> + * General device interrupt and PCS auto-negociation interrupts are
>
> "negotiation"
>
>> + * level-sensitive while the 4 per-channel interrupts are edge
>> + * sensitive
>> + */
>> + QLIST_FOREACH(intp, &vdev->intp_list, next) {
>> + if (intp->pin == i) {
>> + break;
>> + }
>> + }
>> + if (intp->flags & VFIO_IRQ_INFO_AUTOMASKED) {
>> + irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>> + } else {
>> + irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
>> + }
>> + }
>> + qemu_fdt_setprop(guest_fdt, nodename, "interrupts",
>> + irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
>> +
>> + g_free(host_fdt);
>> + g_strfreev(node_path);
>> + g_free(irq_attr);
>> + g_free(reg_attr);
>> + g_free(nodename);
>> + return 0;
>> +}
>> +
>> +#endif /* CONFIG_LINUX */
>> +
>> /* list of supported dynamic sysbus devices */
>> static const NodeCreationPair add_fdt_node_functions[] = {
>> +#ifdef CONFIG_LINUX
>> {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
>> + {TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node},
>> +#endif
>> {"", NULL}, /* last element */
>> };
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH v5 8/8] hw/arm/sysbus-fdt: remove qemu_fdt_setprop returned value check
2016-01-18 15:16 [Qemu-devel] [PATCH v5 0/8] AMD XGBE KVM platform passthrough Eric Auger
` (6 preceding siblings ...)
2016-01-18 15:16 ` [Qemu-devel] [PATCH v5 7/8] hw/arm/sysbus-fdt: enable amd-xgbe dynamic instantiation Eric Auger
@ 2016-01-18 15:16 ` Eric Auger
7 siblings, 0 replies; 20+ messages in thread
From: Eric Auger @ 2016-01-18 15:16 UTC (permalink / raw)
To: eric.auger, eric.auger, qemu-devel, qemu-arm, peter.maydell,
david, alex.williamson
Cc: thomas.lendacky, thuth, b.reynal, patches, crosthwaitepeter,
suravee.suthikulpanit, pbonzini, alex.bennee, christoffer.dall
qemu_fdt_setprop self-asserts in case of error hence no need to check
the returned value.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v3 -> v4: fix returned value
---
hw/arm/sysbus-fdt.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index bf6ec24..d179214 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -216,7 +216,7 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
PlatformBusDevice *pbus = data->pbus;
void *fdt = data->fdt;
const char *parent_node = data->pbus_node_name;
- int compat_str_len, i, ret = -1;
+ int compat_str_len, i;
char *nodename;
uint32_t *irq_attr, *reg_attr;
uint64_t mmio_base, irq_number;
@@ -241,12 +241,8 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
reg_attr[2 * i + 1] = cpu_to_be32(
memory_region_size(&vdev->regions[i]->mem));
}
- ret = qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
- vbasedev->num_regions * 2 * sizeof(uint32_t));
- if (ret) {
- error_report("could not set reg property of node %s", nodename);
- goto fail_reg;
- }
+ qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
+ vbasedev->num_regions * 2 * sizeof(uint32_t));
irq_attr = g_new(uint32_t, vbasedev->num_irqs * 3);
for (i = 0; i < vbasedev->num_irqs; i++) {
@@ -256,17 +252,12 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
irq_attr[3 * i + 1] = cpu_to_be32(irq_number);
irq_attr[3 * i + 2] = cpu_to_be32(GIC_FDT_IRQ_FLAGS_LEVEL_HI);
}
- ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
+ qemu_fdt_setprop(fdt, nodename, "interrupts",
irq_attr, vbasedev->num_irqs * 3 * sizeof(uint32_t));
- if (ret) {
- error_report("could not set interrupts property of node %s",
- nodename);
- }
g_free(irq_attr);
-fail_reg:
g_free(reg_attr);
g_free(nodename);
- return ret;
+ return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread