* [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling
@ 2015-09-10 12:36 Jan Beulich
2015-09-14 9:50 ` George Dunlap
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2015-09-10 12:36 UTC (permalink / raw
To: xen-devel; +Cc: Ian Campbell, Ian Jackson, Wei Liu, Stefano Stabellini
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling
2015-09-10 12:36 [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling Jan Beulich
@ 2015-09-14 9:50 ` George Dunlap
2015-09-15 9:31 ` Ian Campbell
0 siblings, 1 reply; 4+ messages in thread
From: George Dunlap @ 2015-09-14 9:50 UTC (permalink / raw
To: Jan Beulich
Cc: Ian Campbell, xen-devel, Ian Jackson, Wei Liu, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling
2015-09-14 9:50 ` George Dunlap
@ 2015-09-15 9:31 ` Ian Campbell
2015-09-15 11:16 ` Ian Campbell
0 siblings, 1 reply; 4+ messages in thread
From: Ian Campbell @ 2015-09-15 9:31 UTC (permalink / raw
To: George Dunlap, Jan Beulich
Cc: xen-devel, Ian Jackson, Wei Liu, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling
2015-09-15 9:31 ` Ian Campbell
@ 2015-09-15 11:16 ` Ian Campbell
0 siblings, 0 replies; 4+ messages in thread
From: Ian Campbell @ 2015-09-15 11:16 UTC (permalink / raw
To: George Dunlap, Jan Beulich
Cc: xen-devel, Ian Jackson, Wei Liu, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-09-15 11:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-10 12:36 [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling Jan Beulich
2015-09-14 9:50 ` George Dunlap
2015-09-15 9:31 ` Ian Campbell
2015-09-15 11:16 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).