[Xen-devel] [PATCH 0/2] libxl fixes with pci passthrough

[Xen-devel] [PATCH 0/2] libxl fixes with pci passthrough

[Anthony PERARD]

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.fixes-for-pci-pt-libxl-v1

Hi,

Some fixes following the recent changes to use ev_qmp in libxl.

Anthony PERARD (2):
  libxl_pci: Don't ignore PCI PT error at guest creation
  libxl_pci: Fix guest shutdown with PCI PT attached

 tools/libxl/libxl_pci.c | 43 +++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

-- 
Anthony PERARD


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

[Xen-devel] [PATCH 1/2] libxl_pci: Don't ignore PCI PT error at guest creation

[Anthony PERARD]

Fixes: 11db56f9a6291
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 47258177bc71..3b31cfd417be 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1694,6 +1694,8 @@ static void add_pcidevs_done(libxl__egc *egc, libxl__multidev *multidev,
     libxl_domid domid = apds->domid;
     libxl__ao_device *aodev = apds->outer_aodev;
 
+    if (rc) goto out;
+
     if (d_config->num_pcidevs > 0) {
         rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
             d_config->num_pcidevs);
-- 
Anthony PERARD


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

[Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Anthony PERARD]

Before the problematic commit, libxl used to ignore error when
destroying (force == true) a passthrough device. If the DM failed to
detach the pci device within the allowed time, the timed out error
raised skip part of pci_remove_*, but also raise the error up to the
caller of libxl__device_pci_destroy_all, libxl__destroy_domid, and
thus the destruction of the domain fails.

When a *pci_destroy* function is called (so we have force=true), error
should mostly be ignored. If the DM didn't confirmed that the device
is removed, we will print a warning and keep going if force=true.
The patch reorder the functions so that pci_remove_timeout() calls
pci_remove_detatched() like it's done when DM calls are successful.

We also clean the QMP states and associated timeouts earlier, as soon
as they are not needed anymore.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_pci.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 3b31cfd417be..cb849b241581 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1778,12 +1778,12 @@ static void pci_remove_qmp_retry_timer_cb(libxl__egc *egc,
     libxl__ev_time *ev, const struct timeval *requested_abs, int rc);
 static void pci_remove_qmp_query_cb(libxl__egc *egc,
     libxl__ev_qmp *qmp, const libxl__json_object *response, int rc);
+static void pci_remove_timeout(libxl__egc *egc,
+    libxl__ev_time *ev, const struct timeval *requested_abs, int rc);
 static void pci_remove_detatched(libxl__egc *egc,
     pci_remove_state *prs, int rc);
 static void pci_remove_stubdom_done(libxl__egc *egc,
     libxl__ao_device *aodev);
-static void pci_remove_timeout(libxl__egc *egc,
-    libxl__ev_time *ev, const struct timeval *requested_abs, int rc);
 static void pci_remove_done(libxl__egc *egc,
     pci_remove_state *prs, int rc);
 
@@ -2044,6 +2044,25 @@ static void pci_remove_qmp_query_cb(libxl__egc *egc,
     pci_remove_detatched(egc, prs, rc); /* must be last */
 }
 
+static void pci_remove_timeout(libxl__egc *egc, libxl__ev_time *ev,
+                               const struct timeval *requested_abs,
+                               int rc)
+{
+    EGC_GC;
+    pci_remove_state *prs = CONTAINER_OF(ev, *prs, timeout);
+
+    /* Convenience aliases */
+    libxl_device_pci *const pcidev = prs->pcidev;
+
+    LOGD(WARN, prs->domid, "timed out waiting for DM to remove "
+         PCI_PT_QDEV_ID, pcidev->bus, pcidev->dev, pcidev->func);
+
+    /* If we timed out, we might still want to keep destroying the device
+     * (when force==true), so let the next function decide what to do on
+     * error */
+    pci_remove_detatched(egc, prs, rc);
+}
+
 static void pci_remove_detatched(libxl__egc *egc,
                                  pci_remove_state *prs,
                                  int rc)
@@ -2057,6 +2076,11 @@ static void pci_remove_detatched(libxl__egc *egc,
     libxl_device_pci *const pcidev = prs->pcidev;
     libxl_domid domid = prs->domid;
 
+    /* Cleaning QMP states ASAP */
+    libxl__ev_qmp_dispose(gc, &prs->qmp);
+    libxl__ev_time_deregister(gc, &prs->timeout);
+    libxl__ev_time_deregister(gc, &prs->retry_timer);
+
     if (rc && !prs->force)
         goto out;
 
@@ -2104,15 +2128,6 @@ static void pci_remove_stubdom_done(libxl__egc *egc,
     pci_remove_done(egc, prs, 0);
 }
 
-static void pci_remove_timeout(libxl__egc *egc, libxl__ev_time *ev,
-                               const struct timeval *requested_abs,
-                               int rc)
-{
-    pci_remove_state *prs = CONTAINER_OF(ev, *prs, timeout);
-
-    pci_remove_done(egc, prs, rc);
-}
-
 static void pci_remove_done(libxl__egc *egc,
                             pci_remove_state *prs,
                             int rc)
@@ -2121,10 +2136,6 @@ static void pci_remove_done(libxl__egc *egc,
 
     if (rc) goto out;
 
-    libxl__ev_qmp_dispose(gc, &prs->qmp);
-    libxl__ev_time_deregister(gc, &prs->timeout);
-    libxl__ev_time_deregister(gc, &prs->retry_timer);
-
     libxl__device_pci_remove_xenstore(gc, prs->domid, prs->pcidev);
 out:
     device_pci_remove_common_next(egc, prs, rc);
-- 
Anthony PERARD


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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Sander Eikelenboom]

On 30/09/2019 19:23, Anthony PERARD wrote:
> Before the problematic commit, libxl used to ignore error when
> destroying (force == true) a passthrough device. If the DM failed to
> detach the pci device within the allowed time, the timed out error
> raised skip part of pci_remove_*, but also raise the error up to the
> caller of libxl__device_pci_destroy_all, libxl__destroy_domid, and
> thus the destruction of the domain fails.
> 
> When a *pci_destroy* function is called (so we have force=true), error
> should mostly be ignored. If the DM didn't confirmed that the device
> is removed, we will print a warning and keep going if force=true.
> The patch reorder the functions so that pci_remove_timeout() calls
> pci_remove_detatched() like it's done when DM calls are successful.
> 
> We also clean the QMP states and associated timeouts earlier, as soon
> as they are not needed anymore.
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Hi Anthony,

Just tested and it works for me, thanks !

--
Sander

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Anthony PERARD]

Rewrite of the commit message:

Before the problematic commit, libxl used to ignore error when
destroying (force == true) a passthrough device, especially error that
happens when dealing with the DM.

Since fae4880c45fe, if the DM failed to detach the pci device within
the allowed time, the timed out error raised skip part of
pci_remove_*, but also raise the error up to the caller of
libxl__device_pci_destroy_all, libxl__destroy_domid, and thus the
destruction of the domain fails.

In this patch, if the DM didn't confirmed that the device is removed,
we will print a warning and keep going if force=true.  The patch
reorder the functions so that pci_remove_timeout() calls
pci_remove_detatched() like it's done when DM calls are successful.

We also clean the QMP states and associated timeouts earlier, as soon
as they are not needed anymore.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

Re: [Xen-devel] [PATCH 0/2] libxl fixes with pci passthrough

[Ian Jackson]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 328 bytes --]

Hi Juergen.  Here's another bugfix series
  https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03320.html
  https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git
  #br.fixes-for-pci-pt-libxl-v1
needing your release review.

Here's the cover letter.  I can bounce you a digest if you like.

Thanks,
Ian.


[-- Attachment #2: forwarded message --]
[-- Type: message/rfc822, Size: 2620 bytes --]

From: Anthony PERARD <anthony.perard@citrix.com>
To: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Anthony Perard <anthony.perard@citrix.com>, Wei Liu <wl@xen.org>, "Ian Jackson" <Ian.Jackson@citrix.com>
Subject: [PATCH 0/2] libxl fixes with pci passthrough
Date: Mon, 30 Sep 2019 18:23:25 +0100
Message-ID: <20190930172327.784520-1-anthony.perard@citrix.com>

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.fixes-for-pci-pt-libxl-v1

Hi,

Some fixes following the recent changes to use ev_qmp in libxl.

Anthony PERARD (2):
  libxl_pci: Don't ignore PCI PT error at guest creation
  libxl_pci: Fix guest shutdown with PCI PT attached

 tools/libxl/libxl_pci.c | 43 +++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

-- 
Anthony PERARD


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 0/2] libxl fixes with pci passthrough

[Jürgen Groß]

On 02.10.19 17:45, Ian Jackson wrote:
> Hi Juergen.  Here's another bugfix series
>    https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03320.html
>    https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git
>    #br.fixes-for-pci-pt-libxl-v1
> needing your release review.

For the series:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [Xen-devel] [PATCH 0/2] libxl fixes with pci passthrough

[Ian Jackson]

Jürgen Groß writes ("Re: [PATCH 0/2] libxl fixes with pci passthrough"):
> On 02.10.19 17:45, Ian Jackson wrote:
> > Hi Juergen.  Here's another bugfix series
> >    https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03320.html
> >    https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git
> >    #br.fixes-for-pci-pt-libxl-v1
> > needing your release review.
> 
> For the series:
> 
> Release-acked-by: Juergen Gross <jgross@suse.com>

Thanks, put my formal ack on it, and pushed.

Ian.

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Sander Eikelenboom]

On 01/10/2019 12:35, Anthony PERARD wrote:
> Rewrite of the commit message:
> 
> Before the problematic commit, libxl used to ignore error when
> destroying (force == true) a passthrough device, especially error that
> happens when dealing with the DM.
> 
> Since fae4880c45fe, if the DM failed to detach the pci device within
> the allowed time, the timed out error raised skip part of
> pci_remove_*, but also raise the error up to the caller of
> libxl__device_pci_destroy_all, libxl__destroy_domid, and thus the
> destruction of the domain fails.
> 
> In this patch, if the DM didn't confirmed that the device is removed,
> we will print a warning and keep going if force=true.  The patch
> reorder the functions so that pci_remove_timeout() calls
> pci_remove_detatched() like it's done when DM calls are successful.
> 
> We also clean the QMP states and associated timeouts earlier, as soon
> as they are not needed anymore.
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 

Hi Anthony / Chao,

I have to come back to this, a bit because perhaps there is an underlying issue.
While it earlier occurred to me that the VM to which I passed through most pci-devices 
(8 to be exact) became very slow to shutdown, but I  didn't investigate it further.

But after you commit messages from this patch it kept nagging, so today I did some testing
and bisecting.

The difference in tear-down time at least from what the IOMMU code logs is quite large:

xen-4.12.0
	Setup: 	    7.452 s
	Tear-down:  7.626 s

xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
	Setup:      7.468 s
	Tear-down: 50.239 s

Bisection turned up:
	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
	Author: Chao Gao <chao.gao@intel.com>
	Date:   Fri Jul 19 10:24:08 2019 +0100
	libxl_qmp: wait for completion of device removal

Which makes me wonder if there is something going wrong in Qemu ?

--
Sander



xen-4.12.0 setup:
	(XEN) [2019-10-10 09:54:14.846] AMD-Vi: Disable: device id = 0x900, domain = 0, paging mode = 3
	(XEN) [2019-10-10 09:54:14.846] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x4aa847000, domain = 1, paging mode = 3
	(XEN) [2019-10-10 09:54:14.846] AMD-Vi: Re-assign 0000:09:00.0 from dom0 to dom1
	...
	(XEN) [2019-10-10 09:54:22.298] AMD-Vi: Disable: device id = 0x907, domain = 0, paging mode = 3
	(XEN) [2019-10-10 09:54:22.298] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x4aa847000, domain = 1, paging mode = 3
	(XEN) [2019-10-10 09:54:22.298] AMD-Vi: Re-assign 0000:09:00.7 from dom0 to dom1


xen-4.12.0 tear-down:
	(XEN) [2019-10-10 10:01:11.971] AMD-Vi: Disable: device id = 0x900, domain = 1, paging mode = 3
	(XEN) [2019-10-10 10:01:11.971] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x53572c000, domain = 0, paging mode = 3
	(XEN) [2019-10-10 10:01:11.971] AMD-Vi: Re-assign 0000:09:00.0 from dom1 to dom0
	...
	(XEN) [2019-10-10 10:01:19.597] AMD-Vi: Disable: device id = 0x907, domain = 1, paging mode = 3
	(XEN) [2019-10-10 10:01:19.597] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x53572c000, domain = 0, paging mode = 3
	(XEN) [2019-10-10 10:01:19.597] AMD-Vi: Re-assign 0000:09:00.7 from dom1 to dom0

xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d setup:
	(XEN) [2019-10-10 10:21:38.549] d1: bind: m_gsi=47 g_gsi=36 dev=00.00.5 intx=0
	(XEN) [2019-10-10 10:21:38.621] AMD-Vi: Disable: device id = 0x900, domain = 0, paging mode = 3
	(XEN) [2019-10-10 10:21:38.621] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x4aa83b000, domain = 1, paging mode = 3
	(XEN) [2019-10-10 10:21:38.621] AMD-Vi: Re-assign 0000:09:00.0 from dom0 to dom1
	...
	(XEN) [2019-10-10 10:21:46.069] d1: bind: m_gsi=46 g_gsi=36 dev=00.01.4 intx=3
	(XEN) [2019-10-10 10:21:46.089] AMD-Vi: Disable: device id = 0x907, domain = 0, paging mode = 3
	(XEN) [2019-10-10 10:21:46.089] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x4aa83b000, domain = 1, paging mode = 3
	(XEN) [2019-10-10 10:21:46.089] AMD-Vi: Re-assign 0000:09:00.7 from dom0 to dom1


xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d tear-down:
	(XEN) [2019-10-10 10:23:53.167] AMD-Vi: Disable: device id = 0x900, domain = 1, paging mode = 3
	(XEN) [2019-10-10 10:23:53.167] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x5240f8000, domain = 0, paging mode = 3
	(XEN) [2019-10-10 10:23:53.167] AMD-Vi: Re-assign 0000:09:00.0 from dom1 to dom0
	...
	(XEN) [2019-10-10 10:24:43.406] AMD-Vi: Disable: device id = 0x907, domain = 1, paging mode = 3
	(XEN) [2019-10-10 10:24:43.406] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x5240f8000, domain = 0, paging mode = 3
	(XEN) [2019-10-10 10:24:43.406] AMD-Vi: Re-assign 0000:09:00.7 from dom1 to dom0

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Chao Gao]

On Thu, Oct 10, 2019 at 06:13:43PM +0200, Sander Eikelenboom wrote:
>On 01/10/2019 12:35, Anthony PERARD wrote:
>> Rewrite of the commit message:
>> 
>> Before the problematic commit, libxl used to ignore error when
>> destroying (force == true) a passthrough device, especially error that
>> happens when dealing with the DM.
>> 
>> Since fae4880c45fe, if the DM failed to detach the pci device within
>> the allowed time, the timed out error raised skip part of
>> pci_remove_*, but also raise the error up to the caller of
>> libxl__device_pci_destroy_all, libxl__destroy_domid, and thus the
>> destruction of the domain fails.
>> 
>> In this patch, if the DM didn't confirmed that the device is removed,
>> we will print a warning and keep going if force=true.  The patch
>> reorder the functions so that pci_remove_timeout() calls
>> pci_remove_detatched() like it's done when DM calls are successful.
>> 
>> We also clean the QMP states and associated timeouts earlier, as soon
>> as they are not needed anymore.
>> 
>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>> Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> 
>
>Hi Anthony / Chao,
>
>I have to come back to this, a bit because perhaps there is an underlying issue.
>While it earlier occurred to me that the VM to which I passed through most pci-devices 
>(8 to be exact) became very slow to shutdown, but I  didn't investigate it further.
>
>But after you commit messages from this patch it kept nagging, so today I did some testing
>and bisecting.
>
>The difference in tear-down time at least from what the IOMMU code logs is quite large:
>
>xen-4.12.0
>	Setup: 	    7.452 s
>	Tear-down:  7.626 s
>
>xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
>	Setup:      7.468 s
>	Tear-down: 50.239 s
>
>Bisection turned up:
>	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
>	Author: Chao Gao <chao.gao@intel.com>
>	Date:   Fri Jul 19 10:24:08 2019 +0100
>	libxl_qmp: wait for completion of device removal
>
>Which makes me wonder if there is something going wrong in Qemu ?

Hi Sander,

Thanks for your testing and the bisection.

I tried on my machine, the destruction time of a guest with 8 pass-thru
devices increased from 4s to 12s after applied the commit above. In my
understanding, I guess you might get the error message "timed out
waiting for DM to remove...". There might be some issues on your assigned
devices' drivers. You can first unbind the devices with their drivers in
VM and then tear down the VM, and check whether the VM teardown gets
much faster.

Anthony & Wei,

The commit above basically serializes and synchronizes detaching
assigned devices and thus increases VM teardown time significantly if
there are multiple assigned devices. The commit aimed to avoid qemu's
access to PCI configuration space coinciding with the device reset
initiated by xl (which is not desired and is exactly the case which
triggers the assertion in Xen [1]). I personally insist that xl should
wait for DM's completion of device detaching. Otherwise, besides Xen
panic (which can be fixed in another way), in theory, such sudden
unawared device reset might cause a disaster (e.g. data loss for a
storage device).

[1]: https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03287.html

But considering fast creation and teardown is an important benefit of
virtualization, I am not sure how to deal with the situation. Anyway,
you can make the decision. To fix the regression on VM teardown, we can
revert the commit by removing the timeout logic.

What's your opinion?

Thanks
Chao

>
>--
>Sander
>
>
>
>xen-4.12.0 setup:
>	(XEN) [2019-10-10 09:54:14.846] AMD-Vi: Disable: device id = 0x900, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 09:54:14.846] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x4aa847000, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 09:54:14.846] AMD-Vi: Re-assign 0000:09:00.0 from dom0 to dom1
>	...
>	(XEN) [2019-10-10 09:54:22.298] AMD-Vi: Disable: device id = 0x907, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 09:54:22.298] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x4aa847000, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 09:54:22.298] AMD-Vi: Re-assign 0000:09:00.7 from dom0 to dom1
>
>
>xen-4.12.0 tear-down:
>	(XEN) [2019-10-10 10:01:11.971] AMD-Vi: Disable: device id = 0x900, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 10:01:11.971] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x53572c000, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 10:01:11.971] AMD-Vi: Re-assign 0000:09:00.0 from dom1 to dom0
>	...
>	(XEN) [2019-10-10 10:01:19.597] AMD-Vi: Disable: device id = 0x907, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 10:01:19.597] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x53572c000, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 10:01:19.597] AMD-Vi: Re-assign 0000:09:00.7 from dom1 to dom0
>
>xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d setup:
>	(XEN) [2019-10-10 10:21:38.549] d1: bind: m_gsi=47 g_gsi=36 dev=00.00.5 intx=0
>	(XEN) [2019-10-10 10:21:38.621] AMD-Vi: Disable: device id = 0x900, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 10:21:38.621] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x4aa83b000, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 10:21:38.621] AMD-Vi: Re-assign 0000:09:00.0 from dom0 to dom1
>	...
>	(XEN) [2019-10-10 10:21:46.069] d1: bind: m_gsi=46 g_gsi=36 dev=00.01.4 intx=3
>	(XEN) [2019-10-10 10:21:46.089] AMD-Vi: Disable: device id = 0x907, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 10:21:46.089] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x4aa83b000, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 10:21:46.089] AMD-Vi: Re-assign 0000:09:00.7 from dom0 to dom1
>
>
>xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d tear-down:
>	(XEN) [2019-10-10 10:23:53.167] AMD-Vi: Disable: device id = 0x900, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 10:23:53.167] AMD-Vi: Setup I/O page table: device id = 0x900, type = 0x1, root table = 0x5240f8000, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 10:23:53.167] AMD-Vi: Re-assign 0000:09:00.0 from dom1 to dom0
>	...
>	(XEN) [2019-10-10 10:24:43.406] AMD-Vi: Disable: device id = 0x907, domain = 1, paging mode = 3
>	(XEN) [2019-10-10 10:24:43.406] AMD-Vi: Setup I/O page table: device id = 0x907, type = 0x1, root table = 0x5240f8000, domain = 0, paging mode = 3
>	(XEN) [2019-10-10 10:24:43.406] AMD-Vi: Re-assign 0000:09:00.7 from dom1 to dom0

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Sander Eikelenboom]

On 14/10/2019 17:03, Chao Gao wrote:
> On Thu, Oct 10, 2019 at 06:13:43PM +0200, Sander Eikelenboom wrote:
>> On 01/10/2019 12:35, Anthony PERARD wrote:
>>> Rewrite of the commit message:
>>>
>>> Before the problematic commit, libxl used to ignore error when
>>> destroying (force == true) a passthrough device, especially error that
>>> happens when dealing with the DM.
>>>
>>> Since fae4880c45fe, if the DM failed to detach the pci device within
>>> the allowed time, the timed out error raised skip part of
>>> pci_remove_*, but also raise the error up to the caller of
>>> libxl__device_pci_destroy_all, libxl__destroy_domid, and thus the
>>> destruction of the domain fails.
>>>
>>> In this patch, if the DM didn't confirmed that the device is removed,
>>> we will print a warning and keep going if force=true.  The patch
>>> reorder the functions so that pci_remove_timeout() calls
>>> pci_remove_detatched() like it's done when DM calls are successful.
>>>
>>> We also clean the QMP states and associated timeouts earlier, as soon
>>> as they are not needed anymore.
>>>
>>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>>> Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>
>>
>> Hi Anthony / Chao,
>>
>> I have to come back to this, a bit because perhaps there is an underlying issue.
>> While it earlier occurred to me that the VM to which I passed through most pci-devices 
>> (8 to be exact) became very slow to shutdown, but I  didn't investigate it further.
>>
>> But after you commit messages from this patch it kept nagging, so today I did some testing
>> and bisecting.
>>
>> The difference in tear-down time at least from what the IOMMU code logs is quite large:
>>
>> xen-4.12.0
>> 	Setup: 	    7.452 s
>> 	Tear-down:  7.626 s
>>
>> xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
>> 	Setup:      7.468 s
>> 	Tear-down: 50.239 s
>>
>> Bisection turned up:
>> 	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
>> 	Author: Chao Gao <chao.gao@intel.com>
>> 	Date:   Fri Jul 19 10:24:08 2019 +0100
>> 	libxl_qmp: wait for completion of device removal
>>
>> Which makes me wonder if there is something going wrong in Qemu ?
 
> Hi Sander,
Hi Chao,

> 
> Thanks for your testing and the bisection.
> 
> I tried on my machine, the destruction time of a guest with 8 pass-thru
> devices increased from 4s to 12s after applied the commit above.

To what patch are you referring Anthony's or c4b1ef0f89aa6a74faa4618ce3efed1de246ec40 ?

> In my understanding, I guess you might get the error message "timed out
> waiting for DM to remove...". There might be some issues on your assigned
> devices' drivers. You can first unbind the devices with their drivers in
> VM and then tear down the VM, and check whether the VM teardown gets
> much faster.

I get that error message when I test with Anthony's patch applied, the destruction time with that patch is low.

How ever my point was if that patch is correct in the sense that there seems to be an underlying issue 
which causes it to take so long. That issue was uncovered by c4b1ef0f89aa6a74faa4618ce3efed1de246ec40, so I'm not
saying that commit is wrong in any sense, it just uncovered another issue that was already present,
but hard to detect as we just didn't wait at destruction time (and thus the same effect as a timeout).

One or the other way that was just a minor issue until fae4880c45fe015e567afa223f78bf17a6d98e1b, where the long
destruction time now caused the domain destruction to stall, which was then fixed by Antony's patch, but that uses
a timeout which kinds of circumvents the issue, instead of finding out where is comes from and solve it there (
if that is possible of course).

And I wonder if Anthony's patch doesn't interfere with the case you made c4b1ef0f89aa6a74faa4618ce3efed1de246ec40 for, 
if you get the timeout error message as well, then that is kind of not waiting for the destruction to finish, isn't it ?

Chao, 
could you perhaps test for me Xen with as latest commit ee7170822f1fc209f33feb47b268bab35541351d ?
That is before Anthony's patch series, but after your c4b1ef0f89aa6a74faa4618ce3efed1de246ec40.

I would expect to see longer destruction times in the case of 8 pass-throuh devices as well.

Unfortunately Qemu doesn't seem to do much verbose logging even when i enable the debug defines in hw/xen,
especially for the destruction side of things (it mostly logs setting up stuff).

--
Sander
 



> Anthony & Wei,
> 
> The commit above basically serializes and synchronizes detaching
> assigned devices and thus increases VM teardown time significantly if
> there are multiple assigned devices. The commit aimed to avoid qemu's
> access to PCI configuration space coinciding with the device reset
> initiated by xl (which is not desired and is exactly the case which
> triggers the assertion in Xen [1]). I personally insist that xl should
> wait for DM's completion of device detaching. Otherwise, besides Xen
> panic (which can be fixed in another way), in theory, such sudden
> unawared device reset might cause a disaster (e.g. data loss for a
> storage device).
> 
> [1]: https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03287.html
> 
> But considering fast creation and teardown is an important benefit of
> virtualization, I am not sure how to deal with the situation. Anyway,
> you can make the decision. To fix the regression on VM teardown, we can
> revert the commit by removing the timeout logic.
> 
> What's your opinion?
> 
> Thanks
> Chao
> 

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Sander Eikelenboom]

On 15/10/2019 18:59, Sander Eikelenboom wrote:
> On 14/10/2019 17:03, Chao Gao wrote:
>> On Thu, Oct 10, 2019 at 06:13:43PM +0200, Sander Eikelenboom wrote:
>>> On 01/10/2019 12:35, Anthony PERARD wrote:
>>>> Rewrite of the commit message:
>>>>
>>>> Before the problematic commit, libxl used to ignore error when
>>>> destroying (force == true) a passthrough device, especially error that
>>>> happens when dealing with the DM.
>>>>
>>>> Since fae4880c45fe, if the DM failed to detach the pci device within
>>>> the allowed time, the timed out error raised skip part of
>>>> pci_remove_*, but also raise the error up to the caller of
>>>> libxl__device_pci_destroy_all, libxl__destroy_domid, and thus the
>>>> destruction of the domain fails.
>>>>
>>>> In this patch, if the DM didn't confirmed that the device is removed,
>>>> we will print a warning and keep going if force=true.  The patch
>>>> reorder the functions so that pci_remove_timeout() calls
>>>> pci_remove_detatched() like it's done when DM calls are successful.
>>>>
>>>> We also clean the QMP states and associated timeouts earlier, as soon
>>>> as they are not needed anymore.
>>>>
>>>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>>>> Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>>
>>>
>>> Hi Anthony / Chao,
>>>
>>> I have to come back to this, a bit because perhaps there is an underlying issue.
>>> While it earlier occurred to me that the VM to which I passed through most pci-devices 
>>> (8 to be exact) became very slow to shutdown, but I  didn't investigate it further.
>>>
>>> But after you commit messages from this patch it kept nagging, so today I did some testing
>>> and bisecting.
>>>
>>> The difference in tear-down time at least from what the IOMMU code logs is quite large:
>>>
>>> xen-4.12.0
>>> 	Setup: 	    7.452 s
>>> 	Tear-down:  7.626 s
>>>
>>> xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
>>> 	Setup:      7.468 s
>>> 	Tear-down: 50.239 s
>>>
>>> Bisection turned up:
>>> 	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
>>> 	Author: Chao Gao <chao.gao@intel.com>
>>> 	Date:   Fri Jul 19 10:24:08 2019 +0100
>>> 	libxl_qmp: wait for completion of device removal
>>>
>>> Which makes me wonder if there is something going wrong in Qemu ?
>  
>> Hi Sander,
> Hi Chao,
> 
>>
>> Thanks for your testing and the bisection.
>>
>> I tried on my machine, the destruction time of a guest with 8 pass-thru
>> devices increased from 4s to 12s after applied the commit above.
> 
> To what patch are you referring Anthony's or c4b1ef0f89aa6a74faa4618ce3efed1de246ec40 ?
> 
>> In my understanding, I guess you might get the error message "timed out
>> waiting for DM to remove...". There might be some issues on your assigned
>> devices' drivers. You can first unbind the devices with their drivers in
>> VM and then tear down the VM, and check whether the VM teardown gets
>> much faster.

Sorry I forgot to answer your question, I tried unbinding the drivers in
the guest prior to shutting it down, but it didn't make any difference.

--
Sander


> I get that error message when I test with Anthony's patch applied, the destruction time with that patch is low.
> 
> How ever my point was if that patch is correct in the sense that there seems to be an underlying issue 
> which causes it to take so long. That issue was uncovered by c4b1ef0f89aa6a74faa4618ce3efed1de246ec40, so I'm not
> saying that commit is wrong in any sense, it just uncovered another issue that was already present,
> but hard to detect as we just didn't wait at destruction time (and thus the same effect as a timeout).
> 
> One or the other way that was just a minor issue until fae4880c45fe015e567afa223f78bf17a6d98e1b, where the long
> destruction time now caused the domain destruction to stall, which was then fixed by Antony's patch, but that uses
> a timeout which kinds of circumvents the issue, instead of finding out where is comes from and solve it there (
> if that is possible of course).
> 
> And I wonder if Anthony's patch doesn't interfere with the case you made c4b1ef0f89aa6a74faa4618ce3efed1de246ec40 for, 
> if you get the timeout error message as well, then that is kind of not waiting for the destruction to finish, isn't it ?
> 
> Chao, 
> could you perhaps test for me Xen with as latest commit ee7170822f1fc209f33feb47b268bab35541351d ?
> That is before Anthony's patch series, but after your c4b1ef0f89aa6a74faa4618ce3efed1de246ec40.
> 
> I would expect to see longer destruction times in the case of 8 pass-throuh devices as well.
> 
> Unfortunately Qemu doesn't seem to do much verbose logging even when i enable the debug defines in hw/xen,
> especially for the destruction side of things (it mostly logs setting up stuff).
> 
> --
> Sander
>  
> 
> 
> 
>> Anthony & Wei,
>>
>> The commit above basically serializes and synchronizes detaching
>> assigned devices and thus increases VM teardown time significantly if
>> there are multiple assigned devices. The commit aimed to avoid qemu's
>> access to PCI configuration space coinciding with the device reset
>> initiated by xl (which is not desired and is exactly the case which
>> triggers the assertion in Xen [1]). I personally insist that xl should
>> wait for DM's completion of device detaching. Otherwise, besides Xen
>> panic (which can be fixed in another way), in theory, such sudden
>> unawared device reset might cause a disaster (e.g. data loss for a
>> storage device).
>>
>> [1]: https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03287.html
>>
>> But considering fast creation and teardown is an important benefit of
>> virtualization, I am not sure how to deal with the situation. Anyway,
>> you can make the decision. To fix the regression on VM teardown, we can
>> revert the commit by removing the timeout logic.
>>
>> What's your opinion?
>>
>> Thanks
>> Chao
>>
> 
> 


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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Chao Gao]

On Tue, Oct 15, 2019 at 06:59:37PM +0200, Sander Eikelenboom wrote:
>On 14/10/2019 17:03, Chao Gao wrote:
>> On Thu, Oct 10, 2019 at 06:13:43PM +0200, Sander Eikelenboom wrote:
>>> On 01/10/2019 12:35, Anthony PERARD wrote:
>>>> Rewrite of the commit message:
>>>>
>>>> Before the problematic commit, libxl used to ignore error when
>>>> destroying (force == true) a passthrough device, especially error that
>>>> happens when dealing with the DM.
>>>>
>>>> Since fae4880c45fe, if the DM failed to detach the pci device within
>>>> the allowed time, the timed out error raised skip part of
>>>> pci_remove_*, but also raise the error up to the caller of
>>>> libxl__device_pci_destroy_all, libxl__destroy_domid, and thus the
>>>> destruction of the domain fails.
>>>>
>>>> In this patch, if the DM didn't confirmed that the device is removed,
>>>> we will print a warning and keep going if force=true.  The patch
>>>> reorder the functions so that pci_remove_timeout() calls
>>>> pci_remove_detatched() like it's done when DM calls are successful.
>>>>
>>>> We also clean the QMP states and associated timeouts earlier, as soon
>>>> as they are not needed anymore.
>>>>
>>>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>>>> Fixes: fae4880c45fe015e567afa223f78bf17a6d98e1b
>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>>
>>>
>>> Hi Anthony / Chao,
>>>
>>> I have to come back to this, a bit because perhaps there is an underlying issue.
>>> While it earlier occurred to me that the VM to which I passed through most pci-devices 
>>> (8 to be exact) became very slow to shutdown, but I  didn't investigate it further.
>>>
>>> But after you commit messages from this patch it kept nagging, so today I did some testing
>>> and bisecting.
>>>
>>> The difference in tear-down time at least from what the IOMMU code logs is quite large:
>>>
>>> xen-4.12.0
>>> 	Setup: 	    7.452 s
>>> 	Tear-down:  7.626 s
>>>
>>> xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
>>> 	Setup:      7.468 s
>>> 	Tear-down: 50.239 s
>>>
>>> Bisection turned up:
>>> 	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
>>> 	Author: Chao Gao <chao.gao@intel.com>
>>> 	Date:   Fri Jul 19 10:24:08 2019 +0100
>>> 	libxl_qmp: wait for completion of device removal
>>>
>>> Which makes me wonder if there is something going wrong in Qemu ?
> 
>> Hi Sander,
>Hi Chao,
>
>> 
>> Thanks for your testing and the bisection.
>> 
>> I tried on my machine, the destruction time of a guest with 8 pass-thru
>> devices increased from 4s to 12s after applied the commit above.
>
>To what patch are you referring Anthony's or c4b1ef0f89aa6a74faa4618ce3efed1de246ec40 ?

The latter.

>
>> In my understanding, I guess you might get the error message "timed out
>> waiting for DM to remove...". There might be some issues on your assigned
>> devices' drivers. You can first unbind the devices with their drivers in
>> VM and then tear down the VM, and check whether the VM teardown gets
>> much faster.
>
>I get that error message when I test with Anthony's patch applied, the destruction time with that patch is low.
>
>How ever my point was if that patch is correct in the sense that there seems to be an underlying issue 
>which causes it to take so long. That issue was uncovered by c4b1ef0f89aa6a74faa4618ce3efed1de246ec40, so I'm not
>saying that commit is wrong in any sense, it just uncovered another issue that was already present,
>but hard to detect as we just didn't wait at destruction time (and thus the same effect as a timeout).

Actually, it is introduced by c4b1ef0f89, though it did fix another
issue.

>
>One or the other way that was just a minor issue until fae4880c45fe015e567afa223f78bf17a6d98e1b, where the long
>destruction time now caused the domain destruction to stall, which was then fixed by Antony's patch, but that uses
>a timeout which kinds of circumvents the issue, instead of finding out where is comes from and solve it there (
>if that is possible of course).
>
>And I wonder if Anthony's patch doesn't interfere with the case you made c4b1ef0f89aa6a74faa4618ce3efed1de246ec40 for, 
>if you get the timeout error message as well, then that is kind of not waiting for the destruction to finish, isn't it ?
>
>Chao, 
>could you perhaps test for me Xen with as latest commit ee7170822f1fc209f33feb47b268bab35541351d ?
>That is before Anthony's patch series, but after your c4b1ef0f89aa6a74faa4618ce3efed1de246ec40.

It's actually what I did. VM teardown with 8 pass-thru devices on my
side takes 12s which only took 4s without my patch.

Thanks
Chao

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Anthony PERARD]

On Mon, Oct 14, 2019 at 11:03:43PM +0800, Chao Gao wrote:
> On Thu, Oct 10, 2019 at 06:13:43PM +0200, Sander Eikelenboom wrote:
> >Hi Anthony / Chao,
> >
> >I have to come back to this, a bit because perhaps there is an underlying issue.
> >While it earlier occurred to me that the VM to which I passed through most pci-devices 
> >(8 to be exact) became very slow to shutdown, but I  didn't investigate it further.
> >
> >But after you commit messages from this patch it kept nagging, so today I did some testing
> >and bisecting.
> >
> >The difference in tear-down time at least from what the IOMMU code logs is quite large:
> >
> >xen-4.12.0
> >	Setup: 	    7.452 s
> >	Tear-down:  7.626 s
> >
> >xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
> >	Setup:      7.468 s
> >	Tear-down: 50.239 s
> >
> >Bisection turned up:
> >	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
> >	Author: Chao Gao <chao.gao@intel.com>
> >	Date:   Fri Jul 19 10:24:08 2019 +0100
> >	libxl_qmp: wait for completion of device removal
> >
> >Which makes me wonder if there is something going wrong in Qemu ?
> 
> Hi Sander,
> 
> Thanks for your testing and the bisection.
> 
> I tried on my machine, the destruction time of a guest with 8 pass-thru
> devices increased from 4s to 12s after applied the commit above. In my
> understanding, I guess you might get the error message "timed out
> waiting for DM to remove...". There might be some issues on your assigned
> devices' drivers. You can first unbind the devices with their drivers in
> VM and then tear down the VM, and check whether the VM teardown gets
> much faster.

Hi,

Chao, I think you've tested `xl destroy`, and Sander, I think your are
speaking about `xl shutdown` or simply power off of a guest. Well, these
two operations are a bit different, on destroy the guest kernel is
still running when the pci devices are been removed, but on shutdown the
guest kernel is gone.

I don't think there's anything wrong with QEMU or with the devices, it
just that when the toolstack ask QEMU to unplug the pci device, QEMU
will ask the guest kernel first. So the guest may never acknowledge the
removal and QEMU will not let go of the pci device. There is actually an
old Xen commit about that:
77fea72b068d25afb7e63947aba32b487d7124a2, and a comment in the code:
    /* This depends on guest operating system acknowledging the
     * SCI, if it doesn't respond in time then we may wish to
     * force the removal. */


> Anthony & Wei,
> 
> The commit above basically serializes and synchronizes detaching
> assigned devices and thus increases VM teardown time significantly if
> there are multiple assigned devices. The commit aimed to avoid qemu's
> access to PCI configuration space coinciding with the device reset
> initiated by xl (which is not desired and is exactly the case which
> triggers the assertion in Xen [1]). I personally insist that xl should
> wait for DM's completion of device detaching. Otherwise, besides Xen
> panic (which can be fixed in another way), in theory, such sudden
> unawared device reset might cause a disaster (e.g. data loss for a
> storage device).
> 
> [1]: https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03287.html
> 
> But considering fast creation and teardown is an important benefit of
> virtualization, I am not sure how to deal with the situation. Anyway,
> you can make the decision. To fix the regression on VM teardown, we can
> revert the commit by removing the timeout logic.
> 
> What's your opinion?

It probably a good idea to wait a bit until QEMU has detach the device.
For cases where QEMU will never detach the device (the guest kernel is
shutdown), we could reduce the timeout. Following my changes to pci
passthrough handling in libxl, the timeout is 10s for one device (and
probably 10s for many; I don't think libxl will even ask qemu to remove
the other devices if the first one timeout).

So, maybe we could wait for 5s for QEMU to detach the pci device? As
past that time, it will probably never happen. This still mean about +5s
to tear-down compare to previous releases. (Or maybe +5s per device if
we have to do one device at a time.)

There are other issues with handling multiple pci passthrough devices,
so I don't have patches yet.

Cheers,

-- 
Anthony PERARD

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

Re: [Xen-devel] [PATCH 2/2] libxl_pci: Fix guest shutdown with PCI PT attached

[Sander Eikelenboom]

On 18/10/2019 18:11, Anthony PERARD wrote:
> On Mon, Oct 14, 2019 at 11:03:43PM +0800, Chao Gao wrote:
>> On Thu, Oct 10, 2019 at 06:13:43PM +0200, Sander Eikelenboom wrote:
>>> Hi Anthony / Chao,
>>>
>>> I have to come back to this, a bit because perhaps there is an underlying issue.
>>> While it earlier occurred to me that the VM to which I passed through most pci-devices 
>>> (8 to be exact) became very slow to shutdown, but I  didn't investigate it further.
>>>
>>> But after you commit messages from this patch it kept nagging, so today I did some testing
>>> and bisecting.
>>>
>>> The difference in tear-down time at least from what the IOMMU code logs is quite large:
>>>
>>> xen-4.12.0
>>> 	Setup: 	    7.452 s
>>> 	Tear-down:  7.626 s
>>>
>>> xen-unstable-ee7170822f1fc209f33feb47b268bab35541351d
>>> 	Setup:      7.468 s
>>> 	Tear-down: 50.239 s
>>>
>>> Bisection turned up:
>>> 	commit c4b1ef0f89aa6a74faa4618ce3efed1de246ec40
>>> 	Author: Chao Gao <chao.gao@intel.com>
>>> 	Date:   Fri Jul 19 10:24:08 2019 +0100
>>> 	libxl_qmp: wait for completion of device removal
>>>
>>> Which makes me wonder if there is something going wrong in Qemu ?
>>
>> Hi Sander,
>>
>> Thanks for your testing and the bisection.
>>
>> I tried on my machine, the destruction time of a guest with 8 pass-thru
>> devices increased from 4s to 12s after applied the commit above. In my
>> understanding, I guess you might get the error message "timed out
>> waiting for DM to remove...". There might be some issues on your assigned
>> devices' drivers. You can first unbind the devices with their drivers in
>> VM and then tear down the VM, and check whether the VM teardown gets
>> much faster.
> 
> Hi,
> 
> Chao, I think you've tested `xl destroy`, and Sander, I think your are
> speaking about `xl shutdown` or simply power off of a guest. Well, these
> two operations are a bit different, on destroy the guest kernel is
> still running when the pci devices are been removed, but on shutdown the
> guest kernel is gone.
> 
> I don't think there's anything wrong with QEMU or with the devices, it
> just that when the toolstack ask QEMU to unplug the pci device, QEMU
> will ask the guest kernel first. So the guest may never acknowledge the
> removal and QEMU will not let go of the pci device. There is actually an
> old Xen commit about that:
> 77fea72b068d25afb7e63947aba32b487d7124a2, and a comment in the code:
>     /* This depends on guest operating system acknowledging the
>      * SCI, if it doesn't respond in time then we may wish to
>      * force the removal. */

Hi Anthony,

Correct I was referring to the behavior with "xl shutdown".

The above is interesting, my follow up question would be:
Does Qemu know / keep a state when the guest kernel is gone ?
Because if it does, we could make the need to ackknowledge removal be
conditional on that ?

--
Sander


>> Anthony & Wei,
>>
>> The commit above basically serializes and synchronizes detaching
>> assigned devices and thus increases VM teardown time significantly if
>> there are multiple assigned devices. The commit aimed to avoid qemu's
>> access to PCI configuration space coinciding with the device reset
>> initiated by xl (which is not desired and is exactly the case which
>> triggers the assertion in Xen [1]). I personally insist that xl should
>> wait for DM's completion of device detaching. Otherwise, besides Xen
>> panic (which can be fixed in another way), in theory, such sudden
>> unawared device reset might cause a disaster (e.g. data loss for a
>> storage device).
>>
>> [1]: https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg03287.html
>>
>> But considering fast creation and teardown is an important benefit of
>> virtualization, I am not sure how to deal with the situation. Anyway,
>> you can make the decision. To fix the regression on VM teardown, we can
>> revert the commit by removing the timeout logic.
>>
>> What's your opinion?
> 
> It probably a good idea to wait a bit until QEMU has detach the device.
> For cases where QEMU will never detach the device (the guest kernel is
> shutdown), we could reduce the timeout. Following my changes to pci
> passthrough handling in libxl, the timeout is 10s for one device (and
> probably 10s for many; I don't think libxl will even ask qemu to remove
> the other devices if the first one timeout).
> 
> So, maybe we could wait for 5s for QEMU to detach the pci device? As
> past that time, it will probably never happen. This still mean about +5s
> to tear-down compare to previous releases. (Or maybe +5s per device if
> we have to do one device at a time.)
> 
> There are other issues with handling multiple pci passthrough devices,
> so I don't have patches yet.
> 
> Cheers,
> 


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