Re: [Xen-devel] [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  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 

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 

> > 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 
> > ---
> > 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.

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

Re: [Xen-devel] [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  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 
> 
> 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 

and applied.

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

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

[George Dunlap]

On Thu, Sep 10, 2015 at 1:36 PM, Jan Beulich  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 

> 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 
> ---
> 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

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

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

[Jan Beulich]

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 
---
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,



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 
---
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