Re: [libvirt] [PATCH 1/4 v2] pci: Move some pci sriov helper code out of node device driver to util/pci

On 08/12/2011 07:14 PM, Roopa Prabhu wrote: > From: Roopa Prabhu&lt;roprabhu(a)cisco.com&gt; > > This patch moves some of the sriov related pci code from node_device driver > to src/util/pci.[ch]. Some functions had to go thru name and argument list > change to accommodate the move. > > Signed-off-by: Roopa Prabhu&lt;roprabhu(a)cisco.com&gt; > Signed-off-by: Christian Benvenuti&lt;benve(a)cisco.com&gt; > Signed-off-by: David Wang&lt;dwang2(a)cisco.com&gt; > --- > src/Makefile.am | 5 + > src/conf/node_device_conf.c | 1 > src/conf/node_device_conf.h | 7 - > src/node_device/node_device_driver.h | 14 -- > src/node_device/node_device_hal.c | 10 + > src/node_device/node_device_linux_sysfs.c | 191 > ---------------------------- > src/node_device/node_device_udev.c | 10 + > src/util/pci.c | 196 > +++++++++++++++++++++++++++++ > src/util/pci.h | 25 ++++ > 9 files changed, 242 insertions(+), 217 deletions(-) > > > diff --git a/src/Makefile.am b/src/Makefile.am > index 009ff25..4246823 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -953,7 +953,10 @@ libvirt_driver_nodedev_la_SOURCES = > $(NODE_DEVICE_DRIVER_SOURCES) > libvirt_driver_nodedev_la_CFLAGS = \ > -I@top_srcdir@/src/conf $(AM_CFLAGS) > libvirt_driver_nodedev_la_LDFLAGS = $(AM_LDFLAGS) > -libvirt_driver_nodedev_la_LIBADD = > +libvirt_driver_nodedev_la_LIBADD = \ > + libvirt_util.la \ > + ../gnulib/lib/libgnu.la > + > if HAVE_HAL > libvirt_driver_nodedev_la_SOURCES += $(NODE_DEVICE_DRIVER_HAL_SOURCES) > libvirt_driver_nodedev_la_CFLAGS += $(HAL_CFLAGS) > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index dde2921..548bbff 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -36,6 +36,7 @@ > #include "util.h" > #include "buf.h" > #include "uuid.h" > +#include "pci.h" > > #define VIR_FROM_THIS VIR_FROM_NODEDEV > > diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h > index cef86d4..17be031 100644 > --- a/src/conf/node_device_conf.h > +++ b/src/conf/node_device_conf.h > @@ -82,13 +82,6 @@ enum virNodeDevPCICapFlags { > VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION = (1<< 1), > }; > > -struct pci_config_address { > - unsigned int domain; > - unsigned int bus; > - unsigned int slot; > - unsigned int function; > -}; > - > typedef struct _virNodeDevCapsDef virNodeDevCapsDef; > typedef virNodeDevCapsDef *virNodeDevCapsDefPtr; > struct _virNodeDevCapsDef { > diff --git a/src/node_device/node_device_driver.h > b/src/node_device/node_device_driver.h > index 08779b1..673e95b 100644 > --- a/src/node_device/node_device_driver.h > +++ b/src/node_device/node_device_driver.h > @@ -37,10 +37,6 @@ > # define LINUX_SYSFS_VPORT_CREATE_POSTFIX "/vport_create" > # define LINUX_SYSFS_VPORT_DELETE_POSTFIX "/vport_delete" > > -# define SRIOV_FOUND 0 > -# define SRIOV_NOT_FOUND 1 > -# define SRIOV_ERROR -1 > - > # define LINUX_NEW_DEVICE_WAIT_TIME 60 > > # ifdef HAVE_HAL > @@ -63,14 +59,6 @@ int check_fc_host_linux(union _virNodeDevCapData *d); > # define check_vport_capable(d) check_vport_capable_linux(d) > int check_vport_capable_linux(union _virNodeDevCapData *d); > > -# define get_physical_function(s,d) get_physical_function_linux(s,d) > -int get_physical_function_linux(const char *sysfs_path, > - union _virNodeDevCapData *d); > - > -# define get_virtual_functions(s,d) get_virtual_functions_linux(s,d) > -int get_virtual_functions_linux(const char *sysfs_path, > - union _virNodeDevCapData *d); > - > # define read_wwn(host, file, wwn) read_wwn_linux(host, file, wwn) > int read_wwn_linux(int host, const char *file, char **wwn); > > @@ -78,8 +66,6 @@ int read_wwn_linux(int host, const char *file, char **wwn); > > # define check_fc_host(d) (-1) > # define check_vport_capable(d) (-1) > -# define get_physical_function(sysfs_path, d) > -# define get_virtual_functions(sysfs_path, d) > # define read_wwn(host, file, wwn) > > # endif /* __linux__ */ > diff --git a/src/node_device/node_device_hal.c > b/src/node_device/node_device_hal.c > index 421f5ad..481be97 100644 > --- a/src/node_device/node_device_hal.c > +++ b/src/node_device/node_device_hal.c > @@ -35,6 +35,7 @@ > #include "datatypes.h" > #include "memory.h" > #include "uuid.h" > +#include "pci.h" > #include "logging.h" > #include "node_device_driver.h" > > @@ -146,8 +147,13 @@ static int gather_pci_cap(LibHalContext *ctx, const char > *udi, > (void)virStrToLong_ui(p+1,&p, 16,&d->pci_dev.function); > } > > - get_physical_function(sysfs_path, d); > - get_virtual_functions(sysfs_path, d); > + if > (!pciGetPhysicalFunction(sysfs_path,&d->pci_dev.physical_function)) > + d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; > + > + if > (!pciGetVirtualFunctions(sysfs_path,&d->pci_dev.virtual_functions, > +&d->pci_dev.num_virtual_functions) || > + d->pci_dev.num_virtual_functions> 0) > + d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; > > VIR_FREE(sysfs_path); > } > diff --git a/src/node_device/node_device_linux_sysfs.c > b/src/node_device/node_device_linux_sysfs.c > index f9ff20f..844231a 100644 > --- a/src/node_device/node_device_linux_sysfs.c > +++ b/src/node_device/node_device_linux_sysfs.c > @@ -205,195 +205,4 @@ out: > return retval; > } > > - > -static int logStrToLong_ui(char const *s, > - char **end_ptr, > - int base, > - unsigned int *result) > -{ > - int ret = 0; > - > - ret = virStrToLong_ui(s, end_ptr, base, result); > - if (ret != 0) { > - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); > - } else { > - VIR_DEBUG("Converted '%s' to unsigned int %u", s, *result); > - } > - > - return ret; > -} > - > - > -static int parse_pci_config_address(char *address, struct pci_config_address > *bdf) > -{ > - char *p = NULL; > - int ret = -1; > - > - if ((address == NULL) || (logStrToLong_ui(address,&p, 16, > -&bdf->domain) == -1)) { > - goto out; > - } > - > - if ((p == NULL) || (logStrToLong_ui(p+1,&p, 16, > -&bdf->bus) == -1)) { > - goto out; > - } > - > - if ((p == NULL) || (logStrToLong_ui(p+1,&p, 16, > -&bdf->slot) == -1)) { > - goto out; > - } > - > - if ((p == NULL) || (logStrToLong_ui(p+1,&p, 16, > -&bdf->function) == -1)) { > - goto out; > - } > - > - ret = 0; > - > -out: > - return ret; > -} > - > - > - > - > -static int get_sriov_function(const char *device_link, > - struct pci_config_address **bdf) > -{ > - char *config_address = NULL; > - char *device_path = NULL; > - char errbuf[64]; > - int ret = SRIOV_ERROR; > - > - VIR_DEBUG("Attempting to resolve device path from device link '%s'", > - device_link); > - > - if (!virFileExists(device_link)) { > - > - VIR_DEBUG("SR IOV function link '%s' does not exist", device_link); > - /* Not an SR IOV device, not an error, either. */ > - ret = SRIOV_NOT_FOUND; > - > - goto out; > - > - } > - > - device_path = canonicalize_file_name (device_link); > - if (device_path == NULL) { > - memset(errbuf, '\0', sizeof(errbuf)); > - VIR_ERROR(_("Failed to resolve device link '%s': '%s'"), > device_link, > - virStrerror(errno, errbuf, sizeof(errbuf))); > - goto out; > - } > - > - VIR_DEBUG("SR IOV device path is '%s'", device_path); > - config_address = basename(device_path); > - if (VIR_ALLOC(*bdf) != 0) { > - VIR_ERROR(_("Failed to allocate memory for PCI device name")); > - goto out; > - } > - > - if (parse_pci_config_address(config_address, *bdf) != 0) { > - VIR_ERROR(_("Failed to parse PCI config address '%s'"), > config_address); > - goto out; > - } > - > - VIR_DEBUG("SR IOV function %.4x:%.2x:%.2x.%.1x", > - (*bdf)->domain, > - (*bdf)->bus, > - (*bdf)->slot, > - (*bdf)->function); > - > - ret = SRIOV_FOUND; > - > -out: > - VIR_FREE(device_path); > - return ret; > -} > - > - > -int get_physical_function_linux(const char *sysfs_path, > - union _virNodeDevCapData *d > ATTRIBUTE_UNUSED) > -{ > - int ret = -1; > - char *device_link = NULL; > - > - VIR_DEBUG("Attempting to get SR IOV physical function for device " > - "with sysfs path '%s'", sysfs_path); > - > - if (virBuildPath(&device_link, sysfs_path, "physfn") == -1) { > - virReportOOMError(); > - } else { > - ret = get_sriov_function(device_link,&d->pci_dev.physical_function); > - if (ret == SRIOV_FOUND) { > - d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; > - } > - } > - > - VIR_FREE(device_link); > - return ret; > -} > - > - > -int get_virtual_functions_linux(const char *sysfs_path, > - union _virNodeDevCapData *d) > -{ > - int ret = -1; > - DIR *dir = NULL; > - struct dirent *entry = NULL; > - char *device_link = NULL; > - > - VIR_DEBUG("Attempting to get SR IOV virtual functions for device" > - "with sysfs path '%s'", sysfs_path); > - > - dir = opendir(sysfs_path); > - if (dir == NULL) { > - goto out; > - } > - > - while ((entry = readdir(dir))) { > - if (STRPREFIX(entry->d_name, "virtfn")) { > - /* This local is just to avoid lines of code much> 80 col. */ > - unsigned int *num_funcs =&d->pci_dev.num_virtual_functions; > - > - if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) > { > - virReportOOMError(); > - goto out; > - } > - > - VIR_DEBUG("Number of virtual functions: %d", *num_funcs); > - if (VIR_REALLOC_N(d->pci_dev.virtual_functions, > - (*num_funcs) + 1) != 0) { > - virReportOOMError(); > - goto out; > - } > - > - if (get_sriov_function(device_link, > -&d->pci_dev.virtual_functions[*num_funcs]) > - != SRIOV_FOUND) { > - > - /* We should not get back SRIOV_NOT_FOUND in this > - * case, so if we do, it's an error. */ > - VIR_ERROR(_("Failed to get SR IOV function from device link > '%s'"), > - device_link); > - goto out; > - } else { > - (*num_funcs)++; > - d->pci_dev.flags |= > VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; > - } > - > - VIR_FREE(device_link); > - } > - } > - > - ret = 0; > - > -out: > - if (dir) > - closedir(dir); > - VIR_FREE(device_link); > - return ret; > -} > - > #endif /* __linux__ */ > diff --git a/src/node_device/node_device_udev.c > b/src/node_device/node_device_udev.c > index 2c5d016..badf241 100644 > --- a/src/node_device/node_device_udev.c > +++ b/src/node_device/node_device_udev.c > @@ -37,6 +37,7 @@ > #include "uuid.h" > #include "util.h" > #include "buf.h" > +#include "pci.h" > > #define VIR_FROM_THIS VIR_FROM_NODEDEV > > @@ -480,8 +481,13 @@ static int udevProcessPCI(struct udev_device *device, > goto out; > } > > - get_physical_function(syspath, data); > - get_virtual_functions(syspath, data); > + if (!pciGetPhysicalFunction(syspath,&data->pci_dev.physical_function)) > + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; > + > + if (!pciGetVirtualFunctions(syspath,&data->pci_dev.virtual_functions, > +&data->pci_dev.num_virtual_functions) || > + data->pci_dev.num_virtual_functions> 0) > + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; > > ret = 0; > > diff --git a/src/util/pci.c b/src/util/pci.c > index a79c164..e017db9 100644 > --- a/src/util/pci.c > +++ b/src/util/pci.c > @@ -32,6 +32,7 @@ > #include<sys/types.h> > #include<sys/stat.h> > #include<unistd.h> > +#include<stdlib.h> > > #include "logging.h" > #include "memory.h" > @@ -48,6 +49,10 @@ > #define PCI_ID_LEN 10 /* "XXXX XXXX" */ > #define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */ > > +# define SRIOV_FOUND 0 > +# define SRIOV_NOT_FOUND 1 > +# define SRIOV_ERROR -1 > + > struct _pciDevice { > unsigned domain; > unsigned bus; > @@ -1679,3 +1684,194 @@ int pciDeviceIsAssignable(pciDevice *dev, > > return 1; > } > + > +static int > +logStrToLong_ui(char const *s, > + char **end_ptr, > + int base, > + unsigned int *result) > +{ > + int ret = 0; > + > + ret = virStrToLong_ui(s, end_ptr, base, result); > + if (ret != 0) { > + VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); > + } else { > + VIR_DEBUG("Converted '%s' to unsigned int %u", s, *result); > + } > + > + return ret; > +} > + > +static int > +pciParsePciConfigAddress(char *address, > + struct pci_config_address *bdf) > +{ > + char *p = NULL; > + int ret = -1; > + > + if ((address == NULL) || (logStrToLong_ui(address,&p, 16, > +&bdf->domain) == -1)) { > + goto out; > + } > + > + if ((p == NULL) || (logStrToLong_ui(p+1,&p, 16, > +&bdf->bus) == -1)) { > + goto out; > + } > + > + if ((p == NULL) || (logStrToLong_ui(p+1,&p, 16, > +&bdf->slot) == -1)) { > + goto out; > + } > + > + if ((p == NULL) || (logStrToLong_ui(p+1,&p, 16, > +&bdf->function) == -1)) { > + goto out; > + } > + > + ret = 0; > + > +out: > + return ret; > +} > + > +static int > +pciGetPciConfigAddressFromSysfsDeviceLink(const char *device_link, > + struct pci_config_address **bdf) > +{ > + char *config_address = NULL; > + char *device_path = NULL; > + char errbuf[64]; > + int ret = -1; > + > + VIR_DEBUG("Attempting to resolve device path from device link '%s'", > + device_link); > + > + if (!virFileExists(device_link)) { > + VIR_DEBUG("sysfs_path '%s' does not exist", device_link); > + goto out; > + } > + > + device_path = canonicalize_file_name (device_link); > + if (device_path == NULL) { > + memset(errbuf, '\0', sizeof(errbuf)); > + VIR_ERROR(_("Failed to resolve device link '%s': '%s'"), > device_link, > + virStrerror(errno, errbuf, sizeof(errbuf))); > + goto out; > + } > + > + config_address = basename(device_path); > + if (VIR_ALLOC(*bdf) != 0) { > + VIR_ERROR(_("Failed to allocate memory for PCI device name")); > + goto out; > + } > + > + if (pciParsePciConfigAddress(config_address, *bdf) != 0) { > + VIR_ERROR(_("Failed to parse PCI config address '%s'"), > + config_address); > + goto out; > + } > + > + VIR_DEBUG("pci_config_address %.4x:%.2x:%.2x.%.1x", > + (*bdf)->domain, > + (*bdf)->bus, > + (*bdf)->slot, > + (*bdf)->function); > + > + ret = 0; > + > +out: > + VIR_FREE(device_path); Caller likely does not expect *bdf to have a valid value in case of an error. Free bdf ?

> + > + return ret; > +} > + > +/* > + * Returns Physical function given a virtual function > + */ > +int > +pciGetPhysicalFunctionLinux(const char *vf_sysfs_path, > + struct pci_config_address **physical_function) > +{ > + int ret = -1; > + char *device_link = NULL; > + > + VIR_DEBUG("Attempting to get SR IOV physical function for device " > + "with sysfs path '%s'", vf_sysfs_path); > + > + if (virBuildPath(&device_link, vf_sysfs_path, "physfn") == -1) { > + virReportOOMError(); > + } else { > + ret = pciGetPciConfigAddressFromSysfsDeviceLink(device_link, > + physical_function); > + } > + > + VIR_FREE(device_link); > + > + return ret; > +} > + > +/* > + * Returns virtual functions of a physical function > + */ > +int > +pciGetVirtualFunctionsLinux(const char *sysfs_path, > + struct pci_config_address ***virtual_functions, 3 '*' necessary ?

> + unsigned int *num_virtual_functions) > +{ > + int ret = -1; > + DIR *dir = NULL; > + struct dirent *entry = NULL; > + char *device_link = NULL; > + > + VIR_DEBUG("Attempting to get SR IOV virtual functions for device" > + "with sysfs path '%s'", sysfs_path); > + > + dir = opendir(sysfs_path); > + if (dir == NULL) > + goto out; > + > + *virtual_functions = NULL; > + *num_virtual_functions = 0; > + while ((entry = readdir(dir))) { > + if (STRPREFIX(entry->d_name, "virtfn")) { > + > + if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) > { > + virReportOOMError(); > + goto out; > + } > + > + VIR_DEBUG("Number of virtual functions: %d", > + *num_virtual_functions); > + if (VIR_REALLOC_N(*virtual_functions, > + (*num_virtual_functions) + 1) != 0) { > + virReportOOMError(); > + goto out; > + } > + > + if (pciGetPciConfigAddressFromSysfsDeviceLink(device_link, > +&((*virtual_functions)[*num_virtual_functions])) != > + SRIOV_FOUND) { > + /* We should not get back SRIOV_NOT_FOUND in this > + * case, so if we do, it's an error. */ > + VIR_ERROR(_("Failed to get SR IOV function from device " > + "link '%s'"), device_link); > + goto out; > + } else { > + (*num_virtual_functions)++; > + } > + VIR_FREE(device_link); > + } > + } > + > + ret = 0; > + > +out: > + if (dir) > + closedir(dir); > + > + VIR_FREE(device_link); > + > + return ret; > +} > diff --git a/src/util/pci.h b/src/util/pci.h > index a351baf..367881e 100644 > --- a/src/util/pci.h > +++ b/src/util/pci.h > @@ -27,6 +27,13 @@ > typedef struct _pciDevice pciDevice; > typedef struct _pciDeviceList pciDeviceList; > > +struct pci_config_address { > + unsigned int domain; > + unsigned int bus; > + unsigned int slot; > + unsigned int function; > +}; > + > pciDevice *pciGetDevice (unsigned domain, > unsigned bus, > unsigned slot, > @@ -74,4 +81,22 @@ int pciDeviceIsAssignable(pciDevice *dev, > int strict_acs_check); > int pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher); > > +#ifdef __linux__ There should probably be a space between the '#' and the 'ifdef'. Try with 'make syntax-check'.

> + > +# define pciGetPhysicalFunction(s,a) pciGetPhysicalFunctionLinux(s,a) > +int pciGetPhysicalFunctionLinux(const char *sysfs_path, > + struct pci_config_address **phys_fn); > + > +# define pciGetVirtualFunctions(s,a,n) pciGetVirtualFunctionsLinux(s,a,n) > +int pciGetVirtualFunctionsLinux(const char *sysfs_path, > + struct pci_config_address > ***virtual_functions, > + unsigned int *num_virtual_functions); > + > +#else /* __linux__ */ Same single space here... > + > +# define pciGetPhysicalFunction(s,a) > +# define pciGetVirtualFunctions(s,a,n) > + > +#endif and here. > + > #endif /* __VIR_PCI_H__ */ > ACK with those nits addressed. Stefan