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