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
