Re: [PATCH] libxl/PCI: defer backend wait upon attaching to PV guest

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Dec 14, 2021 at 2:50 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> Attempting to wait when the backend hasn't been created yet can't work:
> the function will complain "Backend ... does not exist". Move the
> waiting past the creation of the backend (and that of other related
> nodes), hoping that there are no other dependencies that would now be
> broken.
>
> Fixes: 0fdb48ffe7a1 ("libxl: Make sure devices added by pci-attach are 
> reflected in the config")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Just to make it explicit: I have no idea why the waiting is needed in
> the first place. It's been there from the very introduction of PCI
> passthrough support (commit b0a1af61678b). I therefore can't exclude
> that an even better fix would be to simply omit the 2nd hunk here.

The first time a device is attached, the backend does not exist, and
the wait is not needed.  However, when a second device is attached,
the backend does exist.  Since pciback goes through Reconfiguring and
Reconfigured, I believe the wait exists to let the frontend/backend
settle back to Connected before modifying the xenstore entries to add
the additional device.  I could be wrong, but that is my best answer
for why someone went to the trouble of adding a wait in the first
place.

Prior to 0fdb48ffe7a1, the backend was created before the watch:
     num_devs = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/num_devs", be_path));
-    if (!num_devs)
-        return libxl__create_pci_backend(gc, domid, pci, 1);

     libxl_domain_type domtype = libxl__domain_type(gc, domid);
     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;
     }

Here and elsewhere, num_devs has been used to identify pre-existing
backends.  That's why I went with the following to address this:
-    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
-        if (libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d",
XenbusStateConnected)) < 0)
+    /* wait is only needed if the backend already exists (num_devs != NULL) */
+    if (num_devs && !starting && domtype == LIBXL_DOMAIN_TYPE_PV) {
+        if (libxl__wait_for_backend(gc, be_path,
+                                    GCSPRINTF("%d", XenbusStateConnected)) < 0)

Regards,
Jason

> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -157,11 +157,6 @@ static int libxl__device_pci_add_xenstor
>      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;
> -    }
> -
>      back = flexarray_make(gc, 16, 1);
>
>      LOGD(DEBUG, domid, "Adding new pci device to xenstore");
> @@ -213,6 +208,9 @@ static int libxl__device_pci_add_xenstor
>          if (rc < 0) goto out;
>      }
>
> +    if (!starting && domtype == LIBXL_DOMAIN_TYPE_PV)
> +        rc = libxl__wait_for_backend(gc, be_path, GCSPRINTF("%d", 
> XenbusStateConnected));
> +
>  out:
>      libxl__xs_transaction_abort(gc, &t);
>      if (lock) libxl__unlock_file(lock);
>
>