Re: [PATCH] Inform users about busy device assignment attempt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 15, 2009 at 04:18:00PM +0100, Alexander Graf wrote:
> Michael S. Tsirkin wrote:
> > 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 <berrange@xxxxxxxxxx>
> >> Signed-off-by: Alexander Graf <agraf@xxxxxxx>
> >>     
> >
> > 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.
> >   
> 
> It's just a move of existing code. I tried to change it as little as
> possible. Cleanups for that are welcome for later.
> 
> >   
> >> +    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?
> >   
> 
> Interesting. I don't think it was done before, but I can put it in.
> 
> >   
> >> +    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?
> >   
> 
> It's the code move. I split it now.
> 
> >
> >   
> >> -    /* 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.
> >   
> 
> Used MAX_PATH now.
> 
> >   
> >> +    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.
> >   
> 
> Ok.
> 
> >   
> >>  	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.
> >   
> 
> Ok.
> 
> >   
> >> +                 "/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.
> >   
> 
> Ok.
> 
> >   
> >> +
> >> +        snprintf(name, sizeof(name), "%sdriver", dir);
> >>     
> >
> > So why do sprintf twice? Just put "driver" as part
> > of the template above.
> >   
> 
> We're using dir later in the code.
> 
> >   
> >> +
> >> +        memset(driver, 0, sizeof(driver));
> >>     
> >
> > just initialize driver to 0 by = {};
> >
> >   
> 
> That initializes it to 0? I mean, all elements?

Yes.

> >> +        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.
> >   
> 
> Ok.
> 
> >   
> >> +        if ((v <= 0) || !(ns = strrchr(driver, '/'))) {
> >> +            return r;
> >>     
> >
> > Add some fprintf here. Maybe report errno as well.
> >   
> 
> Ok.
> 
> >   
> >> +        }
> >> +
> >> +        ns++;
> >> +
> >> +        if (get_real_vendor_id(dir, &vendor_id) ||
> >> +            get_real_device_id(dir, &device_id)) {
> >> +            return r;
> >>     
> >
> > And here.
> >   
> 
> Yep.
> 
> >   
> >> +        }
> >> +
> >> +        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.
> >   
> 
> Same as above, right? In fact, a lot of the code assumes that so it's
> more of a generic TODO :-(.
> 
> Alex


Yes it is, unfortunately :(

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux