To remove a device from a domain, a qmp command is sent to qemu. But it is
handled by qemu asychronously. Even the qmp command is claimed to be done,
the actual handling in qemu side may happen later.
This behavior brings two questions:
1. Attaching a device back to a domain right after detaching the device from
that domain would fail with error:

libxl: error: libxl_qmp.c:341:qmp_handle_error_response: Domain 1:received an
error message from QMP server: Duplicate ID 'pci-pt-60_00.0' for device

2. Accesses to PCI configuration space in Qemu may overlap with later device
reset issued by 'xl' or by pciback.

In order to avoid mentioned questions, wait for the completion of device
removal by querying all pci devices using qmp command and ensuring the target
device isn't listed. Only retry 5 times to avoid 'xl' potentially being blocked
by qemu.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v2:
 - Break out early if we found an error during querying pci devices.
 - Print a message to warn admin that device removal may not be done
   in device model's side.
---
 tools/libxl/libxl_qmp.c | 61 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 42c8ab8d8d..9c4480a2b1 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -916,6 +916,38 @@ out:
     return rc;
 }
 
+static int pci_del_callback(libxl__qmp_handler *qmp,
+                            const libxl__json_object *response, void *opaque)
+{
+    const char *asked_id = opaque;
+    const libxl__json_object *bus = NULL;
+    GC_INIT(qmp->ctx);
+    int i, j, rc = 0;
+
+    for (i = 0; (bus = libxl__json_array_get(response, i)); i++) {
+        const libxl__json_object *devices = NULL;
+        const libxl__json_object *device = NULL;
+        const libxl__json_object *o = NULL;
+        const char *id = NULL;
+
+        devices = libxl__json_map_get("devices", bus, JSON_ARRAY);
+
+        for (j = 0; (device = libxl__json_array_get(devices, j)); j++) {
+             o = libxl__json_map_get("qdev_id", device, JSON_STRING);
+             id = libxl__json_object_get_string(o);
+
+             if (id && strcmp(asked_id, id) == 0) {
+                 rc = 1;
+                 goto out;
+             }
+        }
+    }
+
+out:
+    GC_FREE;
+    return rc;
+}
+
 static int qmp_run_command(libxl__gc *gc, int domid,
                            const char *cmd, libxl__json_object *args,
                            qmp_callback_t callback, void *opaque)
@@ -1000,9 +1032,36 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
 static int qmp_device_del(libxl__gc *gc, int domid, char *id)
 {
     libxl__json_object *args = NULL;
+    libxl__qmp_handler *qmp = NULL;
+    int rc = 0;
+
+    qmp = libxl__qmp_initialize(gc, domid);
+    if (!qmp)
+        return ERROR_FAIL;
 
     qmp_parameters_add_string(gc, &args, "id", id);
-    return qmp_run_command(gc, domid, "device_del", args, NULL, NULL);
+    rc = qmp_synchronous_send(qmp, "device_del", args,
+                              NULL, NULL, qmp->timeout);
+    if (rc == 0) {
+        unsigned int retry = 0;
+
+        do {
+            rc = qmp_synchronous_send(qmp, "query-pci", NULL,
+                                      pci_del_callback, id, qmp->timeout);
+            if (rc != 1) {
+                break;
+            }
+            sleep(1);
+        } while (retry++ < 5);
+
+        if (rc != 0) {
+            LOGD(WARN, qmp->domid,
+                 "device model may not complete removing device %s", id);
+        }
+    }
+
+    libxl__qmp_close(qmp);
+    return rc;
 }
 
 int libxl__qmp_pci_del(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
-- 
2.17.1


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

On Wed, Jul 03, 2019 at 01:56:13PM +0800, Chao Gao wrote:
> To remove a device from a domain, a qmp command is sent to qemu. But it is
> handled by qemu asychronously. Even the qmp command is claimed to be done,
> the actual handling in qemu side may happen later.
> This behavior brings two questions:
> 1. Attaching a device back to a domain right after detaching the device from
> that domain would fail with error:
> 
> libxl: error: libxl_qmp.c:341:qmp_handle_error_response: Domain 1:received an
> error message from QMP server: Duplicate ID 'pci-pt-60_00.0' for device
> 
> 2. Accesses to PCI configuration space in Qemu may overlap with later device
> reset issued by 'xl' or by pciback.
> 
> In order to avoid mentioned questions, wait for the completion of device
> removal by querying all pci devices using qmp command and ensuring the target
> device isn't listed. Only retry 5 times to avoid 'xl' potentially being blocked
> by qemu.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> Changes in v2:
>  - Break out early if we found an error during querying pci devices.
>  - Print a message to warn admin that device removal may not be done
>    in device model's side.

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

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

On Wed, Jul 03, 2019 at 05:03:17PM +0100, Anthony PERARD wrote:
>On Wed, Jul 03, 2019 at 01:56:13PM +0800, Chao Gao wrote:
>> To remove a device from a domain, a qmp command is sent to qemu. But it is
>> handled by qemu asychronously. Even the qmp command is claimed to be done,
>> the actual handling in qemu side may happen later.
>> This behavior brings two questions:
>> 1. Attaching a device back to a domain right after detaching the device from
>> that domain would fail with error:
>> 
>> libxl: error: libxl_qmp.c:341:qmp_handle_error_response: Domain 1:received an
>> error message from QMP server: Duplicate ID 'pci-pt-60_00.0' for device
>> 
>> 2. Accesses to PCI configuration space in Qemu may overlap with later device
>> reset issued by 'xl' or by pciback.
>> 
>> In order to avoid mentioned questions, wait for the completion of device
>> removal by querying all pci devices using qmp command and ensuring the target
>> device isn't listed. Only retry 5 times to avoid 'xl' potentially being blocked
>> by qemu.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> Changes in v2:
>>  - Break out early if we found an error during querying pci devices.
>>  - Print a message to warn admin that device removal may not be done
>>    in device model's side.
>
>Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>

Could we merge this patch? or need comments from someone else?

Thanks
Chao

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

Chao Gao writes ("Re: [PATCH v2] libxl_qmp: wait for completion of device removal"):
> On Wed, Jul 03, 2019 at 05:03:17PM +0100, Anthony PERARD wrote:
> >On Wed, Jul 03, 2019 at 01:56:13PM +0800, Chao Gao wrote:
> >> To remove a device from a domain, a qmp command is sent to qemu. But it is
> >> handled by qemu asychronously. Even the qmp command is claimed to be done,
> >> the actual handling in qemu side may happen later.
> >> This behavior brings two questions:
> >> 1. Attaching a device back to a domain right after detaching the device from
> >> that domain would fail with error:
> >> 
> >> libxl: error: libxl_qmp.c:341:qmp_handle_error_response: Domain 1:received an
> >> error message from QMP server: Duplicate ID 'pci-pt-60_00.0' for device
> >> 
> >> 2. Accesses to PCI configuration space in Qemu may overlap with later device
> >> reset issued by 'xl' or by pciback.
> >> 
> >> In order to avoid mentioned questions, wait for the completion of device
> >> removal by querying all pci devices using qmp command and ensuring the target
> >> device isn't listed. Only retry 5 times to avoid 'xl' potentially being blocked
> >> by qemu.
...
> Could we merge this patch? or need comments from someone else?

I see this patch was in fact merged.

However, there is a problem.

It adds a new call to "qmp_synchronous_send".  We have been trying to
make libxl not trust qemu and that means abolishing all uses of
qmp_synchronous_send.  Now I know that currently we haven't done that
for PCI passthrough but we should not be adding more techncial debt
unless we have to.

Unfortunately the place where this patch has to add code already uses
this synchronous interface and currently it is therefore not easy to
do what Chao Gao needed to do.  So it was probably right to accept
this patch.

Maybe we should introduce a thing which can make a libxl__ev_qmp from
a libxl__qmp_handler, or make libxl__qmp_handler contain a
libxl__ev_qmp, or something ?  If we had had that, Chao Gao could have
provided a patch introducing new calls to libxl__ev_qmp_send etc.

Additionally: is it really the case that there is no way to have qemu
send us a signal when the work is done ?  This polling is rather poor.

Thanks,
Ian.

From bff0b5dfb692546a12e1f3e124b6691955560112 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Mon, 5 Aug 2019 15:44:46 +0100
Subject: [PATCH RFC]: tools: libxl_qmp: Deprecate synchronous interface

Currently it is not always possible to use the asynchronous interface
because the two qmp connection structs libxl__ev_qmp and
libxl__qmp_handler are different and incompatible.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_qmp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 9c4480a2b1..c0667a1520 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -23,17 +23,23 @@
 
 /*
  * Logic used to send command to QEMU
+ */
+
+/*
+ * *DEPRECATED*  No new calls to these function should be introduced,
+ * ideally.  Instead, where possible, use the asynchronous calls, see
+ * "QMP asynchronous calls" in libxl_internal.h.
  *
- * qmp_open():
+ * qmp_open(): *DEPRECATED*
  *  Will open a socket and connect to QEMU.
  *
- * qmp_next():
+ * qmp_next(): *DEPRECATED*
  *  Will read data sent by QEMU and then call qmp_handle_response() once a
  *  complete QMP message is received.
  *  The function return on timeout/error or once every data received as been
  *  processed.
  *
- * qmp_handle_response()
+ * qmp_handle_response() *DEPRECATED*
  *  This process json messages received from QEMU and update different list and
  *  may call callback function.
  *  `libxl__qmp_handler.wait_for_id` is reset once a message with this ID is
@@ -42,12 +48,12 @@
  *    optional assotiated callback function. The return value of a callback is
  *    set in context.
  *
- * qmp_send():
+ * qmp_send(): *DEPRECATED*
  *  Simply prepare a QMP command and send it to QEMU.
  *  It also add a `struct callback_id_pair` on the
  *  `libxl__qmp_handler.callback_list` via qmp_send_prepare().
  *
- * qmp_synchronous_send():
+ * qmp_synchronous_send(): *DEPRECATED*
  *  This function calls qmp_send(), then wait for QEMU to reply to the command.
  *  The wait is done by calling qmp_next() over and over again until either
  *  there is a response for the command or there is an error.
-- 
2.11.0


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

On Mon, Aug 05, 2019 at 03:46:35PM +0100, Ian Jackson wrote:
> Chao Gao writes ("Re: [PATCH v2] libxl_qmp: wait for completion of device removal"):
> > On Wed, Jul 03, 2019 at 05:03:17PM +0100, Anthony PERARD wrote:
> > >On Wed, Jul 03, 2019 at 01:56:13PM +0800, Chao Gao wrote:
> > >> To remove a device from a domain, a qmp command is sent to qemu. But it is
> > >> handled by qemu asychronously. Even the qmp command is claimed to be done,
> > >> the actual handling in qemu side may happen later.
> > >> This behavior brings two questions:
> > >> 1. Attaching a device back to a domain right after detaching the device from
> > >> that domain would fail with error:
> > >> 
> > >> libxl: error: libxl_qmp.c:341:qmp_handle_error_response: Domain 1:received an
> > >> error message from QMP server: Duplicate ID 'pci-pt-60_00.0' for device
> > >> 
> > >> 2. Accesses to PCI configuration space in Qemu may overlap with later device
> > >> reset issued by 'xl' or by pciback.
> > >> 
> > >> In order to avoid mentioned questions, wait for the completion of device
> > >> removal by querying all pci devices using qmp command and ensuring the target
> > >> device isn't listed. Only retry 5 times to avoid 'xl' potentially being blocked
> > >> by qemu.
> ...
> > Could we merge this patch? or need comments from someone else?
> 
> I see this patch was in fact merged.
> 
> However, there is a problem.
> 
> It adds a new call to "qmp_synchronous_send".  We have been trying to
> make libxl not trust qemu and that means abolishing all uses of
> qmp_synchronous_send.  Now I know that currently we haven't done that
> for PCI passthrough but we should not be adding more techncial debt
> unless we have to.
> 
> Unfortunately the place where this patch has to add code already uses
> this synchronous interface and currently it is therefore not easy to
> do what Chao Gao needed to do.  So it was probably right to accept
> this patch.
> 
> Maybe we should introduce a thing which can make a libxl__ev_qmp from
> a libxl__qmp_handler, or make libxl__qmp_handler contain a
> libxl__ev_qmp, or something ?  If we had had that, Chao Gao could have
> provided a patch introducing new calls to libxl__ev_qmp_send etc.

That looks complicated. I'd rather get rid of that qmp_synchronous_send
stuff.

> Additionally: is it really the case that there is no way to have qemu
> send us a signal when the work is done ?  This polling is rather poor.

There is, QEMU sends an event when the device is removed. Maybe we could
design a new API for libxl__ev_qmp to handle events, I think that's for
another day.

Also, see [1] which reimplements that polling with ev_qmp and ev_timer
and have a note about using events instead:
[1] https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg00241.html

Thanks,

-- 
Anthony PERARD

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

On Mon, Aug 05, 2019 at 03:46:35PM +0100, Ian Jackson wrote:
> From bff0b5dfb692546a12e1f3e124b6691955560112 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Mon, 5 Aug 2019 15:44:46 +0100
> Subject: [PATCH RFC]: tools: libxl_qmp: Deprecate synchronous interface
> 
> Currently it is not always possible to use the asynchronous interface
> because the two qmp connection structs libxl__ev_qmp and
> libxl__qmp_handler are different and incompatible.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl_qmp.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 9c4480a2b1..c0667a1520 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -23,17 +23,23 @@
>  
>  /*
>   * Logic used to send command to QEMU
> + */
> +
> +/*
> + * *DEPRECATED*  No new calls to these function should be introduced,
> + * ideally.  Instead, where possible, use the asynchronous calls, see
> + * "QMP asynchronous calls" in libxl_internal.h.
>   *
> - * qmp_open():
> + * qmp_open(): *DEPRECATED*
>   *  Will open a socket and connect to QEMU.
>   *
> - * qmp_next():
> + * qmp_next(): *DEPRECATED*
>   *  Will read data sent by QEMU and then call qmp_handle_response() once a
>   *  complete QMP message is received.
>   *  The function return on timeout/error or once every data received as been
>   *  processed.
>   *
> - * qmp_handle_response()
> + * qmp_handle_response() *DEPRECATED*
>   *  This process json messages received from QEMU and update different list and
>   *  may call callback function.
>   *  `libxl__qmp_handler.wait_for_id` is reset once a message with this ID is
> @@ -42,12 +48,12 @@
>   *    optional assotiated callback function. The return value of a callback is
>   *    set in context.
>   *
> - * qmp_send():
> + * qmp_send(): *DEPRECATED*
>   *  Simply prepare a QMP command and send it to QEMU.
>   *  It also add a `struct callback_id_pair` on the
>   *  `libxl__qmp_handler.callback_list` via qmp_send_prepare().
>   *
> - * qmp_synchronous_send():
> + * qmp_synchronous_send(): *DEPRECATED*
>   *  This function calls qmp_send(), then wait for QEMU to reply to the command.
>   *  The wait is done by calling qmp_next() over and over again until either
>   *  there is a response for the command or there is an error.

Looks fine, but maybe you would like to add some more "deprecated" in
libxl_internal.h, for the type `libxl__qmp_handler' and the function
libxl__qmp_initialize() and libxl__qmp_close().

-- 
Anthony PERARD

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