diff mbox series

[v4] libxl/PCI: Fix PV hotplug & stubdom coldplug

Message ID 20220113040142.20503-1-jandryuk@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v4] libxl/PCI: Fix PV hotplug & stubdom coldplug | expand

Commit Message

Jason Andryuk Jan. 13, 2022, 4:01 a.m. UTC
commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are
reflected in the config" broken PCI hotplug (xl pci-attach) for PV
domains when it moved libxl__create_pci_backend() later in the function.

This also broke HVM + stubdom PCI passthrough coldplug.  For that, the
PCI devices are hotplugged to a running PV stubdom, and then the QEMU
QMP device_add commands are made to QEMU inside the stubdom.

A running PV domain calls libxl__wait_for_backend().  With the current
placement of libxl__create_pci_backend(), the path does not exist and
the call immediately fails:
libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist
libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3)
libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices

The wait is only relevant when:
1) The domain is PV
3) The domain is running
2) The backend is already present

1) xen-pcifront is only used for PV.  It does not load for HVM domains
   where QEMU is used.

2) If the domain is not running (starting), then the frontend state will
   be Initialising.  xen-pciback waits for the frontend to transition to
   at Initialised before attempting to connect.  So a wait for a
   non-running domain is not applicable as the backend will not
   transition to Connected.

3) For presence, num_devs is already used to determine if the backend
   needs to be created.  Re-use num_devs to determine if the backend
   wait is necessary.  The wait is necessary to avoid racing with
   another PCI attachment reconfiguring the front/back or changing to
   some other state like closing.  If we are creating the backend, then
   we don't have to worry about the state since it is being created.

Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are
reflected in the config")

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Alternative to Jan's patch:
https://lore.kernel.org/xen-devel/5114ae87-bc0e-3d58-e16e-6d9d2fee0801@suse.com/

v4:
Use if (rc) return rc
Capitalize comment sentence
Change commit message

v3:
Change title & commit message

v2:
Add Fixes
Expand num_devs use in commit message
---
 tools/libs/light/libxl_pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Paul Durrant Jan. 13, 2022, 8:30 a.m. UTC | #1
On 13/01/2022 04:01, Jason Andryuk wrote:
> commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are
> reflected in the config" broken PCI hotplug (xl pci-attach) for PV
> domains when it moved libxl__create_pci_backend() later in the function.
> 
> This also broke HVM + stubdom PCI passthrough coldplug.  For that, the
> PCI devices are hotplugged to a running PV stubdom, and then the QEMU
> QMP device_add commands are made to QEMU inside the stubdom.
> 
> A running PV domain calls libxl__wait_for_backend().  With the current
> placement of libxl__create_pci_backend(), the path does not exist and
> the call immediately fails:
> libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist
> libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3)
> libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices
> 
> The wait is only relevant when:
> 1) The domain is PV
> 3) The domain is running
> 2) The backend is already present

The numbering above needs fixing and I guess you want a "Because..." here.

> 
> 1) xen-pcifront is only used for PV.  It does not load for HVM domains
>     where QEMU is used.
> 
> 2) If the domain is not running (starting), then the frontend state will
>     be Initialising.  xen-pciback waits for the frontend to transition to
>     at Initialised before attempting to connect.  So a wait for a
>     non-running domain is not applicable as the backend will not
>     transition to Connected.
> 
> 3) For presence, num_devs is already used to determine if the backend
>     needs to be created.  Re-use num_devs to determine if the backend
>     wait is necessary.  The wait is necessary to avoid racing with
>     another PCI attachment reconfiguring the front/back or changing to
>     some other state like closing.  If we are creating the backend, then
>     we don't have to worry about the state since it is being created.
> 
> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are
> reflected in the config")
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

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

> ---
> Alternative to Jan's patch:
> https://lore.kernel.org/xen-devel/5114ae87-bc0e-3d58-e16e-6d9d2fee0801@suse.com/
> 
> v4:
> Use if (rc) return rc
> Capitalize comment sentence
> Change commit message
> 
> v3:
> Change title & commit message
> 
> v2:
> Add Fixes
> Expand num_devs use in commit message
> ---
>   tools/libs/light/libxl_pci.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 4c2d7aeefb..4bbbfe9f16 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -157,9 +157,11 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc,
>       if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
>           return ERROR_FAIL;
>   
> -    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
> -        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0)
> -            return ERROR_FAIL;
> +    /* Wait is only needed if the backend already exists (num_devs != NULL) */
> +    if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
> +        rc = libxl__wait_for_backend(gc, be_path,
> +                                     GCSPRINTF("%d", XenbusStateConnected));
> +        if (rc) return rc;
>       }
>   
>       back = flexarray_make(gc, 16, 1);
Anthony PERARD Jan. 13, 2022, 10:39 a.m. UTC | #2
On Wed, Jan 12, 2022 at 11:01:42PM -0500, Jason Andryuk wrote:
> commit 0fdb48ffe7a1 "libxl: Make sure devices added by pci-attach are
> reflected in the config" broken PCI hotplug (xl pci-attach) for PV
> domains when it moved libxl__create_pci_backend() later in the function.
> 
> This also broke HVM + stubdom PCI passthrough coldplug.  For that, the
> PCI devices are hotplugged to a running PV stubdom, and then the QEMU
> QMP device_add commands are made to QEMU inside the stubdom.
> 
> A running PV domain calls libxl__wait_for_backend().  With the current
> placement of libxl__create_pci_backend(), the path does not exist and
> the call immediately fails:
> libxl: error: libxl_device.c:1388:libxl__wait_for_backend: Backend /local/domain/0/backend/pci/43/0 does not exist
> libxl: error: libxl_pci.c:1764:device_pci_add_done: Domain 42:libxl__device_pci_add failed for PCI device 0:2:0.0 (rc -3)
> libxl: error: libxl_create.c:1857:domcreate_attach_devices: Domain 42:unable to add pci devices
> 
> The wait is only relevant when:
> 1) The domain is PV
> 3) The domain is running
> 2) The backend is already present
> 
> 1) xen-pcifront is only used for PV.  It does not load for HVM domains
>    where QEMU is used.
> 
> 2) If the domain is not running (starting), then the frontend state will
>    be Initialising.  xen-pciback waits for the frontend to transition to
>    at Initialised before attempting to connect.  So a wait for a
>    non-running domain is not applicable as the backend will not
>    transition to Connected.
> 
> 3) For presence, num_devs is already used to determine if the backend
>    needs to be created.  Re-use num_devs to determine if the backend
>    wait is necessary.  The wait is necessary to avoid racing with
>    another PCI attachment reconfiguring the front/back or changing to
>    some other state like closing.  If we are creating the backend, then
>    we don't have to worry about the state since it is being created.
> 
> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are
> reflected in the config")
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

That patch is probably good enough for now, even if there is probably need to
rework the function with regards to issue raised in
https://lore.kernel.org/xen-devel/24859.43747.87671.739214@mariner.uk.xensource.com/
in the thread.

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

Thanks,
diff mbox series

Patch

diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 4c2d7aeefb..4bbbfe9f16 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -157,9 +157,11 @@  static int libxl__device_pci_add_xenstore(libxl__gc *gc,
     if (domtype == LIBXL_DOMAIN_TYPE_INVALID)
         return ERROR_FAIL;
 
-    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
-        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", XenbusStateConnected)) < 0)
-            return ERROR_FAIL;
+    /* Wait is only needed if the backend already exists (num_devs != NULL) */
+    if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
+        rc = libxl__wait_for_backend(gc, be_path,
+                                     GCSPRINTF("%d", XenbusStateConnected));
+        if (rc) return rc;
     }
 
     back = flexarray_make(gc, 16, 1);