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

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(a)redhat.com&gt; 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 ?

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

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

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

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.