On 8/15/11 3:47 AM, "Stefan Berger" <stefanb@xxxxxxxxxxxxxxxxxx> wrote:
> On 08/12/2011 07:14 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu<roprabhu@xxxxxxxxx>
>>
>> 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<roprabhu@xxxxxxxxx>
>> Signed-off-by: Christian Benvenuti<benve@xxxxxxxxx>
>> Signed-off-by: David Wang<dwang2@xxxxxxxxx>
>> ---
>> 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 ?
Yeah, will fix it.
>> +
>> + 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 ?
Yes I think so, Cause I have to allocate the whole ** inside the function
and the caller is the one who frees it.
>> + 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'.
Ok will fix it.
>> +
>> +# 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
>
Thanks. will fix them and resubmit.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list