Xen devel - Feb 2011 - [PATCH 1/2] libxl: do not ignore errors from libxl_device_pci_add_xenstore in do_pci_add

Without this, some failures of PCI device passthrough would be
ignored.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index ba59650..d6457b9 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -718,8 +718,8 @@ out:
         }
     }
 
-    libxl_device_pci_add_xenstore(gc, domid, pcidev);
-    return 0;
+    rc = libxl_device_pci_add_xenstore(gc, domid, pcidev, starting);
+    return rc;
 }
 
 static int libxl_device_pci_reset(libxl__gc *gc, unsigned int domain, unsigned
int bus,
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

When doing a PCI passthrough, the code checks to see whether there is
an existing backend directory in xenstore with a nonzero "num_devs".
If there isn''t, it creates the backend directory with just the
required device.

If there is, it would assume that it was doing hotplug.  If doing
hotplug, it needs to set the "state" node in xenstore to "7"
(reconfiguring) and thus avoid racing with the backend needs to wait
for the backend to be "4" (connected).

However during guest creation, the presence of "num_devs"
doesn''t
necessarily mean hotplug.  If we are still creating the initial
xenstore setup (ie, adding devices as a subroutine of domain
creation), we can just write the new devices to xenstore.  So do that.

This involves adding a new parameter "starting", indicating that we
are still in domain creation, to libxl_device_pci_add_xenstore (a
misnamed internal function) and its callers.  Its callers include
libxl_device_pci_add which we therefore split into an internal version
with the new parameter, and an external version used only for hotplug
by libxl-using applications.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_create.c   |    2 +-
 tools/libxl/libxl_internal.h |    4 ++++
 tools/libxl/libxl_pci.c      |   18 ++++++++++++------
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d94480e..b0939cc 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -538,7 +538,7 @@ static int do_domain_create(libxl_ctx *ctx,
libxl_domain_config *d_config,
     }
 
     for (i = 0; i < d_config->num_pcidevs; i++)
-        libxl_device_pci_add(ctx, domid, &d_config->pcidevs[i]);
+        libxl__device_pci_add(ctx, domid, &d_config->pcidevs[i], 1);
 
     if ( cb && (d_config->c_info.hvm ||
d_config->b_info.u.pv.bootloader )) {
         if ( (*cb)(ctx, domid, priv) )
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index bc6dfa7..277b04c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -200,6 +200,10 @@ _hidden int libxl__wait_for_device_model(libxl_ctx *ctx,
                                 void *check_callback_userdata);
 _hidden int libxl__wait_for_backend(libxl_ctx *ctx, char *be_path, char
*state);
 
+/* from libxl_pci */
+
+_hidden int libxl__device_pci_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_pci *pcidev, int starting);
+
 /* xl_exec */
 
  /* higher-level double-fork and separate detach eg as for device models */
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index d6457b9..6a9cdba 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -286,7 +286,7 @@ out:
     return 0;
 }
 
-static int libxl_device_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev)
+static int libxl_device_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int starting)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     flexarray_t *back;
@@ -299,7 +299,7 @@ static int libxl_device_pci_add_xenstore(libxl__gc *gc,
uint32_t domid, libxl_de
     if (!num_devs)
         return libxl_create_pci_backend(gc, domid, pcidev, 1);
 
-    if (!libxl__domain_is_hvm(ctx, domid)) {
+    if (!starting && !libxl__domain_is_hvm(ctx, domid)) {
         if (libxl__wait_for_backend(ctx, be_path, "4") < 0)
             return ERROR_FAIL;
     }
@@ -312,7 +312,8 @@ static int libxl_device_pci_add_xenstore(libxl__gc *gc,
uint32_t domid, libxl_de
     num = atoi(num_devs);
     libxl_create_pci_backend_device(gc, back, num, pcidev);
     flexarray_vappend(back, "num_devs", libxl__sprintf(gc,
"%d", num + 1), NULL);
-    flexarray_vappend(back, "state", libxl__sprintf(gc,
"%d", 7), NULL);
+    if (!starting)
+        flexarray_vappend(back, "state", libxl__sprintf(gc,
"%d", 7), NULL);
 
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
@@ -616,7 +617,7 @@ static int pci_ins_check(libxl_ctx *ctx, uint32_t domid,
const char *state, void
     return 1;
 }
  
-static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev)
+static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev,
int starting)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *path;
@@ -760,6 +761,11 @@ static int libxl_device_pci_reset(libxl__gc *gc, unsigned
int domain, unsigned i
 
 int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci
*pcidev)
 {
+    return libxl__device_pci_add(ctx, domid, pcidev, 0);
+}
+
+int libxl__device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci
*pcidev, int starting)
+{
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     unsigned int orig_vdev, pfunc_mask;
     libxl_device_pci *assigned;
@@ -783,7 +789,7 @@ int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_pci *pcide
     stubdomid = libxl_get_stubdom_id(ctx, domid);
     if (stubdomid != 0) {
         libxl_device_pci pcidev_s = *pcidev;
-        rc = do_pci_add(&gc, stubdomid, &pcidev_s);
+        rc = do_pci_add(&gc, stubdomid, &pcidev_s, starting);
         if ( rc )
             goto out;
     }
@@ -818,7 +824,7 @@ int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_pci *pcide
                  */
                 pcidev->vdevfn = orig_vdev;
             }
-            if ( do_pci_add(&gc, domid, pcidev) )
+            if ( do_pci_add(&gc, domid, pcidev, starting) )
                 rc = ERROR_FAIL;
         }
     }
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 23 Feb 2011, Ian Jackson wrote:
> When doing a PCI passthrough, the code checks to see whether there is
> an existing backend directory in xenstore with a nonzero
"num_devs".
> If there isn''t, it creates the backend directory with just the
> required device.
> 
> If there is, it would assume that it was doing hotplug.  If doing
> hotplug, it needs to set the "state" node in xenstore to
"7"
> (reconfiguring) and thus avoid racing with the backend needs to wait
> for the backend to be "4" (connected).
> 
> However during guest creation, the presence of "num_devs"
doesn''t
> necessarily mean hotplug.  If we are still creating the initial
> xenstore setup (ie, adding devices as a subroutine of domain
> creation), we can just write the new devices to xenstore.  So do that.
> 
> This involves adding a new parameter "starting", indicating that
we
> are still in domain creation, to libxl_device_pci_add_xenstore (a
> misnamed internal function) and its callers.  Its callers include
> libxl_device_pci_add which we therefore split into an internal version
> with the new parameter, and an external version used only for hotplug
> by libxl-using applications.
> 

nice and simple 

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 2011-02-23 at 16:25 +0000, Stefano Stabellini
wrote:
> On Wed, 23 Feb 2011, Ian Jackson wrote:
> > When doing a PCI passthrough, the code checks to see whether there is
> > an existing backend directory in xenstore with a nonzero
"num_devs".
> > If there isn''t, it creates the backend directory with just
the
> > required device.
> > 
> > If there is, it would assume that it was doing hotplug.  If doing
> > hotplug, it needs to set the "state" node in xenstore to
"7"
> > (reconfiguring) and thus avoid racing with the backend needs to wait
> > for the backend to be "4" (connected).
> > 
> > However during guest creation, the presence of "num_devs"
doesn''t
> > necessarily mean hotplug.  If we are still creating the initial
> > xenstore setup (ie, adding devices as a subroutine of domain
> > creation), we can just write the new devices to xenstore.  So do that.
> > 
> > This involves adding a new parameter "starting", indicating
that we
> > are still in domain creation, to libxl_device_pci_add_xenstore (a
> > misnamed internal function) and its callers.  Its callers include
> > libxl_device_pci_add which we therefore split into an internal version
> > with the new parameter, and an external version used only for hotplug
> > by libxl-using applications.
> > 
> 
> 
> nice and simple 
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Yep, looks good:

Acked-by: Ian Campbell <ian.campbell@citrix.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] libxl: Multi-device
passthrough coldplug:	do not wait for unstarted
guest"):
> On Wed, 2011-02-23 at 16:25 +0000, Stefano Stabellini wrote:
> > On Wed, 23 Feb 2011, Ian Jackson wrote:
> > > When doing a PCI passthrough, the code checks to see whether
there is
> > > an existing backend directory in xenstore with a nonzero
"num_devs".
> > > If there isn''t, it creates the backend directory with
just the
> > > required device.
Thanks for your responses, I have applied those two patches.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel