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

[Alexander Graf <agraf@suse.de>]

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@redhat.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>     
>
> 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?

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