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

Re: [libvirt] [PATCH 1/5] Add pci utility functions



On Wed, 2009-02-25 at 15:54 +0000, Daniel P. Berrange wrote:
> On Tue, Feb 24, 2009 at 10:21:47PM +0000, Mark McLoughlin wrote:
> > Add implementations of dettach, reattach and reset for
> > PCI devices.
> > 
> > Background to this code can be found here:
> > 
> >   http://marc.info/?l=kvm&m=123454366317045
> > 
> > Some notes:
> > 
> >   * pci-stub was first introduced in 2.6.29; if it's
> >     not available, then dettach can only unbind the
> >     device from the host driver
> > 
> >   * remove_id is queued up for 2.6.30; in it's absence
> >     reattach cannot re-bind the device to the original
> >     driver
> > 
> >   * If the device supports Function Level Reset, we
> >     just don't bother doing the reset - KVM will do
> >     this before and after a guest runs
> > 
> >   * Currently, if a reset would affect another device
> >     on the same bus, or another function on the same
> >     multi-function device, then we just refuse to do
> >     the reset. In future, we could allow the reset
> >     as long as it doesn't affect a device in use by
> >     the host or assigned to another guest
> > 
> >   * The device reset code is similar in concept to
> >     some of Xen's code
> > 
> > Signed-off-by: Mark McLoughlin <markmc redhat com>
> 
> I can't claim to understand all the gory PCI code in here,
> but a few API level suggestions.
> 
> Also, would it be worth (or possible) using libpciaccess.so
> for this instead of poking Linux sysfs files directly ?

Ah, yes - I knew I forgot another note!

Originally the code used libpci, but since that doesn't handle malloc
failure (well, you can handle it, but only by exiting) I implemented it
directly.

libpciaccess looks much saner, so we should use that. It doesn't take
much code to implement it, though, so I don't consider switching to the
library to be very high priority.

> > +static int
> > +pciRead(pciDevice *dev, unsigned pos, uint8_t *buf, unsigned buflen)
> > +{
> > +    memset(buf, 0, buflen);
> > +
> > +    if (pciOpenConfig(dev) < 0)
> > +        return -1;
> > +
> > +    if (pread(dev->fd, buf, buflen, pos) < 0) {
> > +        char ebuf[1024];
> > +        VIR_WARN(_("Failed to read from '%s' : %s"), dev->path,
> > +                 virStrerror(errno, ebuf, sizeof(ebuf)));
> > +        return -1;
> > +    }
> > +    return 0;
> > +}
> 
> I think this needs to check for a possible short read too.
> Perhaps better off doing a seek and then saferead(), or
> defining a safepread() function.
> 
> > +static int
> > +pciWrite(pciDevice *dev, unsigned pos, uint8_t *buf, unsigned buflen)
> > +{
> > +    if (pciOpenConfig(dev) < 0)
> > +        return -1;
> > +
> > +    if (pwrite(dev->fd, buf, buflen, pos) < 0) {
> > +        char ebuf[1024];
> > +        VIR_WARN(_("Failed to write to '%s' : %s"), dev->path,
> > +                 virStrerror(errno, ebuf, sizeof(ebuf)));
> > +        return -1;
> > +    }
> > +    return 0;
> > +}
> 
> Likewise here needs to validate actual num of bytes written

Yep, except we shouldn't see such failures with sysfs - e.g. the likes
of libpci doesn't handle such failures.

Suggest it's not a big issue and fix it by switching to libpciaccess

> > +int
> > +pciDettachDevice(pciDevice *dev)
> > +{
> > +    char path[PATH_MAX];
> > +
> > +    /* Try loading the pci-stub module if it isn't already loaded;
> > +     * return an error if there is no pci-stub available.
> > +     */
> > +    if (!virFileExists(PCI_SYSFS "drivers/pci-stub")) {
> > +        const char *const modprobeargv[] = { MODPROBE, "pci-stub", NULL };
> > +
> > +        if (virRun(dev->conn, modprobeargv, NULL) < 0) {
> > +            char ebuf[1024];
> > +            VIR_WARN(_("modprobe pci-stub failed: %s"),
> > +                     virStrerror(errno, ebuf, sizeof ebuf));
> > +        }
> > +    }
> > +
> > +    if (!virFileExists(PCI_SYSFS "drivers/pci-stub")) {
> > +        VIR_WARN(_("pci-stub driver not available, cannot bind device %s to it"),
> > +                 dev->name);
> > +    } else {
> > +        /* Add the PCI device ID to pci-stub's dynamic ID table;
> > +         * this is needed to allow us to bind the device to pci-stub.
> > +         * Note: if the device is not currently bound to any driver,
> > +         * pci-stub will immediately be bound to the device. Also, note
> > +         * that if a new device with this ID is hotplugged, or if a probe
> > +         * is triggered for such a device, it will also be immediately
> > +         * bound by pci-stub.
> > +         */
> > +        if (virFileWriteStr(PCI_SYSFS "drivers/pci-stub/new_id", dev->id) < 0) {
> > +            virReportSystemError(dev->conn, errno,
> > +                                 _("Failed to add PCI device ID '%s' to pci-stub/new_id"),
> > +                                 dev->id);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    /* If the device is already bound to a driver, unbind it.
> > +     * Note, this will have rather unpleasant side effects if this
> > +     * PCI device happens to be IDE controller for the disk hosting
> > +     * your root filesystem.
> > +     */
> > +    snprintf(path, sizeof(path),
> > +             PCI_SYSFS "devices/%s/driver/unbind", dev->name);
> > +    if (virFileExists(path) && virFileWriteStr(path, dev->name) < 0) {
> > +        virReportSystemError(dev->conn, errno,
> > +                             _("Failed to unbind PCI device '%s'"), dev->name);
> > +        return -1;
> > +    }
> > +
> > +    if (virFileExists(PCI_SYSFS "drivers/pci-stub")) {
> > +        /* If the device isn't already bound to pci-stub, try binding it now.
> > +         */
> > +        snprintf(path, sizeof(path), PCI_SYSFS "devices/%s/driver", dev->name);
> > +        if (!virFileLinkPointsTo(path, PCI_SYSFS "drivers/pci-stub") &&
> > +            virFileWriteStr(PCI_SYSFS "drivers/pci-stub/bind", dev->name) < 0) {
> > +            virReportSystemError(dev->conn, errno,
> > +                                 _("Failed to bind PCI device '%s' to pci-stub"),
> > +                                 dev->name);
> > +            return -1;
> > +        }
> > +
> > +        /* If 'remove_id' exists, remove the device id from pci-stub's dynamic
> > +         * ID table so that 'drivers_probe' works below.
> > +         */
> > +        if (virFileExists(PCI_SYSFS "drivers/pci-stub/remove_id") &&
> > +            virFileWriteStr(PCI_SYSFS "drivers/pci-stub/remove_id", dev->id) < 0) {
> > +            virReportSystemError(dev->conn, errno,
> > +                                 _("Failed to remove PCI ID '%s' with pci-stub/remove_id"),
> > +                                 dev->id);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int
> > +pciReAttachDevice(pciDevice *dev)
> > +{
> > +    if (virFileExists(PCI_SYSFS "drivers/pci-stub")) {
> > +        char path[PATH_MAX];
> > +
> > +        /* If the device is bound to pci-stub, unbind it.
> > +         */
> > +        snprintf(path, sizeof(path), PCI_SYSFS "devices/%s/driver", dev->name);
> > +        if (virFileLinkPointsTo(path, PCI_SYSFS "drivers/pci-stub") &&
> > +            virFileWriteStr(PCI_SYSFS "drivers/pci-stub/unbind", dev->name) < 0) {
> > +            virReportSystemError(dev->conn, errno,
> > +                                 _("Failed to bind PCI device '%s' to pci-stub"),
> > +                                 dev->name);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    /* Trigger a re-probe of the device is not in pci-stub's dynamic
> > +     * ID table. If pci-stub is available, but 'remove_id' isn't
> > +     * available, then re-probing would just cause the device to be
> > +     * re-bound to pci-stub.
> > +     */
> > +    if (!virFileExists(PCI_SYSFS "drivers/pci-stub") ||
> > +        virFileExists(PCI_SYSFS "drivers/pci-stub/remove_id")) {
> > +        if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name) < 0) {
> > +            virReportSystemError(dev->conn, errno,
> > +                                 _("Failed to trigger a re-probe for PCI device '%s'"),
> > +                                 dev->name);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> 
> These functions really need to take the driver name as an argument,
> so we can pass 'pci-stub' for KVM (or modern pv_ops Xen dom0), or
> 'pciback' for old pre-pv_ops Xen dom0.

Okay.

> > +pciDevice *pciGetDevice      (virConnectPtr  conn,
> > +                              unsigned       vendor,
> > +                              unsigned       product,
> > +                              unsigned       domain,
> > +                              unsigned       bus,
> > +                              unsigned       slot,
> > +                              unsigned       function);
> > +void       pciFreeDevice     (pciDevice     *dev);
> > +int        pciDettachDevice  (pciDevice     *dev);
> > +int        pciReAttachDevice (pciDevice     *dev);
> > +int        pciResetDevice    (pciDevice     *dev);
> 
> I prefer it to pass the virConnectPtr conn into each of
> these calls rather than storing it in the pciDevice
> struct.

Okay.

>  It'd also be good to remove the vendor/product
> ID here, and have this code lookup them up from te address
> info, since the domain XML format only provides the caller
> with the domain/bus/slot/function address info.

Okay.

I'll follow up with a new series with those changes.

Cheers,
Mark.


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