From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] Inform users about busy device assignment attempt Date: Fri, 11 Dec 2009 13:38:40 +0200 Message-ID: <20091211113839.GE29972@redhat.com> References: <1260486386-5898-1-git-send-email-agraf@suse.de> <1260486386-5898-2-git-send-email-agraf@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm list , Gleb Natapov , Muli Ben-Yehuda , "Daniel P. Berrange" To: Alexander Graf Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47908 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762388AbZLKLlZ (ORCPT ); Fri, 11 Dec 2009 06:41:25 -0500 Content-Disposition: inline In-Reply-To: <1260486386-5898-2-git-send-email-agraf@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Dec 11, 2009 at 12:06:26AM +0100, Alexander Graf wrote: > When using -pcidevice on a device that is already in use by a kernel driver > all the user gets is the following (very useful) information: > > Failed to assign device "04:00.0" : Device or resource busy > Failed to deassign device "04:00.0" : Invalid argument > Error initializing device pci-assign > > Since I usually prefer to have my computer do the thinking for me, I figured > it might be a good idea to check and see if a device is actually used by a > driver. If so, tell the user. > > So with this patch applied you get the following output: > > Failed to assign device "04:00.0" : Device or resource busy > *** The driver 'igb' is occupying your device 04:00.0. > *** > *** You can try the following commands to free it: > *** > *** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/new_id > *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/igb/unbind > *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/pci-stub/bind > *** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/remove_id > *** > Failed to deassign device "04:00.0" : Invalid argument > Error initializing device pci-assign > > That should keep people like me from doing the most obvious misuses :-). > > CC: Daniel P. Berrange > Signed-off-by: Alexander Graf Minor nits and a bug. > --- > > v1 -> v2: > > - add more helpful guidance thanks to Daniel Berrange > > v2 -> v3: > > - clear name variable before using it, thus 0-terminating the string > - fix region numbers > - use correct unbind/bind names > --- > hw/device-assignment.c | 109 +++++++++++++++++++++++++++++++++++++----------- > 1 files changed, 85 insertions(+), 24 deletions(-) > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > index 5cee929..98faa83 100644 > --- a/hw/device-assignment.c > +++ b/hw/device-assignment.c > @@ -564,14 +564,44 @@ static int assigned_dev_register_regions(PCIRegion *io_regions, > return 0; > } > > +static int get_real_id(const char *devpath, const char *idname, uint16_t *val) > +{ > + FILE *f; > + char name[128]; let's not introduce arbitraty file name length limitations. strlen is not hard to use. I know all this module is broken this way, but let's not add more. > + long id; > + > + snprintf(name, sizeof(name), "%s%s", devpath, idname); > + f = fopen(name, "r"); > + if (f == NULL) { > + fprintf(stderr, "%s: %s: %m\n", __func__, name); > + return -1; > + } > + if (fscanf(f, "%li\n", &id) == 1) { > + *val = id; > + } handle fscanf error? > + fclose(f); > + > + return 0; > +} > + > +static int get_real_vendor_id(const char *devpath, uint16_t *val) > +{ > + return get_real_id(devpath, "vendor", val); > +} > + > +static int get_real_device_id(const char *devpath, uint16_t *val) > +{ > + return get_real_id(devpath, "device", val); > +} > + > static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus, > uint8_t r_dev, uint8_t r_func) > { > char dir[128], name[128]; > - int fd, r = 0; > + int fd, r = 0, v; > FILE *f; > unsigned long long start, end, size, flags; > - unsigned long id; > + uint16_t id; > struct stat statbuf; > PCIRegion *rp; > PCIDevRegions *dev = &pci_dev->real_device; > @@ -637,31 +667,21 @@ again: > > fclose(f); > > - /* read and fill device ID */ > - snprintf(name, sizeof(name), "%svendor", dir); > - f = fopen(name, "r"); > - if (f == NULL) { > - fprintf(stderr, "%s: %s: %m\n", __func__, name); > + /* read and fill vendor ID */ > + v = get_real_vendor_id(dir, &id); > + if (v) { > return 1; > } > - if (fscanf(f, "%li\n", &id) == 1) { > - pci_dev->dev.config[0] = id & 0xff; > - pci_dev->dev.config[1] = (id & 0xff00) >> 8; > - } > - fclose(f); > + pci_dev->dev.config[0] = id & 0xff; > + pci_dev->dev.config[1] = (id & 0xff00) >> 8; > this seems an unrelated cleanup? If so better as a separate patch? > - /* read and fill vendor ID */ > - snprintf(name, sizeof(name), "%sdevice", dir); > - f = fopen(name, "r"); > - if (f == NULL) { > - fprintf(stderr, "%s: %s: %m\n", __func__, name); > + /* read and fill device ID */ > + v = get_real_device_id(dir, &id); > + if (v) { > return 1; > } > - if (fscanf(f, "%li\n", &id) == 1) { > - pci_dev->dev.config[2] = id & 0xff; > - pci_dev->dev.config[3] = (id & 0xff00) >> 8; > - } > - fclose(f); > + pci_dev->dev.config[2] = id & 0xff; > + pci_dev->dev.config[3] = (id & 0xff00) >> 8; > > /* dealing with virtual function device */ > snprintf(name, sizeof(name), "%sphysfn/", dir); > @@ -739,7 +759,9 @@ static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn) > static int assign_device(AssignedDevice *dev) > { > struct kvm_assigned_pci_dev assigned_dev_data; > - int r; > + char name[128], dir[128], driver[128], *ns; Yes 128 will be enough for now. But it's pretty ugly. In this case, something like char dir[] = "/sys/bus/pci/devices/0000:00:00.0/"; will allocate just enough memory. Or use MAX PATH. > + uint16_t vendor_id, device_id; > + int r, v; > > memset(&assigned_dev_data, 0, sizeof(assigned_dev_data)); > assigned_dev_data.assigned_dev_id = > @@ -761,9 +783,48 @@ static int assign_device(AssignedDevice *dev) > #endif > > r = kvm_assign_pci_device(kvm_context, &assigned_dev_data); > - if (r < 0) > + if (r < 0) { Please put all of the below in a separate function. > fprintf(stderr, "Failed to assign device \"%s\" : %s\n", > dev->dev.qdev.id, strerror(-r)); > + > + snprintf(dir, sizeof(dir), snprintf? So you worry about overflowing dir? But dir will not be 0 terminated on overflow, so use of %s below would crash anyway. As in fact we know this can not overflow, just use sprintf. > + "/sys/bus/pci/devices/0000:%02x:%02x.%x/", > + dev->host.bus, dev->host.dev, dev->host.func); This assumes domain 0. I know multidomain is broken with device assignment, but pls add TOIDO here so we don't forget to fix it. > + > + snprintf(name, sizeof(name), "%sdriver", dir); So why do sprintf twice? Just put "driver" as part of the template above. > + > + memset(driver, 0, sizeof(driver)); just initialize driver to 0 by = {}; > + v = readlink(name, driver, sizeof(driver)); So if readlink fills up all of driver, strrchr below will cause coredump, right? Better check v against sizeof driver. > + if ((v <= 0) || !(ns = strrchr(driver, '/'))) { > + return r; Add some fprintf here. Maybe report errno as well. > + } > + > + ns++; > + > + if (get_real_vendor_id(dir, &vendor_id) || > + get_real_device_id(dir, &device_id)) { > + return r; And here. > + } > + > + fprintf(stderr, "*** The driver '%s' is occupying your device " > + "%02x:%02x.%x.\n", > + ns, dev->host.bus, dev->host.dev, dev->host.func); > + fprintf(stderr, "***\n"); > + fprintf(stderr, "*** You can try the following commands to free " > + "it:\n"); > + fprintf(stderr, "***\n"); > + fprintf(stderr, "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/" > + "pci-stub/new_id\n", vendor_id, device_id); > + fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci" > + "/drivers/%s /unbind\n", > + dev->host.bus, dev->host.dev, dev->host.func, ns); > + fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci" > + "/drivers/ pci-stub/bind\n", > + dev->host.bus, dev->host.dev, dev->host.func); > + fprintf(stderr, "*** $ echo \"%x %x\" > /sys/bus/pci/drivers/pci-stub" > + "/remove_id\n", vendor_id, device_id); > + fprintf(stderr, "***\n"); above assumes domain zero. Please add a TODO to fix. > + } > return r; > } > > -- > 1.6.0.2