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

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

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@xxxxxxxxx>
Reviewed-by: Paul Durrant <paul@xxxxxxx>

> ---
> Alternative to Jan's patch:
> https://lore.kernel.org/xen-devel/5114ae87-bc0e-3d58-e16e-6d9d2fee0801@xxxxxxxx/
>
> 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);