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