[Xen-devel] [PATCH 0/3] Cleanup IOREQ server on exit

[Xen-devel] [PATCH 0/3] Cleanup IOREQ server on exit

[Maximilian Heyne]

Following up on commit 9c0eed61 ("qemu-trad: stop using the default IOREQ
server"), clean up the IOREQ server on exit. This fixes a bug with soft-reset
that shows up as "bind interdomain ioctl error 22" because the event channels
were not closed at the soft-reset and can't be bound again.

For this I used the exit notifiers from QEMU that I backported together with the
required generic notifier lists.

Anthony Liguori (1):
  Add support for generic notifier lists

Gerd Hoffmann (1):
  Add exit notifiers.

Maximilian Heyne (1):
  xen: cleanup IOREQ server on exit

 Makefile            |  1 +
 hw/xen_machine_fv.c | 11 +++++++++++
 notify.c            | 39 +++++++++++++++++++++++++++++++++++++++
 notify.h            | 43 +++++++++++++++++++++++++++++++++++++++++++
 sys-queue.h         |  5 +++++
 sysemu.h            |  5 +++++
 vl.c                | 20 ++++++++++++++++++++
 7 files changed, 124 insertions(+)
 create mode 100644 notify.c
 create mode 100644 notify.h

-- 
2.16.6




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/3] Add support for generic notifier lists

[Maximilian Heyne]

From: Anthony Liguori <aliguori@us.ibm.com>

Notifiers are data-less callbacks and a notifier list is a list of registered
notifiers that all are interested in a particular event.

We'll use this in a few patches to implement mouse change notification.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - Do not do memory allocations by placing list nodes in notifier

[cherry-picked from d1e70c5e6d1472856c52969301247fe8c3c8389d
    conflicts: used the sys-qeue interface and added required
    LIST_REMOVE_SAFE function to that]
Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
---
 Makefile    |  1 +
 notify.c    | 39 +++++++++++++++++++++++++++++++++++++++
 notify.h    | 43 +++++++++++++++++++++++++++++++++++++++++++
 sys-queue.h |  5 +++++
 4 files changed, 88 insertions(+)
 create mode 100644 notify.c
 create mode 100644 notify.h

diff --git a/Makefile b/Makefile
index 0fbec990b..d921bcdf8 100644
--- a/Makefile
+++ b/Makefile
@@ -93,6 +93,7 @@ OBJS+=sd.o ssi-sd.o
 OBJS+=bt.o bt-host.o bt-vhci.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o usb-bt.o
 OBJS+=buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o
 OBJS+=qemu-char.o aio.o net-checksum.o savevm.o cache-utils.o
+OBJS+=notify.o
 
 ifdef CONFIG_BRLAPI
 OBJS+= baum.o
diff --git a/notify.c b/notify.c
new file mode 100644
index 000000000..59e1e7c7d
--- /dev/null
+++ b/notify.c
@@ -0,0 +1,39 @@
+/*
+ * Notifier lists
+ *
+ * Copyright IBM, Corp. 2010
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "notify.h"
+
+void notifier_list_init(NotifierList *list)
+{
+    LIST_INIT(&list->notifiers);
+}
+
+void notifier_list_add(NotifierList *list, Notifier *notifier)
+{
+    LIST_INSERT_HEAD(&list->notifiers, notifier, node);
+}
+
+void notifier_list_remove(Notifier *notifier)
+{
+    LIST_REMOVE(notifier, node);
+}
+
+void notifier_list_notify(NotifierList *list)
+{
+    Notifier *notifier, *next;
+
+    LIST_FOREACH_SAFE(notifier, &list->notifiers, node, next) {
+        notifier->notify(notifier);
+    }
+}
diff --git a/notify.h b/notify.h
new file mode 100644
index 000000000..093c63f19
--- /dev/null
+++ b/notify.h
@@ -0,0 +1,43 @@
+/*
+ * Notifier lists
+ *
+ * Copyright IBM, Corp. 2010
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_NOTIFY_H
+#define QEMU_NOTIFY_H
+
+#include "sys-queue.h"
+
+typedef struct Notifier Notifier;
+
+struct Notifier
+{
+    void (*notify)(Notifier *notifier);
+    LIST_ENTRY(Notifier) node;
+};
+
+typedef struct NotifierList
+{
+    LIST_HEAD(, Notifier) notifiers;
+} NotifierList;
+
+#define NOTIFIER_LIST_INITIALIZER(head) \
+    { LIST_HEAD_INITIALIZER((head).notifiers) }
+
+void notifier_list_init(NotifierList *list);
+
+void notifier_list_add(NotifierList *list, Notifier *notifier);
+
+void notifier_list_remove(Notifier *notifier);
+
+void notifier_list_notify(NotifierList *list);
+
+#endif
diff --git a/sys-queue.h b/sys-queue.h
index 55c26fe7f..81ab044a8 100644
--- a/sys-queue.h
+++ b/sys-queue.h
@@ -132,6 +132,11 @@ struct {                                                                \
                 (var);                                                  \
                 (var) = ((var)->field.le_next))
 
+#define LIST_FOREACH_SAFE(var, head, field, next_var)                   \
+        for ((var) = ((head)->lh_first);                                \
+                (var) && ((next_var) = ((var)->field.le_next), 1);      \
+                (var) = (next_var))
+
 /*
  * List access methods.
  */
-- 
2.16.6




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

RE: [PATCH 1/3] Add support for generic notifier lists

[Paul Durrant]

> -----Original Message-----
> From: Maximilian Heyne <mheyne@amazon.de>
> Sent: 13 March 2020 12:33
> To: xen-devel@lists.xenproject.org
> Cc: Ian Jackson <ian.jackson@citrix.com>; Paul Durrant <paul@xen.org>
> Subject: [PATCH 1/3] Add support for generic notifier lists
> 
> From: Anthony Liguori <aliguori@us.ibm.com>
> 
> Notifiers are data-less callbacks and a notifier list is a list of registered
> notifiers that all are interested in a particular event.
> 
> We'll use this in a few patches to implement mouse change notification.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> v1 -> v2
>  - Do not do memory allocations by placing list nodes in notifier
> 
> [cherry-picked from d1e70c5e6d1472856c52969301247fe8c3c8389d
>     conflicts: used the sys-qeue interface and added required
>     LIST_REMOVE_SAFE function to that]
> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>

Reviewed-by: Paul Durrant <paul@xen.org>

[Xen-devel] [PATCH 2/3] Add exit notifiers.

[Maximilian Heyne]

From: Gerd Hoffmann <kraxel@redhat.com>

Hook up any cleanup work which needs to be done here.  Advantages over
using atexit(3):

  (1) You get passed in a pointer to the notifier.  If you embed that
      into your state struct you can use container_of() to get get your
      state info.
  (2) You can unregister, say when un-plugging a device.

[ v2: move code out of #ifndef _WIN32 ]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
(cherry picked from commit fd42deeb4cb42f90084046e3ebdb4383953195e3)
Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
---
 sysemu.h |  5 +++++
 vl.c     | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/sysemu.h b/sysemu.h
index 968258a84..759d0e9d5 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -2,6 +2,8 @@
 #define SYSEMU_H
 /* Misc. things related to the system emulator.  */
 
+#include "notify.h"
+
 /* vl.c */
 extern const char *bios_name;
 extern const char *bios_dir;
@@ -39,6 +41,9 @@ void qemu_system_powerdown(void);
 #endif
 void qemu_system_reset(void);
 
+void qemu_add_exit_notifier(Notifier *notify);
+void qemu_remove_exit_notifier(Notifier *notify);
+
 void do_savevm(const char *name);
 void do_loadvm(const char *name);
 void do_delvm(const char *name);
diff --git a/vl.c b/vl.c
index c3c5d630e..2163217ec 100644
--- a/vl.c
+++ b/vl.c
@@ -282,6 +282,9 @@ uint8_t qemu_uuid[16];
 
 #include "xen-vl-extra.c"
 
+static NotifierList exit_notifiers =
+    NOTIFIER_LIST_INITIALIZER(exit_notifiers);
+
 /***********************************************************/
 /* x86 ISA bus support */
 
@@ -4843,6 +4846,21 @@ static void vcpu_hex_str_to_bitmap(const char *optarg)
     }
 }
 
+void qemu_add_exit_notifier(Notifier *notify)
+{
+    notifier_list_add(&exit_notifiers, notify);
+}
+
+void qemu_remove_exit_notifier(Notifier *notify)
+{
+    notifier_list_remove(notify);
+}
+
+static void qemu_run_exit_notifiers(void)
+{
+    notifier_list_notify(&exit_notifiers);
+}
+
 int main(int argc, char **argv, char **envp)
 {
 #ifdef CONFIG_GDBSTUB
@@ -4887,6 +4905,8 @@ int main(int argc, char **argv, char **envp)
     const char *chroot_dir = NULL;
     const char *run_as = NULL;
 
+    atexit(qemu_run_exit_notifiers);
+
     qemu_cache_utils_init(envp);
     logfile = stderr; /* initial value */
 
-- 
2.16.6




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

RE: [PATCH 2/3] Add exit notifiers.

[Paul Durrant]

> -----Original Message-----
> From: Maximilian Heyne <mheyne@amazon.de>
> Sent: 13 March 2020 12:33
> To: xen-devel@lists.xenproject.org
> Cc: Ian Jackson <ian.jackson@citrix.com>; Paul Durrant <paul@xen.org>
> Subject: [PATCH 2/3] Add exit notifiers.
> 
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> Hook up any cleanup work which needs to be done here.  Advantages over
> using atexit(3):
> 
>   (1) You get passed in a pointer to the notifier.  If you embed that
>       into your state struct you can use container_of() to get get your
>       state info.
>   (2) You can unregister, say when un-plugging a device.
> 
> [ v2: move code out of #ifndef _WIN32 ]
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> (cherry picked from commit fd42deeb4cb42f90084046e3ebdb4383953195e3)
> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>

Reviewed-by: Paul Durrant <paul@xen.org>

[Xen-devel] [PATCH 3/3] xen: cleanup IOREQ server on exit

[Maximilian Heyne]

Use the backported Notifier interface to register an atexit handler to
cleanup the IOREQ server. This is required since Xen commit a5a180f9
("x86/domain: don't destroy IOREQ servers on soft reset") is introduced
which requires Qemu to explicitly close the IOREQ server.

This is can be seen as a backport of ba7fdd64 ("xen: cleanup IOREQ
server on exit").

Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
---
 hw/xen_machine_fv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
index f0989fad4..66eb4a1eb 100644
--- a/hw/xen_machine_fv.c
+++ b/hw/xen_machine_fv.c
@@ -31,6 +31,7 @@
 #include "qemu-aio.h"
 #include "xen_backend.h"
 #include "pci.h"
+#include "sysemu.h"
 
 #include <xen/hvm/params.h>
 #include <sys/mman.h>
@@ -67,6 +68,8 @@ TAILQ_HEAD(map_cache_head, map_cache_rev) locked_entries = TAILQ_HEAD_INITIALIZE
 static unsigned long last_address_page = ~0UL;
 static uint8_t      *last_address_vaddr;
 
+static Notifier exit_notifier;
+
 static int qemu_map_cache_init(void)
 {
     unsigned long size;
@@ -283,6 +286,11 @@ void xen_disable_io(void)
     xc_hvm_set_ioreq_server_state(xc_handle, domid, ioservid, 0);
 }
 
+static void xen_exit_notifier(Notifier *n)
+{
+    xc_hvm_destroy_ioreq_server(xc_handle, domid, ioservid);
+}
+
 static void xen_init_fv(ram_addr_t ram_size, int vga_ram_size,
 			const char *boot_device,
 			const char *kernel_filename,const char *kernel_cmdline,
@@ -317,6 +325,9 @@ static void xen_init_fv(ram_addr_t ram_size, int vga_ram_size,
         exit(-1);
     }
 
+    exit_notifier.notify = xen_exit_notifier;
+    qemu_add_exit_notifier(&exit_notifier);
+
     if (xc_hvm_get_ioreq_server_info(xc_handle, domid, ioservid,
                                      &ioreq_pfn, &bufioreq_pfn,
                                      &bufioreq_evtchn)) {
-- 
2.16.6




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

RE: [PATCH 3/3] xen: cleanup IOREQ server on exit

[Paul Durrant]

> -----Original Message-----
> From: Maximilian Heyne <mheyne@amazon.de>
> Sent: 13 March 2020 12:33
> To: xen-devel@lists.xenproject.org
> Cc: Ian Jackson <ian.jackson@citrix.com>; Paul Durrant <paul@xen.org>
> Subject: [PATCH 3/3] xen: cleanup IOREQ server on exit
> 
> Use the backported Notifier interface to register an atexit handler to
> cleanup the IOREQ server. This is required since Xen commit a5a180f9
> ("x86/domain: don't destroy IOREQ servers on soft reset") is introduced
> which requires Qemu to explicitly close the IOREQ server.
> 
> This is can be seen as a backport of ba7fdd64 ("xen: cleanup IOREQ
> server on exit").
> 
> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 0/3] Cleanup IOREQ server on exit

[Maximilian Heyne]

Could someone please have a look at this patch? It solves an actual issue:
Try soft-reset with qemu-xen-traditional and it will fail.

On 3/13/20 1:33 PM, Maximilian Heyne wrote:
> Following up on commit 9c0eed61 ("qemu-trad: stop using the default IOREQ
> server"), clean up the IOREQ server on exit. This fixes a bug with soft-reset
> that shows up as "bind interdomain ioctl error 22" because the event channels
> were not closed at the soft-reset and can't be bound again.
> 
> For this I used the exit notifiers from QEMU that I backported together with the
> required generic notifier lists.
> 
> Anthony Liguori (1):
>    Add support for generic notifier lists
> 
> Gerd Hoffmann (1):
>    Add exit notifiers.
> 
> Maximilian Heyne (1):
>    xen: cleanup IOREQ server on exit
> 
>   Makefile            |  1 +
>   hw/xen_machine_fv.c | 11 +++++++++++
>   notify.c            | 39 +++++++++++++++++++++++++++++++++++++++
>   notify.h            | 43 +++++++++++++++++++++++++++++++++++++++++++
>   sys-queue.h         |  5 +++++
>   sysemu.h            |  5 +++++
>   vl.c                | 20 ++++++++++++++++++++
>   7 files changed, 124 insertions(+)
>   create mode 100644 notify.c
>   create mode 100644 notify.h
> 



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

RE: [PATCH 0/3] Cleanup IOREQ server on exit

[Paul Durrant]

> -----Original Message-----
> From: Maximilian Heyne <mheyne@amazon.de>
> Sent: 07 April 2020 10:16
> To: xen-devel@lists.xenproject.org
> Cc: Ian Jackson <ian.jackson@citrix.com>; Paul Durrant <paul@xen.org>
> Subject: Re: [PATCH 0/3] Cleanup IOREQ server on exit
> 
> Could someone please have a look at this patch? It solves an actual issue:
> Try soft-reset with qemu-xen-traditional and it will fail.
> 

I'll take a look today.

Ian, obviously we know that qemu trad is largely dead but this series does address a serious shortcoming. Could you take a look?

  Paul

RE: [PATCH 0/3] Cleanup IOREQ server on exit

[Paul Durrant]

Ping v2?

> -----Original Message-----
> From: Maximilian Heyne <mheyne@amazon.de>
> Sent: 07 April 2020 10:16
> To: xen-devel@lists.xenproject.org
> Cc: Ian Jackson <ian.jackson@citrix.com>; Paul Durrant <paul@xen.org>
> Subject: Re: [PATCH 0/3] Cleanup IOREQ server on exit
> 
> Could someone please have a look at this patch? It solves an actual issue:
> Try soft-reset with qemu-xen-traditional and it will fail.
> 
> On 3/13/20 1:33 PM, Maximilian Heyne wrote:
> > Following up on commit 9c0eed61 ("qemu-trad: stop using the default IOREQ
> > server"), clean up the IOREQ server on exit. This fixes a bug with soft-reset
> > that shows up as "bind interdomain ioctl error 22" because the event channels
> > were not closed at the soft-reset and can't be bound again.
> >
> > For this I used the exit notifiers from QEMU that I backported together with the
> > required generic notifier lists.
> >
> > Anthony Liguori (1):
> >    Add support for generic notifier lists
> >
> > Gerd Hoffmann (1):
> >    Add exit notifiers.
> >
> > Maximilian Heyne (1):
> >    xen: cleanup IOREQ server on exit
> >
> >   Makefile            |  1 +
> >   hw/xen_machine_fv.c | 11 +++++++++++
> >   notify.c            | 39 +++++++++++++++++++++++++++++++++++++++
> >   notify.h            | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   sys-queue.h         |  5 +++++
> >   sysemu.h            |  5 +++++
> >   vl.c                | 20 ++++++++++++++++++++
> >   7 files changed, 124 insertions(+)
> >   create mode 100644 notify.c
> >   create mode 100644 notify.h
> >
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 

Re: [PATCH 0/3] Cleanup IOREQ server on exit

[Ian Jackson]

Maximilian Heyne writes ("Re: [PATCH 0/3] Cleanup IOREQ server on exit"):
> Could someone please have a look at this patch? It solves an actual issue:
> Try soft-reset with qemu-xen-traditional and it will fail.

Thanks.  I reviewed this.

qemu is in deep freeze but the changes looked correct and are indeed
solving a regression.  I convinced myself that they were appropriately
low risk, so

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

for all three and I have pushed them.

In theory a backport might be appropriate since this is a bugfix but
my inclination is to leave existing releases where they are, since
anyone using qemu-trad probably wants things super-stable.  Contrary
opinions welcome.

It has been a very long time since I did an update of qemu trad so it
is possible that I have mangled the process somehow.  We will see I
guess...

Thanks also to Paul for chasing me about this.

Regards,
Ian.