[PATCH] libxl: slightly refine pci-assignable-{add, remove} handling

[PATCH] libxl: slightly refine pci-assignable-{add, remove} handling

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3938 bytes --]

While it appears to be intentional for "xl pci-assignable-remove" to
not re-bind the original driver by default (requires the -r option),
permanently losing the information which driver was originally used
seems bad. Make "add; remove; add; remove -r" re-bind the original
driver by allowing "remove" to delete the information only upon
successful re-bind.

In the course of this I also noticed that binding information is lost
when upon first "add" pciback isn't loaded yet, due to its presence not
being checked for early enough. Adjust pciback_dev_is_assigned()
accordingly, and properly distinguish "yes" and "error" returns in the
"add" case (removing a redundant error message from the "remove" path
for consistency).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
backported later on.

--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -543,6 +543,17 @@ static int pciback_dev_is_assigned(libxl
     int rc;
     struct stat st;
 
+    if ( access(SYSFS_PCIBACK_DRIVER, F_OK) < 0 ) {
+        if ( errno == ENOENT ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "Looks like pciback driver is not loaded");
+        } else {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "Can't access "SYSFS_PCIBACK_DRIVER);
+        }
+        return -1;
+    }
+
     spath = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
                            pcidev->domain, pcidev->bus,
                            pcidev->dev, pcidev->func);
@@ -658,6 +669,7 @@ static int libxl__device_pci_assignable_
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned dom, bus, dev, func;
     char *spath, *driver_path = NULL;
+    int rc;
     struct stat st;
 
     /* Local copy for convenience */
@@ -674,7 +686,11 @@ static int libxl__device_pci_assignable_
     }
 
     /* Check to see if it's already assigned to pciback */
-    if ( pciback_dev_is_assigned(gc, pcidev) ) {
+    rc = pciback_dev_is_assigned(gc, pcidev);
+    if ( rc < 0 ) {
+        return ERROR_FAIL;
+    }
+    if ( rc ) {
         LIBXL__LOG(ctx, LIBXL__LOG_WARNING, PCI_BDF" already assigned to pciback",
                    dom, bus, dev, func);
         return 0;
@@ -692,11 +708,18 @@ static int libxl__device_pci_assignable_
     if ( rebind ) {
         if ( driver_path ) {
             pci_assignable_driver_path_write(gc, pcidev, driver_path);
+        } else if ( (driver_path =
+                     pci_assignable_driver_path_read(gc, pcidev)) != NULL ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_INFO,
+                       PCI_BDF" not bound to a driver, will be rebound to %s",
+                       dom, bus, dev, func, driver_path);
         } else {
             LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
                        PCI_BDF" not bound to a driver, will not be rebound.",
                        dom, bus, dev, func);
         }
+    } else {
+        pci_assignable_driver_path_remove(gc, pcidev);
     }
 
     if ( pciback_dev_assign(gc, pcidev) ) {
@@ -717,7 +740,6 @@ static int libxl__device_pci_assignable_
 
     /* Unbind from pciback */
     if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Checking if pciback was assigned");
         return ERROR_FAIL;
     } else if ( rc ) {
         pciback_dev_unassign(gc, pcidev);
@@ -741,9 +763,9 @@ static int libxl__device_pci_assignable_
                                  "Couldn't bind device to %s", driver_path);
                 return -1;
             }
-        }
 
-        pci_assignable_driver_path_remove(gc, pcidev);
+            pci_assignable_driver_path_remove(gc, pcidev);
+        }
     } else {
         if ( rebind ) {
             LIBXL__LOG(ctx, LIBXL__LOG_WARNING,




[-- Attachment #2: libxl-pci-assignable.patch --]
[-- Type: text/plain, Size: 3995 bytes --]

libxl: slightly refine pci-assignable-{add,remove} handling

While it appears to be intentional for "xl pci-assignable-remove" to
not re-bind the original driver by default (requires the -r option),
permanently losing the information which driver was originally used
seems bad. Make "add; remove; add; remove -r" re-bind the original
driver by allowing "remove" to delete the information only upon
successful re-bind.

In the course of this I also noticed that binding information is lost
when upon first "add" pciback isn't loaded yet, due to its presence not
being checked for early enough. Adjust pciback_dev_is_assigned()
accordingly, and properly distinguish "yes" and "error" returns in the
"add" case (removing a redundant error message from the "remove" path
for consistency).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
backported later on.

--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -543,6 +543,17 @@ static int pciback_dev_is_assigned(libxl
     int rc;
     struct stat st;
 
+    if ( access(SYSFS_PCIBACK_DRIVER, F_OK) < 0 ) {
+        if ( errno == ENOENT ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "Looks like pciback driver is not loaded");
+        } else {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "Can't access "SYSFS_PCIBACK_DRIVER);
+        }
+        return -1;
+    }
+
     spath = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
                            pcidev->domain, pcidev->bus,
                            pcidev->dev, pcidev->func);
@@ -658,6 +669,7 @@ static int libxl__device_pci_assignable_
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned dom, bus, dev, func;
     char *spath, *driver_path = NULL;
+    int rc;
     struct stat st;
 
     /* Local copy for convenience */
@@ -674,7 +686,11 @@ static int libxl__device_pci_assignable_
     }
 
     /* Check to see if it's already assigned to pciback */
-    if ( pciback_dev_is_assigned(gc, pcidev) ) {
+    rc = pciback_dev_is_assigned(gc, pcidev);
+    if ( rc < 0 ) {
+        return ERROR_FAIL;
+    }
+    if ( rc ) {
         LIBXL__LOG(ctx, LIBXL__LOG_WARNING, PCI_BDF" already assigned to pciback",
                    dom, bus, dev, func);
         return 0;
@@ -692,11 +708,18 @@ static int libxl__device_pci_assignable_
     if ( rebind ) {
         if ( driver_path ) {
             pci_assignable_driver_path_write(gc, pcidev, driver_path);
+        } else if ( (driver_path =
+                     pci_assignable_driver_path_read(gc, pcidev)) != NULL ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_INFO,
+                       PCI_BDF" not bound to a driver, will be rebound to %s",
+                       dom, bus, dev, func, driver_path);
         } else {
             LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
                        PCI_BDF" not bound to a driver, will not be rebound.",
                        dom, bus, dev, func);
         }
+    } else {
+        pci_assignable_driver_path_remove(gc, pcidev);
     }
 
     if ( pciback_dev_assign(gc, pcidev) ) {
@@ -717,7 +740,6 @@ static int libxl__device_pci_assignable_
 
     /* Unbind from pciback */
     if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Checking if pciback was assigned");
         return ERROR_FAIL;
     } else if ( rc ) {
         pciback_dev_unassign(gc, pcidev);
@@ -741,9 +763,9 @@ static int libxl__device_pci_assignable_
                                  "Couldn't bind device to %s", driver_path);
                 return -1;
             }
-        }
 
-        pci_assignable_driver_path_remove(gc, pcidev);
+            pci_assignable_driver_path_remove(gc, pcidev);
+        }
     } else {
         if ( rebind ) {
             LIBXL__LOG(ctx, LIBXL__LOG_WARNING,

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

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

Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling

[George Dunlap]

On Thu, Sep 10, 2015 at 1:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> While it appears to be intentional for "xl pci-assignable-remove" to
> not re-bind the original driver by default (requires the -r option),
> permanently losing the information which driver was originally used
> seems bad. Make "add; remove; add; remove -r" re-bind the original
> driver by allowing "remove" to delete the information only upon
> successful re-bind.

I would be open to the argument that I was being overly paranoid in
making "xl pci-assignable-remove" not re-bind by default.  But either
way:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> In the course of this I also noticed that binding information is lost
> when upon first "add" pciback isn't loaded yet, due to its presence not
> being checked for early enough. Adjust pciback_dev_is_assigned()
> accordingly, and properly distinguish "yes" and "error" returns in the
> "add" case (removing a redundant error message from the "remove" path
> for consistency).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
> backported later on.

I wouldn't really consider this a bug fix, but an improvement.  As
such, I don't think it should be given a freeze exception, and my
inclination would be to say that it shouldn't be backported.  But the
strength of my opinion isn't very strong.

 -George

Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling

[Ian Campbell]

On Mon, 2015-09-14 at 10:50 +0100, George Dunlap wrote:
> On Thu, Sep 10, 2015 at 1:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > While it appears to be intentional for "xl pci-assignable-remove" to
> > not re-bind the original driver by default (requires the -r option),
> > permanently losing the information which driver was originally used
> > seems bad. Make "add; remove; add; remove -r" re-bind the original
> > driver by allowing "remove" to delete the information only upon
> > successful re-bind.
> 
> I would be open to the argument that I was being overly paranoid in
> making "xl pci-assignable-remove" not re-bind by default.  But either
> way:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

The use of "rc" to hold a non-libxl error code (0 or -1 in this case) in
_add is not allowed by libxl coding style, but is consistent with the same
thing existing in _remove, also this code is mostly in hypervisor coding
style so it seems tolerable for this new code to be so too.

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

> > In the course of this I also noticed that binding information is lost
> > when upon first "add" pciback isn't loaded yet, due to its presence not
> > being checked for early enough. Adjust pciback_dev_is_assigned()
> > accordingly, and properly distinguish "yes" and "error" returns in the
> > "add" case (removing a redundant error message from the "remove" path
> > for consistency).
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
> > backported later on.
> 
> I wouldn't really consider this a bug fix, but an improvement.  As
> such, I don't think it should be given a freeze exception, and my
> inclination would be to say that it shouldn't be backported.  But the
> strength of my opinion isn't very strong.

I wouldn't argue either way.

Ian.

Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling

[Ian Campbell]

On Tue, 2015-09-15 at 10:31 +0100, Ian Campbell wrote:
> On Mon, 2015-09-14 at 10:50 +0100, George Dunlap wrote:
> > On Thu, Sep 10, 2015 at 1:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > > While it appears to be intentional for "xl pci-assignable-remove" to
> > > not re-bind the original driver by default (requires the -r option),
> > > permanently losing the information which driver was originally used
> > > seems bad. Make "add; remove; add; remove -r" re-bind the original
> > > driver by allowing "remove" to delete the information only upon
> > > successful re-bind.
> > 
> > I would be open to the argument that I was being overly paranoid in
> > making "xl pci-assignable-remove" not re-bind by default.  But either
> > way:
> > 
> > Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> 
> The use of "rc" to hold a non-libxl error code (0 or -1 in this case) in
> _add is not allowed by libxl coding style, but is consistent with the
> same
> thing existing in _remove, also this code is mostly in hypervisor coding
> style so it seems tolerable for this new code to be so too.
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

and applied.