[libvirt] [PATCH] pci: Use virPCIDeviceAddress in virPCIDevice
Laine Stump
laine at laine.org
Mon Dec 14 19:46:43 UTC 2015
On 12/14/2015 09:42 AM, Andrea Bolognani wrote:
> Instead of replicating the information (domain, bus, slot, function)
> inside the virPCIDevice structure, use the already-existing
> virPCIDeviceAddress structure.
You use virPCIDeviceAddressPtr rather than virPCIDeviceAddress,
resulting in an extra bit of memory to malloc, and an extra indirection
when the members are accessed. Was there a particular reason for that? I
had figured the domain/bus/slot/function would just be replaced by a
plain virPCIDeviceAddress rather than a pointer to one...
> Outside of the module, the only visible change is that the return value
> of virPCIDeviceGetAddress() must no longer be freed by the caller.
> ---
> Suggested by Laine[1] in the context of a patch series; since the idea
> is not really tied to the series, sending as a stand-alone cleanup.
>
> [1] https://www.redhat.com/archives/libvir-list/2015-December/msg00125.html
>
> src/util/virhostdev.c | 4 --
> src/util/virpci.c | 182 ++++++++++++++++++++++++++++++++++----------------
> src/util/virpci.h | 7 ++
> 3 files changed, 133 insertions(+), 60 deletions(-)
>
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index de029a0..173ac58 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -585,7 +585,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
> goto cleanup;
> }
>
> - VIR_FREE(devAddr);
> if (!(devAddr = virPCIDeviceGetAddress(dev)))
> goto cleanup;
>
> @@ -728,7 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
> virObjectUnlock(hostdev_mgr->activePCIHostdevs);
> virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
> virObjectUnref(pcidevs);
> - VIR_FREE(devAddr);
> return ret;
> }
>
> @@ -1581,7 +1579,6 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr,
> out:
> virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
> virObjectUnlock(hostdev_mgr->activePCIHostdevs);
> - VIR_FREE(devAddr);
> return ret;
> }
>
> @@ -1613,7 +1610,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr,
> out:
> virObjectUnlock(hostdev_mgr->inactivePCIHostdevs);
> virObjectUnlock(hostdev_mgr->activePCIHostdevs);
> - VIR_FREE(devAddr);
> return ret;
> }
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index bececb5..2818bf7 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -56,10 +56,7 @@ VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST,
> "", "2.5", "5", "8")
>
> struct _virPCIDevice {
> - unsigned int domain;
> - unsigned int bus;
> - unsigned int slot;
> - unsigned int function;
> + virPCIDeviceAddressPtr address;
>
> char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */
> char id[PCI_ID_LEN]; /* product vendor */
> @@ -653,12 +650,14 @@ static int
> virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)
> {
> virPCIDeviceList *inactiveDevs = data;
> + virPCIDeviceAddressPtr devAddr = dev->address;
> + virPCIDeviceAddressPtr checkAddr = check->address;
>
> /* Different domain, different bus, or simply identical device */
> - if (dev->domain != check->domain ||
> - dev->bus != check->bus ||
> - (dev->slot == check->slot &&
> - dev->function == check->function))
> + if (devAddr->domain != checkAddr->domain ||
> + devAddr->bus != checkAddr->bus ||
> + (devAddr->slot == checkAddr->slot &&
> + devAddr->function == checkAddr->function))
> return 0;
>
> /* same bus, but inactive, i.e. about to be assigned to guest */
> @@ -686,10 +685,12 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)
> uint16_t device_class;
> uint8_t header_type, secondary, subordinate;
> virPCIDevicePtr *best = data;
> + virPCIDeviceAddressPtr devAddr = dev->address;
> + virPCIDeviceAddressPtr checkAddr = check->address;
> int ret = 0;
> int fd;
>
> - if (dev->domain != check->domain)
> + if (devAddr->domain != checkAddr->domain)
> return 0;
>
> if ((fd = virPCIDeviceConfigOpen(check, false)) < 0)
> @@ -713,7 +714,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)
> /* if the secondary bus exactly equals the device's bus, then we found
> * the direct parent. No further work is necessary
> */
> - if (dev->bus == secondary) {
> + if (devAddr->bus == secondary) {
> ret = 1;
> goto cleanup;
> }
> @@ -722,10 +723,10 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)
> * In this case, what we need to do is look for the "best" match; i.e.
> * the most restrictive match that still satisfies all of the conditions.
> */
> - if (dev->bus > secondary && dev->bus <= subordinate) {
> + if (devAddr->bus > secondary && devAddr->bus <= subordinate) {
> if (*best == NULL) {
> - *best = virPCIDeviceNew(check->domain, check->bus, check->slot,
> - check->function);
> + *best = virPCIDeviceNew(checkAddr->domain, checkAddr->bus,
> + checkAddr->slot, checkAddr->function);
> if (*best == NULL) {
> ret = -1;
> goto cleanup;
> @@ -745,8 +746,8 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)
>
> if (secondary > best_secondary) {
> virPCIDeviceFree(*best);
> - *best = virPCIDeviceNew(check->domain, check->bus, check->slot,
> - check->function);
> + *best = virPCIDeviceNew(checkAddr->domain, checkAddr->bus,
> + checkAddr->slot, checkAddr->function);
> if (*best == NULL) {
> ret = -1;
> goto cleanup;
> @@ -925,6 +926,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
> char *drvName = NULL;
> int ret = -1;
> int fd = -1;
> + virPCIDeviceAddressPtr devAddr = dev->address;
>
> if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -970,7 +972,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
> ret = virPCIDeviceTryPowerManagementReset(dev, fd);
>
> /* Bus reset is not an option with the root bus */
> - if (ret < 0 && dev->bus != 0)
> + if (ret < 0 && devAddr->bus != 0)
> ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs);
>
> if (ret < 0) {
> @@ -1428,6 +1430,7 @@ virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher)
> bool in_matching_device;
> int ret;
> size_t match_depth;
> + virPCIDeviceAddressPtr devAddr = dev->address;
>
> fp = fopen("/proc/iomem", "r");
> if (!fp) {
> @@ -1483,8 +1486,8 @@ virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher)
> virStrToLong_ui(tmp + 1, &tmp, 16, &function) < 0 || *tmp != '\n')
> continue;
>
> - if (domain != dev->domain || bus != dev->bus || slot != dev->slot ||
> - function != dev->function)
> + if (domain != devAddr->domain || bus != devAddr->bus ||
> + slot != devAddr->slot || function != devAddr->function)
> continue;
> in_matching_device = true;
> match_depth = strspn(line, " ");
> @@ -1547,6 +1550,72 @@ virPCIGetAddrString(unsigned int domain,
> return ret;
> }
>
> +/**
> + * virPCIDeviceAddressNew:
> + * @domain: PCI domain
> + * @bus: PCI bus
> + * @slot: PCI slot
> + * @function: PCI function
> + *
> + * Create a new virPCIDeviceAddress object.
> + *
> + * Returns: a new address, or NULL on failure.
> + */
> +virPCIDeviceAddressPtr
> +virPCIDeviceAddressNew(unsigned int domain,
> + unsigned int bus,
> + unsigned int slot,
> + unsigned int function)
> +{
> + virPCIDeviceAddressPtr addr;
> +
> + if (VIR_ALLOC(addr) < 0)
> + return NULL;
> +
> + addr->domain = domain;
> + addr->bus = bus;
> + addr->slot = slot;
> + addr->function = function;
> +
> + return addr;
> +}
> +
> +/**
> + * virPCIDeviceAddressCopy:
> + * @addr: PCI address
> + *
> + * Create a copy of a PCI address.
> + *
> + * Returns: a copy of @addr, or NULL on failure.
> + */
> +virPCIDeviceAddressPtr
> +virPCIDeviceAddressCopy(virPCIDeviceAddressPtr addr)
> +{
> + virPCIDeviceAddressPtr copy;
> +
> + if (!addr)
> + return NULL;
> +
> + if (VIR_ALLOC(copy) < 0)
> + return NULL;
> +
> + *copy = *addr;
> +
> + return copy;
> +}
> +
> +/**
> + * virPCIDeviceAddressFree:
> + * @addr: PCI address
> + *
> + * Release all resources associated with a PCI address.
> + */
> +void
> +virPCIDeviceAddressFree(virPCIDeviceAddressPtr addr)
> +{
> + VIR_FREE(addr);
> +}
> +
> virPCIDevicePtr
> virPCIDeviceNew(unsigned int domain,
> unsigned int bus,
> @@ -1560,17 +1629,14 @@ virPCIDeviceNew(unsigned int domain,
> if (VIR_ALLOC(dev) < 0)
> return NULL;
>
> - dev->domain = domain;
> - dev->bus = bus;
> - dev->slot = slot;
> - dev->function = function;
> + if (!(dev->address = virPCIDeviceAddressNew(domain, bus, slot, function)))
> + goto error;
>
> if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x",
> - dev->domain, dev->bus, dev->slot,
> - dev->function) >= sizeof(dev->name)) {
> + domain, bus, slot, function) >= sizeof(dev->name)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"),
> - dev->domain, dev->bus, dev->slot, dev->function);
> + domain, bus, slot, function);
> goto error;
> }
> if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config",
> @@ -1627,9 +1693,13 @@ virPCIDeviceCopy(virPCIDevicePtr dev)
>
> /* shallow copy to take care of most attributes */
> *copy = *dev;
> +
> + /* deep copy for the rest */
> + copy->address = NULL;
> copy->path = copy->stubDriver = NULL;
> copy->used_by_drvname = copy->used_by_domname = NULL;
> - if (VIR_STRDUP(copy->path, dev->path) < 0 ||
> + if (!(copy->address = virPCIDeviceAddressCopy(dev->address)) ||
> + VIR_STRDUP(copy->path, dev->path) < 0 ||
> VIR_STRDUP(copy->stubDriver, dev->stubDriver) < 0 ||
> VIR_STRDUP(copy->used_by_drvname, dev->used_by_drvname) < 0 ||
> VIR_STRDUP(copy->used_by_domname, dev->used_by_domname) < 0) {
> @@ -1649,6 +1719,7 @@ virPCIDeviceFree(virPCIDevicePtr dev)
> if (!dev)
> return;
> VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
> + virPCIDeviceAddressFree(dev->address);
> VIR_FREE(dev->path);
> VIR_FREE(dev->stubDriver);
> VIR_FREE(dev->used_by_drvname);
> @@ -1661,25 +1732,14 @@ virPCIDeviceFree(virPCIDevicePtr dev)
> * @dev: device to get address from
> *
> * Take a PCI device on input and return its PCI address. The
> - * caller must free the returned value when no longer needed.
> + * returned object is owned by the device and must not be freed.
> *
> * Returns NULL on failure, the device address on success.
> */
> virPCIDeviceAddressPtr
> virPCIDeviceGetAddress(virPCIDevicePtr dev)
> {
> -
> - virPCIDeviceAddressPtr pciAddrPtr;
> -
> - if (!dev || (VIR_ALLOC(pciAddrPtr) < 0))
> - return NULL;
> -
> - pciAddrPtr->domain = dev->domain;
> - pciAddrPtr->bus = dev->bus;
> - pciAddrPtr->slot = dev->slot;
> - pciAddrPtr->function = dev->function;
> -
> - return pciAddrPtr;
> + return dev->address;
> }
>
> const char *
> @@ -1888,14 +1948,18 @@ virPCIDeviceListDel(virPCIDeviceListPtr list,
> int
> virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev)
> {
> + virPCIDeviceAddressPtr devAddr = dev->address;
> + virPCIDeviceAddressPtr tmpAddr;
> size_t i;
>
> - for (i = 0; i < list->count; i++)
> - if (list->devs[i]->domain == dev->domain &&
> - list->devs[i]->bus == dev->bus &&
> - list->devs[i]->slot == dev->slot &&
> - list->devs[i]->function == dev->function)
> + for (i = 0; i < list->count; i++) {
> + tmpAddr = list->devs[i]->address;
> + if (tmpAddr->domain == devAddr->domain &&
> + tmpAddr->bus == devAddr->bus &&
> + tmpAddr->slot == devAddr->slot &&
> + tmpAddr->function == devAddr->function)
> return i;
> + }
> return -1;
> }
>
> @@ -1907,13 +1971,16 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list,
> unsigned int slot,
> unsigned int function)
> {
> + virPCIDeviceAddressPtr tmpAddr;
> size_t i;
>
> for (i = 0; i < list->count; i++) {
> - if (list->devs[i]->domain == domain &&
> - list->devs[i]->bus == bus &&
> - list->devs[i]->slot == slot &&
> - list->devs[i]->function == function)
> + tmpAddr = list->devs[i]->address;
> + if (tmpAddr &&
> + tmpAddr->domain == domain &&
> + tmpAddr->bus == bus &&
> + tmpAddr->slot == slot &&
> + tmpAddr->function == function)
> return list->devs[i];
> }
> return NULL;
> @@ -1942,9 +2009,11 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev,
> int ret = -1;
> struct dirent *ent;
> int direrr;
> + virPCIDeviceAddressPtr devAddr = dev->address;
>
> if (virAsprintf(&pcidir, "/sys/bus/pci/devices/%04x:%02x:%02x.%x",
> - dev->domain, dev->bus, dev->slot, dev->function) < 0)
> + devAddr->domain, devAddr->bus,
> + devAddr->slot, devAddr->function) < 0)
> goto cleanup;
>
> if (!(dir = opendir(pcidir))) {
> @@ -2074,13 +2143,12 @@ virPCIDeviceListPtr
> virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev)
> {
> virPCIDeviceListPtr groupList = virPCIDeviceListNew();
> - virPCIDeviceAddress devAddr = { dev->domain, dev->bus,
> - dev->slot, dev->function };
> + virPCIDeviceAddressPtr devAddr = dev->address;
>
> if (!groupList)
> goto error;
>
> - if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr,
> + if (virPCIDeviceAddressIOMMUGroupIterate(devAddr,
> virPCIDeviceGetIOMMUGroupAddOne,
> groupList) < 0)
> goto error;
> @@ -2286,6 +2354,7 @@ static int
> virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev)
> {
> virPCIDevicePtr parent;
> + virPCIDeviceAddressPtr devAddr = dev->address;
>
> if (virPCIDeviceGetParent(dev, &parent) < 0)
> return -1;
> @@ -2294,7 +2363,7 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev)
> * into play since devices on the root bus can't P2P without going
> * through the root IOMMU.
> */
> - if (dev->bus == 0) {
> + if (devAddr->bus == 0) {
> return 0;
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2664,12 +2733,13 @@ virPCIGetSysfsFile(char *virPCIDeviceName, char **pci_sysfs_device_link)
> }
>
> int
> -virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr dev,
> +virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr,
> char **pci_sysfs_device_link)
> {
> if (virAsprintf(pci_sysfs_device_link,
> - PCI_SYSFS "devices/%04x:%02x:%02x.%x", dev->domain,
> - dev->bus, dev->slot, dev->function) < 0)
> + PCI_SYSFS "devices/%04x:%02x:%02x.%x",
> + addr->domain, addr->bus,
> + addr->slot, addr->function) < 0)
> return -1;
> return 0;
> }
> diff --git a/src/util/virpci.h b/src/util/virpci.h
> index 6516f05..a137a9a 100644
> --- a/src/util/virpci.h
> +++ b/src/util/virpci.h
> @@ -69,6 +69,13 @@ struct _virPCIEDeviceInfo {
> virPCIELink *link_sta; /* Actually negotiated capabilities */
> };
>
> +virPCIDeviceAddressPtr virPCIDeviceAddressNew(unsigned int domain,
> + unsigned int bus,
> + unsigned int slot,
> + unsigned int function);
> +virPCIDeviceAddressPtr virPCIDeviceAddressCopy(virPCIDeviceAddressPtr addr);
> +void virPCIDeviceAddressFree(virPCIDeviceAddressPtr addr);
> +
> virPCIDevicePtr virPCIDeviceNew(unsigned int domain,
> unsigned int bus,
> unsigned int slot,
More information about the libvir-list
mailing list