* [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-11 14:28 ` Stefan Hajnoczi
2018-07-09 9:11 ` [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes Emanuele Giuseppe Esposito
` (7 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Add qgraph API that allows to add/remove nodes and edges from the graph,
implementation of Depth First Search to discover the paths and basic unit
test to check correctness of the API.
graph.h provides the public API to manage the graph nodes/edges
graph_extra.h provides a more private API used successively by the gtest integration part
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
configure | 2 +-
include/qemu/module.h | 2 +
tests/Makefile.include | 5 +
tests/libqos/qgraph.c | 676 ++++++++++++++++++++++++++++++++++++
tests/libqos/qgraph.h | 259 ++++++++++++++
| 155 +++++++++
tests/test-qgraph.c | 446 ++++++++++++++++++++++++
7 files changed, 1544 insertions(+), 1 deletion(-)
create mode 100644 tests/libqos/qgraph.c
create mode 100644 tests/libqos/qgraph.h
create mode 100644 tests/libqos/qgraph_extra.h
create mode 100644 tests/test-qgraph.c
diff --git a/configure b/configure
index 2a7796ea80..85a108506a 100755
--- a/configure
+++ b/configure
@@ -7352,7 +7352,7 @@ if test "$ccache_cpp2" = "yes"; then
fi
# build tree in object directory in case the source is not in the current directory
-DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests tests/vm"
+DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests tests/vm tests/qgraph"
DIRS="$DIRS docs docs/interop fsdev scsi"
DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
DIRS="$DIRS roms/seabios roms/vgabios"
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 54300ab6e5..1fcdca084d 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -44,6 +44,7 @@ typedef enum {
MODULE_INIT_OPTS,
MODULE_INIT_QOM,
MODULE_INIT_TRACE,
+ MODULE_INIT_LIBQOS,
MODULE_INIT_MAX
} module_init_type;
@@ -51,6 +52,7 @@ typedef enum {
#define opts_init(function) module_init(function, MODULE_INIT_OPTS)
#define type_init(function) module_init(function, MODULE_INIT_QOM)
#define trace_init(function) module_init(function, MODULE_INIT_TRACE)
+#define libqos_init(function) module_init(function, MODULE_INIT_LIBQOS)
#define block_module_load_one(lib) module_load_one("block-", lib)
#define ui_module_load_one(lib) module_load_one("ui-", lib)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index a49282704e..b16bbd55df 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -769,6 +769,11 @@ libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o
+libqgraph-obj-y = tests/libqos/qgraph.o
+
+check-unit-y += tests/test-qgraph$(EXESUF)
+tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
+
tests/qmp-test$(EXESUF): tests/qmp-test.o
tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
tests/rtc-test$(EXESUF): tests/rtc-test.o
diff --git a/tests/libqos/qgraph.c b/tests/libqos/qgraph.c
new file mode 100644
index 0000000000..5e37ba9f6d
--- /dev/null
+++ b/tests/libqos/qgraph.c
@@ -0,0 +1,676 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qemu/queue.h"
+#include "qgraph.h"
+#include "qgraph_extra.h"
+
+#define QOS_ROOT ""
+typedef struct QOSStackElement QOSStackElement;
+
+/* Graph Edge.*/
+struct QOSGraphEdge {
+ QOSEdgeType type;
+ char *dest;
+ char *arg; /* just for CONTAIS and CONSUMED_BY */
+ QSLIST_ENTRY(QOSGraphEdge) edge_list;
+};
+
+/* Linked list grouping all edges with the same source node */
+QSLIST_HEAD(QOSGraphEdgeList, QOSGraphEdge);
+
+
+/**
+ * Stack used to keep track of the discovered path when using
+ * the DFS algorithm
+ */
+struct QOSStackElement {
+ QOSGraphNode *node;
+ QOSStackElement *parent;
+ QOSGraphEdge *parent_edge;
+ int length;
+};
+
+/* Each enty in these hash table will consist of <string, node/edge> pair. */
+static GHashTable *edge_table;
+static GHashTable *node_table;
+
+/* stack used by the DFS algorithm to store the path from machine to test */
+static QOSStackElement qos_node_stack[QOS_PATH_MAX_ELEMENT_SIZE];
+static int qos_node_tos;
+
+/**
+ * add_edge_arg(): creates an edge of type @type
+ * from @source to @dest node, and inserts it in the
+ * edges hash table
+ *
+ * Nodes @source and @dest do not necessarily need to exist.
+ * Adds also an optional command line arg.
+ */
+static void add_edge_arg(const char *source, const char *dest,
+ QOSEdgeType type, const char *arg)
+{
+ char *key;
+ QOSGraphEdgeList *list = g_hash_table_lookup(edge_table, source);
+ if (!list) {
+ list = g_new0(QOSGraphEdgeList, 1);
+ key = g_strdup(source);
+ g_hash_table_insert(edge_table, key, list);
+ }
+
+ QOSGraphEdge *edge = g_new0(QOSGraphEdge, 1);
+ edge->type = type;
+ edge->dest = g_strdup(dest);
+ if (arg) {
+ edge->arg = g_strconcat(",", arg, NULL);
+ }
+ QSLIST_INSERT_HEAD(list, edge, edge_list);
+}
+
+/* add_edge(): same as add_edge_arg, but the arg is null */
+static void add_edge(const char *source, const char *dest, QOSEdgeType type)
+{
+ add_edge_arg(source, dest, type, NULL);
+}
+
+/* remove_edges(): removes all edges inside a given @list */
+static void remove_edges(void *list)
+{
+ QOSGraphEdge *temp;
+ QOSGraphEdgeList *elist = list;
+
+ while (!QSLIST_EMPTY(elist)) {
+ temp = QSLIST_FIRST(elist);
+ QSLIST_REMOVE_HEAD(elist, edge_list);
+ free(temp);
+ }
+ g_free(elist);
+}
+
+/**
+ * create_node(): creates a node @name of type @type
+ * and inserts it to the nodes hash table.
+ * By default, node is not available.
+ */
+static QOSGraphNode *create_node(const char *name, QOSNodeType type)
+{
+ if (g_hash_table_lookup(node_table, name)) {
+ g_printerr("Node %s already created\n", name);
+ abort();
+ }
+
+ QOSGraphNode *node = g_new0(QOSGraphNode, 1);
+ node->type = type;
+ node->available = FALSE;
+ node->name = g_strdup(name);
+ g_hash_table_insert(node_table, node->name, node);
+ return node;
+}
+
+/**
+ * remove_node(): removes a node @val from the nodes hash table.
+ * Note that node->name is not free'd since it will represent the
+ * hash table key
+ */
+static void remove_node(void *val)
+{
+ QOSGraphNode *node = (QOSGraphNode *) val;
+ g_free(node->command_line);
+ g_free(node);
+}
+
+/**
+ * remove_string(): removes @key from the nodes hash table.
+ * Actually frees the node->name
+ */
+static void remove_string(void *key)
+{
+ g_free(key);
+}
+
+/**
+ * search_node(): search for a node @key in the nodes hash table
+ * Returns the QOSGraphNode if found, fails otherwise
+ */
+static QOSGraphNode *search_node(const char *key)
+{
+ return g_hash_table_lookup(node_table, key);
+}
+
+/**
+ * get_edgelist(): returns the edge list (value) assigned to
+ * the @key in the edge hash table.
+ * This list will contain all edges with source equal to @key
+ *
+ * Returns: on success: the %QOSGraphEdgeList
+ * otherwise: abort()
+ */
+static QOSGraphEdgeList *get_edgelist(const char *key)
+{
+ return g_hash_table_lookup(edge_table, key);
+}
+
+/**
+ * search_list_edges(): search for an edge with destination @dest
+ * in the given @edgelist.
+ *
+ * Returns: on success: the %QOSGraphEdge
+ * otherwise: #NULL
+ */
+static QOSGraphEdge *search_list_edges(QOSGraphEdgeList *edgelist,
+ const char *dest)
+{
+ QOSGraphEdge *tmp, *next;
+ if (!edgelist) {
+ return NULL;
+ }
+ QSLIST_FOREACH_SAFE(tmp, edgelist, edge_list, next) {
+ if (g_strcmp0(tmp->dest, dest) == 0) {
+ break;
+ }
+ }
+ return tmp;
+}
+
+/**
+ * search_machine(): search for a machine @name in the node hash
+ * table. A machine is the child of the root node.
+ * This function forces the research in the childs of the root,
+ * to check the node is a proper machine
+ *
+ * Returns: on success: the %QOSGraphNode
+ * otherwise: #NULL
+ */
+static QOSGraphNode *search_machine(const char *name)
+{
+ QOSGraphNode *n;
+ QOSGraphEdgeList *root_list = get_edgelist(QOS_ROOT);
+ QOSGraphEdge *e = search_list_edges(root_list, name);
+ if (!e) {
+ return NULL;
+ }
+ n = search_node(e->dest);
+ if (n->type == MACHINE) {
+ return n;
+ }
+ return NULL;
+}
+
+/**
+ * build_machine_cmd_line(): builds the command line for the machine
+ * @node. The node name must be a valid qemu identifier, since it
+ * will be used to build the command line.
+ *
+ * It is also possible to pass an optional @args that will be
+ * concatenated to the command line.
+ *
+ * For machines, prepend -M to the machine name.
+ */
+static void build_machine_cmd_line(QOSGraphNode *node, const char *args)
+{
+ char *arch, *machine;
+ qos_separate_arch_machine(node->name, &arch, &machine);
+ if (args) {
+ node->command_line = g_strconcat("-M ", machine, ",", args, NULL);
+ } else {
+ node->command_line = g_strconcat("-M ", machine, NULL);
+ }
+}
+
+/**
+ * build_driver_cmd_line(): builds the command line for the driver
+ * @node. The node name must be a valid qemu identifier, since it
+ * will be used to build the command line.
+ *
+ * It is also possible to pass an optional @args that will be
+ * concatenated to the command line.
+ *
+ * For drivers, prepend -device to the driver name.
+ */
+static void build_driver_cmd_line(QOSGraphNode *node, const char *args)
+{
+ if (args) {
+ node->command_line = g_strconcat("-device ", node->name, ",",
+ args, NULL);
+ } else {
+ node->command_line = g_strconcat("-device ", node->name, NULL);
+ }
+}
+
+/**
+ * build_test_cmd_line(): builds the command line for the test
+ * @node. The node name need not to be a valid qemu identifier, since it
+ * will not be used to build the command line.
+ *
+ * It is also possible to pass an optional @args that will be
+ * used as additional command line.
+ */
+static void build_test_cmd_line(QOSGraphNode *node, const char *args)
+{
+ if (args) {
+ node->command_line = g_strdup(args);
+ } else {
+ node->command_line = NULL;
+ }
+}
+
+/* qos_print_cb(): callback prints all path found by the DFS algorithm. */
+static void qos_print_cb(QOSGraphNode *path, int length)
+{
+ #if PRINT_DEBUG
+ printf("%d elements\n", length);
+
+ if (!path) {
+ return;
+ }
+
+ while (path->path_edge) {
+ printf("%s ", path->name);
+ switch (path->path_edge->type) {
+ case PRODUCES:
+ printf("--PRODUCES--> ");
+ break;
+ case CONSUMED_BY:
+ printf("--CONSUMED_BY--> ");
+ break;
+ case CONTAINS:
+ printf("--CONTAINS--> ");
+ break;
+ }
+ path = search_node(path->path_edge->dest);
+ }
+
+ printf("%s\n\n", path->name);
+ #endif
+}
+
+/* qos_push(): push a node @el and edge @e in the qos_node_stack */
+static void qos_push(QOSGraphNode *el, QOSStackElement *parent,
+ QOSGraphEdge *e)
+{
+ int len = 0; /* root is not counted */
+ if (qos_node_tos == QOS_PATH_MAX_ELEMENT_SIZE) {
+ g_printerr("QOSStack: full stack, cannot push");
+ abort();
+ }
+
+ if (parent) {
+ len = parent->length + 1;
+ }
+ qos_node_stack[qos_node_tos++] = (QOSStackElement) {el, parent, e, len};
+}
+
+/* qos_tos(): returns the top of stack, without popping */
+static QOSStackElement *qos_tos(void)
+{
+ return &qos_node_stack[(qos_node_tos - 1)];
+}
+
+/* qos_pop(): pops an element from the tos, setting it unvisited*/
+static QOSStackElement *qos_pop(void)
+{
+ if (qos_node_tos == 0) {
+ g_printerr("QOSStack: empty stack, cannot pop");
+ abort();
+ }
+ QOSStackElement *e = qos_tos();
+ e->node->visited = FALSE;
+ qos_node_tos--;
+ return e;
+}
+
+/**
+ * qos_reverse_path(): reverses the found path, going from
+ * test-to-machine to machine-to-test
+ */
+static QOSGraphNode *qos_reverse_path(QOSStackElement *el)
+{
+ if (!el) {
+ return NULL;
+ }
+
+ el->node->path_edge = NULL;
+
+ while (el->parent->length > 0) {
+ el->parent->node->path_edge = el->parent_edge;
+ el = el->parent;
+ }
+
+ return el->node;
+}
+
+/**
+ * qos_traverse_graph(): graph-walking algorithm, using Depth First Search it
+ * starts from the root @machine and walks all possible path until it
+ * reaches a test node.
+ * At that point, it reverses the path found and invokes the @callback.
+ *
+ * Being Depth First Search, time complexity is O(|V| + |E|), while
+ * space is O(|V|). In this case, the maximum stack size is set by
+ * QOS_PATH_MAX_ELEMENT_SIZE.
+ */
+static void qos_traverse_graph(QOSGraphNode *machine, QOSTestCallback callback)
+{
+ QOSGraphNode *v, *dest_node, *path;
+ QOSStackElement *s_el;
+ QOSGraphEdge *e, *next;
+ QOSGraphEdgeList *list;
+
+ qos_push(machine, NULL, NULL);
+
+ while (qos_node_tos > 0) {
+ s_el = qos_tos();
+ v = s_el->node;
+ if (v->visited) {
+ qos_pop();
+ continue;
+ }
+ v->visited = TRUE;
+ list = get_edgelist(v->name);
+ if (!list) {
+ qos_pop();
+ if (v->type == TEST) {
+ v->visited = FALSE;
+ path = qos_reverse_path(s_el);
+ callback(path, s_el->length);
+ }
+ } else {
+ QSLIST_FOREACH_SAFE(e, list, edge_list, next) {
+ dest_node = search_node(e->dest);
+
+ if (!dest_node) {
+ printf("node %s in %s -> %s does not exist\n",
+ e->dest, v->name, e->dest);
+ abort();
+ }
+
+ if (!dest_node->visited && dest_node->available) {
+ qos_push(dest_node, s_el, e);
+ }
+ }
+ }
+ }
+}
+
+/* QGRAPH API*/
+
+QOSGraphNode *qos_graph_get_node(const char *key)
+{
+ return search_node(key);
+}
+
+bool qos_graph_has_node(const char *node)
+{
+ QOSGraphNode *n = search_node(node);
+ return n != NULL;
+}
+
+QOSNodeType qos_graph_get_node_type(const char *node)
+{
+ QOSGraphNode *n = search_node(node);
+ if (n) {
+ return n->type;
+ }
+ return -1;
+}
+
+bool qos_graph_get_node_availability(const char *node)
+{
+ QOSGraphNode *n = search_node(node);
+ if (n) {
+ return n->available;
+ }
+ return FALSE;
+}
+
+QOSGraphEdge *qos_graph_get_edge(const char *node, const char *dest)
+{
+ QOSGraphEdgeList *list = get_edgelist(node);
+ return search_list_edges(list, dest);
+}
+
+QOSEdgeType qos_graph_get_edge_type(const char *node1, const char *node2)
+{
+ QOSGraphEdgeList *list = get_edgelist(node1);
+ QOSGraphEdge *e = search_list_edges(list, node2);
+ if (e) {
+ return e->type;
+ }
+ return -1;
+}
+
+char *qos_graph_get_edge_dest(QOSGraphEdge *edge)
+{
+ if (!edge) {
+ return NULL;
+ }
+ return edge->dest;
+}
+
+char *qos_graph_get_edge_arg(QOSGraphEdge *edge)
+{
+ if (!edge) {
+ return NULL;
+ }
+ return edge->arg;
+}
+
+bool qos_graph_has_edge(const char *start, const char *dest)
+{
+ QOSGraphEdgeList *list = get_edgelist(start);
+ QOSGraphEdge *e = search_list_edges(list, dest);
+ if (e) {
+ return TRUE;
+ }
+ return FALSE;
+}
+
+QOSGraphNode *qos_graph_get_machine(const char *node)
+{
+ return search_machine(node);
+}
+
+bool qos_graph_has_machine(const char *node)
+{
+ QOSGraphNode *m = search_machine(node);
+ return m != NULL;
+}
+
+void qos_print_graph(void)
+{
+ qos_graph_foreach_test_path(qos_print_cb);
+}
+
+void qos_graph_init(void)
+{
+ if (!node_table) {
+ node_table = g_hash_table_new_full(g_str_hash, g_str_equal,
+ remove_string, remove_node);
+ create_node(QOS_ROOT, DRIVER);
+ }
+
+ if (!edge_table) {
+ edge_table = g_hash_table_new_full(g_str_hash, g_str_equal,
+ remove_string, remove_edges);
+ }
+}
+
+void qos_graph_destroy(void)
+{
+ if (node_table) {
+ g_hash_table_destroy(node_table);
+ }
+
+ if (edge_table) {
+ g_hash_table_destroy(edge_table);
+ }
+
+ node_table = NULL;
+ edge_table = NULL;
+}
+
+void qos_node_destroy(void *key)
+{
+ g_hash_table_remove(node_table, key);
+}
+
+void qos_edge_destroy(void *key)
+{
+ g_hash_table_remove(edge_table, key);
+}
+
+void qos_add_test(const char *name, const char *driver, QOSTestFunc test_func)
+{
+ qos_add_test_data_args(name, driver, test_func, NULL, NULL);
+}
+
+void qos_add_test_args(const char *name, const char *driver,
+ QOSTestFunc test_func, const char *extra_args)
+{
+ qos_add_test_data_args(name, driver, test_func, NULL, extra_args);
+}
+
+void qos_add_test_data(const char *name, const char *driver,
+ QOSTestFunc test_func, void *arg)
+{
+ qos_add_test_data_args(name, driver, test_func, arg, NULL);
+}
+
+void qos_add_test_data_args(const char *name, const char *driver,
+ QOSTestFunc test_func, void *arg,
+ const char *extra_args)
+{
+ QOSGraphNode *node = create_node(name, TEST);
+ build_test_cmd_line(node, extra_args);
+ node->u.test.function = test_func;
+ node->u.test.arg = arg;
+ node->available = TRUE;
+ add_edge(driver, name, CONSUMED_BY);
+}
+
+void qos_node_create_machine(const char *name, QOSCreateMachineFunc function)
+{
+ qos_node_create_machine_args(name, function, NULL);
+}
+
+void qos_node_create_machine_args(const char *name,
+ QOSCreateMachineFunc function,
+ const char *extra_args)
+{
+ QOSGraphNode *node = create_node(name, MACHINE);
+ build_machine_cmd_line(node, extra_args);
+ node->u.machine.constructor = function;
+ add_edge(QOS_ROOT, name, CONTAINS);
+}
+
+void qos_node_create_driver(const char *name, QOSCreateDriverFunc function)
+{
+ qos_node_create_driver_args(name, function, NULL);
+}
+
+void qos_node_create_driver_args(const char *name,
+ QOSCreateDriverFunc function,
+ const char *extra_args)
+{
+ QOSGraphNode *node = create_node(name, DRIVER);
+ build_driver_cmd_line(node, extra_args);
+ node->u.driver.constructor = function;
+}
+
+void qos_node_create_interface(const char *name)
+{
+ create_node(name, INTERFACE);
+}
+
+void qos_node_contains(const char *container, const char *contained)
+{
+ add_edge(container, contained, CONTAINS);
+}
+
+void qos_node_contains_arg(const char *container, const char *contained,
+ const char *arg)
+{
+ add_edge_arg(container, contained, CONTAINS, arg);
+}
+
+void qos_node_produces(const char *producer, const char *produced)
+{
+ add_edge(producer, produced, PRODUCES);
+}
+
+void qos_node_consumes(const char *consumer, const char *consumed)
+{
+ add_edge(consumed, consumer, CONSUMED_BY);
+}
+
+void qos_node_consumes_arg(const char *consumer, const char *consumed,
+ const char *arg)
+{
+ add_edge_arg(consumed, consumer, CONSUMED_BY, arg);
+}
+
+void qos_graph_node_set_availability(const char *node, bool av)
+{
+ QOSGraphEdgeList *elist;
+ QOSGraphNode *n = search_node(node);
+ QOSGraphEdge *e, *next;
+ if (!n) {
+ return;
+ }
+ n->available = av;
+ elist = get_edgelist(node);
+ if (!elist) {
+ return;
+ }
+ QSLIST_FOREACH_SAFE(e, elist, edge_list, next) {
+ if (e->type == CONTAINS || e->type == PRODUCES) {
+ qos_graph_node_set_availability(e->dest, av);
+ }
+ }
+}
+
+void qos_graph_foreach_test_path(QOSTestCallback fn)
+{
+ QOSGraphNode *root = qos_graph_get_node(QOS_ROOT);
+ qos_traverse_graph(root, fn);
+}
+
+void qos_destroy_object(QOSGraphObject *obj)
+{
+ if (!obj || !(obj->destructor)) {
+ return;
+ }
+ obj->destructor(obj);
+}
+
+void qos_separate_arch_machine(char *name, char **arch, char **machine)
+{
+ *arch = name;
+ while (*name != '\0' && *name != '/') {
+ name++;
+ }
+
+ if (*name == '/' && (*name + 1) != '\0') {
+ *machine = name + 1;
+ } else {
+ printf("Machine name has to be of the form <arch>/<machine>\n");
+ abort();
+ }
+}
diff --git a/tests/libqos/qgraph.h b/tests/libqos/qgraph.h
new file mode 100644
index 0000000000..54a1786c1e
--- /dev/null
+++ b/tests/libqos/qgraph.h
@@ -0,0 +1,259 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#ifndef QGRAPH_H
+#define QGRAPH_H
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <gmodule.h>
+#include <glib.h>
+#include "qemu/module.h"
+
+/* maximum path length */
+#define QOS_PATH_MAX_ELEMENT_SIZE 50
+
+typedef struct QOSGraphMachine QOSGraphMachine;
+typedef struct QOSGraphMachineList QOSGraphMachineList;
+typedef struct QOSGraphObject QOSGraphObject;
+typedef struct QOSGraphNode QOSGraphNode;
+typedef struct QOSGraphEdge QOSGraphEdge;
+typedef struct QOSGraphEdgeList QOSGraphEdgeList;
+typedef enum QOSEdgeType QOSEdgeType;
+typedef enum QOSNodeType QOSNodeType;
+
+typedef void *(*QOSCreateDriverFunc) (void *parent, QOSGraphObject *alloc);
+typedef void *(*QOSCreateMachineFunc) (void);
+typedef void (*QOSTestFunc) (void *parent, void *arg);
+typedef void (*QOSTestCallback) (QOSGraphNode *path, int len);
+
+typedef void *(*QOSGetDriver) (void *object, const char *interface);
+typedef QOSGraphObject *(*QOSGetDevice) (void *object, const char *name);
+typedef void (*QOSDestructorFunc) (QOSGraphObject *object);
+
+/* edge types*/
+enum QOSEdgeType {
+ CONTAINS,
+ PRODUCES,
+ CONSUMED_BY
+};
+
+/* node types*/
+enum QOSNodeType {
+ MACHINE,
+ DRIVER,
+ INTERFACE,
+ TEST
+};
+
+/**
+ * Each driver, test or machine will have this as first field.
+ * Depending on the edge, the node will call the corresponding
+ * function when walking the path.
+ *
+ * QOSGraphObject also provides a destructor, used to deallocate the
+ * after the test has been executed.
+ */
+struct QOSGraphObject {
+ /* for produces, returns void * */
+ QOSGetDriver get_driver;
+ /* for contains, returns a QOSGraphObject * */
+ QOSGetDevice get_device;
+ /* destroy this QOSGraphObject */
+ QOSDestructorFunc destructor;
+};
+
+/* Graph Node */
+struct QOSGraphNode {
+ QOSNodeType type;
+ bool available; /* set by QEMU via QMP, used during graph walk */
+ bool visited; /* used during graph walk */
+ char *name; /* used to identify the node */
+ char *command_line; /* used to start QEMU at test execution */
+ union {
+ struct {
+ QOSCreateDriverFunc constructor;
+ } driver;
+ struct {
+ QOSCreateMachineFunc constructor;
+ } machine;
+ struct {
+ QOSTestFunc function;
+ void *arg;
+ } test;
+ } u;
+
+ /**
+ * only used when traversing the path, never rely on that except in the
+ * qos_traverse_graph callback function
+ */
+ QOSGraphEdge *path_edge;
+};
+
+/**
+ * qos_graph_init(): initialize the framework, creates two hash
+ * tables: one for the nodes and another for the edges.
+ */
+void qos_graph_init(void);
+
+/**
+ * qos_graph_destroy(): deallocates all the hash tables,
+ * freeing all nodes and edges.
+ */
+void qos_graph_destroy(void);
+
+/**
+ * qos_node_destroy(): removes and frees a node from the,
+ * nodes hash table.
+ */
+void qos_node_destroy(void *key);
+
+/**
+ * qos_edge_destroy(): removes and frees an edge from the,
+ * edges hash table.
+ */
+void qos_edge_destroy(void *key);
+
+/**
+ * qos_add_test(): adds a test node @name to the nodes hash table.
+ *
+ * The test will consume a @driver node, and once the
+ * graph walking algorithm has found it, the @test_func will be
+ * executed.
+ */
+void qos_add_test(const char *name, const char *driver, QOSTestFunc test_func);
+
+/**
+ * qos_add_test_args(): same as qos_add_test, with the possibility to
+ * add an optional @extra_args for the command line.
+ */
+void qos_add_test_args(const char *name, const char *driver,
+ QOSTestFunc test_func,
+ const char *extra_args);
+
+/**
+ * qos_add_test(): adds a test node @name to the nodes hash table.
+ *
+ * The test will consume a @driver node, and once the
+ * graph walking algorithm has found it, the @test_func will be
+ * executed passing @arg as parameter.
+ */
+void qos_add_test_data(const char *name, const char *driver,
+ QOSTestFunc test_func, void *arg);
+
+/**
+ * qos_add_test_data_args(): same as qos_add_test_data, with the possibility to
+ * add an optional @extra_args for the command line.
+ */
+void qos_add_test_data_args(const char *name, const char *driver,
+ QOSTestFunc test_func, void *arg,
+ const char *extra_args);
+
+/**
+ * qos_node_create_machine(): creates the machine @name and
+ * adds it to the node hash table.
+ *
+ * This node will be of type MACHINE and have @function as constructor
+ */
+void qos_node_create_machine(const char *name, QOSCreateMachineFunc function);
+
+/**
+ * qos_node_create_machine_args(): same as qos_node_create_machine,
+ * but with the possibility to add an optional @extra_args to the
+ * command line.
+ */
+void qos_node_create_machine_args(const char *name,
+ QOSCreateMachineFunc function,
+ const char *extra_args);
+
+/**
+ * qos_node_create_driver(): creates the driver @name and
+ * adds it to the node hash table.
+ *
+ * This node will be of type DRIVER and have @function as constructor
+ */
+void qos_node_create_driver(const char *name, QOSCreateDriverFunc function);
+
+/**
+ * qos_node_create_driver_args(): same as qos_node_create_driver,
+ * but with the possibility to add an optional @extra_args to the
+ * command line.
+ */
+void qos_node_create_driver_args(const char *name,
+ QOSCreateDriverFunc function,
+ const char *extra_args);
+
+/**
+ * qos_node_create_driver(): creates the interface @name and
+ * adds it to the node hash table.
+ *
+ * This node will be of type INTERFACE and won't have any constructor
+ */
+void qos_node_create_interface(const char *name);
+
+/**
+ * qos_node_contains(): creates the edge CONTAINS and
+ * adds it to the edge list mapped to @container in the
+ * edge hash table.
+ *
+ * This edge will have @container as source and @contained as destination.
+ */
+void qos_node_contains(const char *container, const char *contained);
+
+/**
+ * qos_node_contains_arg(): same as qos_node_contains,
+ * but with the possibility to add an optional @arg to the
+ * command line.
+ */
+void qos_node_contains_arg(const char *container, const char *contained,
+ const char *arg);
+
+/**
+ * qos_node_produces(): creates the edge PRODUCES and
+ * adds it to the edge list mapped to @producer in the
+ * edge hash table.
+ *
+ * This edge will have @producer as source and @produced as destination.
+ */
+void qos_node_produces(const char *producer, const char *produced);
+
+/**
+ * qos_node_consumes(): creates the edge CONSUMED_BY and
+ * adds it to the edge list mapped to @consumed in the
+ * edge hash table.
+ *
+ * This edge will have @consumed as source and @consumer as destination.
+ */
+void qos_node_consumes(const char *consumer, const char *consumed);
+
+/**
+ * qos_node_consumes_arg(): same as qos_node_consumes,
+ * but with the possibility to add an optional @arg to the
+ * command line.
+ */
+void qos_node_consumes_arg(const char *consumer, const char *consumed,
+ const char *arg);
+
+/**
+ * qos_graph_node_set_availability(): sets the node identified
+ * by @node with availability @av.
+ */
+void qos_graph_node_set_availability(const char *node, bool av);
+
+#endif
--git a/tests/libqos/qgraph_extra.h b/tests/libqos/qgraph_extra.h
new file mode 100644
index 0000000000..22850c0368
--- /dev/null
+++ b/tests/libqos/qgraph_extra.h
@@ -0,0 +1,155 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#ifndef QGRAPH_EXTRA_H
+#define QGRAPH_EXTRA_H
+
+#include "qgraph.h"
+#define PRINT_DEBUG 0
+
+/**
+ * qos_graph_get_node(): returns the node mapped to that @key.
+ * It performs an hash map search O(1)
+ *
+ * Returns: on success: the %QOSGraphNode
+ * otherwise: #NULL
+ */
+QOSGraphNode *qos_graph_get_node(const char *key);
+
+/**
+ * qos_graph_has_node(): returns #TRUE if the node
+ * has map has a node mapped to that @key.
+ */
+bool qos_graph_has_node(const char *node);
+
+/**
+ * qos_graph_get_node_type(): returns the %QOSNodeType
+ * of the node @node.
+ * It performs an hash map search O(1)
+ * Returns: on success: the %QOSNodeType
+ * otherwise: #-1
+ */
+QOSNodeType qos_graph_get_node_type(const char *node);
+
+/**
+ * qos_graph_get_node_availability(): returns the availability (boolean)
+ * of the node @node.
+ */
+bool qos_graph_get_node_availability(const char *node);
+
+/**
+ * qos_graph_get_edge(): returns the edge
+ * linking of the node @node with @dest.
+ *
+ * Returns: on success: the %QOSGraphEdge
+ * otherwise: #NULL
+ */
+QOSGraphEdge *qos_graph_get_edge(const char *node, const char *dest);
+
+/**
+ * qos_graph_get_edge_type(): returns the edge type
+ * of the edge linking of the node @start with @dest.
+ *
+ * Returns: on success: the %QOSEdgeType
+ * otherwise: #-1
+ */
+QOSEdgeType qos_graph_get_edge_type(const char *start, const char *dest);
+
+/**
+ * qos_graph_get_edge_dest(): returns the name of the node
+ * pointed as destination of edge @edge.
+ *
+ * Returns: on success: the destination
+ * otherwise: #NULL
+ */
+char *qos_graph_get_edge_dest(QOSGraphEdge *edge);
+
+/**
+ * qos_graph_has_edge(): returns #TRUE if there
+ * exists an edge from @start to @dest.
+ */
+bool qos_graph_has_edge(const char *start, const char *dest);
+
+/**
+ * qos_graph_get_edge_arg(): returns the args assigned
+ * to that @edge.
+ *
+ * Returns: on success: the arg
+ * otherwise: #NULL
+ */
+char *qos_graph_get_edge_arg(QOSGraphEdge *edge);
+
+/**
+ * qos_graph_get_machine(): returns the machine assigned
+ * to that @node name.
+ *
+ * It performs a search only trough the list of machines
+ * (i.e. the QOS_ROOT child).
+ *
+ * Returns: on success: the %QOSGraphNode
+ * otherwise: #NULL
+ */
+QOSGraphNode *qos_graph_get_machine(const char *node);
+
+/**
+ * qos_graph_has_machine(): returns #TRUE if the node
+ * has map has a node mapped to that @node.
+ */
+bool qos_graph_has_machine(const char *node);
+
+
+/**
+ * qos_print_graph(): walks the graph and prints
+ * all machine-to-test paths.
+ */
+void qos_print_graph(void);
+
+/**
+ * qos_graph_foreach_test_path(): executes the Depth First search
+ * algorithm and applies @fn to all discovered paths.
+ *
+ * See qos_traverse_graph() in qgraph.c for more info on
+ * how it works.
+ */
+void qos_graph_foreach_test_path(QOSTestCallback fn);
+
+/**
+ * qos_destroy_object(): calls the destructor for @obj
+ */
+void qos_destroy_object(QOSGraphObject *obj);
+
+/**
+ * qos_separate_arch_machine(): separate arch from machine.
+ * This function requires every machine @name to be in the form
+ * <arch>/<machine_name>, like "arm/raspi2" or "x86_64/pc".
+ *
+ * The function will split then the string in two parts,
+ * assigning @arch to point to <arch>/<machine_name>, and
+ * @machine to <machine_name>.
+ *
+ * For example, "x86_64/pc" will be split in this way:
+ * *arch = "x86_64/pc"
+ * *machine = "pc"
+ *
+ * Note that this function *does not* allocate any new string,
+ * but just sets the pointer *arch and *machine to the respective
+ * part of the string.
+ */
+void qos_separate_arch_machine(char *name, char **arch, char **machine);
+
+#endif
diff --git a/tests/test-qgraph.c b/tests/test-qgraph.c
new file mode 100644
index 0000000000..64bd32ec86
--- /dev/null
+++ b/tests/test-qgraph.c
@@ -0,0 +1,446 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "libqos/qgraph.h"
+#include "libqos/qgraph_extra.h"
+
+#define MACHINE_PC "x86_64/pc"
+#define MACHINE_RASPI2 "arm/raspi2"
+#define I440FX "i440FX-pcihost"
+#define PCIBUS_PC "pcibus-pc"
+#define SDHCI "sdhci"
+#define PCIBUS "pci-bus"
+#define SDHCI_PCI "sdhci-pci"
+#define SDHCI_MM "generic-sdhci"
+#define REGISTER_TEST "register-test"
+
+int npath;
+
+static void *machinefunct(void)
+{
+ return NULL;
+}
+
+static void *driverfunct(void *obj, QOSGraphObject *machine)
+{
+ return NULL;
+}
+
+static void testfunct(void *obj, void *arg)
+{
+ return;
+}
+
+static void check_machine(const char *machine)
+{
+ qos_node_create_machine(machine, machinefunct);
+ g_assert_nonnull(qos_graph_get_machine(machine));
+ g_assert_cmpint(qos_graph_has_machine(machine), ==, TRUE);
+ g_assert_nonnull(qos_graph_get_node(machine));
+ g_assert_cmpint(qos_graph_get_node_availability(machine), ==, FALSE);
+ qos_graph_node_set_availability(machine, TRUE);
+ g_assert_cmpint(qos_graph_get_node_availability(machine), ==, TRUE);
+ g_assert_cmpint(qos_graph_has_node(machine), ==, TRUE);
+ g_assert_cmpint(qos_graph_get_node_type(machine), ==, MACHINE);
+}
+
+static void check_contains(const char *machine, const char *i440fx)
+{
+ qos_node_contains(machine, i440fx);
+ g_assert_nonnull(qos_graph_get_edge(machine, i440fx));
+ g_assert_cmpint(qos_graph_get_edge_type(machine, i440fx), ==, CONTAINS);
+ g_assert_cmpint(qos_graph_has_edge(machine, i440fx), ==, TRUE);
+}
+
+static void check_produces(const char *machine, const char *i440fx)
+{
+ qos_node_produces(machine, i440fx);
+ g_assert_nonnull(qos_graph_get_edge(machine, i440fx));
+ g_assert_cmpint(qos_graph_get_edge_type(machine, i440fx), ==, PRODUCES);
+ g_assert_cmpint(qos_graph_has_edge(machine, i440fx), ==, TRUE);
+}
+
+static void check_consumes(const char *interface, const char *driver)
+{
+ qos_node_consumes(driver, interface);
+ g_assert_nonnull(qos_graph_get_edge(interface, driver));
+ g_assert_cmpint(
+ qos_graph_get_edge_type(interface, driver), ==, CONSUMED_BY);
+ g_assert_cmpint(qos_graph_has_edge(interface, driver), ==, TRUE);
+}
+
+static void check_driver(const char *driver)
+{
+ qos_node_create_driver(driver, driverfunct);
+ g_assert_cmpint(qos_graph_has_machine(driver), ==, FALSE);
+ g_assert_nonnull(qos_graph_get_node(driver));
+ g_assert_cmpint(qos_graph_has_node(driver), ==, TRUE);
+ g_assert_cmpint(qos_graph_get_node_type(driver), ==, DRIVER);
+ g_assert_cmpint(qos_graph_get_node_availability(driver), ==, FALSE);
+ qos_graph_node_set_availability(driver, TRUE);
+ g_assert_cmpint(qos_graph_get_node_availability(driver), ==, TRUE);
+}
+
+static void check_interface(const char *interface)
+{
+ qos_node_create_interface(interface);
+ g_assert_cmpint(qos_graph_has_machine(interface), ==, FALSE);
+ g_assert_nonnull(qos_graph_get_node(interface));
+ g_assert_cmpint(qos_graph_has_node(interface), ==, TRUE);
+ g_assert_cmpint(qos_graph_get_node_type(interface), ==, INTERFACE);
+ g_assert_cmpint(qos_graph_get_node_availability(interface), ==, FALSE);
+ qos_graph_node_set_availability(interface, TRUE);
+ g_assert_cmpint(qos_graph_get_node_availability(interface), ==, TRUE);
+}
+
+static void check_test(const char *test, const char *interface)
+{
+ qos_add_test(test, interface, testfunct);
+ g_assert_cmpint(qos_graph_has_machine(test), ==, FALSE);
+ g_assert_nonnull(qos_graph_get_node(test));
+ g_assert_cmpint(qos_graph_has_node(test), ==, TRUE);
+ g_assert_cmpint(qos_graph_get_node_type(test), ==, TEST);
+ g_assert_nonnull(qos_graph_get_edge(interface, test));
+ g_assert_cmpint(qos_graph_get_edge_type(interface, test), ==, CONSUMED_BY);
+ g_assert_cmpint(qos_graph_has_edge(interface, test), ==, TRUE);
+ g_assert_cmpint(qos_graph_get_node_availability(test), ==, TRUE);
+ qos_graph_node_set_availability(test, FALSE);
+ g_assert_cmpint(qos_graph_get_node_availability(test), ==, FALSE);
+}
+
+static void count_each_test(QOSGraphNode *path, int len)
+{
+ npath++;
+}
+
+static void check_leaf_discovered(int n)
+{
+ npath = 0;
+ qos_graph_foreach_test_path(count_each_test);
+ g_assert_cmpint(n, ==, npath);
+}
+
+/* G_Test functions */
+
+static void init_nop(void)
+{
+ qos_graph_init();
+ qos_graph_destroy();
+}
+
+static void test_machine(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_PC);
+ qos_graph_destroy();
+}
+
+static void test_contains(void)
+{
+ qos_graph_init();
+ check_contains(MACHINE_PC, I440FX);
+ g_assert_null(qos_graph_get_machine(MACHINE_PC));
+ g_assert_null(qos_graph_get_machine(I440FX));
+ g_assert_null(qos_graph_get_node(MACHINE_PC));
+ g_assert_null(qos_graph_get_node(I440FX));
+ qos_graph_destroy();
+}
+
+static void test_multiple_contains(void)
+{
+ qos_graph_init();
+ check_contains(MACHINE_PC, I440FX);
+ check_contains(MACHINE_PC, PCIBUS_PC);
+ qos_graph_destroy();
+}
+
+static void test_produces(void)
+{
+ qos_graph_init();
+ check_produces(MACHINE_PC, I440FX);
+ g_assert_null(qos_graph_get_machine(MACHINE_PC));
+ g_assert_null(qos_graph_get_machine(I440FX));
+ g_assert_null(qos_graph_get_node(MACHINE_PC));
+ g_assert_null(qos_graph_get_node(I440FX));
+ qos_graph_destroy();
+}
+
+static void test_multiple_produces(void)
+{
+ qos_graph_init();
+ check_produces(MACHINE_PC, I440FX);
+ check_produces(MACHINE_PC, PCIBUS_PC);
+ qos_graph_destroy();
+}
+
+static void test_consumed_by(void)
+{
+ qos_graph_init();
+ check_consumes(SDHCI, I440FX);
+ g_assert_null(qos_graph_get_machine(I440FX));
+ g_assert_null(qos_graph_get_machine(SDHCI));
+ g_assert_null(qos_graph_get_node(I440FX));
+ g_assert_null(qos_graph_get_node(SDHCI));
+ qos_graph_destroy();
+}
+
+static void test_multiple_consumed_by(void)
+{
+ qos_graph_init();
+ check_consumes(SDHCI, I440FX);
+ check_consumes(SDHCI, PCIBUS_PC);
+ qos_graph_destroy();
+}
+
+static void test_driver(void)
+{
+ qos_graph_init();
+ check_driver(I440FX);
+ qos_graph_destroy();
+}
+
+static void test_interface(void)
+{
+ qos_graph_init();
+ check_interface(SDHCI);
+ qos_graph_destroy();
+}
+
+static void test_test(void)
+{
+ qos_graph_init();
+ check_test(REGISTER_TEST, SDHCI);
+ qos_graph_destroy();
+}
+
+static void test_machine_contains_driver(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_PC);
+ check_driver(I440FX);
+ check_contains(MACHINE_PC, I440FX);
+ qos_graph_destroy();
+}
+
+static void test_driver_contains_driver(void)
+{
+ qos_graph_init();
+ check_driver(PCIBUS_PC);
+ check_driver(I440FX);
+ check_contains(PCIBUS_PC, I440FX);
+ qos_graph_destroy();
+}
+
+static void test_machine_produces_interface(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_PC);
+ check_interface(SDHCI);
+ check_produces(MACHINE_PC, SDHCI);
+ qos_graph_destroy();
+}
+
+static void test_driver_produces_interface(void)
+{
+ qos_graph_init();
+ check_driver(I440FX);
+ check_interface(SDHCI);
+ check_produces(I440FX, SDHCI);
+ qos_graph_destroy();
+}
+
+static void test_interface_consumed_by_machine(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_PC);
+ check_interface(SDHCI);
+ check_consumes(SDHCI, MACHINE_PC);
+ qos_graph_destroy();
+}
+
+static void test_interface_consumed_by_driver(void)
+{
+ qos_graph_init();
+ check_driver(I440FX);
+ check_interface(SDHCI);
+ check_consumes(SDHCI, I440FX);
+ qos_graph_destroy();
+}
+
+static void test_interface_consumed_by_test(void)
+{
+ qos_graph_init();
+ check_interface(SDHCI);
+ check_test(REGISTER_TEST, SDHCI);
+ qos_graph_destroy();
+}
+
+static void test_full_sample(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_PC);
+ check_contains(MACHINE_PC, I440FX);
+ check_driver(I440FX);
+ check_driver(PCIBUS_PC);
+ check_contains(I440FX, PCIBUS_PC);
+ check_interface(PCIBUS);
+ check_produces(PCIBUS_PC, PCIBUS);
+ check_driver(SDHCI_PCI);
+ qos_node_consumes(SDHCI_PCI, PCIBUS);
+ check_produces(SDHCI_PCI, SDHCI);
+ check_interface(SDHCI);
+ check_driver(SDHCI_MM);
+ check_produces(SDHCI_MM, SDHCI);
+ qos_add_test(REGISTER_TEST, SDHCI, testfunct);
+ check_leaf_discovered(1);
+ qos_print_graph();
+ qos_graph_destroy();
+}
+
+static void test_full_sample_raspi(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_PC);
+ check_contains(MACHINE_PC, I440FX);
+ check_driver(I440FX);
+ check_driver(PCIBUS_PC);
+ check_contains(I440FX, PCIBUS_PC);
+ check_interface(PCIBUS);
+ check_produces(PCIBUS_PC, PCIBUS);
+ check_driver(SDHCI_PCI);
+ qos_node_consumes(SDHCI_PCI, PCIBUS);
+ check_produces(SDHCI_PCI, SDHCI);
+ check_interface(SDHCI);
+ check_machine(MACHINE_RASPI2);
+ check_contains(MACHINE_RASPI2, SDHCI_MM);
+ check_driver(SDHCI_MM);
+ check_produces(SDHCI_MM, SDHCI);
+ qos_add_test(REGISTER_TEST, SDHCI, testfunct);
+ qos_print_graph();
+ check_leaf_discovered(2);
+ qos_graph_destroy();
+}
+
+static void test_full_alternative_path(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_RASPI2);
+ check_driver("B");
+ check_driver("C");
+ check_driver("D");
+ check_driver("E");
+ check_driver("F");
+ check_contains(MACHINE_RASPI2, "B");
+ check_contains("B", "C");
+ check_contains("C", "D");
+ check_contains("D", "E");
+ check_contains("D", "F");
+ qos_add_test("G", "D", testfunct);
+ check_contains("F", "G");
+ check_contains("E", "B");
+ qos_print_graph();
+ check_leaf_discovered(2);
+ qos_graph_destroy();
+}
+
+static void test_cycle(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_RASPI2);
+ check_driver("B");
+ check_driver("C");
+ check_driver("D");
+ check_contains(MACHINE_RASPI2, "B");
+ check_contains("B", "C");
+ check_contains("C", "D");
+ check_contains("D", MACHINE_RASPI2);
+ check_leaf_discovered(0);
+ qos_print_graph();
+ qos_graph_destroy();
+}
+
+static void test_two_test_same_interface(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_RASPI2);
+ check_interface("B");
+ qos_add_test("C", "B", testfunct);
+ qos_add_test("D", "B", testfunct);
+ check_contains(MACHINE_RASPI2, "B");
+ check_leaf_discovered(2);
+ qos_print_graph();
+ qos_graph_destroy();
+}
+
+static void test_double_edge(void)
+{
+ qos_graph_init();
+ check_machine(MACHINE_RASPI2);
+ check_driver("B");
+ check_driver("C");
+ check_produces("B", "C");
+ qos_node_consumes("C", "B");
+ qos_add_test("D", "C", testfunct);
+ check_contains(MACHINE_RASPI2, "B");
+ qos_print_graph();
+ qos_graph_destroy();
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/qgraph/init_nop", init_nop);
+ g_test_add_func("/qgraph/test_machine", test_machine);
+ g_test_add_func("/qgraph/test_contains", test_contains);
+ g_test_add_func("/qgraph/test_multiple_contains", test_multiple_contains);
+ g_test_add_func("/qgraph/test_produces", test_produces);
+ g_test_add_func("/qgraph/test_multiple_produces", test_multiple_produces);
+ g_test_add_func("/qgraph/test_consumed_by", test_consumed_by);
+ g_test_add_func("/qgraph/test_multiple_consumed_by",
+ test_multiple_consumed_by);
+ g_test_add_func("/qgraph/test_driver", test_driver);
+ g_test_add_func("/qgraph/test_interface", test_interface);
+ g_test_add_func("/qgraph/test_test", test_test);
+ g_test_add_func("/qgraph/test_machine_contains_driver",
+ test_machine_contains_driver);
+ g_test_add_func("/qgraph/test_driver_contains_driver",
+ test_driver_contains_driver);
+ g_test_add_func("/qgraph/test_machine_produces_interface",
+ test_machine_produces_interface);
+ g_test_add_func("/qgraph/test_driver_produces_interface",
+ test_driver_produces_interface);
+ g_test_add_func("/qgraph/test_interface_consumed_by_machine",
+ test_interface_consumed_by_machine);
+ g_test_add_func("/qgraph/test_interface_consumed_by_driver",
+ test_interface_consumed_by_driver);
+ g_test_add_func("/qgraph/test_interface_consumed_by_test",
+ test_interface_consumed_by_test);
+ g_test_add_func("/qgraph/test_full_sample", test_full_sample);
+ g_test_add_func("/qgraph/test_full_sample_raspi", test_full_sample_raspi);
+ g_test_add_func("/qgraph/test_full_alternative_path",
+ test_full_alternative_path);
+ g_test_add_func("/qgraph/test_cycle", test_cycle);
+ g_test_add_func("/qgraph/test_two_test_same_interface",
+ test_two_test_same_interface);
+ g_test_add_func("/qgraph/test_double_edge", test_double_edge);
+
+ g_test_run();
+ return 0;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-09 9:11 ` [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest " Emanuele Giuseppe Esposito
@ 2018-07-11 14:28 ` Stefan Hajnoczi
2018-07-11 14:58 ` Paolo Bonzini
0 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 14:28 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]
On Mon, Jul 09, 2018 at 11:11:30AM +0200, Emanuele Giuseppe Esposito wrote:
> +/* Graph Edge.*/
> +struct QOSGraphEdge {
> + QOSEdgeType type;
> + char *dest;
> + char *arg; /* just for CONTAIS and CONSUMED_BY */
CONTAINS?
> +/**
> + * remove_node(): removes a node @val from the nodes hash table.
> + * Note that node->name is not free'd since it will represent the
> + * hash table key
> + */
> +static void remove_node(void *val)
This is a confusing function name and doc comment. It does not remove
the node from a hash table, it just frees it.
Please call this destroy_node(). The g_hash_table_new_full() argument
is GDestroyNotify value_destroy_func, so that would be consistent with
glib naming too.
> +{
> + QOSGraphNode *node = (QOSGraphNode *) val;
There is no need to cast void pointers in C. Just "QOSGraphNode *node =
val;" will do.
> +/**
> + * build_driver_cmd_line(): builds the command line for the driver
> + * @node. The node name must be a valid qemu identifier, since it
> + * will be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * concatenated to the command line.
> + *
> + * For drivers, prepend -device to the driver name.
> + */
> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)
Why is this called "driver" instead of "device"?
> +{
> + if (args) {
> + node->command_line = g_strconcat("-device ", node->name, ",",
> + args, NULL);
> + } else {
> + node->command_line = g_strconcat("-device ", node->name, NULL);
> + }
> +}
> +
> +/**
> + * build_test_cmd_line(): builds the command line for the test
> + * @node. The node name need not to be a valid qemu identifier, since it
> + * will not be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * used as additional command line.
> + */
> +static void build_test_cmd_line(QOSGraphNode *node, const char *args)
> +{
> + if (args) {
> + node->command_line = g_strdup(args);
> + } else {
> + node->command_line = NULL;
> + }
This is equivalent to:
node->command_line = g_strdup(args);
https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup
> diff --git a/tests/libqos/qgraph.h b/tests/libqos/qgraph.h
> new file mode 100644
> index 0000000000..54a1786c1e
> --- /dev/null
> +++ b/tests/libqos/qgraph.h
> @@ -0,0 +1,259 @@
> +/*
> + * libqos driver framework
The per-function doc comment are good. Please also include higher-level
examples of how to use this. At first glance all these functions are
overwhelming and it's not clear how to implement new tests, drivers,
interfaces, or machines.
See include/qom/object.h for inspiration.
> +/* edge types*/
> +enum QOSEdgeType {
> + CONTAINS,
> + PRODUCES,
> + CONSUMED_BY
> +};
> +
> +/* node types*/
> +enum QOSNodeType {
> + MACHINE,
> + DRIVER,
> + INTERFACE,
> + TEST
> +};
The QOSEdgeType and QOSNodeType enum constants use commonly-used names
in the global namespace. Please prefix them since this is a header
file that will be included from many source files.
> +
> +/**
> + * Each driver, test or machine will have this as first field.
> + * Depending on the edge, the node will call the corresponding
> + * function when walking the path.
> + *
> + * QOSGraphObject also provides a destructor, used to deallocate the
> + * after the test has been executed.
> + */
> +struct QOSGraphObject {
> + /* for produces, returns void * */
> + QOSGetDriver get_driver;
Unused?
> + /* for contains, returns a QOSGraphObject * */
> + QOSGetDevice get_device;
Unused?
> diff --git a/tests/libqos/qgraph_extra.h b/tests/libqos/qgraph_extra.h
> new file mode 100644
> index 0000000000..22850c0368
> --- /dev/null
> +++ b/tests/libqos/qgraph_extra.h
> @@ -0,0 +1,155 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#ifndef QGRAPH_EXTRA_H
> +#define QGRAPH_EXTRA_H
> +
> +#include "qgraph.h"
> +#define PRINT_DEBUG 0
I would expect the #ifdef to be in the C file that uses it. PRINT_DEBUG
is too generic a name for a .h file, it may cause namespace collisions.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-11 14:28 ` Stefan Hajnoczi
@ 2018-07-11 14:58 ` Paolo Bonzini
2018-07-18 14:23 ` Stefan Hajnoczi
0 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-11 14:58 UTC (permalink / raw)
To: Stefan Hajnoczi, Emanuele Giuseppe Esposito
Cc: Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 11/07/2018 16:28, Stefan Hajnoczi wrote:
>> + * build_driver_cmd_line(): builds the command line for the driver
>> + * @node. The node name must be a valid qemu identifier, since it
>> + * will be used to build the command line.
>> + *
>> + * It is also possible to pass an optional @args that will be
>> + * concatenated to the command line.
>> + *
>> + * For drivers, prepend -device to the driver name.
>> + */
>> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)
> Why is this called "driver" instead of "device"?
>
It's the command line that is needed for the driver to work; it can
include also e.g. a -netdev or -blockdev option, though the most common
case is to have just -device.
>> + *
>> + * QOSGraphObject also provides a destructor, used to deallocate the
>> + * after the test has been executed.
>> + */
>> +struct QOSGraphObject {
>> + /* for produces, returns void * */
>> + QOSGetDriver get_driver;
>
> Unused?
>
>> + /* for contains, returns a QOSGraphObject * */
>> + QOSGetDevice get_device;
>
> Unused?
What is unused?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-11 14:58 ` Paolo Bonzini
@ 2018-07-18 14:23 ` Stefan Hajnoczi
2018-07-18 18:05 ` Emanuele
2018-07-18 19:28 ` Paolo Bonzini
0 siblings, 2 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-18 14:23 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Emanuele Giuseppe Esposito, Laurent Vivier,
Philippe Mathieu-Daudé,
qemu-devel
[-- Attachment #1: Type: text/plain, Size: 778 bytes --]
On Wed, Jul 11, 2018 at 04:58:41PM +0200, Paolo Bonzini wrote:
> On 11/07/2018 16:28, Stefan Hajnoczi wrote:
> >> + *
> >> + * QOSGraphObject also provides a destructor, used to deallocate the
> >> + * after the test has been executed.
> >> + */
> >> +struct QOSGraphObject {
> >> + /* for produces, returns void * */
> >> + QOSGetDriver get_driver;
> >
> > Unused?
> >
> >> + /* for contains, returns a QOSGraphObject * */
> >> + QOSGetDevice get_device;
> >
> > Unused?
>
> What is unused?
Neither of these fields are used in this patch. Please introduce them
in the first patch that actually uses them. This way code review can
proceed linearly and it also prevents deadcode when just part of a patch
series is merged or backported.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-18 14:23 ` Stefan Hajnoczi
@ 2018-07-18 18:05 ` Emanuele
2018-07-18 19:28 ` Paolo Bonzini
1 sibling, 0 replies; 39+ messages in thread
From: Emanuele @ 2018-07-18 18:05 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/18/2018 04:23 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 11, 2018 at 04:58:41PM +0200, Paolo Bonzini wrote:
>> On 11/07/2018 16:28, Stefan Hajnoczi wrote:
>>>> + *
>>>> + * QOSGraphObject also provides a destructor, used to deallocate the
>>>> + * after the test has been executed.
>>>> + */
>>>> +struct QOSGraphObject {
>>>> + /* for produces, returns void * */
>>>> + QOSGetDriver get_driver;
>>> Unused?
>>>
>>>> + /* for contains, returns a QOSGraphObject * */
>>>> + QOSGetDevice get_device;
>>> Unused?
>> What is unused?
> Neither of these fields are used in this patch. Please introduce them
> in the first patch that actually uses them. This way code review can
> proceed linearly and it also prevents deadcode when just part of a patch
> series is merged or backported.
I'm sorry but at this point also sdhci.c, raspi2-machine.c and
x86_64_pc-machine.c (patch 3-4-5) are not actually even compiled before
qos-test.c (patch 6). Is that wrong too?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-18 14:23 ` Stefan Hajnoczi
2018-07-18 18:05 ` Emanuele
@ 2018-07-18 19:28 ` Paolo Bonzini
2018-07-18 21:13 ` Emanuele
1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-18 19:28 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Laurent Vivier, Emanuele Giuseppe Esposito,
Philippe Mathieu-Daudé,
qemu-devel
On 18/07/2018 16:23, Stefan Hajnoczi wrote:
>>>> +struct QOSGraphObject {
>>>> + /* for produces, returns void * */
>>>> + QOSGetDriver get_driver;
>>> Unused?
>>>
>>>> + /* for contains, returns a QOSGraphObject * */
>>>> + QOSGetDevice get_device;
>>> Unused?
>> What is unused?
> Neither of these fields are used in this patch. Please introduce them
> in the first patch that actually uses them. This way code review can
> proceed linearly and it also prevents deadcode when just part of a patch
> series is merged or backported.
So do you suggest to squash patch 6 into this one, so that a user of
QOSGraphObject exists already here?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-18 19:28 ` Paolo Bonzini
@ 2018-07-18 21:13 ` Emanuele
2018-07-27 12:54 ` Stefan Hajnoczi
0 siblings, 1 reply; 39+ messages in thread
From: Emanuele @ 2018-07-18 21:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/18/2018 09:28 PM, Paolo Bonzini wrote:
> On 18/07/2018 16:23, Stefan Hajnoczi wrote:
>>>>> +struct QOSGraphObject {
>>>>> + /* for produces, returns void * */
>>>>> + QOSGetDriver get_driver;
>>>> Unused?
>>>>
>>>>> + /* for contains, returns a QOSGraphObject * */
>>>>> + QOSGetDevice get_device;
>>>> Unused?
>>> What is unused?
>> Neither of these fields are used in this patch. Please introduce them
>> in the first patch that actually uses them. This way code review can
>> proceed linearly and it also prevents deadcode when just part of a patch
>> series is merged or backported.
> So do you suggest to squash patch 6 into this one, so that a user of
> QOSGraphObject exists already here?
I think he's right, it makes no sense to have qos-graph as patch 6, it
could go as patch 2, even though by that time no object/node exist yet.
Moreover, right now no file in patch 3-4-5 is actually compiled until
patch 6, which is easily prone to errors in case of future edits (I too
have this problem right now, when I need to modify sdhci.c and I'm
forced to return to the branch head to compile), so adding it before
would make things more clear.
> Thanks,
>
> Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework
2018-07-18 21:13 ` Emanuele
@ 2018-07-27 12:54 ` Stefan Hajnoczi
0 siblings, 0 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-27 12:54 UTC (permalink / raw)
To: Emanuele
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]
On Wed, Jul 18, 2018 at 11:13:18PM +0200, Emanuele wrote:
>
>
> On 07/18/2018 09:28 PM, Paolo Bonzini wrote:
> > On 18/07/2018 16:23, Stefan Hajnoczi wrote:
> > > > > > +struct QOSGraphObject {
> > > > > > + /* for produces, returns void * */
> > > > > > + QOSGetDriver get_driver;
> > > > > Unused?
> > > > >
> > > > > > + /* for contains, returns a QOSGraphObject * */
> > > > > > + QOSGetDevice get_device;
> > > > > Unused?
> > > > What is unused?
> > > Neither of these fields are used in this patch. Please introduce them
> > > in the first patch that actually uses them. This way code review can
> > > proceed linearly and it also prevents deadcode when just part of a patch
> > > series is merged or backported.
> > So do you suggest to squash patch 6 into this one, so that a user of
> > QOSGraphObject exists already here?
> I think he's right, it makes no sense to have qos-graph as patch 6, it could
> go as patch 2, even though by that time no object/node exist yet.
>
> Moreover, right now no file in patch 3-4-5 is actually compiled until patch
> 6, which is easily prone to errors in case of future edits (I too have this
> problem right now, when I need to modify sdhci.c and I'm forced to return to
> the branch head to compile), so adding it before would make things more
> clear.
Sounds good.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
2018-07-09 9:11 ` [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest " Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-11 14:49 ` Stefan Hajnoczi
2018-07-11 20:05 ` Philippe Mathieu-Daudé
2018-07-09 9:11 ` [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci " Emanuele Giuseppe Esposito
` (6 subsequent siblings)
8 siblings, 2 replies; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Add pci-bus-pc node and pci-bus interface, moved QPCIBusPC struct
declaration in its header (since it will be needed by other drivers)
and introduced a setter method for drivers that do not need to allocate
but have to initialize QPCIBusPC.
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
tests/libqos/pci-pc.c | 53 ++++++++++++++++++++++++++++++++++++-------
tests/libqos/pci-pc.h | 8 +++++++
tests/libqos/pci.c | 8 +++++++
3 files changed, 61 insertions(+), 8 deletions(-)
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index a7803308b7..f1c1741279 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -18,15 +18,9 @@
#include "qemu-common.h"
-
#define ACPI_PCIHP_ADDR 0xae00
#define PCI_EJ_BASE 0x0008
-typedef struct QPCIBusPC
-{
- QPCIBus bus;
-} QPCIBusPC;
-
static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr)
{
return inb(addr);
@@ -115,10 +109,33 @@ static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint3
outl(0xcfc, value);
}
-QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+static void *qpci_get_driver(void *obj, const char *interface)
{
- QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+ QPCIBusPC *qpci = obj;
+ if (!g_strcmp0(interface, "pci-bus")) {
+ return &qpci->bus;
+ }
+ printf("%s not present in pci-bus-pc", interface);
+ abort();
+}
+void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
+{
+ if (!bus) {
+ return;
+ }
+ dev->bus = bus;
+ dev->devfn = devfn;
+
+ if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
+ printf("PCI Device not found\n");
+ abort();
+ }
+ qpci_device_enable(dev);
+}
+
+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
+{
assert(qts);
ret->bus.pio_readb = qpci_pc_pio_readb;
@@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
ret->bus.mmio_alloc_ptr = 0xE0000000;
ret->bus.mmio_limit = 0x100000000ULL;
+ ret->obj.get_driver = qpci_get_driver;
+}
+
+QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+{
+ QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+ qpci_set_pc(ret, qts, alloc);
+
return &ret->bus;
}
void qpci_free_pc(QPCIBus *bus)
{
+ if (!bus) {
+ return;
+ }
+
QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
g_free(s);
@@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
qmp_eventwait("DEVICE_DELETED");
}
+
+static void qpci_pc(void)
+{
+ qos_node_create_driver("pci-bus-pc", NULL);
+ qos_node_produces("pci-bus-pc", "pci-bus");
+}
+
+libqos_init(qpci_pc);
diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
index 491eeac756..ee381c5667 100644
--- a/tests/libqos/pci-pc.h
+++ b/tests/libqos/pci-pc.h
@@ -15,7 +15,15 @@
#include "libqos/pci.h"
#include "libqos/malloc.h"
+#include "qgraph.h"
+typedef struct QPCIBusPC {
+ QOSGraphObject obj;
+ QPCIBus bus;
+} QPCIBusPC;
+
+void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
void qpci_free_pc(QPCIBus *bus);
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 0b73cb23d0..c51c186867 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -15,6 +15,7 @@
#include "hw/pci/pci_regs.h"
#include "qemu/host-utils.h"
+#include "qgraph.h"
void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
void (*func)(QPCIDevice *dev, int devfn, void *data),
@@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
opts ? ", " : "", opts ? opts : "");
}
+
+static void qpci(void)
+{
+ qos_node_create_interface("pci-bus");
+}
+
+libqos_init(qpci);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-09 9:11 ` [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes Emanuele Giuseppe Esposito
@ 2018-07-11 14:49 ` Stefan Hajnoczi
2018-07-11 15:18 ` Paolo Bonzini
2018-07-11 17:46 ` Emanuele
2018-07-11 20:05 ` Philippe Mathieu-Daudé
1 sibling, 2 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 14:49 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4821 bytes --]
On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
> {
> - QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> + QPCIBusPC *qpci = obj;
> + if (!g_strcmp0(interface, "pci-bus")) {
> + return &qpci->bus;
> + }
> + printf("%s not present in pci-bus-pc", interface);
> + abort();
> +}
At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance. qgraph duplicates part of
the object model.
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> +{
> + if (!bus) {
> + return;
> + }
When does this happen and why?
> + dev->bus = bus;
> + dev->devfn = devfn;
> +
> + if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
> + printf("PCI Device not found\n");
> + abort();
> + }
> + qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC. How about inlining this in
qpci_init_pc() instead of keeping a separate function?
> +{
> assert(qts);
>
> ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> ret->bus.mmio_alloc_ptr = 0xE0000000;
> ret->bus.mmio_limit = 0x100000000ULL;
>
> + ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> + QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> + qpci_set_pc(ret, qts, alloc);
> +
> return &ret->bus;
> }
>
> void qpci_free_pc(QPCIBus *bus)
> {
> + if (!bus) {
> + return;
> + }
Why is this needed now?
> +
> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>
> g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>
> qmp_eventwait("DEVICE_DELETED");
> }
> +
> +static void qpci_pc(void)
> +{
> + qos_node_create_driver("pci-bus-pc", NULL);
> + qos_node_produces("pci-bus-pc", "pci-bus");
In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.
Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.
I'll think about this more as I read the rest of the patches.
> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>
> #include "libqos/pci.h"
> #include "libqos/malloc.h"
> +#include "qgraph.h"
>
> +typedef struct QPCIBusPC {
> + QOSGraphObject obj;
> + QPCIBus bus;
> +} QPCIBusPC;
Why does this need to be public?
> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
Why does this need to be public?
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
Why does this need to be public?
> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
> void qpci_free_pc(QPCIBus *bus);
>
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>
> #include "hw/pci/pci_regs.h"
> #include "qemu/host-utils.h"
> +#include "qgraph.h"
>
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
> qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
> opts ? ", " : "", opts ? opts : "");
> }
> +
> +static void qpci(void)
> +{
> + qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);
Why does an interface need to be created? The drivers declare which
interfaces they support?
I don't think this can be used to detect typoes in the driver's
qos_node_produces() call since there is no explicit control over the
order in which libqos_init() functions are called. So the driver may
call qos_node_produces() before the qos_node_create_interface() is
called?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-11 14:49 ` Stefan Hajnoczi
@ 2018-07-11 15:18 ` Paolo Bonzini
2018-07-18 14:29 ` Stefan Hajnoczi
2018-07-11 17:46 ` Emanuele
1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-11 15:18 UTC (permalink / raw)
To: Stefan Hajnoczi, Emanuele Giuseppe Esposito
Cc: Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
>> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +static void *qpci_get_driver(void *obj, const char *interface)
>> {
>> - QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> + QPCIBusPC *qpci = obj;
>> + if (!g_strcmp0(interface, "pci-bus")) {
>> + return &qpci->bus;
>> + }
>> + printf("%s not present in pci-bus-pc", interface);
>> + abort();
>> +}
>
> At this point I wonder if it makes sense to use the QEMU Object Model
> (QOM), which has interfaces and inheritance. qgraph duplicates part of
> the object model.
Replying for Emanuele on this point since we didn't really go through
QOM yet; I'll let him answer the comments that are more related to the code.
QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
- QOM properties interact with QAPI and bring in a lot of baggage;
qgraph would only use "child" properties to implement containment.
- QOM objects are long-lived, qgraph objects only last for the duration
of a single test. qgraph doesn't need reference counting or complex
two-phase initialization like "realize" or "user_complete"
- QOM's hierarchy is shallow, but qgraph doesn't really need _any_
hierarchy. Because it focuses on interface rather than hierarchy,
everything can be expressed simply through structs contained into one
another.
Consider that qgraph is 1/4th the size of QOM, and a large part of it is
the graph data structure that (as you said) would not be provided by QOM.
There are two things where using QOM would save a little bit of
duplicated concepts, namely the get_driver (get interface, what you
quote above) and get_device (get contained object) callbacks. However,
it wouldn't simplify the code at all, because it would require the
introduction of separate instance and class structs. We didn't want to
add all too much boilerplate for people that want to write qtest, as you
pointed out in the review of patch 4.
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
>
> It's not clear to me what the purpose of this function is - at least the
> name is a bit cryptic since it seems more like an initialization
> function than 'setting pc' on QPCIBusPC. How about inlining this in
> qpci_init_pc() instead of keeping a separate function?
I agree about the naming. Perhaps we can rename the existing
qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
Would you prefer if the split was done in the patch that introduces the
user for qpci_set_pc (patch 5)? We did it here because this patch
prepares qpci-pc
Thanks,
Paolo
>> +{
>> assert(qts);
>>
>> ret->bus.pio_readb = qpci_pc_pio_readb;
>> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> ret->bus.mmio_alloc_ptr = 0xE0000000;
>> ret->bus.mmio_limit = 0x100000000ULL;
>>
>> + ret->obj.get_driver = qpci_get_driver;
>> +}
>> +
>> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +{
>> + QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> + qpci_set_pc(ret, qts, alloc);
>> +
>> return &ret->bus;
>> }
>>
>> void qpci_free_pc(QPCIBus *bus)
>> {
>> + if (!bus) {
>> + return;
>> + }
>
> Why is this needed now?
>
>> +
>> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>>
>> g_free(s);
>> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>>
>> qmp_eventwait("DEVICE_DELETED");
>> }
>> +
>> +static void qpci_pc(void)
>> +{
>> + qos_node_create_driver("pci-bus-pc", NULL);
>> + qos_node_produces("pci-bus-pc", "pci-bus");
>
> In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
> a driver perspective it seems QOM can already do what is needed and the
> qgraph infrastructure isn't necessary.
>
> Obviously the depth-first search *is* unique and not in QOM, although
> QOM does offer a tree namespace which can be used for looking up object
> instances and I guess this could be used to configure tests at runtime.
>
> I'll think about this more as I read the rest of the patches.
>
>> +}
>> +
>> +libqos_init(qpci_pc);
>> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
>> index 491eeac756..ee381c5667 100644
>> --- a/tests/libqos/pci-pc.h
>> +++ b/tests/libqos/pci-pc.h
>> @@ -15,7 +15,15 @@
>>
>> #include "libqos/pci.h"
>> #include "libqos/malloc.h"
>> +#include "qgraph.h"
>>
>> +typedef struct QPCIBusPC {
>> + QOSGraphObject obj;
>> + QPCIBus bus;
>> +} QPCIBusPC;
>
> Why does this need to be public?
>
>> +
>> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
>
> Why does this need to be public?
>
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
>
> Why does this need to be public?
>
>> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>> void qpci_free_pc(QPCIBus *bus);
>>
>> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
>> index 0b73cb23d0..c51c186867 100644
>> --- a/tests/libqos/pci.c
>> +++ b/tests/libqos/pci.c
>> @@ -15,6 +15,7 @@
>>
>> #include "hw/pci/pci_regs.h"
>> #include "qemu/host-utils.h"
>> +#include "qgraph.h"
>>
>> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>> void (*func)(QPCIDevice *dev, int devfn, void *data),
>> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
>> qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
>> opts ? ", " : "", opts ? opts : "");
>> }
>> +
>> +static void qpci(void)
>> +{
>> + qos_node_create_interface("pci-bus");
>> +}
>> +
>> +libqos_init(qpci);
>
> Why does an interface need to be created? The drivers declare which
> interfaces they support?
>
> I don't think this can be used to detect typoes in the driver's
> qos_node_produces() call since there is no explicit control over the
> order in which libqos_init() functions are called. So the driver may
> call qos_node_produces() before the qos_node_create_interface() is
> called?
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-11 15:18 ` Paolo Bonzini
@ 2018-07-18 14:29 ` Stefan Hajnoczi
2018-07-18 18:29 ` Emanuele
0 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-18 14:29 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Emanuele Giuseppe Esposito, Laurent Vivier,
Philippe Mathieu-Daudé,
qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]
On Wed, Jul 11, 2018 at 05:18:03PM +0200, Paolo Bonzini wrote:
> On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> > On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> >> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> >> +static void *qpci_get_driver(void *obj, const char *interface)
> >> {
> >> - QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> >> + QPCIBusPC *qpci = obj;
> >> + if (!g_strcmp0(interface, "pci-bus")) {
> >> + return &qpci->bus;
> >> + }
> >> + printf("%s not present in pci-bus-pc", interface);
> >> + abort();
> >> +}
> >
> > At this point I wonder if it makes sense to use the QEMU Object Model
> > (QOM), which has interfaces and inheritance. qgraph duplicates part of
> > the object model.
>
> Replying for Emanuele on this point since we didn't really go through
> QOM yet; I'll let him answer the comments that are more related to the code.
>
> QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
>
> - QOM properties interact with QAPI and bring in a lot of baggage;
> qgraph would only use "child" properties to implement containment.
>
> - QOM objects are long-lived, qgraph objects only last for the duration
> of a single test. qgraph doesn't need reference counting or complex
> two-phase initialization like "realize" or "user_complete"
>
> - QOM's hierarchy is shallow, but qgraph doesn't really need _any_
> hierarchy. Because it focuses on interface rather than hierarchy,
> everything can be expressed simply through structs contained into one
> another.
>
> Consider that qgraph is 1/4th the size of QOM, and a large part of it is
> the graph data structure that (as you said) would not be provided by QOM.
>
> There are two things where using QOM would save a little bit of
> duplicated concepts, namely the get_driver (get interface, what you
> quote above) and get_device (get contained object) callbacks. However,
> it wouldn't simplify the code at all, because it would require the
> introduction of separate instance and class structs. We didn't want to
> add all too much boilerplate for people that want to write qtest, as you
> pointed out in the review of patch 4.
Yes, I think these are good points. QOM does involve a lot of
boilerplate for defining objects.
> >> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> >
> > It's not clear to me what the purpose of this function is - at least the
> > name is a bit cryptic since it seems more like an initialization
> > function than 'setting pc' on QPCIBusPC. How about inlining this in
> > qpci_init_pc() instead of keeping a separate function?
>
> I agree about the naming. Perhaps we can rename the existing
> qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
>
> Would you prefer if the split was done in the patch that introduces the
> user for qpci_set_pc (patch 5)? We did it here because this patch
> prepares qpci-pc
Either way is fine. I suggested inlining mainly because it avoids the
need to pick a good name :).
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-18 14:29 ` Stefan Hajnoczi
@ 2018-07-18 18:29 ` Emanuele
2018-07-18 19:33 ` Paolo Bonzini
0 siblings, 1 reply; 39+ messages in thread
From: Emanuele @ 2018-07-18 18:29 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/18/2018 04:29 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 11, 2018 at 05:18:03PM +0200, Paolo Bonzini wrote:
>> On 11/07/2018 16:49, Stefan Hajnoczi wrote:
>>> On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
>>>> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>>>> +static void *qpci_get_driver(void *obj, const char *interface)
>>>> {
>>>> - QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>>>> + QPCIBusPC *qpci = obj;
>>>> + if (!g_strcmp0(interface, "pci-bus")) {
>>>> + return &qpci->bus;
>>>> + }
>>>> + printf("%s not present in pci-bus-pc", interface);
>>>> + abort();
>>>> +}
>>> At this point I wonder if it makes sense to use the QEMU Object Model
>>> (QOM), which has interfaces and inheritance. qgraph duplicates part of
>>> the object model.
>> Replying for Emanuele on this point since we didn't really go through
>> QOM yet; I'll let him answer the comments that are more related to the code.
>>
>> QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
>>
>> - QOM properties interact with QAPI and bring in a lot of baggage;
>> qgraph would only use "child" properties to implement containment.
>>
>> - QOM objects are long-lived, qgraph objects only last for the duration
>> of a single test. qgraph doesn't need reference counting or complex
>> two-phase initialization like "realize" or "user_complete"
>>
>> - QOM's hierarchy is shallow, but qgraph doesn't really need _any_
>> hierarchy. Because it focuses on interface rather than hierarchy,
>> everything can be expressed simply through structs contained into one
>> another.
>>
>> Consider that qgraph is 1/4th the size of QOM, and a large part of it is
>> the graph data structure that (as you said) would not be provided by QOM.
>>
>> There are two things where using QOM would save a little bit of
>> duplicated concepts, namely the get_driver (get interface, what you
>> quote above) and get_device (get contained object) callbacks. However,
>> it wouldn't simplify the code at all, because it would require the
>> introduction of separate instance and class structs. We didn't want to
>> add all too much boilerplate for people that want to write qtest, as you
>> pointed out in the review of patch 4.
> Yes, I think these are good points. QOM does involve a lot of
> boilerplate for defining objects.
>
>>>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
>>> It's not clear to me what the purpose of this function is - at least the
>>> name is a bit cryptic since it seems more like an initialization
>>> function than 'setting pc' on QPCIBusPC. How about inlining this in
>>> qpci_init_pc() instead of keeping a separate function?
>> I agree about the naming. Perhaps we can rename the existing
>> qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
>>
>> Would you prefer if the split was done in the patch that introduces the
>> user for qpci_set_pc (patch 5)? We did it here because this patch
>> prepares qpci-pc
> Either way is fine. I suggested inlining mainly because it avoids the
> need to pick a good name :).
I had to put this patch here because it also introduces
qpci_device_init, used by sdhci (patch 3).
For the next version I plan to have a patch X where I rename all
occurrences of qpci_init_pc in qpci_pc_new, and a patch X+1 that
introduces qpci_init_pc (was qpci_set_pc) and the other changes.
Should I only introduce qpci_device_init in patch 3 and the remaining
things in patch 5?
I think the general problem here is that in some patches I create
functions that are planned to only be used only in next patches (of the
current series).
> Stefan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-18 18:29 ` Emanuele
@ 2018-07-18 19:33 ` Paolo Bonzini
2018-07-18 20:49 ` Emanuele
0 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-18 19:33 UTC (permalink / raw)
To: Emanuele, Stefan Hajnoczi
Cc: Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 18/07/2018 20:29, Emanuele wrote:
> I had to put this patch here because it also introduces
> qpci_device_init, used by sdhci (patch 3).
>
> For the next version I plan to have a patch X where I rename all
> occurrences of qpci_init_pc in qpci_pc_new, and a patch X+1 that
> introduces qpci_init_pc (was qpci_set_pc) and the other changes.
>
> Should I only introduce qpci_device_init in patch 3 and the remaining
> things in patch 5?
>
> I think the general problem here is that in some patches I create
> functions that are planned to only be used only in next patches (of the
> current series).
I think it's okay this way, however you should justify the changes you
make to "qgraph-ify" each component.
For patch 1, let's wait for Stefan's reply. Because patch 1 is
introducing the infrastructure, I think it is acceptable that some
definitions are introduced early as long as they have doc comments; it
would make little sense to introduce get_device in patch 4 just because
there are no "contains" edges until then.
However, introducing the qos-test directly at the beginning is also a
possibility.
In either case, we need better doc comments for the function pointers in
QOSGraphObject.
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-18 19:33 ` Paolo Bonzini
@ 2018-07-18 20:49 ` Emanuele
0 siblings, 0 replies; 39+ messages in thread
From: Emanuele @ 2018-07-18 20:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/18/2018 09:33 PM, Paolo Bonzini wrote:
> On 18/07/2018 20:29, Emanuele wrote:
>> I had to put this patch here because it also introduces
>> qpci_device_init, used by sdhci (patch 3).
>>
>> For the next version I plan to have a patch X where I rename all
>> occurrences of qpci_init_pc in qpci_pc_new, and a patch X+1 that
>> introduces qpci_init_pc (was qpci_set_pc) and the other changes.
>>
>> Should I only introduce qpci_device_init in patch 3 and the remaining
>> things in patch 5?
>>
>> I think the general problem here is that in some patches I create
>> functions that are planned to only be used only in next patches (of the
>> current series).
> I think it's okay this way, however you should justify the changes you
> make to "qgraph-ify" each component.
>
> For patch 1, let's wait for Stefan's reply. Because patch 1 is
> introducing the infrastructure, I think it is acceptable that some
> definitions are introduced early as long as they have doc comments; it
> would make little sense to introduce get_device in patch 4 just because
> there are no "contains" edges until then.
>
> However, introducing the qos-test directly at the beginning is also a
> possibility.
>
> In either case, we need better doc comments for the function pointers in
> QOSGraphObject.
What would you suggest as better doc comments? For version 2 I have
written a little introduction like in qom/object.h, essentially pasting
the cover letter and a working example.
> Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-11 14:49 ` Stefan Hajnoczi
2018-07-11 15:18 ` Paolo Bonzini
@ 2018-07-11 17:46 ` Emanuele
2018-07-18 15:02 ` Stefan Hajnoczi
2018-07-18 19:38 ` Paolo Bonzini
1 sibling, 2 replies; 39+ messages in thread
From: Emanuele @ 2018-07-11 17:46 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/11/2018 04:49 PM, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
>> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +static void *qpci_get_driver(void *obj, const char *interface)
>> {
>> - QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> + QPCIBusPC *qpci = obj;
>> + if (!g_strcmp0(interface, "pci-bus")) {
>> + return &qpci->bus;
>> + }
>> + printf("%s not present in pci-bus-pc", interface);
>> + abort();
>> +}
> At this point I wonder if it makes sense to use the QEMU Object Model
> (QOM), which has interfaces and inheritance. qgraph duplicates part of
> the object model.
>
>> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
>> +{
>> + if (!bus) {
>> + return;
>> + }
> When does this happen and why?
Ok maybe this is unneeded, I added it because I was creating a test with
a NULL bus, but we ended up putting it aside. So you're right, this
should be eliminated.
>> + dev->bus = bus;
>> + dev->devfn = devfn;
>> +
>> + if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
>> + printf("PCI Device not found\n");
>> + abort();
>> + }
>> + qpci_device_enable(dev);
>> +}
>> +
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> It's not clear to me what the purpose of this function is - at least the
> name is a bit cryptic since it seems more like an initialization
> function than 'setting pc' on QPCIBusPC. How about inlining this in
> qpci_init_pc() instead of keeping a separate function?
The idea is that, following the graph that Paolo wrote in the GSoC
project wiki (https://wiki.qemu.org/Features/qtest_driver_framework),
QPCIBusPC is "contained" in i440FX-pcihost. This means that the
i440FX-pcihost struct has a QPCIBusPC field.
Therefore I had to put QPCIBusPC as public (in order to have it as
field), even though qpci_init_pc() was not what I needed to initialize
it, because that function is allocating a new QPCIBusPC, while I just
needed to "set" its values.
From here, qpci_set_pc().
>> +{
>> assert(qts);
>>
>> ret->bus.pio_readb = qpci_pc_pio_readb;
>> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> ret->bus.mmio_alloc_ptr = 0xE0000000;
>> ret->bus.mmio_limit = 0x100000000ULL;
>>
>> + ret->obj.get_driver = qpci_get_driver;
>> +}
>> +
>> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +{
>> + QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> + qpci_set_pc(ret, qts, alloc);
>> +
>> return &ret->bus;
>> }
>>
>> void qpci_free_pc(QPCIBus *bus)
>> {
>> + if (!bus) {
>> + return;
>> + }
> Why is this needed now?
Not having this would mean failure of tests like vrtio-user-test,
because now QPCIBusPC has two fields, and QPCIBus bus; is not the first
one either.
Therefore, if I remember correctly doing something like
container_of(NULL, QPCIBusPC, bus)
where bus is not the first field of QPCIBusPC would result in a negative
number, and freeing that would make the test crash.
I discovered this issue while doing some checks, and already proposed a
patch for it, even though we ended up agreeing that this fix was only
needed in my patch and not in the current QEMU implementation.
>> +
>> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>>
>> g_free(s);
>> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>>
>> qmp_eventwait("DEVICE_DELETED");
>> }
>> +
>> +static void qpci_pc(void)
>> +{
>> + qos_node_create_driver("pci-bus-pc", NULL);
>> + qos_node_produces("pci-bus-pc", "pci-bus");
> In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
> a driver perspective it seems QOM can already do what is needed and the
> qgraph infrastructure isn't necessary.
>
> Obviously the depth-first search *is* unique and not in QOM, although
> QOM does offer a tree namespace which can be used for looking up object
> instances and I guess this could be used to configure tests at runtime.
>
> I'll think about this more as I read the rest of the patches.
>
>> +}
>> +
>> +libqos_init(qpci_pc);
>> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
>> index 491eeac756..ee381c5667 100644
>> --- a/tests/libqos/pci-pc.h
>> +++ b/tests/libqos/pci-pc.h
>> @@ -15,7 +15,15 @@
>>
>> #include "libqos/pci.h"
>> #include "libqos/malloc.h"
>> +#include "qgraph.h"
>>
>> +typedef struct QPCIBusPC {
>> + QOSGraphObject obj;
>> + QPCIBus bus;
>> +} QPCIBusPC;
> Why does this need to be public?
See previous answer
>
>> +
>> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
> Why does this need to be public?
I'll use that in the next patches. I also realized this function should
go in pci.h, and not pci-pc.h
>
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> Why does this need to be public?
Because I use it in the next patch to set the QPCIBusPC in the x86_64/pc
machine.
>
>> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>> void qpci_free_pc(QPCIBus *bus);
>>
>> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
>> index 0b73cb23d0..c51c186867 100644
>> --- a/tests/libqos/pci.c
>> +++ b/tests/libqos/pci.c
>> @@ -15,6 +15,7 @@
>>
>> #include "hw/pci/pci_regs.h"
>> #include "qemu/host-utils.h"
>> +#include "qgraph.h"
>>
>> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>> void (*func)(QPCIDevice *dev, int devfn, void *data),
>> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
>> qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
>> opts ? ", " : "", opts ? opts : "");
>> }
>> +
>> +static void qpci(void)
>> +{
>> + qos_node_create_interface("pci-bus");
>> +}
>> +
>> +libqos_init(qpci);
> Why does an interface need to be created? The drivers declare which
> interfaces they support?
>
> I don't think this can be used to detect typoes in the driver's
> qos_node_produces() call since there is no explicit control over the
> order in which libqos_init() functions are called. So the driver may
> call qos_node_produces() before the qos_node_create_interface() is
> called?
The interface is what is actually given to the test, so from there it
can test the functions and fields regardless of which driver is actually
implementing them.
For example, sdhci-test uses the QSDHCI functions, and depending on the
path the generic-sdhci or sdhci-pci functions will be used.
It is possible to call produce() before create(), since an edge just
keeps a reference on the name of the node, and not the actual node. If a
produce relationship includes the name of a node that does not exist,
the application will detect it only during the graph walk, triggering an
abort(). If both nodes of the edge will refer to non-existent nodes, the
edge won't be detected during the graph walk.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-11 17:46 ` Emanuele
@ 2018-07-18 15:02 ` Stefan Hajnoczi
2018-07-18 19:38 ` Paolo Bonzini
1 sibling, 0 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-18 15:02 UTC (permalink / raw)
To: Emanuele
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 8574 bytes --]
On Wed, Jul 11, 2018 at 07:46:09PM +0200, Emanuele wrote:
> On 07/11/2018 04:49 PM, Stefan Hajnoczi wrote:
> > On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> > > +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> > > +{
> > > + if (!bus) {
> > > + return;
> > > + }
> > When does this happen and why?
> Ok maybe this is unneeded, I added it because I was creating a test with a
> NULL bus, but we ended up putting it aside. So you're right, this should be
> eliminated.
Thanks!
> > > + dev->bus = bus;
> > > + dev->devfn = devfn;
> > > +
> > > + if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
> > > + printf("PCI Device not found\n");
> > > + abort();
> > > + }
> > > + qpci_device_enable(dev);
> > > +}
> > > +
> > > +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> > It's not clear to me what the purpose of this function is - at least the
> > name is a bit cryptic since it seems more like an initialization
> > function than 'setting pc' on QPCIBusPC. How about inlining this in
> > qpci_init_pc() instead of keeping a separate function?
> The idea is that, following the graph that Paolo wrote in the GSoC project
> wiki (https://wiki.qemu.org/Features/qtest_driver_framework), QPCIBusPC is
> "contained" in i440FX-pcihost. This means that the i440FX-pcihost struct has
> a QPCIBusPC field.
>
> Therefore I had to put QPCIBusPC as public (in order to have it as field),
> even though qpci_init_pc() was not what I needed to initialize it, because
> that function is allocating a new QPCIBusPC, while I just needed to "set"
> its values.
> From here, qpci_set_pc().
I see. Renaming qpci_set_pc() to qpci_pc_init() and including a doc
comment explaining that this is the in-place initialization function
would make things clear.
> > > +{
> > > assert(qts);
> > > ret->bus.pio_readb = qpci_pc_pio_readb;
> > > @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> > > ret->bus.mmio_alloc_ptr = 0xE0000000;
> > > ret->bus.mmio_limit = 0x100000000ULL;
> > > + ret->obj.get_driver = qpci_get_driver;
> > > +}
> > > +
> > > +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> > > +{
> > > + QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> > > + qpci_set_pc(ret, qts, alloc);
> > > +
> > > return &ret->bus;
> > > }
> > > void qpci_free_pc(QPCIBus *bus)
> > > {
> > > + if (!bus) {
> > > + return;
> > > + }
> > Why is this needed now?
> Not having this would mean failure of tests like vrtio-user-test, because
> now QPCIBusPC has two fields, and QPCIBus bus; is not the first one either.
> Therefore, if I remember correctly doing something like
> container_of(NULL, QPCIBusPC, bus)
> where bus is not the first field of QPCIBusPC would result in a negative
> number, and freeing that would make the test crash.
>
> I discovered this issue while doing some checks, and already proposed a
> patch for it, even though we ended up agreeing that this fix was only needed
> in my patch and not in the current QEMU implementation.
I see now:
void free_ahci_device(QPCIDevice *dev)
{
QPCIBus *pcibus = dev ? dev->bus : NULL;
/* libqos doesn't have a function for this, so free it manually */
g_free(dev);
qpci_free_pc(pcibus);
}
The caller is assuming it's okay to pass NULL.
This is a good candidate for a separate patch so that you can explain
the rationale ("Although containerof(NULL, QPCIBusPC, bus) happens to
evaluate to NULL today thanks to the QPCIBusPC struct layout, it's
generally bad practice to rely on that. Normally containerof() is used
precisely because an offset into the struct is required and a
straightforward cast would not work. We got lucky, but don't depend on
it anymore.").
> > > +
> > > QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
> > > g_free(s);
> > > @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> > > qmp_eventwait("DEVICE_DELETED");
> > > }
> > > +
> > > +static void qpci_pc(void)
> > > +{
> > > + qos_node_create_driver("pci-bus-pc", NULL);
> > > + qos_node_produces("pci-bus-pc", "pci-bus");
> > In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
> > a driver perspective it seems QOM can already do what is needed and the
> > qgraph infrastructure isn't necessary.
> >
> > Obviously the depth-first search *is* unique and not in QOM, although
> > QOM does offer a tree namespace which can be used for looking up object
> > instances and I guess this could be used to configure tests at runtime.
> >
> > I'll think about this more as I read the rest of the patches.
> >
> > > +}
> > > +
> > > +libqos_init(qpci_pc);
> > > diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> > > index 491eeac756..ee381c5667 100644
> > > --- a/tests/libqos/pci-pc.h
> > > +++ b/tests/libqos/pci-pc.h
> > > @@ -15,7 +15,15 @@
> > > #include "libqos/pci.h"
> > > #include "libqos/malloc.h"
> > > +#include "qgraph.h"
> > > +typedef struct QPCIBusPC {
> > > + QOSGraphObject obj;
> > > + QPCIBus bus;
> > > +} QPCIBusPC;
> > Why does this need to be public?
> See previous answer
> >
> > > +
> > > +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
> > Why does this need to be public?
> I'll use that in the next patches. I also realized this function should go
> in pci.h, and not pci-pc.h
> >
> > > +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> > Why does this need to be public?
> Because I use it in the next patch to set the QPCIBusPC in the x86_64/pc
> machine.
These changes aren't self-contained, making it hard to review the patch
series. I review patch series in order, so anything that anticipates
future changes without an explanation in the comments or commit
description causes me to ask questions.
Patches are easiest to review when there are no forward references.
This can be achieved by only introducing things when they are needed.
> >
> > > QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
> > > void qpci_free_pc(QPCIBus *bus);
> > > diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> > > index 0b73cb23d0..c51c186867 100644
> > > --- a/tests/libqos/pci.c
> > > +++ b/tests/libqos/pci.c
> > > @@ -15,6 +15,7 @@
> > > #include "hw/pci/pci_regs.h"
> > > #include "qemu/host-utils.h"
> > > +#include "qgraph.h"
> > > void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> > > void (*func)(QPCIDevice *dev, int devfn, void *data),
> > > @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
> > > qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
> > > opts ? ", " : "", opts ? opts : "");
> > > }
> > > +
> > > +static void qpci(void)
> > > +{
> > > + qos_node_create_interface("pci-bus");
> > > +}
> > > +
> > > +libqos_init(qpci);
> > Why does an interface need to be created? The drivers declare which
> > interfaces they support?
> >
> > I don't think this can be used to detect typoes in the driver's
> > qos_node_produces() call since there is no explicit control over the
> > order in which libqos_init() functions are called. So the driver may
> > call qos_node_produces() before the qos_node_create_interface() is
> > called?
> The interface is what is actually given to the test, so from there it can
> test the functions and fields regardless of which driver is actually
> implementing them.
> For example, sdhci-test uses the QSDHCI functions, and depending on the path
> the generic-sdhci or sdhci-pci functions will be used.
>
> It is possible to call produce() before create(), since an edge just keeps a
> reference on the name of the node, and not the actual node. If a produce
> relationship includes the name of a node that does not exist, the
> application will detect it only during the graph walk, triggering an
> abort(). If both nodes of the edge will refer to non-existent nodes, the
> edge won't be detected during the graph walk.
Drivers declare which interface they implement. Tests request an
interface. That should be enough information to connect the two
together. Why is qos_node_create_interface() necessary?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-11 17:46 ` Emanuele
2018-07-18 15:02 ` Stefan Hajnoczi
@ 2018-07-18 19:38 ` Paolo Bonzini
1 sibling, 0 replies; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-18 19:38 UTC (permalink / raw)
To: Emanuele, Stefan Hajnoczi
Cc: Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 11/07/2018 19:46, Emanuele wrote:
>>> +static void qpci(void)
>>> +{
>>> + qos_node_create_interface("pci-bus");
>>> +}
>>> +
>>> +libqos_init(qpci);
>> Why does an interface need to be created? The drivers declare which
>> interfaces they support?
>>
>> I don't think this can be used to detect typoes in the driver's
>> qos_node_produces() call since there is no explicit control over the
>> order in which libqos_init() functions are called. So the driver may
>> call qos_node_produces() before the qos_node_create_interface() is
>> called?
> The interface is what is actually given to the test, so from there it
> can test the functions and fields regardless of which driver is actually
> implementing them.
> For example, sdhci-test uses the QSDHCI functions, and depending on the
> path the generic-sdhci or sdhci-pci functions will be used.
I think Stefan is right, if you adjust qos_node_create_interface so that
it is idempotent(*), you can make it static and call it from
qos_node_produces and qos_node_consumes.
(*) that is, fail like now if the node exists and is not an interface;
but, succeed if the node exists and is an interface.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
2018-07-09 9:11 ` [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes Emanuele Giuseppe Esposito
2018-07-11 14:49 ` Stefan Hajnoczi
@ 2018-07-11 20:05 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-07-11 20:05 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito; +Cc: Paolo Bonzini, Laurent Vivier, qemu-devel
Hi Emanuele,
On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:
> Add pci-bus-pc node and pci-bus interface, moved QPCIBusPC struct
"move"
> declaration in its header (since it will be needed by other drivers)
> and introduced a setter method for drivers that do not need to allocate
"introduce"
> but have to initialize QPCIBusPC.
>
> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> ---
> tests/libqos/pci-pc.c | 53 ++++++++++++++++++++++++++++++++++++-------
> tests/libqos/pci-pc.h | 8 +++++++
> tests/libqos/pci.c | 8 +++++++
> 3 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index a7803308b7..f1c1741279 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -18,15 +18,9 @@
>
> #include "qemu-common.h"
>
> -
> #define ACPI_PCIHP_ADDR 0xae00
> #define PCI_EJ_BASE 0x0008
>
> -typedef struct QPCIBusPC
> -{
> - QPCIBus bus;
> -} QPCIBusPC;
> -
> static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr)
> {
> return inb(addr);
> @@ -115,10 +109,33 @@ static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint3
> outl(0xcfc, value);
> }
>
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
> {
> - QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> + QPCIBusPC *qpci = obj;
> + if (!g_strcmp0(interface, "pci-bus")) {
> + return &qpci->bus;
> + }
> + printf("%s not present in pci-bus-pc", interface);
> + abort();
> +}
>
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
Maybe this one belongs to "pci.c" (not being 'pc' related).
> +{
> + if (!bus) {
> + return;
> + }
> + dev->bus = bus;
> + dev->devfn = devfn;
> +
> + if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
> + printf("PCI Device not found\n");
> + abort();
> + }
> + qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> +{
> assert(qts);
>
> ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> ret->bus.mmio_alloc_ptr = 0xE0000000;
> ret->bus.mmio_limit = 0x100000000ULL;
>
> + ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> + QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> + qpci_set_pc(ret, qts, alloc);
> +
> return &ret->bus;
> }
>
> void qpci_free_pc(QPCIBus *bus)
> {
> + if (!bus) {
> + return;
> + }
> +
> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>
> g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>
> qmp_eventwait("DEVICE_DELETED");
> }
> +
> +static void qpci_pc(void)
> +{
> + qos_node_create_driver("pci-bus-pc", NULL);
> + qos_node_produces("pci-bus-pc", "pci-bus");
> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>
> #include "libqos/pci.h"
> #include "libqos/malloc.h"
> +#include "qgraph.h"
>
> +typedef struct QPCIBusPC {
> + QOSGraphObject obj;
> + QPCIBus bus;
> +} QPCIBusPC;
> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
Ditto, belongs to "libqos/pci.h".
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
> void qpci_free_pc(QPCIBus *bus);
>
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>
> #include "hw/pci/pci_regs.h"
> #include "qemu/host-utils.h"
> +#include "qgraph.h"
>
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const char *id,
> qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
> opts ? ", " : "", opts ? opts : "");
> }
> +
> +static void qpci(void)
> +{
> + qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci driver and interface nodes
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
2018-07-09 9:11 ` [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest " Emanuele Giuseppe Esposito
2018-07-09 9:11 ` [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-11 20:13 ` Philippe Mathieu-Daudé
2018-07-09 9:11 ` [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node Emanuele Giuseppe Esposito
` (5 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Add qgraph nodes for sdhci-pci and generic-sdhci (memory mapped) drivers.
Both drivers implement (produce) the same interface sdhci, that provides the
readw - readq - writeq functions.
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
tests/Makefile.include | 1 +
tests/libqos/sdhci.c | 142 +++++++++++++++++++++++++++++++++++++++++
tests/libqos/sdhci.h | 68 ++++++++++++++++++++
3 files changed, 211 insertions(+)
create mode 100644 tests/libqos/sdhci.c
create mode 100644 tests/libqos/sdhci.h
diff --git a/tests/Makefile.include b/tests/Makefile.include
index b16bbd55df..acbf704a8a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -770,6 +770,7 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o
libqgraph-obj-y = tests/libqos/qgraph.o
+libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
check-unit-y += tests/test-qgraph$(EXESUF)
tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
diff --git a/tests/libqos/sdhci.c b/tests/libqos/sdhci.c
new file mode 100644
index 0000000000..213c2657c3
--- /dev/null
+++ b/tests/libqos/sdhci.c
@@ -0,0 +1,142 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "pci.h"
+#include "pci-pc.h"
+#include "qgraph.h"
+#include "sdhci.h"
+#include "hw/pci/pci.h"
+
+static void set_qsdhci_fields(QSDHCI *s, uint8_t version, uint8_t baseclock,
+ bool sdma, uint64_t reg)
+{
+ s->props.version = version;
+ s->props.baseclock = baseclock;
+ s->props.capab.sdma = sdma;
+ s->props.capab.reg = reg;
+}
+
+/* Memory mapped implementation of QSDHCI */
+
+static uint16_t sdhci_mm_readw(QSDHCI *s, uint32_t reg)
+{
+ QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
+ return qtest_readw(global_qtest, smm->addr + reg);
+}
+
+static uint64_t sdhci_mm_readq(QSDHCI *s, uint32_t reg)
+{
+ QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
+ return qtest_readq(global_qtest, smm->addr + reg);
+}
+
+static void sdhci_mm_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
+{
+ QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
+ qtest_writeq(global_qtest, smm->addr + reg, val);
+}
+
+static void *sdhci_mm_get_driver(void *obj, const char *interface)
+{
+ QSDHCI_MemoryMapped *spci = obj;
+ if (!g_strcmp0(interface, "sdhci")) {
+ return &spci->sdhci;
+ }
+ printf("%s not present in generic-sdhci\n", interface);
+ abort();
+}
+
+void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
+ QSDHCIProperties *common)
+{
+ sdhci->obj.get_driver = sdhci_mm_get_driver;
+ sdhci->sdhci.sdhci_readw = sdhci_mm_readw;
+ sdhci->sdhci.sdhci_readq = sdhci_mm_readq;
+ sdhci->sdhci.sdhci_writeq = sdhci_mm_writeq;
+ memcpy(&sdhci->sdhci.props, common, sizeof(QSDHCIProperties));
+ sdhci->addr = addr;
+}
+
+/* PCI implementation of QSDHCI */
+
+static uint16_t sdhci_pci_readw(QSDHCI *s, uint32_t reg)
+{
+ QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
+ return qpci_io_readw(&spci->dev, spci->mem_bar, reg);
+}
+
+static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
+{
+ QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
+ return qpci_io_readq(&spci->dev, spci->mem_bar, reg);
+}
+
+static void sdhci_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
+{
+ QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
+ return qpci_io_writeq(&spci->dev, spci->mem_bar, reg, val);
+}
+
+static void *sdhci_pci_get_driver(void *object, const char *interface)
+{
+ QSDHCI_PCI *spci = object;
+ if (!g_strcmp0(interface, "sdhci")) {
+ return &spci->sdhci;
+ }
+
+ printf("%s not present in sdhci-pci\n", interface);
+ abort();
+}
+
+static void sdhci_destroy(QOSGraphObject *obj)
+{
+ g_free(obj);
+}
+
+static void *sdhci_pci_create(void *pci_bus, QOSGraphObject *alloc)
+{
+ QSDHCI_PCI *spci = g_new0(QSDHCI_PCI, 1);
+ QPCIBus *bus = pci_bus;
+ uint64_t barsize;
+
+ qpci_device_init(&spci->dev, bus, QPCI_DEVFN(4, 0));
+ if (bus) {
+ spci->mem_bar = qpci_iomap(&spci->dev, 0, &barsize);
+ }
+ spci->obj.get_driver = sdhci_pci_get_driver;
+ spci->obj.destructor = sdhci_destroy;
+ spci->sdhci.sdhci_readw = sdhci_pci_readw;
+ spci->sdhci.sdhci_readq = sdhci_readq;
+ spci->sdhci.sdhci_writeq = sdhci_writeq;
+ set_qsdhci_fields(&spci->sdhci, 2, 0, 1, 0x057834b4);
+ return &spci->obj;
+}
+
+static void qsdhci(void)
+{
+ qos_node_create_interface("sdhci");
+ qos_node_create_driver("generic-sdhci", NULL);
+ qos_node_produces("generic-sdhci", "sdhci");
+ qos_node_create_driver("sdhci-pci", sdhci_pci_create);
+ qos_node_produces("sdhci-pci", "sdhci");
+ qos_node_consumes_arg("sdhci-pci", "pci-bus", "addr=04.0");
+}
+
+libqos_init(qsdhci);
diff --git a/tests/libqos/sdhci.h b/tests/libqos/sdhci.h
new file mode 100644
index 0000000000..dfd043f160
--- /dev/null
+++ b/tests/libqos/sdhci.h
@@ -0,0 +1,68 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#ifndef QGRAPH_QSDHCI
+#define QGRAPH_QSDHCI
+#include "pci.h"
+
+typedef struct QSDHCI QSDHCI;
+typedef struct QSDHCI_MemoryMapped QSDHCI_MemoryMapped;
+typedef struct QSDHCI_PCI QSDHCI_PCI;
+typedef struct QSDHCIProperties QSDHCIProperties;
+
+/* Properties common to all QSDHCI devices */
+struct QSDHCIProperties {
+ uint8_t version;
+ uint8_t baseclock;
+ struct {
+ bool sdma;
+ uint64_t reg;
+ } capab;
+};
+
+struct QSDHCI {
+ QOSGraphObject obj;
+ uint16_t (*sdhci_readw)(QSDHCI *s, uint32_t reg);
+ uint64_t (*sdhci_readq)(QSDHCI *s, uint32_t reg);
+ void (*sdhci_writeq)(QSDHCI *s, uint32_t reg, uint64_t val);
+ QSDHCIProperties props;
+};
+
+/* Memory Mapped implementation of QSDHCI */
+struct QSDHCI_MemoryMapped {
+ QOSGraphObject obj;
+ QSDHCI sdhci;
+ uint64_t addr;
+};
+
+/* PCI implementation of QSDHCI */
+struct QSDHCI_PCI {
+ QOSGraphObject obj;
+ QPCIDevice dev;
+ QSDHCI sdhci;
+ QPCIBar mem_bar;
+};
+
+/**
+ * qos_create_sdhci_mm(): external constructor used by all drivers/machines
+ * that "contain" a #QSDHCI_MemoryMapped driver
+ */
+void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
+ QSDHCIProperties *common);
+
+#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci driver and interface nodes
2018-07-09 9:11 ` [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci " Emanuele Giuseppe Esposito
@ 2018-07-11 20:13 ` Philippe Mathieu-Daudé
2018-07-11 20:44 ` Emanuele
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-07-11 20:13 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito; +Cc: Paolo Bonzini, Laurent Vivier, qemu-devel
Hi Emanuele,
On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:
> Add qgraph nodes for sdhci-pci and generic-sdhci (memory mapped) drivers.
> Both drivers implement (produce) the same interface sdhci, that provides the
> readw - readq - writeq functions.
>
> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> ---
> tests/Makefile.include | 1 +
> tests/libqos/sdhci.c | 142 +++++++++++++++++++++++++++++++++++++++++
> tests/libqos/sdhci.h | 68 ++++++++++++++++++++
> 3 files changed, 211 insertions(+)
> create mode 100644 tests/libqos/sdhci.c
> create mode 100644 tests/libqos/sdhci.h
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index b16bbd55df..acbf704a8a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -770,6 +770,7 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
> libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o
>
> libqgraph-obj-y = tests/libqos/qgraph.o
> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
Shouldn't this be:
libqgraph-obj-y = tests/libqos/qgraph.o tests/libqos/sdhci.o
(not PC related)
>
> check-unit-y += tests/test-qgraph$(EXESUF)
> tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
> diff --git a/tests/libqos/sdhci.c b/tests/libqos/sdhci.c
> new file mode 100644
> index 0000000000..213c2657c3
> --- /dev/null
> +++ b/tests/libqos/sdhci.c
> @@ -0,0 +1,142 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "pci.h"
> +#include "pci-pc.h"
> +#include "qgraph.h"
> +#include "sdhci.h"
> +#include "hw/pci/pci.h"
> +
> +static void set_qsdhci_fields(QSDHCI *s, uint8_t version, uint8_t baseclock,
> + bool sdma, uint64_t reg)
> +{
> + s->props.version = version;
> + s->props.baseclock = baseclock;
> + s->props.capab.sdma = sdma;
> + s->props.capab.reg = reg;
> +}
> +
> +/* Memory mapped implementation of QSDHCI */
> +
> +static uint16_t sdhci_mm_readw(QSDHCI *s, uint32_t reg)
> +{
> + QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
> + return qtest_readw(global_qtest, smm->addr + reg);
> +}
> +
> +static uint64_t sdhci_mm_readq(QSDHCI *s, uint32_t reg)
> +{
> + QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
> + return qtest_readq(global_qtest, smm->addr + reg);
> +}
> +
> +static void sdhci_mm_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
> +{
> + QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
> + qtest_writeq(global_qtest, smm->addr + reg, val);
> +}
> +
> +static void *sdhci_mm_get_driver(void *obj, const char *interface)
> +{
> + QSDHCI_MemoryMapped *spci = obj;
> + if (!g_strcmp0(interface, "sdhci")) {
> + return &spci->sdhci;
> + }
> + printf("%s not present in generic-sdhci\n", interface);
> + abort();
> +}
> +
> +void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
> + QSDHCIProperties *common)
> +{
> + sdhci->obj.get_driver = sdhci_mm_get_driver;
> + sdhci->sdhci.sdhci_readw = sdhci_mm_readw;
Lot of 'shdci'!
I'd name the argument 'QSDHCI_MemoryMapped *smm' to be consistent with
the previous sdhci_mm_read/write*(), that'd become more readable IMO:
smm->sdhci.readw = sdhci_mm_readw;
> + sdhci->sdhci.sdhci_readq = sdhci_mm_readq;
> + sdhci->sdhci.sdhci_writeq = sdhci_mm_writeq;
> + memcpy(&sdhci->sdhci.props, common, sizeof(QSDHCIProperties));
> + sdhci->addr = addr;
> +}
> +
> +/* PCI implementation of QSDHCI */
> +
> +static uint16_t sdhci_pci_readw(QSDHCI *s, uint32_t reg)
> +{
> + QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
> + return qpci_io_readw(&spci->dev, spci->mem_bar, reg);
> +}
> +
> +static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
> +{
> + QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
> + return qpci_io_readq(&spci->dev, spci->mem_bar, reg);
> +}
> +
> +static void sdhci_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
> +{
> + QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
> + return qpci_io_writeq(&spci->dev, spci->mem_bar, reg, val);
> +}
> +
> +static void *sdhci_pci_get_driver(void *object, const char *interface)
> +{
> + QSDHCI_PCI *spci = object;
> + if (!g_strcmp0(interface, "sdhci")) {
> + return &spci->sdhci;
> + }
> +
> + printf("%s not present in sdhci-pci\n", interface);
> + abort();
> +}
> +
> +static void sdhci_destroy(QOSGraphObject *obj)
> +{
> + g_free(obj);
> +}
> +
> +static void *sdhci_pci_create(void *pci_bus, QOSGraphObject *alloc)
> +{
> + QSDHCI_PCI *spci = g_new0(QSDHCI_PCI, 1);
> + QPCIBus *bus = pci_bus;
> + uint64_t barsize;
> +
> + qpci_device_init(&spci->dev, bus, QPCI_DEVFN(4, 0));
> + if (bus) {
> + spci->mem_bar = qpci_iomap(&spci->dev, 0, &barsize);
> + }
> + spci->obj.get_driver = sdhci_pci_get_driver;
> + spci->obj.destructor = sdhci_destroy;
> + spci->sdhci.sdhci_readw = sdhci_pci_readw;
> + spci->sdhci.sdhci_readq = sdhci_readq;
> + spci->sdhci.sdhci_writeq = sdhci_writeq;
> + set_qsdhci_fields(&spci->sdhci, 2, 0, 1, 0x057834b4);
> + return &spci->obj;
> +}
> +
> +static void qsdhci(void)
> +{
> + qos_node_create_interface("sdhci");
> + qos_node_create_driver("generic-sdhci", NULL);
> + qos_node_produces("generic-sdhci", "sdhci");
> + qos_node_create_driver("sdhci-pci", sdhci_pci_create);
> + qos_node_produces("sdhci-pci", "sdhci");
> + qos_node_consumes_arg("sdhci-pci", "pci-bus", "addr=04.0");
> +}
> +
> +libqos_init(qsdhci);
> diff --git a/tests/libqos/sdhci.h b/tests/libqos/sdhci.h
> new file mode 100644
> index 0000000000..dfd043f160
> --- /dev/null
> +++ b/tests/libqos/sdhci.h
> @@ -0,0 +1,68 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#ifndef QGRAPH_QSDHCI
> +#define QGRAPH_QSDHCI
> +#include "pci.h"
> +
> +typedef struct QSDHCI QSDHCI;
> +typedef struct QSDHCI_MemoryMapped QSDHCI_MemoryMapped;
> +typedef struct QSDHCI_PCI QSDHCI_PCI;
> +typedef struct QSDHCIProperties QSDHCIProperties;
> +
> +/* Properties common to all QSDHCI devices */
> +struct QSDHCIProperties {
> + uint8_t version;
> + uint8_t baseclock;
> + struct {
> + bool sdma;
> + uint64_t reg;
> + } capab;
> +};
> +
> +struct QSDHCI {
> + QOSGraphObject obj;
> + uint16_t (*sdhci_readw)(QSDHCI *s, uint32_t reg);
> + uint64_t (*sdhci_readq)(QSDHCI *s, uint32_t reg);
> + void (*sdhci_writeq)(QSDHCI *s, uint32_t reg, uint64_t val);
Please simplify as 'readw/readq/writeq'.
> + QSDHCIProperties props;
> +};
> +
> +/* Memory Mapped implementation of QSDHCI */
> +struct QSDHCI_MemoryMapped {
> + QOSGraphObject obj;
> + QSDHCI sdhci;
> + uint64_t addr;
> +};
> +
> +/* PCI implementation of QSDHCI */
> +struct QSDHCI_PCI {
> + QOSGraphObject obj;
> + QPCIDevice dev;
> + QSDHCI sdhci;
> + QPCIBar mem_bar;
> +};
> +
> +/**
> + * qos_create_sdhci_mm(): external constructor used by all drivers/machines
> + * that "contain" a #QSDHCI_MemoryMapped driver
> + */
> +void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
> + QSDHCIProperties *common);
Your IDE uses a weird indentation.
> +
> +#endif
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci driver and interface nodes
2018-07-11 20:13 ` Philippe Mathieu-Daudé
@ 2018-07-11 20:44 ` Emanuele
0 siblings, 0 replies; 39+ messages in thread
From: Emanuele @ 2018-07-11 20:44 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Paolo Bonzini, Laurent Vivier, qemu-devel
Hi Philippe,
On 07/11/2018 10:13 PM, Philippe Mathieu-Daudé wrote:
> Hi Emanuele,
>
> On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:
>> Add qgraph nodes for sdhci-pci and generic-sdhci (memory mapped) drivers.
>> Both drivers implement (produce) the same interface sdhci, that provides the
>> readw - readq - writeq functions.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
>> ---
>> tests/Makefile.include | 1 +
>> tests/libqos/sdhci.c | 142 +++++++++++++++++++++++++++++++++++++++++
>> tests/libqos/sdhci.h | 68 ++++++++++++++++++++
>> 3 files changed, 211 insertions(+)
>> create mode 100644 tests/libqos/sdhci.c
>> create mode 100644 tests/libqos/sdhci.h
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index b16bbd55df..acbf704a8a 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -770,6 +770,7 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
>> libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o
>>
>> libqgraph-obj-y = tests/libqos/qgraph.o
>> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
> Shouldn't this be:
>
> libqgraph-obj-y = tests/libqos/qgraph.o tests/libqos/sdhci.o
>
> (not PC related)
I need to add $(libqos-pc-obj-y) because it will include pc-pci.c and
all the pci targerts, loading their libqos_init() functions and building
the graph.
Doing as you suggested won't work for these reasons:
- pci-bus-pc and pci-bus nodes won't be created, so graph would be
incomplete
- sdhci needs libqos to use global_qtest, qpci_device_init, etc. that
won't be provided by check-unit-y even by adding $(libqos-pc-obj-y).
So since test-qgraph do not need libqos, I (suggested by Paolo) added
the test to the check-unit-y target, adding to it just libqgraph-obj.
For other devices like sdhci that need libqos, I created
libqgraph-pc-obj-y (to be renamed to libqgraph-pci-obj-y, since it will
also contain libqos-spapr ?) that will be added to target
check-qtest-pci-y (that provides libqos).
>> + QSDHCIProperties props;
>> +};
>> +
>> +/* Memory Mapped implementation of QSDHCI */
>> +struct QSDHCI_MemoryMapped {
>> + QOSGraphObject obj;
>> + QSDHCI sdhci;
>> + uint64_t addr;
>> +};
>> +
>> +/* PCI implementation of QSDHCI */
>> +struct QSDHCI_PCI {
>> + QOSGraphObject obj;
>> + QPCIDevice dev;
>> + QSDHCI sdhci;
>> + QPCIBar mem_bar;
>> +};
>> +
>> +/**
>> + * qos_create_sdhci_mm(): external constructor used by all drivers/machines
>> + * that "contain" a #QSDHCI_MemoryMapped driver
>> + */
>> +void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
>> + QSDHCIProperties *common);
> Your IDE uses a weird indentation.
I actually did by hand, not sure how the indentation to split >80 lines
should be.
Maybe I should remove a tab? Like this:
+void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
+ QSDHCIProperties *common);
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
` (2 preceding siblings ...)
2018-07-09 9:11 ` [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci " Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-11 14:59 ` Stefan Hajnoczi
2018-07-09 9:11 ` [Qemu-devel] [PATCH 5/7] tests/qgraph: x86_64/pc " Emanuele Giuseppe Esposito
` (4 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Add arm/raspi2 machine to the graph. This machine contains a generic-sdhci, so
its constructor must take care of setting it properly when called.
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
tests/Makefile.include | 3 +-
tests/libqos/raspi2-machine.c | 68 +++++++++++++++++++++++++++++++++++
2 files changed, 70 insertions(+), 1 deletion(-)
create mode 100644 tests/libqos/raspi2-machine.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index acbf704a8a..de75a7394e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -770,7 +770,8 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o
libqgraph-obj-y = tests/libqos/qgraph.o
-libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
+libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) tests/libqos/sdhci.o
+libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o
check-unit-y += tests/test-qgraph$(EXESUF)
tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
diff --git a/tests/libqos/raspi2-machine.c b/tests/libqos/raspi2-machine.c
new file mode 100644
index 0000000000..47f024076f
--- /dev/null
+++ b/tests/libqos/raspi2-machine.c
@@ -0,0 +1,68 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qgraph.h"
+#include "sdhci.h"
+
+typedef struct QRaspi2Machine QRaspi2Machine;
+
+struct QRaspi2Machine {
+ QOSGraphObject obj;
+ QSDHCI_MemoryMapped sdhci;
+};
+
+static void raspi2_destroy(QOSGraphObject *obj)
+{
+ g_free(obj);
+}
+
+static QOSGraphObject *raspi2_get_device(void *obj, const char *device)
+{
+ QRaspi2Machine *machine = obj;
+ if (!g_strcmp0(device, "generic-sdhci")) {
+ return &machine->sdhci.obj;
+ }
+
+ printf("%s not present in arm/raspi2", device);
+ abort();
+}
+
+static void *qos_create_machine_arm_raspi2(void)
+{
+ QRaspi2Machine *machine = g_new0(QRaspi2Machine, 1);
+
+ machine->obj.get_device = raspi2_get_device;
+ machine->obj.destructor = raspi2_destroy;
+ qos_create_sdhci_mm(&machine->sdhci, 0x3f300000, &(QSDHCIProperties) {
+ .version = 3,
+ .baseclock = 52,
+ .capab.sdma = false,
+ .capab.reg = 0x052134b4
+ });
+ return &machine->obj;
+}
+
+static void raspi2(void)
+{
+ qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
+ qos_node_contains("arm/raspi2", "generic-sdhci");
+}
+
+libqos_init(raspi2);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node
2018-07-09 9:11 ` [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node Emanuele Giuseppe Esposito
@ 2018-07-11 14:59 ` Stefan Hajnoczi
2018-07-11 15:30 ` Paolo Bonzini
0 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 14:59 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4421 bytes --]
On Mon, Jul 09, 2018 at 11:11:33AM +0200, Emanuele Giuseppe Esposito wrote:
> Add arm/raspi2 machine to the graph. This machine contains a generic-sdhci, so
> its constructor must take care of setting it properly when called.
>
> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> ---
> tests/Makefile.include | 3 +-
> tests/libqos/raspi2-machine.c | 68 +++++++++++++++++++++++++++++++++++
> 2 files changed, 70 insertions(+), 1 deletion(-)
> create mode 100644 tests/libqos/raspi2-machine.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index acbf704a8a..de75a7394e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -770,7 +770,8 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
> libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o
>
> libqgraph-obj-y = tests/libqos/qgraph.o
> -libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) tests/libqos/sdhci.o
> +libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o
>
> check-unit-y += tests/test-qgraph$(EXESUF)
> tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
> diff --git a/tests/libqos/raspi2-machine.c b/tests/libqos/raspi2-machine.c
> new file mode 100644
> index 0000000000..47f024076f
> --- /dev/null
> +++ b/tests/libqos/raspi2-machine.c
> @@ -0,0 +1,68 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qgraph.h"
> +#include "sdhci.h"
> +
> +typedef struct QRaspi2Machine QRaspi2Machine;
> +
> +struct QRaspi2Machine {
> + QOSGraphObject obj;
> + QSDHCI_MemoryMapped sdhci;
> +};
> +
> +static void raspi2_destroy(QOSGraphObject *obj)
> +{
> + g_free(obj);
> +}
> +
> +static QOSGraphObject *raspi2_get_device(void *obj, const char *device)
> +{
> + QRaspi2Machine *machine = obj;
> + if (!g_strcmp0(device, "generic-sdhci")) {
> + return &machine->sdhci.obj;
> + }
> +
> + printf("%s not present in arm/raspi2", device);
> + abort();
> +}
> +
> +static void *qos_create_machine_arm_raspi2(void)
> +{
> + QRaspi2Machine *machine = g_new0(QRaspi2Machine, 1);
> +
> + machine->obj.get_device = raspi2_get_device;
> + machine->obj.destructor = raspi2_destroy;
> + qos_create_sdhci_mm(&machine->sdhci, 0x3f300000, &(QSDHCIProperties) {
> + .version = 3,
> + .baseclock = 52,
> + .capab.sdma = false,
> + .capab.reg = 0x052134b4
> + });
> + return &machine->obj;
> +}
> +
> +static void raspi2(void)
> +{
> + qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
> + qos_node_contains("arm/raspi2", "generic-sdhci");
> +}
> +
> +libqos_init(raspi2);
Hmm...I didn't realize it would be necessary to manually define each
machine type. I thought qgraph would use qemu-system-x86_64
-machine/-device/info qtree to introspect QEMU and instantiate the
appropriate drivers.
My concern is that this manual approach of defining machines complicates
qtests. This file will need to be modified by multiple people - each of
them will be interested in a specific driver and not interested in the
driver that other people have added.
This makes things more complicated where previous qtests simply had an
if (x86) { add_test(test_x86_foo); } else if (arm) {
add_test(test_arm_foo); } in their source file. It's ugly but it's
obvious.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node
2018-07-11 14:59 ` Stefan Hajnoczi
@ 2018-07-11 15:30 ` Paolo Bonzini
2018-07-11 20:19 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-11 15:30 UTC (permalink / raw)
To: Stefan Hajnoczi, Emanuele Giuseppe Esposito
Cc: Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>> + machine->obj.get_device = raspi2_get_device;
>> + machine->obj.destructor = raspi2_destroy;
>> + qos_create_sdhci_mm(&machine->sdhci, 0x3f300000, &(QSDHCIProperties) {
>> + .version = 3,
>> + .baseclock = 52,
>> + .capab.sdma = false,
>> + .capab.reg = 0x052134b4
>> + });
>> + return &machine->obj;
>> +}
>> +
>> +static void raspi2(void)
>> +{
>> + qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
>> + qos_node_contains("arm/raspi2", "generic-sdhci");
>> +}
>> +
>> +libqos_init(raspi2);
> Hmm...I didn't realize it would be necessary to manually define each
> machine type. I thought qgraph would use qemu-system-x86_64
> -machine/-device/info qtree to introspect QEMU and instantiate the
> appropriate drivers.
That doesn't provide enough information yet. However, PCI devices are
already discovered automatically and the next step (next year :)) could
be to use device tree or ACPI to discover embedded devices.
Using QOM properties instead of duplicating things like QSDHCIProperties
is a great idea. Of course the duplication was already there ("old
sdhci_t structure" in patch 7), so I don't think it should be a blocker,
but indeed there's a better way to do it.
> My concern is that this manual approach of defining machines complicates
> qtests. This file will need to be modified by multiple people - each of
> them will be interested in a specific driver and not interested in the
> driver that other people have added.
This is exactly the opposite problem that we have now: when you write a
test you are interested typically only in one machine, and the
"if(x86)elseif(arm)" gets in the way. (Also: it's also difficult to
split the ifs across files, i.e. it scales worse; and there's lots of
duplicated code across tests to do "for" loops of g_test_add, because
the ifs don't lend itself to "automatic" generation of test cases via
path walk).
The advantage is that (just like for OS vs. QEMU) the structure of this
file matches closely the board files in hw/arm/, which is something you
should already be familiar with if you're adding a new board to QEMU.
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node
2018-07-11 15:30 ` Paolo Bonzini
@ 2018-07-11 20:19 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-07-11 20:19 UTC (permalink / raw)
To: Paolo Bonzini, Stefan Hajnoczi, Emanuele Giuseppe Esposito
Cc: Laurent Vivier, qemu-devel
On 07/11/2018 12:30 PM, Paolo Bonzini wrote:
> On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>>> + machine->obj.get_device = raspi2_get_device;
>>> + machine->obj.destructor = raspi2_destroy;
>>> + qos_create_sdhci_mm(&machine->sdhci, 0x3f300000, &(QSDHCIProperties) {
>>> + .version = 3,
>>> + .baseclock = 52,
>>> + .capab.sdma = false,
>>> + .capab.reg = 0x052134b4
>>> + });
>>> + return &machine->obj;
>>> +}
>>> +
>>> +static void raspi2(void)
>>> +{
>>> + qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
>>> + qos_node_contains("arm/raspi2", "generic-sdhci");
>>> +}
>>> +
>>> +libqos_init(raspi2);
>> Hmm...I didn't realize it would be necessary to manually define each
>> machine type. I thought qgraph would use qemu-system-x86_64
>> -machine/-device/info qtree to introspect QEMU and instantiate the
>> appropriate drivers.
>
> That doesn't provide enough information yet. However, PCI devices are
> already discovered automatically and the next step (next year :)) could
> be to use device tree or ACPI to discover embedded devices.
>
> Using QOM properties instead of duplicating things like QSDHCIProperties
> is a great idea. Of course the duplication was already there ("old
> sdhci_t structure" in patch 7), so I don't think it should be a blocker,
> but indeed there's a better way to do it.
>
>> My concern is that this manual approach of defining machines complicates
>> qtests. This file will need to be modified by multiple people - each of
>> them will be interested in a specific driver and not interested in the
>> driver that other people have added.
>
> This is exactly the opposite problem that we have now: when you write a
> test you are interested typically only in one machine, and the
> "if(x86)elseif(arm)" gets in the way. (Also: it's also difficult to
> split the ifs across files, i.e. it scales worse; and there's lots of
> duplicated code across tests to do "for" loops of g_test_add, because
> the ifs don't lend itself to "automatic" generation of test cases via
> path walk).
>
> The advantage is that (just like for OS vs. QEMU) the structure of this
> file matches closely the board files in hw/arm/, which is something you
> should already be familiar with if you're adding a new board to QEMU.
Some projects organize their tests alongside the source to be tested.
This is often messy, but might be clearer to see source testing coverage.
That said, I'd rather keep it out of 'source' tree (as of now). Using a
simple script we can notice the same (which device models matches a test).
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 5/7] tests/qgraph: x86_64/pc machine node
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
` (3 preceding siblings ...)
2018-07-09 9:11 ` [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-09 9:11 ` [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration Emanuele Giuseppe Esposito
` (3 subsequent siblings)
8 siblings, 0 replies; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Add pc machine for the x86_64 QEMU binary. This machine contains an i440FX-pcihost
driver, that contains itself a pci-bus-pc that produces the pci-bus interface.
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
tests/Makefile.include | 2 +-
tests/libqos/x86_64_pc-machine.c | 93 ++++++++++++++++++++++++++++++++
2 files changed, 94 insertions(+), 1 deletion(-)
create mode 100644 tests/libqos/x86_64_pc-machine.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index de75a7394e..706ac39ea5 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -771,7 +771,7 @@ libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/virt
libqgraph-obj-y = tests/libqos/qgraph.o
libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) tests/libqos/sdhci.o
-libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o
+libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o tests/libqos/x86_64_pc-machine.o
check-unit-y += tests/test-qgraph$(EXESUF)
tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
diff --git a/tests/libqos/x86_64_pc-machine.c b/tests/libqos/x86_64_pc-machine.c
new file mode 100644
index 0000000000..626e8d7d70
--- /dev/null
+++ b/tests/libqos/x86_64_pc-machine.c
@@ -0,0 +1,93 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qgraph.h"
+#include "sdhci.h"
+#include "pci-pc.h"
+
+typedef struct QX86_64_PCMachine QX86_64_PCMachine;
+typedef struct i440FX_pcihost i440FX_pcihost;
+typedef struct QSDHCI_PCI QSDHCI_PCI;
+
+struct i440FX_pcihost {
+ QOSGraphObject obj;
+ QPCIBusPC pci;
+};
+
+struct QX86_64_PCMachine {
+ QOSGraphObject obj;
+ i440FX_pcihost bridge;
+};
+
+/* i440FX_pcihost */
+
+static QOSGraphObject *i440FX_host_get_device(void *obj, const char *device)
+{
+ i440FX_pcihost *host = obj;
+ if (!g_strcmp0(device, "pci-bus-pc")) {
+ return &host->pci.obj;
+ }
+ printf("%s not present in i440FX-pcihost", device);
+ abort();
+}
+
+static void qos_create_i440FX_host(i440FX_pcihost *host)
+{
+ host->obj.get_device = i440FX_host_get_device;
+ qpci_set_pc(&host->pci, global_qtest, NULL);
+}
+
+/* x86_64/pc machine */
+
+static void pc_destroy(QOSGraphObject *obj)
+{
+ g_free(obj);
+}
+
+static QOSGraphObject *pc_get_device(void *obj, const char *device)
+{
+ QX86_64_PCMachine *machine = obj;
+ if (!g_strcmp0(device, "i440FX-pcihost")) {
+ return &machine->bridge.obj;
+ }
+
+ printf("%s not present in x86_64/pc", device);
+ abort();
+}
+
+static void *qos_create_machine_pc(void)
+{
+ QX86_64_PCMachine *machine = g_new0(QX86_64_PCMachine, 1);
+ machine->obj.get_device = pc_get_device;
+ machine->obj.destructor = pc_destroy;
+ qos_create_i440FX_host(&machine->bridge);
+
+ return &machine->obj;
+}
+
+static void pc_machine(void)
+{
+ qos_node_create_machine("x86_64/pc", qos_create_machine_pc);
+ qos_node_create_driver("i440FX-pcihost", NULL);
+ qos_node_contains("x86_64/pc", "i440FX-pcihost");
+ qos_node_contains("i440FX-pcihost", "pci-bus-pc");
+}
+
+libqos_init(pc_machine);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
` (4 preceding siblings ...)
2018-07-09 9:11 ` [Qemu-devel] [PATCH 5/7] tests/qgraph: x86_64/pc " Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-11 15:02 ` Stefan Hajnoczi
2018-07-09 9:11 ` [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node Emanuele Giuseppe Esposito
` (2 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Add main executable that takes care of starting the framework, create the
nodes, set the available drivers/machines, discover the path and run tests.
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
tests/Makefile.include | 3 +
tests/qos-test.c | 310 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 313 insertions(+)
create mode 100644 tests/qos-test.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 706ac39ea5..e2db3e9d09 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -776,6 +776,9 @@ libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o tests/libqos/x86_64_pc-machi
check-unit-y += tests/test-qgraph$(EXESUF)
tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
+check-qtest-pci-y += tests/qos-test$(EXESUF)
+tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-pc-obj-y)
+
tests/qmp-test$(EXESUF): tests/qmp-test.o
tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
tests/rtc-test$(EXESUF): tests/rtc-test.o
diff --git a/tests/qos-test.c b/tests/qos-test.c
new file mode 100644
index 0000000000..f4748c44a4
--- /dev/null
+++ b/tests/qos-test.c
@@ -0,0 +1,310 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qlist.h"
+#include "libqos/qgraph.h"
+#include "libqos/qgraph_extra.h"
+
+/**
+ * create_machine_name(): appends the architecture to @name if
+ * @is_machine is valid.
+ */
+static void create_machine_name(const char **name, bool is_machine)
+{
+ const char *arch;
+ if (!is_machine) {
+ return;
+ }
+ arch = qtest_get_arch();
+ *name = g_strconcat(arch, "/", *name, NULL);
+}
+
+/**
+ * destroy_machine_name(): frees the given @name if
+ * @is_machine is valid.
+ */
+static void destroy_machine_name(const char *name, bool is_machine)
+{
+ if (!is_machine) {
+ return;
+ }
+ g_free((char *)name);
+}
+
+/**
+ * apply_to_qlist(): using QMP queries QEMU for a list of
+ * machines and devices available, and sets the respective node
+ * as TRUE. If a node is found, also all its produced and contained
+ * child are marked available.
+ *
+ * See qos_graph_node_set_availability() for more info
+ */
+static void apply_to_qlist(QList *list, bool is_machine)
+{
+ const QListEntry *p;
+ const char *name;
+ QDict *minfo;
+ QObject *qobj;
+ QString *qstr;
+
+ for (p = qlist_first(list); p; p = qlist_next(p)) {
+ minfo = qobject_to(QDict, qlist_entry_obj(p));
+ g_assert(minfo);
+ qobj = qdict_get(minfo, "name");
+ g_assert(qobj);
+ qstr = qobject_to(QString, qobj);
+ g_assert(qstr);
+ name = qstring_get_str(qstr);
+ create_machine_name(&name, is_machine);
+ qos_graph_node_set_availability(name, TRUE);
+ destroy_machine_name(name, is_machine);
+ qobj = qdict_get(minfo, "alias");
+ if (qobj) {
+ g_assert(qobj);
+ qstr = qobject_to(QString, qobj);
+ g_assert(qstr);
+ name = qstring_get_str(qstr);
+ create_machine_name(&name, is_machine);
+ qos_graph_node_set_availability(name, TRUE);
+ destroy_machine_name(name, is_machine);
+ }
+ }
+}
+
+/**
+ * qos_set_machines_devices_available(): sets availability of qgraph
+ * machines and devices.
+ *
+ * This function firstly starts QEMU with "-machine none" option,
+ * and then executes the QMP protocol asking for the list of devices
+ * and machines available.
+ *
+ * for each of these items, it looks up the corresponding qgraph node,
+ * setting it as available. The list currently returns all devices that
+ * are either machines or CONSUMED_BY other nodes.
+ * Therefore, in order to mark all other nodes, it recursively sets
+ * all its CONTAINS and PRODUCES child as available too.
+ */
+static void qos_set_machines_devices_available(void)
+{
+ QDict *response;
+ QDict *args = qdict_new();
+ QList *list;
+
+ qtest_start("-machine none");
+ response = qmp("{ 'execute': 'query-machines' }");
+ g_assert(response);
+ list = qdict_get_qlist(response, "return");
+ g_assert(list);
+
+ apply_to_qlist(list, TRUE);
+
+ qobject_unref(response);
+
+ qdict_put_bool(args, "abstract", TRUE);
+ qdict_put_str(args, "implements", "device");
+
+ response = qmp("{'execute': 'qom-list-types',"
+ " 'arguments': %p }", args);
+ g_assert(qdict_haskey(response, "return"));
+ list = qdict_get_qlist(response, "return");
+
+ apply_to_qlist(list, FALSE);
+
+ qtest_end();
+ qobject_unref(response);
+
+}
+
+/**
+ * allocate_objects(): given an array of nodes @arg,
+ * walks the path invoking all constructors and
+ * passing the corresponding parameter in order to
+ * continue the objects allocation.
+ * Once the test is reached, its function is executed.
+ *
+ * Since only the machine and CONSUMED_BY nodes actually
+ * allocate something in the constructor, a garbage collector
+ * saves their pointer in an array, so that after execution
+ * they can be safely free'd.
+ *
+ * Note: as specified in walk_path() too, @arg is an array of
+ * char *, where arg[0] is a pointer to the command line
+ * string that will be used to properly start QEMU when executing
+ * the test, and the remaining elements represent the actual objects
+ * that will be allocated.
+ */
+static void allocate_objects(const void *arg)
+{
+ QOSGraphObject *garbage_collector[(QOS_PATH_MAX_ELEMENT_SIZE * 2)];
+ int garbage_size = 0;
+ QOSEdgeType etype;
+ QOSGraphNode *node;
+ QOSGraphObject *o;
+ int current = 1, has_to_allocate = 0;
+ void *obj = NULL;
+ char **path = (char **) arg;
+
+ node = qos_graph_get_node(path[current]);
+
+ while (current < QOS_PATH_MAX_ELEMENT_SIZE) {
+
+ if (node->type == MACHINE) {
+ global_qtest = qtest_start(path[0]);
+
+ obj = node->u.machine.constructor();
+ garbage_collector[garbage_size++] = obj;
+
+ } else if (has_to_allocate && node->type == DRIVER) {
+ obj = node->u.driver.constructor(obj, NULL);
+ garbage_collector[garbage_size++] = obj;
+
+ } else if (!path[(current + 1)] && node->type == TEST) {
+ node->u.test.function(obj, node->u.test.arg);
+ break;
+ }
+
+ etype = qos_graph_get_edge_type(path[current], path[(current + 1)]);
+ current++;
+ node = qos_graph_get_node(path[current]);
+
+ o = obj;
+
+ switch (etype) {
+ case PRODUCES:
+ obj = o->get_driver(obj, path[current]);
+ break;
+ case CONSUMED_BY:
+ has_to_allocate = 1;
+ break;
+ case CONTAINS:
+ obj = o->get_device(obj, path[current]);
+ break;
+ }
+
+ }
+
+ g_free(path[0]);
+ g_free(path);
+ for (int i = 0; i < garbage_size; i++) {
+ garbage_collector[i]->destructor(garbage_collector[i]);
+ }
+ qtest_end();
+}
+
+/*
+ * in this function, 2 path will be built:
+ * str_path, a one-string path (ex "pc/i440FX-pcihost/...")
+ * ro_path, a string-array path (ex [0] = "pc", [1] = "i440FX-pcihost").
+ *
+ * str_path will be only used to build the test name, and won't need the
+ * architecture name at beginning, since it will be added by qtest_add_func().
+ *
+ * ro_path is used to allocate all constructors of the path nodes.
+ * Each name in this array except position 0 must correspond to a valid
+ * QOSGraphNode name.
+ * Position 0 is special, initially contains just the <machine> name of
+ * the node, (ex for "x86_64/pc" it will be "pc"), used to build the test
+ * path (see below). After it will contain the command line used to start
+ * qemu with all required devices.
+ *
+ * Note that the machine node name must be with format <arch>/<machine>
+ * (ex "x86_64/pc"), because it will identify the node "x86_64/pc"
+ * and start QEMU with "-M pc". For this reason,
+ * when building str_path, ro_path
+ * initially contains the <machine> at position 0 ("pc"),
+ * and the node name at position 1 (<arch>/<machine>)
+ * ("x86_64/pc"), followed by the rest of the nodes.
+ */
+static void walk_path(QOSGraphNode *orig_path, int len)
+{
+ QOSGraphNode *path;
+ /* twice QOS_PATH_MAX_ELEMENT_SIZE since each edge can have its arg */
+ char **ro_path = g_new0(char *, (QOS_PATH_MAX_ELEMENT_SIZE * 2));
+ int ro_path_size = 0;
+ char *machine = NULL, *arch = NULL, *tmp_cmd = NULL, *str_path;
+ char *tmp = orig_path->name, *gfreed;
+ GString *cmd_line = g_string_new("");
+
+ do {
+ path = qos_graph_get_node(tmp);
+ tmp = qos_graph_get_edge_dest(path->path_edge);
+
+ /* append node command line + previous edge command line */
+ if (path->command_line && tmp_cmd) {
+ g_string_append(cmd_line, path->command_line);
+ g_string_append_printf(cmd_line, "%s ", tmp_cmd);
+ }
+
+ if (path->type == MACHINE) {
+ qos_separate_arch_machine(path->name, &arch, &machine);
+ ro_path[ro_path_size++] = arch;
+ ro_path[ro_path_size++] = machine;
+ g_string_append_printf(cmd_line, "%s ", path->command_line);
+ } else {
+ /* detect if edge has command line args */
+ ro_path[ro_path_size++] = path->name;
+ tmp_cmd = qos_graph_get_edge_arg(path->path_edge);
+ }
+
+ } while (path->path_edge);
+
+
+ /* here position 0 has <arch>/<machine>, position 1 <machine>.
+ * the path must not have the <arch>, that's why ro_path + 1
+ */
+ str_path = g_strjoinv("/", (ro_path + 1));
+ gfreed = g_string_free(cmd_line, FALSE);
+ /* put arch/machine in position 1 so allocate_objects can do its work
+ * and add the command line at position 0.
+ */
+ ro_path[0] = g_strdup(gfreed);
+ ro_path[1] = arch;
+
+ qtest_add_data_func(str_path, ro_path, allocate_objects);
+ g_free(str_path);
+}
+
+/**
+ * main(): heart of the qgraph framework.
+ *
+ * - Initializes the glib test framework
+ * - Creates the graph by invoking the various _init constructors
+ * - Starts QEMU to mark the available devices
+ * - Walks the graph, and each path is added to
+ * the glib test framework (walk_path)
+ * - Runs the tests, calling allocate_object() and allocating the
+ * machine/drivers/test objects
+ * - Cleans up everything
+ */
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+ qos_graph_init();
+ module_call_init(MODULE_INIT_LIBQOS);
+ qos_set_machines_devices_available();
+
+ qos_graph_foreach_test_path(walk_path);
+ g_test_run();
+ qos_graph_destroy();
+ return 0;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration
2018-07-09 9:11 ` [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration Emanuele Giuseppe Esposito
@ 2018-07-11 15:02 ` Stefan Hajnoczi
0 siblings, 0 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 15:02 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 12676 bytes --]
On Mon, Jul 09, 2018 at 11:11:35AM +0200, Emanuele Giuseppe Esposito wrote:
> Add main executable that takes care of starting the framework, create the
> nodes, set the available drivers/machines, discover the path and run tests.
This is elegant, I like it.
> Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> ---
> tests/Makefile.include | 3 +
> tests/qos-test.c | 310 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 313 insertions(+)
> create mode 100644 tests/qos-test.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 706ac39ea5..e2db3e9d09 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -776,6 +776,9 @@ libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o tests/libqos/x86_64_pc-machi
> check-unit-y += tests/test-qgraph$(EXESUF)
> tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
>
> +check-qtest-pci-y += tests/qos-test$(EXESUF)
> +tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-pc-obj-y)
> +
> tests/qmp-test$(EXESUF): tests/qmp-test.o
> tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
> tests/rtc-test$(EXESUF): tests/rtc-test.o
> diff --git a/tests/qos-test.c b/tests/qos-test.c
> new file mode 100644
> index 0000000000..f4748c44a4
> --- /dev/null
> +++ b/tests/qos-test.c
> @@ -0,0 +1,310 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qlist.h"
> +#include "libqos/qgraph.h"
> +#include "libqos/qgraph_extra.h"
> +
> +/**
> + * create_machine_name(): appends the architecture to @name if
> + * @is_machine is valid.
> + */
> +static void create_machine_name(const char **name, bool is_machine)
> +{
> + const char *arch;
> + if (!is_machine) {
> + return;
> + }
> + arch = qtest_get_arch();
> + *name = g_strconcat(arch, "/", *name, NULL);
> +}
> +
> +/**
> + * destroy_machine_name(): frees the given @name if
> + * @is_machine is valid.
> + */
> +static void destroy_machine_name(const char *name, bool is_machine)
> +{
> + if (!is_machine) {
> + return;
> + }
> + g_free((char *)name);
> +}
> +
> +/**
> + * apply_to_qlist(): using QMP queries QEMU for a list of
> + * machines and devices available, and sets the respective node
> + * as TRUE. If a node is found, also all its produced and contained
> + * child are marked available.
> + *
> + * See qos_graph_node_set_availability() for more info
> + */
> +static void apply_to_qlist(QList *list, bool is_machine)
> +{
> + const QListEntry *p;
> + const char *name;
> + QDict *minfo;
> + QObject *qobj;
> + QString *qstr;
> +
> + for (p = qlist_first(list); p; p = qlist_next(p)) {
> + minfo = qobject_to(QDict, qlist_entry_obj(p));
> + g_assert(minfo);
> + qobj = qdict_get(minfo, "name");
> + g_assert(qobj);
> + qstr = qobject_to(QString, qobj);
> + g_assert(qstr);
> + name = qstring_get_str(qstr);
> + create_machine_name(&name, is_machine);
> + qos_graph_node_set_availability(name, TRUE);
> + destroy_machine_name(name, is_machine);
> + qobj = qdict_get(minfo, "alias");
> + if (qobj) {
> + g_assert(qobj);
> + qstr = qobject_to(QString, qobj);
> + g_assert(qstr);
> + name = qstring_get_str(qstr);
> + create_machine_name(&name, is_machine);
> + qos_graph_node_set_availability(name, TRUE);
> + destroy_machine_name(name, is_machine);
> + }
> + }
> +}
> +
> +/**
> + * qos_set_machines_devices_available(): sets availability of qgraph
> + * machines and devices.
> + *
> + * This function firstly starts QEMU with "-machine none" option,
> + * and then executes the QMP protocol asking for the list of devices
> + * and machines available.
> + *
> + * for each of these items, it looks up the corresponding qgraph node,
> + * setting it as available. The list currently returns all devices that
> + * are either machines or CONSUMED_BY other nodes.
> + * Therefore, in order to mark all other nodes, it recursively sets
> + * all its CONTAINS and PRODUCES child as available too.
> + */
> +static void qos_set_machines_devices_available(void)
> +{
> + QDict *response;
> + QDict *args = qdict_new();
> + QList *list;
> +
> + qtest_start("-machine none");
> + response = qmp("{ 'execute': 'query-machines' }");
> + g_assert(response);
> + list = qdict_get_qlist(response, "return");
> + g_assert(list);
> +
> + apply_to_qlist(list, TRUE);
> +
> + qobject_unref(response);
> +
> + qdict_put_bool(args, "abstract", TRUE);
> + qdict_put_str(args, "implements", "device");
> +
> + response = qmp("{'execute': 'qom-list-types',"
> + " 'arguments': %p }", args);
> + g_assert(qdict_haskey(response, "return"));
> + list = qdict_get_qlist(response, "return");
> +
> + apply_to_qlist(list, FALSE);
> +
> + qtest_end();
> + qobject_unref(response);
> +
> +}
> +
> +/**
> + * allocate_objects(): given an array of nodes @arg,
> + * walks the path invoking all constructors and
> + * passing the corresponding parameter in order to
> + * continue the objects allocation.
> + * Once the test is reached, its function is executed.
> + *
> + * Since only the machine and CONSUMED_BY nodes actually
> + * allocate something in the constructor, a garbage collector
> + * saves their pointer in an array, so that after execution
> + * they can be safely free'd.
> + *
> + * Note: as specified in walk_path() too, @arg is an array of
> + * char *, where arg[0] is a pointer to the command line
> + * string that will be used to properly start QEMU when executing
> + * the test, and the remaining elements represent the actual objects
> + * that will be allocated.
> + */
> +static void allocate_objects(const void *arg)
> +{
> + QOSGraphObject *garbage_collector[(QOS_PATH_MAX_ELEMENT_SIZE * 2)];
> + int garbage_size = 0;
> + QOSEdgeType etype;
> + QOSGraphNode *node;
> + QOSGraphObject *o;
> + int current = 1, has_to_allocate = 0;
> + void *obj = NULL;
> + char **path = (char **) arg;
> +
> + node = qos_graph_get_node(path[current]);
> +
> + while (current < QOS_PATH_MAX_ELEMENT_SIZE) {
> +
> + if (node->type == MACHINE) {
> + global_qtest = qtest_start(path[0]);
> +
> + obj = node->u.machine.constructor();
> + garbage_collector[garbage_size++] = obj;
> +
> + } else if (has_to_allocate && node->type == DRIVER) {
> + obj = node->u.driver.constructor(obj, NULL);
> + garbage_collector[garbage_size++] = obj;
> +
> + } else if (!path[(current + 1)] && node->type == TEST) {
> + node->u.test.function(obj, node->u.test.arg);
> + break;
> + }
> +
> + etype = qos_graph_get_edge_type(path[current], path[(current + 1)]);
> + current++;
> + node = qos_graph_get_node(path[current]);
> +
> + o = obj;
> +
> + switch (etype) {
> + case PRODUCES:
> + obj = o->get_driver(obj, path[current]);
> + break;
> + case CONSUMED_BY:
> + has_to_allocate = 1;
> + break;
> + case CONTAINS:
> + obj = o->get_device(obj, path[current]);
> + break;
> + }
> +
> + }
> +
> + g_free(path[0]);
> + g_free(path);
> + for (int i = 0; i < garbage_size; i++) {
> + garbage_collector[i]->destructor(garbage_collector[i]);
> + }
> + qtest_end();
> +}
> +
> +/*
> + * in this function, 2 path will be built:
> + * str_path, a one-string path (ex "pc/i440FX-pcihost/...")
> + * ro_path, a string-array path (ex [0] = "pc", [1] = "i440FX-pcihost").
> + *
> + * str_path will be only used to build the test name, and won't need the
> + * architecture name at beginning, since it will be added by qtest_add_func().
> + *
> + * ro_path is used to allocate all constructors of the path nodes.
> + * Each name in this array except position 0 must correspond to a valid
> + * QOSGraphNode name.
> + * Position 0 is special, initially contains just the <machine> name of
> + * the node, (ex for "x86_64/pc" it will be "pc"), used to build the test
> + * path (see below). After it will contain the command line used to start
> + * qemu with all required devices.
> + *
> + * Note that the machine node name must be with format <arch>/<machine>
> + * (ex "x86_64/pc"), because it will identify the node "x86_64/pc"
> + * and start QEMU with "-M pc". For this reason,
> + * when building str_path, ro_path
> + * initially contains the <machine> at position 0 ("pc"),
> + * and the node name at position 1 (<arch>/<machine>)
> + * ("x86_64/pc"), followed by the rest of the nodes.
> + */
> +static void walk_path(QOSGraphNode *orig_path, int len)
> +{
> + QOSGraphNode *path;
> + /* twice QOS_PATH_MAX_ELEMENT_SIZE since each edge can have its arg */
> + char **ro_path = g_new0(char *, (QOS_PATH_MAX_ELEMENT_SIZE * 2));
> + int ro_path_size = 0;
> + char *machine = NULL, *arch = NULL, *tmp_cmd = NULL, *str_path;
> + char *tmp = orig_path->name, *gfreed;
> + GString *cmd_line = g_string_new("");
> +
> + do {
> + path = qos_graph_get_node(tmp);
> + tmp = qos_graph_get_edge_dest(path->path_edge);
> +
> + /* append node command line + previous edge command line */
> + if (path->command_line && tmp_cmd) {
> + g_string_append(cmd_line, path->command_line);
> + g_string_append_printf(cmd_line, "%s ", tmp_cmd);
> + }
> +
> + if (path->type == MACHINE) {
> + qos_separate_arch_machine(path->name, &arch, &machine);
> + ro_path[ro_path_size++] = arch;
> + ro_path[ro_path_size++] = machine;
> + g_string_append_printf(cmd_line, "%s ", path->command_line);
> + } else {
> + /* detect if edge has command line args */
> + ro_path[ro_path_size++] = path->name;
> + tmp_cmd = qos_graph_get_edge_arg(path->path_edge);
> + }
> +
> + } while (path->path_edge);
> +
> +
> + /* here position 0 has <arch>/<machine>, position 1 <machine>.
> + * the path must not have the <arch>, that's why ro_path + 1
> + */
> + str_path = g_strjoinv("/", (ro_path + 1));
> + gfreed = g_string_free(cmd_line, FALSE);
> + /* put arch/machine in position 1 so allocate_objects can do its work
> + * and add the command line at position 0.
> + */
> + ro_path[0] = g_strdup(gfreed);
> + ro_path[1] = arch;
> +
> + qtest_add_data_func(str_path, ro_path, allocate_objects);
> + g_free(str_path);
> +}
> +
> +/**
> + * main(): heart of the qgraph framework.
> + *
> + * - Initializes the glib test framework
> + * - Creates the graph by invoking the various _init constructors
> + * - Starts QEMU to mark the available devices
> + * - Walks the graph, and each path is added to
> + * the glib test framework (walk_path)
> + * - Runs the tests, calling allocate_object() and allocating the
> + * machine/drivers/test objects
> + * - Cleans up everything
> + */
> +int main(int argc, char **argv)
> +{
> + g_test_init(&argc, &argv, NULL);
> + qos_graph_init();
> + module_call_init(MODULE_INIT_LIBQOS);
> + qos_set_machines_devices_available();
> +
> + qos_graph_foreach_test_path(walk_path);
> + g_test_run();
> + qos_graph_destroy();
> + return 0;
> +}
> --
> 2.17.1
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
` (5 preceding siblings ...)
2018-07-09 9:11 ` [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration Emanuele Giuseppe Esposito
@ 2018-07-09 9:11 ` Emanuele Giuseppe Esposito
2018-07-11 15:15 ` Stefan Hajnoczi
2018-07-11 14:00 ` [Qemu-devel] [PATCH 0/7] Qtest driver framework Stefan Hajnoczi
2018-07-11 15:27 ` Stefan Hajnoczi
8 siblings, 1 reply; 39+ messages in thread
From: Emanuele Giuseppe Esposito @ 2018-07-09 9:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Laurent Vivier, Philippe Mathieu-Daudé,
qemu-devel, Emanuele Giuseppe Esposito
Convert tests/sdhci-test in first qgraph test node, sdhci-test. This test
consumes an sdhci interface and checks that its function return the
expected values.
Note that this test does not allocate any sdhci structure, it's all done by the
qtest walking graph mechanism
Signed-off-by: Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>
---
tests/Makefile.include | 8 +-
tests/sdhci-test.c | 222 ++++++++++++-----------------------------
2 files changed, 67 insertions(+), 163 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e2db3e9d09..790abe8c6e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -311,7 +311,6 @@ check-qtest-i386-y += tests/migration-test$(EXESUF)
check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
check-qtest-i386-y += tests/numa-test$(EXESUF)
check-qtest-x86_64-y += $(check-qtest-i386-y)
-check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -385,10 +384,8 @@ gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
gcov-files-arm-y += hw/timer/arm_mptimer.c
check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
-check-qtest-arm-y += tests/sdhci-test$(EXESUF)
check-qtest-aarch64-y = tests/numa-test$(EXESUF)
-check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
@@ -773,11 +770,13 @@ libqgraph-obj-y = tests/libqos/qgraph.o
libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) tests/libqos/sdhci.o
libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o tests/libqos/x86_64_pc-machine.o
+libqgraph-tests-obj-y = $(libqgraph-pc-obj-y) tests/sdhci-test.o
+
check-unit-y += tests/test-qgraph$(EXESUF)
tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
check-qtest-pci-y += tests/qos-test$(EXESUF)
-tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-pc-obj-y)
+tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-tests-obj-y)
tests/qmp-test$(EXESUF): tests/qmp-test.o
tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
@@ -863,7 +862,6 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
tests/numa-test$(EXESUF): tests/numa-test.o
tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
-tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
tests/migration/stress$(EXESUF): tests/migration/stress.o
diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c
index 1d825eb010..3e16c036f4 100644
--- a/tests/sdhci-test.c
+++ b/tests/sdhci-test.c
@@ -12,6 +12,8 @@
#include "libqtest.h"
#include "libqos/pci-pc.h"
#include "hw/pci/pci.h"
+#include "libqos/qgraph.h"
+#include "libqos/sdhci.h"
#define SDHC_CAPAB 0x40
FIELD(SDHC_CAPAB, BASECLKFREQ, 8, 8); /* since v2 */
@@ -20,99 +22,60 @@ FIELD(SDHC_CAPAB, SDR, 32, 3); /* since v3 */
FIELD(SDHC_CAPAB, DRIVER, 36, 3); /* since v3 */
#define SDHC_HCVER 0xFE
-static const struct sdhci_t {
- const char *arch, *machine;
- struct {
- uintptr_t addr;
- uint8_t version;
- uint8_t baseclock;
+/**
+ * Old sdhci_t structure:
+ *
+ struct sdhci_t {
+ const char *arch, *machine;
struct {
- bool sdma;
- uint64_t reg;
- } capab;
- } sdhci;
- struct {
- uint16_t vendor_id, device_id;
- } pci;
-} models[] = {
- /* PC via PCI */
- { "x86_64", "pc",
- {-1, 2, 0, {1, 0x057834b4} },
- .pci = { PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_SDHCI } },
-
- /* Exynos4210 */
- { "arm", "smdkc210",
- {0x12510000, 2, 0, {1, 0x5e80080} } },
-
- /* i.MX 6 */
- { "arm", "sabrelite",
- {0x02190000, 3, 0, {1, 0x057834b4} } },
-
- /* BCM2835 */
- { "arm", "raspi2",
- {0x3f300000, 3, 52, {0, 0x052134b4} } },
-
- /* Zynq-7000 */
- { "arm", "xilinx-zynq-a9", /* Datasheet: UG585 (v1.12.1) */
- {0xe0100000, 2, 0, {1, 0x69ec0080} } },
-
- /* ZynqMP */
- { "aarch64", "xlnx-zcu102", /* Datasheet: UG1085 (v1.7) */
- {0xff160000, 3, 0, {1, 0x280737ec6481} } },
-
-};
-
-typedef struct QSDHCI {
- struct {
- QPCIBus *bus;
- QPCIDevice *dev;
- } pci;
- union {
- QPCIBar mem_bar;
- uint64_t addr;
- };
-} QSDHCI;
-
-static uint16_t sdhci_readw(QSDHCI *s, uint32_t reg)
-{
- uint16_t val;
-
- if (s->pci.dev) {
- val = qpci_io_readw(s->pci.dev, s->mem_bar, reg);
- } else {
- val = qtest_readw(global_qtest, s->addr + reg);
+ uintptr_t addr;
+ uint8_t version;
+ uint8_t baseclock;
+ struct {
+ bool sdma;
+ uint64_t reg;
+ } capab;
+ } sdhci;
+ struct {
+ uint16_t vendor_id, device_id;
+ } pci;
}
+ *
+ * implemented drivers:
+ *
+ PC via PCI
+ { "x86_64", "pc",
+ {-1, 2, 0, {1, 0x057834b4} },
+ .pci = { PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_SDHCI } },
+
+ BCM2835
+ { "arm", "raspi2",
+ {0x3f300000, 3, 52, {0, 0x052134b4} } },
+ *
+ * FIXME: the following drivers are missing:
+ *
+ Exynos4210
+ { "arm", "smdkc210",
+ {0x12510000, 2, 0, {1, 0x5e80080} } },
- return val;
-}
-
-static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
-{
- uint64_t val;
-
- if (s->pci.dev) {
- val = qpci_io_readq(s->pci.dev, s->mem_bar, reg);
- } else {
- val = qtest_readq(global_qtest, s->addr + reg);
- }
+ i.MX 6
+ { "arm", "sabrelite",
+ {0x02190000, 3, 0, {1, 0x057834b4} } },
- return val;
-}
+ Zynq-7000
+ { "arm", "xilinx-zynq-a9", Datasheet: UG585 (v1.12.1)
+ {0xe0100000, 2, 0, {1, 0x69ec0080} } },
-static void sdhci_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
-{
- if (s->pci.dev) {
- qpci_io_writeq(s->pci.dev, s->mem_bar, reg, val);
- } else {
- qtest_writeq(global_qtest, s->addr + reg, val);
- }
-}
+ ZynqMP
+ { "aarch64", "xlnx-zcu102", Datasheet: UG1085 (v1.7)
+ {0xff160000, 3, 0, {1, 0x280737ec6481} } },
+ */
static void check_specs_version(QSDHCI *s, uint8_t version)
{
uint32_t v;
- v = sdhci_readw(s, SDHC_HCVER);
+ v = s->sdhci_readw(s, SDHC_HCVER);
v &= 0xff;
v += 1;
g_assert_cmpuint(v, ==, version);
@@ -122,7 +85,7 @@ static void check_capab_capareg(QSDHCI *s, uint64_t expec_capab)
{
uint64_t capab;
- capab = sdhci_readq(s, SDHC_CAPAB);
+ capab = s->sdhci_readq(s, SDHC_CAPAB);
g_assert_cmphex(capab, ==, expec_capab);
}
@@ -131,11 +94,11 @@ static void check_capab_readonly(QSDHCI *s)
const uint64_t vrand = 0x123456789abcdef;
uint64_t capab0, capab1;
- capab0 = sdhci_readq(s, SDHC_CAPAB);
+ capab0 = s->sdhci_readq(s, SDHC_CAPAB);
g_assert_cmpuint(capab0, !=, vrand);
- sdhci_writeq(s, SDHC_CAPAB, vrand);
- capab1 = sdhci_readq(s, SDHC_CAPAB);
+ s->sdhci_writeq(s, SDHC_CAPAB, vrand);
+ capab1 = s->sdhci_readq(s, SDHC_CAPAB);
g_assert_cmpuint(capab1, !=, vrand);
g_assert_cmpuint(capab1, ==, capab0);
}
@@ -147,7 +110,7 @@ static void check_capab_baseclock(QSDHCI *s, uint8_t expec_freq)
if (!expec_freq) {
return;
}
- capab = sdhci_readq(s, SDHC_CAPAB);
+ capab = s->sdhci_readq(s, SDHC_CAPAB);
capab_freq = FIELD_EX64(capab, SDHC_CAPAB, BASECLKFREQ);
g_assert_cmpuint(capab_freq, ==, expec_freq);
}
@@ -156,7 +119,7 @@ static void check_capab_sdma(QSDHCI *s, bool supported)
{
uint64_t capab, capab_sdma;
- capab = sdhci_readq(s, SDHC_CAPAB);
+ capab = s->sdhci_readq(s, SDHC_CAPAB);
capab_sdma = FIELD_EX64(capab, SDHC_CAPAB, SDMA);
g_assert_cmpuint(capab_sdma, ==, supported);
}
@@ -167,7 +130,7 @@ static void check_capab_v3(QSDHCI *s, uint8_t version)
if (version < 3) {
/* before v3 those fields are RESERVED */
- capab = sdhci_readq(s, SDHC_CAPAB);
+ capab = s->sdhci_readq(s, SDHC_CAPAB);
capab_v3 = FIELD_EX64(capab, SDHC_CAPAB, SDR);
g_assert_cmpuint(capab_v3, ==, 0);
capab_v3 = FIELD_EX64(capab, SDHC_CAPAB, DRIVER);
@@ -175,78 +138,21 @@ static void check_capab_v3(QSDHCI *s, uint8_t version)
}
}
-static QSDHCI *machine_start(const struct sdhci_t *test)
-{
- QSDHCI *s = g_new0(QSDHCI, 1);
-
- if (test->pci.vendor_id) {
- /* PCI */
- uint16_t vendor_id, device_id;
- uint64_t barsize;
-
- global_qtest = qtest_startf("-machine %s -device sdhci-pci",
- test->machine);
-
- s->pci.bus = qpci_init_pc(global_qtest, NULL);
-
- /* Find PCI device and verify it's the right one */
- s->pci.dev = qpci_device_find(s->pci.bus, QPCI_DEVFN(4, 0));
- g_assert_nonnull(s->pci.dev);
- vendor_id = qpci_config_readw(s->pci.dev, PCI_VENDOR_ID);
- device_id = qpci_config_readw(s->pci.dev, PCI_DEVICE_ID);
- g_assert(vendor_id == test->pci.vendor_id);
- g_assert(device_id == test->pci.device_id);
- s->mem_bar = qpci_iomap(s->pci.dev, 0, &barsize);
- qpci_device_enable(s->pci.dev);
- } else {
- /* SysBus */
- global_qtest = qtest_startf("-machine %s", test->machine);
- s->addr = test->sdhci.addr;
- }
-
- return s;
-}
-
-static void machine_stop(QSDHCI *s)
+static void test_machine(void *obj, void *data)
{
- qpci_free_pc(s->pci.bus);
- g_free(s->pci.dev);
- qtest_quit(global_qtest);
- g_free(s);
-}
+ QSDHCI *s = (QSDHCI *)obj;
-static void test_machine(const void *data)
-{
- const struct sdhci_t *test = data;
- QSDHCI *s;
-
- s = machine_start(test);
-
- check_specs_version(s, test->sdhci.version);
- check_capab_capareg(s, test->sdhci.capab.reg);
+ check_specs_version(s, s->props.version);
+ check_capab_capareg(s, s->props.capab.reg);
check_capab_readonly(s);
- check_capab_v3(s, test->sdhci.version);
- check_capab_sdma(s, test->sdhci.capab.sdma);
- check_capab_baseclock(s, test->sdhci.baseclock);
-
- machine_stop(s);
+ check_capab_v3(s, s->props.version);
+ check_capab_sdma(s, s->props.capab.sdma);
+ check_capab_baseclock(s, s->props.baseclock);
}
-int main(int argc, char *argv[])
+static void sdhci_test(void)
{
- const char *arch = qtest_get_arch();
- char *name;
- int i;
-
- g_test_init(&argc, &argv, NULL);
- for (i = 0; i < ARRAY_SIZE(models); i++) {
- if (strcmp(arch, models[i].arch)) {
- continue;
- }
- name = g_strdup_printf("sdhci/%s", models[i].machine);
- qtest_add_data_func(name, &models[i], test_machine);
- g_free(name);
- }
-
- return g_test_run();
+ qos_add_test("sdhci-test", "sdhci", test_machine);
}
+
+libqos_init(sdhci_test);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node
2018-07-09 9:11 ` [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node Emanuele Giuseppe Esposito
@ 2018-07-11 15:15 ` Stefan Hajnoczi
2018-07-11 17:52 ` Emanuele
0 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 15:15 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 416 bytes --]
On Mon, Jul 09, 2018 at 11:11:36AM +0200, Emanuele Giuseppe Esposito wrote:
> +/**
> + * Old sdhci_t structure:
Do you intend to delete this comment before this series is merged? It
seems like a TODO that doesn't need to be kept around.
> + qos_add_test("sdhci-test", "sdhci", test_machine);
How does this work for tests that need access to more than 1 device?
Can they request a driver instance via the API?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node
2018-07-11 15:15 ` Stefan Hajnoczi
@ 2018-07-11 17:52 ` Emanuele
2018-07-12 12:07 ` Paolo Bonzini
0 siblings, 1 reply; 39+ messages in thread
From: Emanuele @ 2018-07-11 17:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/11/2018 05:15 PM, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:36AM +0200, Emanuele Giuseppe Esposito wrote:
>> +/**
>> + * Old sdhci_t structure:
> Do you intend to delete this comment before this series is merged? It
> seems like a TODO that doesn't need to be kept around.
Paolo suggested me to put it there, because there still are some devices
that need to be implemented.
>
>> + qos_add_test("sdhci-test", "sdhci", test_machine);
> How does this work for tests that need access to more than 1 device?
> Can they request a driver instance via the API?
Uhm accessing more than one device is something I did not take into
account. It is possible to add additional command line to the test, with
qos_add_test_args(), but no multiple devices. I'll add it as TODO in my
list, thanks
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node
2018-07-11 17:52 ` Emanuele
@ 2018-07-12 12:07 ` Paolo Bonzini
0 siblings, 0 replies; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-12 12:07 UTC (permalink / raw)
To: Emanuele, Stefan Hajnoczi
Cc: Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 11/07/2018 19:52, Emanuele wrote:
>>>
>> Do you intend to delete this comment before this series is merged? It
>> seems like a TODO that doesn't need to be kept around.
> Paolo suggested me to put it there, because there still are some devices
> that need to be implemented.
>>
>>> + qos_add_test("sdhci-test", "sdhci", test_machine);
>> How does this work for tests that need access to more than 1 device?
>> Can they request a driver instance via the API?
>
> Uhm accessing more than one device is something I did not take into
> account. It is possible to add additional command line to the test, with
> qos_add_test_args(), but no multiple devices. I'll add it as TODO in my
> list, thanks
Note that there is only one test that requires multiple devices on the
same bus, namely usb-hcd-ehci-test.c which instantiates the EHCI
controller under test together with three companion UHCI controllers.
One possibility is to define a driver which contains all of the devices
you need in the test, and make the test depend on the driver. Maybe
there is a better way to do it, the above is just what came to my mind.
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
` (6 preceding siblings ...)
2018-07-09 9:11 ` [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node Emanuele Giuseppe Esposito
@ 2018-07-11 14:00 ` Stefan Hajnoczi
2018-07-11 14:17 ` Emanuele
2018-07-11 15:27 ` Stefan Hajnoczi
8 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 14:00 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 695 bytes --]
On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
> Basic framework steps are the following:
> - All nodes and edges are created in their respective machine/driver/test files
> - The framework starts QEMU and asks for a list of available drivers
> and machines
QEMU doesn't know about qtest drivers. Does "drivers" mean "devices"
(i.e. qemu -device \?) and they have a 1:1 correspondence with qtest
drivers?
The concept of qgraph makes sense. Ease of use and documentation will
be critical to gain adoption. The extra complexity makes it hard for
people writing and running tests, so it's important that it's clear
which tests will get run and why (or why not).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework
2018-07-11 14:00 ` [Qemu-devel] [PATCH 0/7] Qtest driver framework Stefan Hajnoczi
@ 2018-07-11 14:17 ` Emanuele
0 siblings, 0 replies; 39+ messages in thread
From: Emanuele @ 2018-07-11 14:17 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
On 07/11/2018 04:00 PM, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
>> Basic framework steps are the following:
>> - All nodes and edges are created in their respective machine/driver/test files
>> - The framework starts QEMU and asks for a list of available drivers
>> and machines
> QEMU doesn't know about qtest drivers. Does "drivers" mean "devices"
> (i.e. qemu -device \?) and they have a 1:1 correspondence with qtest
> drivers?
Yes, they are QEMU devices, it was a typo.
Not all qtest drivers are mapped with the QEMU devices, though. In fact,
for now all drivers "contained" or "produced" by other nodes are not
QEMU devices.
The others (machines and nodes that "consume"), are mapped to QEMU using
the
{ "execute": "qom-list-types", "arguments" : { "implements" : "device" } }
for "consume" and
{ "execute": "query-machines" }
for machines.
With "consume" I mean in situation like "node X" ---> consumes --> "node
Y",
"X" should be in the output of qom-list-types query.
> The concept of qgraph makes sense. Ease of use and documentation will
> be critical to gain adoption. The extra complexity makes it hard for
> people writing and running tests, so it's important that it's clear
> which tests will get run and why (or why not).
I am trying to document and makes it as easy and clean as possible :)
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework
2018-07-09 9:11 [Qemu-devel] [PATCH 0/7] Qtest driver framework Emanuele Giuseppe Esposito
` (7 preceding siblings ...)
2018-07-11 14:00 ` [Qemu-devel] [PATCH 0/7] Qtest driver framework Stefan Hajnoczi
@ 2018-07-11 15:27 ` Stefan Hajnoczi
2018-07-18 17:14 ` Markus Armbruster
8 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2018-07-11 15:27 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Paolo Bonzini, Laurent Vivier, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 871 bytes --]
On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
> This work is being done as Google Summer of Code 2018 project for QEMU,
> my mentors are Paolo Bonzini and Laurent Vivier.
> Additional infos on the project can be found at:
> https://wiki.qemu.org/Features/qtest_driver_framework
Thanks, I've finished reviewing this version. It looks like a good
start!
The main challenge to me seems "how can we make tests simpler?". The
presence of a new API and object model raises the bar for writing and
running tests. I hope all qtests will use qgraph but if the complexity
is too high then qgraph may only be used in a few niche cases where
authors think it's worth abstracting machine types and others will
continue to write qtests without qgraph.
What are the next steps and do you have plans for making qgraph more
integral to libqos?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework
2018-07-11 15:27 ` Stefan Hajnoczi
@ 2018-07-18 17:14 ` Markus Armbruster
2018-07-18 19:35 ` Paolo Bonzini
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2018-07-18 17:14 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Emanuele Giuseppe Esposito, Laurent Vivier, Paolo Bonzini,
Philippe Mathieu-Daudé,
qemu-devel
Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
>> This work is being done as Google Summer of Code 2018 project for QEMU,
>> my mentors are Paolo Bonzini and Laurent Vivier.
>> Additional infos on the project can be found at:
>> https://wiki.qemu.org/Features/qtest_driver_framework
>
> Thanks, I've finished reviewing this version. It looks like a good
> start!
>
> The main challenge to me seems "how can we make tests simpler?". The
> presence of a new API and object model raises the bar for writing and
> running tests. I hope all qtests will use qgraph but if the complexity
> is too high then qgraph may only be used in a few niche cases where
> authors think it's worth abstracting machine types and others will
> continue to write qtests without qgraph.
The #1 reason for writing code a certain way is following existing
examples. As long as "qtest without qgraph" is prevalent in existing
tests, it'll remain prevalent in new tests. I'm afraid we'll need a
conversion effort.
> What are the next steps and do you have plans for making qgraph more
> integral to libqos?
Good questions.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework
2018-07-18 17:14 ` Markus Armbruster
@ 2018-07-18 19:35 ` Paolo Bonzini
0 siblings, 0 replies; 39+ messages in thread
From: Paolo Bonzini @ 2018-07-18 19:35 UTC (permalink / raw)
To: Markus Armbruster, Stefan Hajnoczi
Cc: Laurent Vivier, Emanuele Giuseppe Esposito,
Philippe Mathieu-Daudé,
qemu-devel
On 18/07/2018 19:14, Markus Armbruster wrote:
>> The main challenge to me seems "how can we make tests simpler?". The
>> presence of a new API and object model raises the bar for writing and
>> running tests. I hope all qtests will use qgraph but if the complexity
>> is too high then qgraph may only be used in a few niche cases where
>> authors think it's worth abstracting machine types and others will
>> continue to write qtests without qgraph.
> The #1 reason for writing code a certain way is following existing
> examples. As long as "qtest without qgraph" is prevalent in existing
> tests, it'll remain prevalent in new tests. I'm afraid we'll need a
> conversion effort.
>
Indeed, Emanuele is already working on converting virtio-serial tests
(virtio is possibly the largest part of libqos). The various nop tests
are also more or less trivial to adjust.
Paolo
^ permalink raw reply [flat|nested] 39+ messages in thread