* [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction
2019-09-11 14:36 [Qemu-devel] [PATCH 0/3] xen: fix a potential crash in xen-bus Paul Durrant
@ 2019-09-11 14:36 ` Paul Durrant
2019-09-12 10:16 ` Anthony PERARD
2019-09-11 14:36 ` [Qemu-devel] [PATCH 2/3] xen: introduce separate XenWatchList for XenDevice objects Paul Durrant
2019-09-11 14:36 ` [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler Paul Durrant
2 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2019-09-11 14:36 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Anthony Perard, Paul Durrant, Stefano Stabellini
Xenstore watch call-backs are already abstracted away from XenBus using
the XenWatch data structure but the associated NotifierList manipulation
and file handle registation is still open coded in various xen_bus_...()
functions.
This patch creates a new XenWatchList data structure to allow these
interactions to be abstracted away from XenBus as well. This is in
preparation for a subsequent patch which will introduce separate watch lists
for XenBus and XenDevice objects.
NOTE: This patch also introduces a new NotifierListEmpty() helper function
for the purposes of adding an assertion that a XenWatchList is not
freed whilst its associated NotifierList is still occupied.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
hw/xen/trace-events | 5 +-
hw/xen/xen-bus.c | 117 +++++++++++++++++++++++++--------------
include/hw/xen/xen-bus.h | 3 +-
include/qemu/notify.h | 2 +
util/notify.c | 5 ++
5 files changed, 87 insertions(+), 45 deletions(-)
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index bc82ecb1a5..ac8d9c20d2 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -19,9 +19,8 @@ xen_bus_unrealize(void) ""
xen_bus_enumerate(void) ""
xen_bus_type_enumerate(const char *type) "type: %s"
xen_bus_backend_create(const char *type, const char *path) "type: %s path: %s"
-xen_bus_add_watch(const char *node, const char *key, char *token) "node: %s key: %s token: %s"
-xen_bus_remove_watch(const char *node, const char *key, char *token) "node: %s key: %s token: %s"
-xen_bus_watch(const char *token) "token: %s"
+xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
+xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
xen_device_realize(const char *type, char *name) "type: %s name: %s"
xen_device_unrealize(const char *type, char *name) "type: %s name: %s"
xen_device_backend_state(const char *type, char *name, const char *state) "type: %s name: %s -> %s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 025df5e59f..28efaccff2 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -157,18 +157,60 @@ static void free_watch(XenWatch *watch)
g_free(watch);
}
-static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
- const char *key, XenWatchHandler handler,
- void *opaque, Error **errp)
+struct XenWatchList {
+ struct xs_handle *xsh;
+ NotifierList notifiers;
+};
+
+static void watch_list_event(void *opaque)
+{
+ XenWatchList *watch_list = opaque;
+ char **v;
+ const char *token;
+
+ v = xs_check_watch(watch_list->xsh);
+ if (!v) {
+ return;
+ }
+
+ token = v[XS_WATCH_TOKEN];
+
+ notifier_list_notify(&watch_list->notifiers, (void *)token);
+
+ free(v);
+}
+
+static XenWatchList *watch_list_create(struct xs_handle *xsh)
+{
+ XenWatchList *watch_list = g_new0(XenWatchList, 1);
+
+ g_assert(xsh);
+
+ watch_list->xsh = xsh;
+ notifier_list_init(&watch_list->notifiers);
+ qemu_set_fd_handler(xs_fileno(watch_list->xsh), watch_list_event, NULL,
+ watch_list);
+
+ return watch_list;
+}
+
+static void watch_list_destroy(XenWatchList *watch_list)
+{
+ g_assert(notifier_list_empty(&watch_list->notifiers));
+ qemu_set_fd_handler(xs_fileno(watch_list->xsh), NULL, NULL, NULL);
+ g_free(watch_list);
+}
+
+static XenWatch *watch_list_add(XenWatchList *watch_list, const char *node,
+ const char *key, XenWatchHandler handler,
+ void *opaque, Error **errp)
{
XenWatch *watch = new_watch(node, key, handler, opaque);
Error *local_err = NULL;
- trace_xen_bus_add_watch(watch->node, watch->key, watch->token);
-
- notifier_list_add(&xenbus->watch_notifiers, &watch->notifier);
+ notifier_list_add(&watch_list->notifiers, &watch->notifier);
- xs_node_watch(xenbus->xsh, node, key, watch->token, &local_err);
+ xs_node_watch(watch_list->xsh, node, key, watch->token, &local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -181,18 +223,34 @@ static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
return watch;
}
-static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
- Error **errp)
+static void watch_list_remove(XenWatchList *watch_list, XenWatch *watch,
+ Error **errp)
{
- trace_xen_bus_remove_watch(watch->node, watch->key, watch->token);
-
- xs_node_unwatch(xenbus->xsh, watch->node, watch->key, watch->token,
+ xs_node_unwatch(watch_list->xsh, watch->node, watch->key, watch->token,
errp);
notifier_remove(&watch->notifier);
free_watch(watch);
}
+static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
+ const char *key, XenWatchHandler handler,
+ void *opaque, Error **errp)
+{
+ trace_xen_bus_add_watch(node, key);
+
+ return watch_list_add(xenbus->watch_list, node, key, handler, opaque,
+ errp);
+}
+
+static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
+ Error **errp)
+{
+ trace_xen_bus_remove_watch(watch->node, watch->key);
+
+ watch_list_remove(xenbus->watch_list, watch, errp);
+}
+
static void xen_bus_backend_create(XenBus *xenbus, const char *type,
const char *name, char *path,
Error **errp)
@@ -338,35 +396,14 @@ static void xen_bus_unrealize(BusState *bus, Error **errp)
xenbus->backend_watch = NULL;
}
- if (!xenbus->xsh) {
- return;
+ if (xenbus->watch_list) {
+ watch_list_destroy(xenbus->watch_list);
+ xenbus->watch_list = NULL;
}
- qemu_set_fd_handler(xs_fileno(xenbus->xsh), NULL, NULL, NULL);
-
- xs_close(xenbus->xsh);
-}
-
-static void xen_bus_watch(void *opaque)
-{
- XenBus *xenbus = opaque;
- char **v;
- const char *token;
-
- g_assert(xenbus->xsh);
-
- v = xs_check_watch(xenbus->xsh);
- if (!v) {
- return;
+ if (xenbus->xsh) {
+ xs_close(xenbus->xsh);
}
-
- token = v[XS_WATCH_TOKEN];
-
- trace_xen_bus_watch(token);
-
- notifier_list_notify(&xenbus->watch_notifiers, (void *)token);
-
- free(v);
}
static void xen_bus_realize(BusState *bus, Error **errp)
@@ -390,9 +427,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
xenbus->backend_id = 0; /* Assume lack of node means dom0 */
}
- notifier_list_init(&xenbus->watch_notifiers);
- qemu_set_fd_handler(xs_fileno(xenbus->xsh), xen_bus_watch, NULL,
- xenbus);
+ xenbus->watch_list = watch_list_create(xenbus->xsh);
module_call_init(MODULE_INIT_XEN_BACKEND);
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 1c2d9dfdb8..88b84e29bb 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -14,6 +14,7 @@
typedef void (*XenWatchHandler)(void *opaque);
+typedef struct XenWatchList XenWatchList;
typedef struct XenWatch XenWatch;
typedef struct XenEventChannel XenEventChannel;
@@ -63,7 +64,7 @@ typedef struct XenBus {
BusState qbus;
domid_t backend_id;
struct xs_handle *xsh;
- NotifierList watch_notifiers;
+ XenWatchList *watch_list;
XenWatch *backend_watch;
} XenBus;
diff --git a/include/qemu/notify.h b/include/qemu/notify.h
index a3d73e4bc7..bcfa70fb2e 100644
--- a/include/qemu/notify.h
+++ b/include/qemu/notify.h
@@ -40,6 +40,8 @@ void notifier_remove(Notifier *notifier);
void notifier_list_notify(NotifierList *list, void *data);
+bool notifier_list_empty(NotifierList *list);
+
/* Same as Notifier but allows .notify() to return errors */
typedef struct NotifierWithReturn NotifierWithReturn;
diff --git a/util/notify.c b/util/notify.c
index aee8d93cb0..76bab212ae 100644
--- a/util/notify.c
+++ b/util/notify.c
@@ -40,6 +40,11 @@ void notifier_list_notify(NotifierList *list, void *data)
}
}
+bool notifier_list_empty(NotifierList *list)
+{
+ return QLIST_EMPTY(&list->notifiers);
+}
+
void notifier_with_return_list_init(NotifierWithReturnList *list)
{
QLIST_INIT(&list->notifiers);
--
2.20.1.2.gb21ebb6
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction
2019-09-11 14:36 ` [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction Paul Durrant
@ 2019-09-12 10:16 ` Anthony PERARD
2019-09-12 11:40 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Anthony PERARD @ 2019-09-12 10:16 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Stefano Stabellini, qemu-devel
On Wed, Sep 11, 2019 at 03:36:16PM +0100, Paul Durrant wrote:
> Xenstore watch call-backs are already abstracted away from XenBus using
> the XenWatch data structure but the associated NotifierList manipulation
> and file handle registation is still open coded in various xen_bus_...()
^ registration
> functions.
> This patch creates a new XenWatchList data structure to allow these
> interactions to be abstracted away from XenBus as well. This is in
> preparation for a subsequent patch which will introduce separate watch lists
> for XenBus and XenDevice objects.
>
> NOTE: This patch also introduces a new NotifierListEmpty() helper function
^ notifier_list_empty() ?
> for the purposes of adding an assertion that a XenWatchList is not
> freed whilst its associated NotifierList is still occupied.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
--
Anthony PERARD
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction
2019-09-12 10:16 ` Anthony PERARD
@ 2019-09-12 11:40 ` Paul Durrant
0 siblings, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2019-09-12 11:40 UTC (permalink / raw)
To: Anthony Perard; +Cc: xen-devel, Stefano Stabellini, qemu-devel
> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 12 September 2019 11:17
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction
>
> On Wed, Sep 11, 2019 at 03:36:16PM +0100, Paul Durrant wrote:
> > Xenstore watch call-backs are already abstracted away from XenBus using
> > the XenWatch data structure but the associated NotifierList manipulation
> > and file handle registation is still open coded in various xen_bus_...()
> ^ registration
Ok.
> > functions.
> > This patch creates a new XenWatchList data structure to allow these
> > interactions to be abstracted away from XenBus as well. This is in
> > preparation for a subsequent patch which will introduce separate watch lists
> > for XenBus and XenDevice objects.
> >
> > NOTE: This patch also introduces a new NotifierListEmpty() helper function
> ^ notifier_list_empty() ?
>
Oops, yes :-)
> > for the purposes of adding an assertion that a XenWatchList is not
> > freed whilst its associated NotifierList is still occupied.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>
Thanks,
Paul
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/3] xen: introduce separate XenWatchList for XenDevice objects
2019-09-11 14:36 [Qemu-devel] [PATCH 0/3] xen: fix a potential crash in xen-bus Paul Durrant
2019-09-11 14:36 ` [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction Paul Durrant
@ 2019-09-11 14:36 ` Paul Durrant
2019-09-12 11:27 ` Anthony PERARD
2019-09-11 14:36 ` [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler Paul Durrant
2 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2019-09-11 14:36 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Anthony Perard, Paul Durrant, Stefano Stabellini
This patch uses the XenWatchList abstraction to add a separate watch list
for each device. This is more scalable than walking a single notifier
list for all watches and is also necessary to implement a bug-fix in a
subsequent patch.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
hw/xen/trace-events | 2 ++
hw/xen/xen-bus.c | 72 ++++++++++++++++++++++++++++++++--------
include/hw/xen/xen-bus.h | 2 ++
3 files changed, 62 insertions(+), 14 deletions(-)
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index ac8d9c20d2..80ce3dafad 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -29,6 +29,8 @@ xen_device_backend_changed(const char *type, char *name) "type: %s name: %s"
xen_device_frontend_state(const char *type, char *name, const char *state) "type: %s name: %s -> %s"
xen_device_frontend_changed(const char *type, char *name) "type: %s name: %s"
xen_device_unplug(const char *type, char *name) "type: %s name: %s"
+xen_device_add_watch(const char *type, char *name, const char *node, const char *key) "type: %s name: %s node: %s key: %s"
+xen_device_remove_watch(const char *type, char *name, const char *node, const char *key) "type: %s name: %s node: %s key: %s"
# xen-bus-helper.c
xs_node_create(const char *node) "%s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 28efaccff2..810a4e2df3 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -235,11 +235,11 @@ static void watch_list_remove(XenWatchList *watch_list, XenWatch *watch,
static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
const char *key, XenWatchHandler handler,
- void *opaque, Error **errp)
+ Error **errp)
{
trace_xen_bus_add_watch(node, key);
- return watch_list_add(xenbus->watch_list, node, key, handler, opaque,
+ return watch_list_add(xenbus->watch_list, node, key, handler, xenbus,
errp);
}
@@ -433,7 +433,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
xenbus->backend_watch =
xen_bus_add_watch(xenbus, "", /* domain root node */
- "backend", xen_bus_enumerate, xenbus, &local_err);
+ "backend", xen_bus_enumerate, &local_err);
if (local_err) {
/* This need not be treated as a hard error so don't propagate */
error_reportf_err(local_err,
@@ -621,6 +621,31 @@ static void xen_device_backend_changed(void *opaque)
}
}
+static XenWatch *xen_device_add_watch(XenDevice *xendev, const char *node,
+ const char *key,
+ XenWatchHandler handler,
+ Error **errp)
+{
+ const char *type = object_get_typename(OBJECT(xendev));
+
+ trace_xen_device_add_watch(type, xendev->name, node, key);
+
+ return watch_list_add(xendev->watch_list, node, key, handler, xendev,
+ errp);
+}
+
+static void xen_device_remove_watch(XenDevice *xendev, XenWatch *watch,
+ Error **errp)
+{
+ const char *type = object_get_typename(OBJECT(xendev));
+
+ trace_xen_device_remove_watch(type, xendev->name, watch->node,
+ watch->key);
+
+ watch_list_remove(xendev->watch_list, watch, errp);
+}
+
+
static void xen_device_backend_create(XenDevice *xendev, Error **errp)
{
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
@@ -645,9 +670,9 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
}
xendev->backend_state_watch =
- xen_bus_add_watch(xenbus, xendev->backend_path,
- "state", xen_device_backend_changed,
- xendev, &local_err);
+ xen_device_add_watch(xendev, xendev->backend_path,
+ "state", xen_device_backend_changed,
+ &local_err);
if (local_err) {
error_propagate_prepend(errp, local_err,
"failed to watch backend state: ");
@@ -655,9 +680,9 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
}
xendev->backend_online_watch =
- xen_bus_add_watch(xenbus, xendev->backend_path,
- "online", xen_device_backend_changed,
- xendev, &local_err);
+ xen_device_add_watch(xendev, xendev->backend_path,
+ "online", xen_device_backend_changed,
+ &local_err);
if (local_err) {
error_propagate_prepend(errp, local_err,
"failed to watch backend online: ");
@@ -671,12 +696,12 @@ static void xen_device_backend_destroy(XenDevice *xendev)
Error *local_err = NULL;
if (xendev->backend_online_watch) {
- xen_bus_remove_watch(xenbus, xendev->backend_online_watch, NULL);
+ xen_device_remove_watch(xendev, xendev->backend_online_watch, NULL);
xendev->backend_online_watch = NULL;
}
if (xendev->backend_state_watch) {
- xen_bus_remove_watch(xenbus, xendev->backend_state_watch, NULL);
+ xen_device_remove_watch(xendev, xendev->backend_state_watch, NULL);
xendev->backend_state_watch = NULL;
}
@@ -812,8 +837,8 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
}
xendev->frontend_state_watch =
- xen_bus_add_watch(xenbus, xendev->frontend_path, "state",
- xen_device_frontend_changed, xendev, &local_err);
+ xen_device_add_watch(xendev, xendev->frontend_path, "state",
+ xen_device_frontend_changed, &local_err);
if (local_err) {
error_propagate_prepend(errp, local_err,
"failed to watch frontend state: ");
@@ -826,7 +851,8 @@ static void xen_device_frontend_destroy(XenDevice *xendev)
Error *local_err = NULL;
if (xendev->frontend_state_watch) {
- xen_bus_remove_watch(xenbus, xendev->frontend_state_watch, NULL);
+ xen_device_remove_watch(xendev, xendev->frontend_state_watch,
+ NULL);
xendev->frontend_state_watch = NULL;
}
@@ -1122,6 +1148,16 @@ static void xen_device_unrealize(DeviceState *dev, Error **errp)
xendev->xgth = NULL;
}
+ if (xendev->watch_list) {
+ watch_list_destroy(xendev->watch_list);
+ xendev->watch_list = NULL;
+ }
+
+ if (xendev->xsh) {
+ xs_close(xendev->xsh);
+ xendev->xsh = NULL;
+ }
+
g_free(xendev->name);
xendev->name = NULL;
}
@@ -1164,6 +1200,14 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
trace_xen_device_realize(type, xendev->name);
+ xendev->xsh = xs_open(0);
+ if (!xendev->xsh) {
+ error_setg_errno(errp, errno, "failed xs_open");
+ goto unrealize;
+ }
+
+ xendev->watch_list = watch_list_create(xendev->xsh);
+
xendev->xgth = xengnttab_open(NULL, 0);
if (!xendev->xgth) {
error_setg_errno(errp, errno, "failed xengnttab_open");
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 88b84e29bb..0d198148f6 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -22,6 +22,8 @@ typedef struct XenDevice {
DeviceState qdev;
domid_t frontend_id;
char *name;
+ struct xs_handle *xsh;
+ XenWatchList *watch_list;
char *backend_path, *frontend_path;
enum xenbus_state backend_state, frontend_state;
Notifier exit;
--
2.20.1.2.gb21ebb6
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler
2019-09-11 14:36 [Qemu-devel] [PATCH 0/3] xen: fix a potential crash in xen-bus Paul Durrant
2019-09-11 14:36 ` [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction Paul Durrant
2019-09-11 14:36 ` [Qemu-devel] [PATCH 2/3] xen: introduce separate XenWatchList for XenDevice objects Paul Durrant
@ 2019-09-11 14:36 ` Paul Durrant
2019-09-11 16:15 ` Paul Durrant
2 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2019-09-11 14:36 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Anthony Perard, Paul Durrant, Stefano Stabellini
Cleaning up offine XenDevice objects directly in
xen_device_backend_changed() is dangerous as xen_device_unrealize() will
modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
used in notifier_list_notify() is insufficient as *two* notifiers (for
the frontend and backend watches) are removed, thus potentially rendering
the 'next' pointer unsafe.
The solution is to use the XenBus backend_watch handler to do the clean-up
instead, as it is invoked whilst walking a separate watch list.
This patch therefore adds a new 'offline_devices' list to XenBus, to which
offline devices are added by xen_device_backend_changed(). The XenBus
backend_watch registration is also changed to not only invoke
xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
walk 'offline_devices' and perform the necessary actions.
For safety a an extra 'online' check is also added to
xen_bus_type_enumerate() to make sure that no attempt is made to create a
new XenDevice object for a backend that is offline.
NOTE: This patch also include some cosmetic changes:
- substitute the local variable name 'backend_state'
in xen_bus_type_enumerate() with 'state', since there
is no ambiguity with any other state in that context.
- change xen_device_state_is_active() to
xen_device_frontend_is_active() (and pass a XenDevice directly)
since the state tests contained therein only apply to a frontend.
- use 'state' rather then 'xendev->backend_state' in
xen_device_backend_changed() to shorten the code.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
hw/xen/trace-events | 2 +
hw/xen/xen-bus.c | 91 +++++++++++++++++++++++++++++-----------
include/hw/xen/xen-bus.h | 2 +
3 files changed, 70 insertions(+), 25 deletions(-)
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 80ce3dafad..e6885bc751 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -17,8 +17,10 @@ xen_domid_restrict(int err) "err: %u"
xen_bus_realize(void) ""
xen_bus_unrealize(void) ""
xen_bus_enumerate(void) ""
+xen_bus_cleanup(void) ""
xen_bus_type_enumerate(const char *type) "type: %s"
xen_bus_backend_create(const char *type, const char *path) "type: %s path: %s"
+xen_bus_device_cleanup(const char *type, char *name) "type: %s name: %s"
xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
xen_device_realize(const char *type, char *name) "type: %s name: %s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 810a4e2df3..055beb7260 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -340,13 +340,18 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
for (i = 0; i < n; i++) {
char *backend_path = g_strdup_printf("%s/%s", domain_path,
backend[i]);
- enum xenbus_state backend_state;
+ enum xenbus_state state;
+ unsigned int online;
if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "state",
- NULL, "%u", &backend_state) != 1)
- backend_state = XenbusStateUnknown;
+ NULL, "%u", &state) != 1)
+ state = XenbusStateUnknown;
- if (backend_state == XenbusStateInitialising) {
+ if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "online",
+ NULL, "%u", &online) != 1)
+ online = 0;
+
+ if (online && state == XenbusStateInitialising) {
Error *local_err = NULL;
xen_bus_backend_create(xenbus, type, backend[i], backend_path,
@@ -365,9 +370,8 @@ out:
g_free(domain_path);
}
-static void xen_bus_enumerate(void *opaque)
+static void xen_bus_enumerate(XenBus *xenbus)
{
- XenBus *xenbus = opaque;
char **type;
unsigned int i, n;
@@ -385,6 +389,44 @@ static void xen_bus_enumerate(void *opaque)
free(type);
}
+static void xen_bus_device_cleanup(XenDevice *xendev)
+{
+ const char *type = object_get_typename(OBJECT(xendev));
+ Error *local_err = NULL;
+
+ trace_xen_bus_device_cleanup(type, xendev->name);
+
+ g_assert(!xendev->backend_online);
+
+ if (!xen_backend_try_device_destroy(xendev, &local_err)) {
+ object_unparent(OBJECT(xendev));
+ }
+
+ if (local_err) {
+ error_report_err(local_err);
+ }
+}
+
+static void xen_bus_cleanup(XenBus *xenbus)
+{
+ XenDevice *xendev, *next;
+
+ trace_xen_bus_cleanup();
+
+ QLIST_FOREACH_SAFE(xendev, &xenbus->offline_devices, list, next) {
+ QLIST_REMOVE(xendev, list);
+ xen_bus_device_cleanup(xendev);
+ }
+}
+
+static void xen_bus_backend_changed(void *opaque)
+{
+ XenBus *xenbus = opaque;
+
+ xen_bus_enumerate(xenbus);
+ xen_bus_cleanup(xenbus);
+}
+
static void xen_bus_unrealize(BusState *bus, Error **errp)
{
XenBus *xenbus = XEN_BUS(bus);
@@ -433,7 +475,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
xenbus->backend_watch =
xen_bus_add_watch(xenbus, "", /* domain root node */
- "backend", xen_bus_enumerate, &local_err);
+ "backend", xen_bus_backend_changed, &local_err);
if (local_err) {
/* This need not be treated as a hard error so don't propagate */
error_reportf_err(local_err,
@@ -555,9 +597,9 @@ static void xen_device_backend_set_online(XenDevice *xendev, bool online)
* Tell from the state whether the frontend is likely alive,
* i.e. it will react to a change of state of the backend.
*/
-static bool xen_device_state_is_active(enum xenbus_state state)
+static bool xen_device_frontend_is_active(XenDevice *xendev)
{
- switch (state) {
+ switch (xendev->frontend_state) {
case XenbusStateInitWait:
case XenbusStateInitialised:
case XenbusStateConnected:
@@ -594,30 +636,29 @@ static void xen_device_backend_changed(void *opaque)
* state to Closing, but there is no active frontend then set the
* backend state to Closed.
*/
- if (xendev->backend_state == XenbusStateClosing &&
- !xen_device_state_is_active(xendev->frontend_state)) {
+ if (state == XenbusStateClosing &&
+ !xen_device_frontend_is_active(xendev)) {
xen_device_backend_set_state(xendev, XenbusStateClosed);
}
/*
* If a backend is still 'online' then we should leave it alone but,
- * if a backend is not 'online', then the device should be destroyed
- * once the state is Closed.
+ * if a backend is not 'online', then the device is a candidate
+ * for destruction. Hence add it to the 'offline' list to be cleaned
+ * by xen_bus_cleanup().
*/
- if (!xendev->backend_online &&
- (xendev->backend_state == XenbusStateClosed ||
- xendev->backend_state == XenbusStateInitialising ||
- xendev->backend_state == XenbusStateInitWait ||
- xendev->backend_state == XenbusStateUnknown)) {
- Error *local_err = NULL;
+ if (!online &&
+ (state == XenbusStateClosed || state == XenbusStateInitialising ||
+ state == XenbusStateInitWait || state == XenbusStateUnknown)) {
+ XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
- if (!xen_backend_try_device_destroy(xendev, &local_err)) {
- object_unparent(OBJECT(xendev));
- }
+ QLIST_INSERT_HEAD(&xenbus->offline_devices, xendev, list);
- if (local_err) {
- error_report_err(local_err);
- }
+ /*
+ * Re-write the state to cause a XenBus backend_watch notification,
+ * resulting in a call to xen_bus_cleanup().
+ */
+ xen_device_backend_printf(xendev, "state", "%u", state);
}
}
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 0d198148f6..15d71aff70 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -33,6 +33,7 @@ typedef struct XenDevice {
xengnttab_handle *xgth;
bool feature_grant_copy;
QLIST_HEAD(, XenEventChannel) event_channels;
+ QLIST_ENTRY(XenDevice) list;
} XenDevice;
typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
@@ -68,6 +69,7 @@ typedef struct XenBus {
struct xs_handle *xsh;
XenWatchList *watch_list;
XenWatch *backend_watch;
+ QLIST_HEAD(, XenDevice) offline_devices;
} XenBus;
typedef struct XenBusClass {
--
2.20.1.2.gb21ebb6
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler
2019-09-11 14:36 ` [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler Paul Durrant
@ 2019-09-11 16:15 ` Paul Durrant
0 siblings, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2019-09-11 16:15 UTC (permalink / raw)
To: Paul Durrant, qemu-devel, xen-devel; +Cc: Anthony Perard, Stefano Stabellini
> -----Original Message-----
> From: Paul Durrant <paul.durrant@citrix.com>
> Sent: 11 September 2019 15:36
> To: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony
> Perard <anthony.perard@citrix.com>
> Subject: [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler
>
> Cleaning up offine XenDevice objects directly in
> xen_device_backend_changed() is dangerous as xen_device_unrealize() will
> modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
> used in notifier_list_notify() is insufficient as *two* notifiers (for
> the frontend and backend watches) are removed, thus potentially rendering
> the 'next' pointer unsafe.
>
> The solution is to use the XenBus backend_watch handler to do the clean-up
> instead, as it is invoked whilst walking a separate watch list.
>
> This patch therefore adds a new 'offline_devices' list to XenBus, to which
> offline devices are added by xen_device_backend_changed(). The XenBus
> backend_watch registration is also changed to not only invoke
> xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
> walk 'offline_devices' and perform the necessary actions.
> For safety a an extra 'online' check is also added to
> xen_bus_type_enumerate() to make sure that no attempt is made to create a
> new XenDevice object for a backend that is offline.
>
> NOTE: This patch also include some cosmetic changes:
> - substitute the local variable name 'backend_state'
> in xen_bus_type_enumerate() with 'state', since there
> is no ambiguity with any other state in that context.
> - change xen_device_state_is_active() to
> xen_device_frontend_is_active() (and pass a XenDevice directly)
> since the state tests contained therein only apply to a frontend.
> - use 'state' rather then 'xendev->backend_state' in
> xen_device_backend_changed() to shorten the code.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
> hw/xen/trace-events | 2 +
> hw/xen/xen-bus.c | 91 +++++++++++++++++++++++++++++-----------
> include/hw/xen/xen-bus.h | 2 +
> 3 files changed, 70 insertions(+), 25 deletions(-)
>
> diff --git a/hw/xen/trace-events b/hw/xen/trace-events
> index 80ce3dafad..e6885bc751 100644
> --- a/hw/xen/trace-events
> +++ b/hw/xen/trace-events
> @@ -17,8 +17,10 @@ xen_domid_restrict(int err) "err: %u"
> xen_bus_realize(void) ""
> xen_bus_unrealize(void) ""
> xen_bus_enumerate(void) ""
> +xen_bus_cleanup(void) ""
> xen_bus_type_enumerate(const char *type) "type: %s"
> xen_bus_backend_create(const char *type, const char *path) "type: %s path: %s"
> +xen_bus_device_cleanup(const char *type, char *name) "type: %s name: %s"
> xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
> xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
> xen_device_realize(const char *type, char *name) "type: %s name: %s"
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 810a4e2df3..055beb7260 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -340,13 +340,18 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
> for (i = 0; i < n; i++) {
> char *backend_path = g_strdup_printf("%s/%s", domain_path,
> backend[i]);
> - enum xenbus_state backend_state;
> + enum xenbus_state state;
> + unsigned int online;
>
> if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "state",
> - NULL, "%u", &backend_state) != 1)
> - backend_state = XenbusStateUnknown;
> + NULL, "%u", &state) != 1)
> + state = XenbusStateUnknown;
>
> - if (backend_state == XenbusStateInitialising) {
> + if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "online",
> + NULL, "%u", &online) != 1)
> + online = 0;
> +
> + if (online && state == XenbusStateInitialising) {
> Error *local_err = NULL;
>
> xen_bus_backend_create(xenbus, type, backend[i], backend_path,
> @@ -365,9 +370,8 @@ out:
> g_free(domain_path);
> }
>
> -static void xen_bus_enumerate(void *opaque)
> +static void xen_bus_enumerate(XenBus *xenbus)
> {
> - XenBus *xenbus = opaque;
> char **type;
> unsigned int i, n;
>
> @@ -385,6 +389,44 @@ static void xen_bus_enumerate(void *opaque)
> free(type);
> }
>
> +static void xen_bus_device_cleanup(XenDevice *xendev)
> +{
> + const char *type = object_get_typename(OBJECT(xendev));
> + Error *local_err = NULL;
> +
> + trace_xen_bus_device_cleanup(type, xendev->name);
> +
> + g_assert(!xendev->backend_online);
> +
> + if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> + object_unparent(OBJECT(xendev));
> + }
> +
> + if (local_err) {
> + error_report_err(local_err);
> + }
> +}
> +
> +static void xen_bus_cleanup(XenBus *xenbus)
> +{
> + XenDevice *xendev, *next;
> +
> + trace_xen_bus_cleanup();
> +
> + QLIST_FOREACH_SAFE(xendev, &xenbus->offline_devices, list, next) {
> + QLIST_REMOVE(xendev, list);
> + xen_bus_device_cleanup(xendev);
> + }
> +}
> +
> +static void xen_bus_backend_changed(void *opaque)
> +{
> + XenBus *xenbus = opaque;
> +
> + xen_bus_enumerate(xenbus);
> + xen_bus_cleanup(xenbus);
> +}
> +
> static void xen_bus_unrealize(BusState *bus, Error **errp)
> {
> XenBus *xenbus = XEN_BUS(bus);
> @@ -433,7 +475,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
>
> xenbus->backend_watch =
> xen_bus_add_watch(xenbus, "", /* domain root node */
> - "backend", xen_bus_enumerate, &local_err);
> + "backend", xen_bus_backend_changed, &local_err);
> if (local_err) {
> /* This need not be treated as a hard error so don't propagate */
> error_reportf_err(local_err,
> @@ -555,9 +597,9 @@ static void xen_device_backend_set_online(XenDevice *xendev, bool online)
> * Tell from the state whether the frontend is likely alive,
> * i.e. it will react to a change of state of the backend.
> */
> -static bool xen_device_state_is_active(enum xenbus_state state)
> +static bool xen_device_frontend_is_active(XenDevice *xendev)
> {
> - switch (state) {
> + switch (xendev->frontend_state) {
> case XenbusStateInitWait:
> case XenbusStateInitialised:
> case XenbusStateConnected:
> @@ -594,30 +636,29 @@ static void xen_device_backend_changed(void *opaque)
> * state to Closing, but there is no active frontend then set the
> * backend state to Closed.
> */
> - if (xendev->backend_state == XenbusStateClosing &&
> - !xen_device_state_is_active(xendev->frontend_state)) {
> + if (state == XenbusStateClosing &&
> + !xen_device_frontend_is_active(xendev)) {
> xen_device_backend_set_state(xendev, XenbusStateClosed);
> }
>
> /*
> * If a backend is still 'online' then we should leave it alone but,
> - * if a backend is not 'online', then the device should be destroyed
> - * once the state is Closed.
> + * if a backend is not 'online', then the device is a candidate
> + * for destruction. Hence add it to the 'offline' list to be cleaned
> + * by xen_bus_cleanup().
> */
> - if (!xendev->backend_online &&
> - (xendev->backend_state == XenbusStateClosed ||
> - xendev->backend_state == XenbusStateInitialising ||
> - xendev->backend_state == XenbusStateInitWait ||
> - xendev->backend_state == XenbusStateUnknown)) {
> - Error *local_err = NULL;
> + if (!online &&
> + (state == XenbusStateClosed || state == XenbusStateInitialising ||
> + state == XenbusStateInitWait || state == XenbusStateUnknown)) {
> + XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>
> - if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> - object_unparent(OBJECT(xendev));
> - }
> + QLIST_INSERT_HEAD(&xenbus->offline_devices, xendev, list);
I realize now that I should have a check here to make sure the above does not happen twice. I'll send a v2.
Paul
>
> - if (local_err) {
> - error_report_err(local_err);
> - }
> + /*
> + * Re-write the state to cause a XenBus backend_watch notification,
> + * resulting in a call to xen_bus_cleanup().
> + */
> + xen_device_backend_printf(xendev, "state", "%u", state);
> }
> }
>
> diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
> index 0d198148f6..15d71aff70 100644
> --- a/include/hw/xen/xen-bus.h
> +++ b/include/hw/xen/xen-bus.h
> @@ -33,6 +33,7 @@ typedef struct XenDevice {
> xengnttab_handle *xgth;
> bool feature_grant_copy;
> QLIST_HEAD(, XenEventChannel) event_channels;
> + QLIST_ENTRY(XenDevice) list;
> } XenDevice;
>
> typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
> @@ -68,6 +69,7 @@ typedef struct XenBus {
> struct xs_handle *xsh;
> XenWatchList *watch_list;
> XenWatch *backend_watch;
> + QLIST_HEAD(, XenDevice) offline_devices;
> } XenBus;
>
> typedef struct XenBusClass {
> --
> 2.20.1.2.gb21ebb6
^ permalink raw reply [flat|nested] 8+ messages in thread